Re: [RFC PATCH v3 01/19] scsi: scsi_error: Define framework for LUN/target based error handle

[Bart Van Assche]

On 3/13/25 6:29 PM, JiangJianJun wrote:
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 815e7d63f3e2..f89de23a6807 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -291,11 +291,48 @@ static void scsi_eh_inc_host_failed(struct rcu_head *head)
>   	spin_unlock_irqrestore(shost->host_lock, flags);
>   }
>   

Please move the new-style error handling functions into a new file. 
scsi_error.c is already way too big. Mixing host-based and device-based
error handling in a single file is confusing. Please don't do this.

> +#define SCSI_EH_NO_HANDLER 1

This should be a new enumeration type instead of a define.

>   /**
>    * scsi_eh_scmd_add - add scsi cmd to error handling.
>    * @scmd:	scmd to run eh on.
>    */
> -void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
> +static void __scsi_eh_scmd_add(struct scsi_cmnd *scmd)

Please choose a better name for this function than __scsi_eh_scmd_add().

> +void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
> +{
> +	struct scsi_device *sdev = scmd->device;
> +	struct scsi_target *starget = scsi_target(sdev);
> +	struct Scsi_Host *shost = sdev->host;
> +
> +	if (unlikely(scsi_host_in_recovery(shost)))
> +		__scsi_eh_scmd_add(scmd);
> +
> +	if (unlikely(scsi_target_in_recovery(starget)))
> +		if (__scsi_eh_scmd_add_starget(scmd))
> +			__scsi_eh_scmd_add(scmd);
> +
> +	if (__scsi_eh_scmd_add_sdev(scmd))
> +		if (__scsi_eh_scmd_add_starget(scmd))
> +			__scsi_eh_scmd_add(scmd);
> +}

Please rename the function __scsi_eh_scmd_add() to make it clear that it 
is used for the host-based error handling strategey.

> +static inline int scsi_device_in_recovery(struct scsi_device *sdev)
> +{
> +	struct scsi_device_eh *eh = sdev->eh;
> +
> +	if (eh && eh->is_busy)
> +		return eh->is_busy(sdev);
> +	return 0;
> +}

Can the return type of this function be changed into 'bool'? Can the
three statements in the function body be combined into the following
statement?

	return eh && eh->is_busy && eh->is_busy(sdev);

> +static inline int scsi_target_in_recovery(struct scsi_target *starget)
> +{
> +	struct scsi_target_eh *eh = starget->eh;
> +
> +	if (eh && eh->is_busy)
> +		return eh->is_busy(starget);
> +	return 0;
> +}

Same questions here.

> +struct scsi_device_eh {
> +	/*
> +	 * add scsi command to error handler so it would be handuled by
> +	 * driver's error handle strategy
> +	 */
> +	void (*add_cmnd)(struct scsi_cmnd *scmd);
> +
> +	/*
> +	 * to judge if the device is busy handling errors, called before
> +	 * dispatch scsi cmnd
> +	 *
> +	 * return 0 if it's ready to accepy scsi cmnd
> +	 * return 0 if it's in error handle, command's would not be dispatched
> +	 */
> +	int (*is_busy)(struct scsi_device *sdev);
> +
> +	/*
> +	 * wakeup device's error handle
> +	 *
> +	 * usually the error handler strategy would not run at once when
> +	 * error command is added. This function would be called when any
> +	 * scsi cmnd is finished or when scsi cmnd is added.
> +	 */
> +	int (*wakeup)(struct scsi_device *sdev);
> +
> +	/*
> +	 * data entity for device specific error handler
> +	 */
> +	unsigned long driver_data[];
> +};

Adding unsigned long driver_data[] at the end of a structure is the old
way for extending a structure. Please don't do this. Instead, leave out
the driver_data[] array, embed the scsi_device_eh in a larger data
structure where necessary and use container_of() to convert
scsi_device_eh pointers into a pointer to the containing structure.

Thanks,

Bart.