[RFC,0/4] UFS patches for Linux kernel v5.14

Message ID 20210619005228.28569-1-bvanassche@acm.org
Headers show
Series
  • UFS patches for Linux kernel v5.14
Related show

Message

Bart Van Assche June 19, 2021, 12:52 a.m.
Hi Martin,

The four patches in this series are what I came up with while reviewing
recent UFS patches. These patches are intended for kernel v5.14. These
patches have been posted as an "RFC" because these have been compile tested
only.

Thanks,

Bart.

Bart Van Assche (4):
  ufs: Rename the second ufshcd_probe_hba() argument
  ufs: Remove a check from ufshcd_queuecommand()
  ufs: Improve static type checking for the host controller state
  ufs: Make host controller state change handling more systematic

 drivers/scsi/ufs/ufshcd.c | 96 +++++++++++++++++----------------------
 drivers/scsi/ufs/ufshcd.h | 25 +++++++++-
 2 files changed, 65 insertions(+), 56 deletions(-)

Comments

Avri Altman June 21, 2021, 8:13 a.m. | #1
> 

> Rename the second argument of ufshcd_probe_hba() such that the name of

> that argument reflects its purpose instead of how the function is called.

> See also commit 1b9e21412f72 ("scsi: ufs: Split ufshcd_probe_hba() based

> on its called flow").

> 

> Cc: Bean Huo <beanhuo@micron.com>

> Cc: Asutosh Das <asutoshd@codeaurora.org>

> Cc: Can Guo <cang@codeaurora.org>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Avri Altman <avri.altman@wdc.com>



> ---

>  drivers/scsi/ufs/ufshcd.c | 8 ++++----

>  1 file changed, 4 insertions(+), 4 deletions(-)

> 

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c

> index 25fe18a36cd2..c230d2a6a55c 100644

> --- a/drivers/scsi/ufs/ufshcd.c

> +++ b/drivers/scsi/ufs/ufshcd.c

> @@ -7964,13 +7964,13 @@ static int ufshcd_clear_ua_wluns(struct ufs_hba

> *hba)

>  }

> 

>  /**

> - * ufshcd_probe_hba - probe hba to detect device and initialize

> + * ufshcd_probe_hba - probe hba to detect device and initialize it

>   * @hba: per-adapter instance

> - * @async: asynchronous execution or not

> + * @init_dev_params: whether or not to call ufshcd_device_params_init().

>   *

>   * Execute link-startup and verify device initialization

>   */

> -static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)

> +static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)

>  {

>         int ret;

>         unsigned long flags;

> @@ -8002,7 +8002,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,

> bool async)

>          * Initialize UFS device parameters used by driver, these

>          * parameters are associated with UFS descriptors.

>          */

> -       if (async) {

> +       if (init_dev_params) {

>                 ret = ufshcd_device_params_init(hba);

>                 if (ret)

>                         goto out;
Avri Altman June 21, 2021, 8:26 a.m. | #2
> Assign a name to the enumeration type for UFS host controller states and

> remove the default clause from switch statements on this enumeration type

> to make the compiler warn about unhandled enumeration labels.

> 

> Cc: Can Guo <cang@codeaurora.org>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Avri Altman <avri.altman@wdc.com>



> ---

>  drivers/scsi/ufs/ufshcd.c | 15 ---------------

>  drivers/scsi/ufs/ufshcd.h | 25 +++++++++++++++++++++++--

>  2 files changed, 23 insertions(+), 17 deletions(-)

> 

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c

> index 71c720d940a3..c213daec20f7 100644

> --- a/drivers/scsi/ufs/ufshcd.c

> +++ b/drivers/scsi/ufs/ufshcd.c

> @@ -128,15 +128,6 @@ enum {

>         UFSHCD_CAN_QUEUE        = 32,

>  };

> 

> -/* UFSHCD states */

> -enum {

> -       UFSHCD_STATE_RESET,

> -       UFSHCD_STATE_ERROR,

> -       UFSHCD_STATE_OPERATIONAL,

> -       UFSHCD_STATE_EH_SCHEDULED_FATAL,

> -       UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,

> -};

> -

>  /* UFSHCD error handling flags */

>  enum {

>         UFSHCD_EH_IN_PROGRESS = (1 << 0),

> @@ -2738,12 +2729,6 @@ static int ufshcd_queuecommand(struct

> Scsi_Host *host, struct scsi_cmnd *cmd)

>                 set_host_byte(cmd, DID_ERROR);

>                 cmd->scsi_done(cmd);

>                 goto out;

> -       default:

> -               dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",

> -                               __func__, hba->ufshcd_state);

> -               set_host_byte(cmd, DID_BAD_TARGET);

> -               cmd->scsi_done(cmd);

> -               goto out;

>         }

> 

>         hba->req_abort_count = 0;

> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h

> index c98d540ac044..f2796ea25598 100644

> --- a/drivers/scsi/ufs/ufshcd.h

> +++ b/drivers/scsi/ufs/ufshcd.h

> @@ -476,6 +476,27 @@ struct ufs_stats {

>         struct ufs_event_hist event[UFS_EVT_CNT];

>  };

> 

> +/**

> + * enum ufshcd_state - UFS host controller state

> + * @UFSHCD_STATE_RESET: Link is not operational. Postpone SCSI

> command

> + *     processing.

> + * @UFSHCD_STATE_OPERATIONAL: The host controller is operational and

> can process

> + *     SCSI commands.

> + * @UFSHCD_STATE_EH_SCHEDULED_NON_FATAL: The error handler has

> been scheduled.

> + *     SCSI commands may be submitted to the controller.

> + * @UFSHCD_STATE_EH_SCHEDULED_FATAL: The error handler has been

> scheduled. Fail

> + *     newly submitted SCSI commands with error code DID_BAD_TARGET.

> + * @UFSHCD_STATE_ERROR: An unrecoverable error occurred, e.g. link

> recovery

> + *     failed. Fail all SCSI commands with error code DID_ERROR.

> + */

> +enum ufshcd_state {

> +       UFSHCD_STATE_RESET,

> +       UFSHCD_STATE_OPERATIONAL,

> +       UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,

> +       UFSHCD_STATE_EH_SCHEDULED_FATAL,

> +       UFSHCD_STATE_ERROR,

> +};

> +

>  enum ufshcd_quirks {

>         /* Interrupt aggregation support is broken */

>         UFSHCD_QUIRK_BROKEN_INTR_AGGR                   = 1 << 0,

> @@ -687,7 +708,7 @@ struct ufs_hba_monitor {

>   * @tmf_tag_set: TMF tag set.

>   * @tmf_queue: Used to allocate TMF tags.

>   * @pwr_done: completion for power mode change

> - * @ufshcd_state: UFSHCD states

> + * @ufshcd_state: UFSHCD state

>   * @eh_flags: Error handling flags

>   * @intr_mask: Interrupt Mask Bits

>   * @ee_ctrl_mask: Exception event control mask

> @@ -785,7 +806,7 @@ struct ufs_hba {

>         struct mutex uic_cmd_mutex;

>         struct completion *uic_async_done;

> 

> -       u32 ufshcd_state;

> +       enum ufshcd_state ufshcd_state;

>         u32 eh_flags;

>         u32 intr_mask;

>         u16 ee_ctrl_mask; /* Exception event mask */
Can Guo June 23, 2021, 7:42 a.m. | #3
Hi Bart,

On 2021-06-19 08:52, Bart Van Assche wrote:
> Assign a name to the enumeration type for UFS host controller states 

> and

> remove the default clause from switch statements on this enumeration 

> type

> to make the compiler warn about unhandled enumeration labels.

> 

> Cc: Can Guo <cang@codeaurora.org>

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> ---

>  drivers/scsi/ufs/ufshcd.c | 15 ---------------

>  drivers/scsi/ufs/ufshcd.h | 25 +++++++++++++++++++++++--

>  2 files changed, 23 insertions(+), 17 deletions(-)

> 

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c

> index 71c720d940a3..c213daec20f7 100644

> --- a/drivers/scsi/ufs/ufshcd.c

> +++ b/drivers/scsi/ufs/ufshcd.c

> @@ -128,15 +128,6 @@ enum {

>  	UFSHCD_CAN_QUEUE	= 32,

>  };

> 

> -/* UFSHCD states */

> -enum {

> -	UFSHCD_STATE_RESET,

> -	UFSHCD_STATE_ERROR,

> -	UFSHCD_STATE_OPERATIONAL,

> -	UFSHCD_STATE_EH_SCHEDULED_FATAL,

> -	UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,

> -};

> -

>  /* UFSHCD error handling flags */

>  enum {

>  	UFSHCD_EH_IN_PROGRESS = (1 << 0),

> @@ -2738,12 +2729,6 @@ static int ufshcd_queuecommand(struct Scsi_Host

> *host, struct scsi_cmnd *cmd)

>  		set_host_byte(cmd, DID_ERROR);

>  		cmd->scsi_done(cmd);

>  		goto out;

> -	default:

> -		dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",

> -				__func__, hba->ufshcd_state);

> -		set_host_byte(cmd, DID_BAD_TARGET);

> -		cmd->scsi_done(cmd);

> -		goto out;

>  	}

> 

>  	hba->req_abort_count = 0;

> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h

> index c98d540ac044..f2796ea25598 100644

> --- a/drivers/scsi/ufs/ufshcd.h

> +++ b/drivers/scsi/ufs/ufshcd.h

> @@ -476,6 +476,27 @@ struct ufs_stats {

>  	struct ufs_event_hist event[UFS_EVT_CNT];

>  };

> 

> +/**

> + * enum ufshcd_state - UFS host controller state

> + * @UFSHCD_STATE_RESET: Link is not operational. Postpone SCSI command

> + *	processing.

> + * @UFSHCD_STATE_OPERATIONAL: The host controller is operational and

> can process

> + *	SCSI commands.

> + * @UFSHCD_STATE_EH_SCHEDULED_NON_FATAL: The error handler has been 

> scheduled.

> + *	SCSI commands may be submitted to the controller.

> + * @UFSHCD_STATE_EH_SCHEDULED_FATAL: The error handler has been 

> scheduled. Fail

> + *	newly submitted SCSI commands with error code DID_BAD_TARGET.

> + * @UFSHCD_STATE_ERROR: An unrecoverable error occurred, e.g. link 

> recovery

> + *	failed. Fail all SCSI commands with error code DID_ERROR.

> + */

> +enum ufshcd_state {

> +	UFSHCD_STATE_RESET,

> +	UFSHCD_STATE_OPERATIONAL,

> +	UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,

> +	UFSHCD_STATE_EH_SCHEDULED_FATAL,

> +	UFSHCD_STATE_ERROR,

> +};

> +


Hi Bart,

FYI, in my error handling update change series, I have one change
(https://lore.kernel.org/patchwork/patch/1450656/) which moves the
enumeration from ufshcd.c to ufshcd.h, which shall conflict with
this one. What shall we do?

Thanks,

Can Guo.

>  enum ufshcd_quirks {

>  	/* Interrupt aggregation support is broken */

>  	UFSHCD_QUIRK_BROKEN_INTR_AGGR			= 1 << 0,

> @@ -687,7 +708,7 @@ struct ufs_hba_monitor {

>   * @tmf_tag_set: TMF tag set.

>   * @tmf_queue: Used to allocate TMF tags.

>   * @pwr_done: completion for power mode change

> - * @ufshcd_state: UFSHCD states

> + * @ufshcd_state: UFSHCD state

>   * @eh_flags: Error handling flags

>   * @intr_mask: Interrupt Mask Bits

>   * @ee_ctrl_mask: Exception event control mask

> @@ -785,7 +806,7 @@ struct ufs_hba {

>  	struct mutex uic_cmd_mutex;

>  	struct completion *uic_async_done;

> 

> -	u32 ufshcd_state;

> +	enum ufshcd_state ufshcd_state;

>  	u32 eh_flags;

>  	u32 intr_mask;

>  	u16 ee_ctrl_mask; /* Exception event mask */
Bart Van Assche June 23, 2021, 4:10 p.m. | #4
On 6/23/21 12:42 AM, Can Guo wrote:
> FYI, in my error handling update change series, I have one change

> (https://lore.kernel.org/patchwork/patch/1450656/) which moves the

> enumeration from ufshcd.c to ufshcd.h, which shall conflict with

> this one. What shall we do?


Hi Can,

Thanks for asking. I will rebase my patch series on top of yours.

Bart.