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.
