Re: [PATCH RFC 3/4] ufs: Improve static type checking for the host controller state

[Can Guo <cang@codeaurora.org>]

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 */