Re: [PATCH 33/84] fas216: Call scsi_done() directly

From: Bart Van Assche <bvanassche@acm.org>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: "Martin K . Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org,
	"James E.J. Bottomley" <jejb@linux.ibm.com>
Subject: Re: [PATCH 33/84] fas216: Call scsi_done() directly
Date: Sat, 18 Sep 2021 19:25:58 -0700	[thread overview]
Message-ID: <9f68887b-e499-ec29-9ebe-207c6c515cd3@acm.org> (raw)
In-Reply-To: <YUUx47L7W9qGGOwz@shell.armlinux.org.uk>

On 9/17/21 17:25, Russell King (Oracle) wrote:
> NAK. I don't think you've bothered to read the driver code to check
> that your change is safe to make.
> 
> SCpnt->scsi_done is not always "scsi_done" but may also be
> fas216_internal_done().

Thanks for the quick feedback. How about replacing this patch by the
two patches below?

[PATCH 1/2] fas216: Introduce struct fas216_cmd_priv

Introduce a structure with driver-private data per SCSI command. This data
structure will be used by the next patch to store a function pointer.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  drivers/scsi/arm/arxescsi.c |  1 +
  drivers/scsi/arm/cumana_2.c |  1 +
  drivers/scsi/arm/eesox.c    |  1 +
  drivers/scsi/arm/fas216.h   | 10 ++++++++++
  drivers/scsi/arm/powertec.c |  2 +-
  5 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/arm/arxescsi.c b/drivers/scsi/arm/arxescsi.c
index 591414120754..7f667c198f6d 100644
--- a/drivers/scsi/arm/arxescsi.c
+++ b/drivers/scsi/arm/arxescsi.c
@@ -243,6 +243,7 @@ static struct scsi_host_template arxescsi_template = {
  	.eh_bus_reset_handler		= fas216_eh_bus_reset,
  	.eh_device_reset_handler	= fas216_eh_device_reset,
  	.eh_abort_handler		= fas216_eh_abort,
+	.cmd_size			= sizeof(struct fas216_cmd_priv),
  	.can_queue			= 0,
  	.this_id			= 7,
  	.sg_tablesize			= SG_ALL,
diff --git a/drivers/scsi/arm/cumana_2.c b/drivers/scsi/arm/cumana_2.c
index 9dcd912267e6..3c00d7773876 100644
--- a/drivers/scsi/arm/cumana_2.c
+++ b/drivers/scsi/arm/cumana_2.c
@@ -363,6 +363,7 @@ static struct scsi_host_template cumanascsi2_template = {
  	.eh_bus_reset_handler		= fas216_eh_bus_reset,
  	.eh_device_reset_handler	= fas216_eh_device_reset,
  	.eh_abort_handler		= fas216_eh_abort,
+	.cmd_size			= sizeof(struct fas216_cmd_priv),
  	.can_queue			= 1,
  	.this_id			= 7,
  	.sg_tablesize			= SG_MAX_SEGMENTS,
diff --git a/drivers/scsi/arm/eesox.c b/drivers/scsi/arm/eesox.c
index 5eb2415dda9d..1394590eecea 100644
--- a/drivers/scsi/arm/eesox.c
+++ b/drivers/scsi/arm/eesox.c
@@ -480,6 +480,7 @@ static struct scsi_host_template eesox_template = {
  	.eh_bus_reset_handler		= fas216_eh_bus_reset,
  	.eh_device_reset_handler	= fas216_eh_device_reset,
  	.eh_abort_handler		= fas216_eh_abort,
+	.cmd_size			= sizeof(struct fas216_cmd_priv),
  	.can_queue			= 1,
  	.this_id			= 7,
  	.sg_tablesize			= SG_MAX_SEGMENTS,
diff --git a/drivers/scsi/arm/fas216.h b/drivers/scsi/arm/fas216.h
index 847413ce14cf..abf960487314 100644
--- a/drivers/scsi/arm/fas216.h
+++ b/drivers/scsi/arm/fas216.h
@@ -310,6 +310,16 @@ typedef struct {
  	unsigned long		magic_end;
  } FAS216_Info;

+/* driver-private data per SCSI command. */
+struct fas216_cmd_priv {
+	void (*scsi_done)(struct scsi_cmnd *cmd);
+};
+
+static inline struct fas216_cmd_priv *fas216_cmd_priv(struct scsi_cmnd *cmd)
+{
+	return scsi_cmd_priv(cmd);
+}
+
  /* Function: int fas216_init (struct Scsi_Host *instance)
   * Purpose : initialise FAS/NCR/AMD SCSI structures.
   * Params  : instance - a driver-specific filled-out structure
diff --git a/drivers/scsi/arm/powertec.c b/drivers/scsi/arm/powertec.c
index 9cc73da4e876..8fec435cee18 100644
--- a/drivers/scsi/arm/powertec.c
+++ b/drivers/scsi/arm/powertec.c
@@ -286,7 +286,7 @@ static struct scsi_host_template powertecscsi_template = {
  	.eh_bus_reset_handler		= fas216_eh_bus_reset,
  	.eh_device_reset_handler	= fas216_eh_device_reset,
  	.eh_abort_handler		= fas216_eh_abort,
-
+	.cmd_size			= sizeof(struct fas216_cmd_priv),
  	.can_queue			= 8,
  	.this_id			= 7,
  	.sg_tablesize			= SG_MAX_SEGMENTS,






[PATCH 2/2] fas216: Stop using scsi_cmnd.scsi_done

Store the completion callback pointer in struct fas216_cmd_priv instead of in
struct scsi_cmnd. This patch prepares for removal of the scsi_done member
from struct scsi_cmnd.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  drivers/scsi/arm/fas216.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/arm/fas216.c b/drivers/scsi/arm/fas216.c
index bbb8635782b1..9926383f79ea 100644
--- a/drivers/scsi/arm/fas216.c
+++ b/drivers/scsi/arm/fas216.c
@@ -2017,7 +2017,7 @@ static void fas216_rq_sns_done(FAS216_Info *info, struct scsi_cmnd *SCpnt,
  	 * correctly by fas216_std_done.
  	 */
  	scsi_eh_restore_cmnd(SCpnt, &info->ses);
-	SCpnt->scsi_done(SCpnt);
+	fas216_cmd_priv(SCpnt)->scsi_done(SCpnt);
  }

  /**
@@ -2088,8 +2088,8 @@ fas216_std_done(FAS216_Info *info, struct scsi_cmnd *SCpnt, unsigned int result)
  	}

  done:
-	if (SCpnt->scsi_done) {
-		SCpnt->scsi_done(SCpnt);
+	if (fas216_cmd_priv(SCpnt)->scsi_done) {
+		fas216_cmd_priv(SCpnt)->scsi_done(SCpnt);
  		return;
  	}

@@ -2205,7 +2205,7 @@ static int fas216_queue_command_lck(struct scsi_cmnd *SCpnt,
  	fas216_log_command(info, LOG_CONNECT, SCpnt,
  			   "received command (%p)", SCpnt);

-	SCpnt->scsi_done = done;
+	fas216_cmd_priv(SCpnt)->scsi_done = done;
  	SCpnt->host_scribble = (void *)fas216_std_done;
  	SCpnt->result = 0;

