From: John Garry <john.g.garry@oracle.com>
To: Hannes Reinecke <hare@suse.de>,
"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: James Bottomley <james.bottomley@hansenpartnership.com>,
linux-scsi@vger.kernel.org, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 2/7] scsi: Use Scsi_Host and channel number as argument for eh_bus_reset_handler()
Date: Thu, 5 Oct 2023 12:54:21 +0100 [thread overview]
Message-ID: <b2182011-258c-cdda-2998-4e98fa510bba@oracle.com> (raw)
In-Reply-To: <20231002155915.109359-3-hare@suse.de>
> diff --git a/Documentation/scsi/scsi_eh.rst b/Documentation/scsi/scsi_eh.rst
> index da95971b4f44..dbbfd754a928 100644
> --- a/Documentation/scsi/scsi_eh.rst
> +++ b/Documentation/scsi/scsi_eh.rst
> @@ -216,7 +216,7 @@ considered to fail always.
>
> int (* eh_abort_handler)(struct scsi_cmnd *);
> int (* eh_device_reset_handler)(struct scsi_cmnd *);
> - int (* eh_bus_reset_handler)(struct scsi_cmnd *);
> + int (* eh_bus_reset_handler)(struct Scsi_Host *, int);
scsi_device.channel and scsi_target.channel are both unsigned int, so
could this also be unsigned int?
> int (* eh_host_reset_handler)(struct Scsi_Host *);
>
> Higher-severity actions are taken only when lower-severity actions
> diff --git a/Documentation/scsi/scsi_mid_low_api.rst b/Documentation/scsi/scsi_mid_low_api.rst
> index 85b1384ba4fd..88bb3b7578ba 100644
> --- a/Documentation/scsi/scsi_mid_low_api.rst
> +++ b/Documentation/scsi/scsi_mid_low_api.rst
> @@ -741,7 +741,8 @@ Details::
...
>
> /* If we can't locate our host adapter structure, return FAILED status.
> */
> - if ((hd = shost_priv(SCpnt->device->host)) == NULL){
> + if ((hd = shost_priv(shost)) == NULL){
> printk(KERN_ERR MYNAM ": bus reset: "
> - "Can't locate host! (sc=%p)\n", SCpnt);
> + "Can't locate host!\n");
> return FAILED;
> }
>
> ioc = hd->ioc;
> - printk(MYIOC_s_INFO_FMT "attempting bus reset! (sc=%p)\n",
> - ioc->name, SCpnt);
> - scsi_print_command(SCpnt);
> + printk(MYIOC_s_INFO_FMT "attempting bus reset!\n",
maybe printing the channel number may be helpful
> + ioc->name);
>
> if (ioc->timeouts < -1)
> ioc->timeouts++;
>
> - vdevice = SCpnt->device->hostdata;
> - if (!vdevice || !vdevice->vtarget)
> - return SUCCESS;
> retval = mptscsih_IssueTaskMgmt(hd,
> MPI_SCSITASKMGMT_TASKTYPE_RESET_BUS,
> - vdevice->vtarget->channel, 0, 0, 0,
> + channel, 0, 0, 0,
> mptscsih_get_tm_timeout(ioc));
>
> - printk(MYIOC_s_INFO_FMT "bus reset: %s (sc=%p)\n",
> - ioc->name, ((retval == 0) ? "SUCCESS" : "FAILED" ), SCpnt);
> + printk(MYIOC_s_INFO_FMT "bus reset: %s\n",
> + ioc->name, ((retval == 0) ? "SUCCESS" : "FAILED" ));
Outer () can be removed
>
> if (retval == 0)
> return SUCCESS;
> diff --git a/drivers/message/fusion/mptscsih.h b/drivers/message/fusion/mptscsih.h
...
> struct AdapterCtlBlk *acb =
> - (struct AdapterCtlBlk *)cmd->device->host->hostdata;
> + (struct AdapterCtlBlk *)shost->hostdata;
> dprintkl(KERN_INFO,
> - "eh_bus_reset: (0%p) target=<%02i-%i> cmd=%p\n",
> - cmd, cmd->device->id, (u8)cmd->device->lun, cmd);
> + "eh_bus_reset: bus=<%02i>\n", channel);
>
> if (timer_pending(&acb->waiting_timer))
> del_timer(&acb->waiting_timer);
>
> /*
> - * disable interrupt
> + * disable interrupt
what changed here?
> */
> DC395x_write8(acb, TRM_S1040_DMA_INTEN, 0x00);
> DC395x_write8(acb, TRM_S1040_SCSI_INTEN, 0x00);
> @@ -1172,7 +1171,7 @@ static int __dc395x_eh_bus_reset(struct scsi_cmnd *cmd)
> HZ * acb->eeprom.delay_time;
>
> /*
> - * re-enable interrupt
> + * re-enable interrupt
> */
...
>
> /* Internal functions */
> diff --git a/drivers/scsi/esas2r/esas2r_main.c b/drivers/scsi/esas2r/esas2r_main.c
> index 168f01a34acc..ae647e2586d0 100644
> --- a/drivers/scsi/esas2r/esas2r_main.c
> +++ b/drivers/scsi/esas2r/esas2r_main.c
> @@ -1097,10 +1097,8 @@ int esas2r_host_reset(struct Scsi_Host *shost)
> return esas2r_host_bus_reset(shost, true);
> }
>
> -int esas2r_bus_reset(struct scsi_cmnd *cmd)
> +int esas2r_bus_reset(struct Scsi_Host *shost, int channel)
> {
> - struct Scsi_Host *shost = cmd->device->host;
> -
> esas2r_log(ESAS2R_LOG_INFO, "bus_reset (%p)", shost);
surely printing the channel would be better than the shost pointer
>
> return esas2r_host_bus_reset(shost, false);
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 355fec046220..c6a5721aef23 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -2604,9 +2604,9 @@ static int esp_eh_abort_handler(struct scsi_cmnd *cmd)
> return FAILED;
> }
>
...
>
> - sdev_printk(KERN_INFO, scmd->device,
> - "Bus reset is %s for scmd(%p)\n",
> - ((retval == SUCCESS) ? "SUCCESS" : "FAILED"), scmd);
> + shost_printk(KERN_INFO, shost,
> + "Bus reset is %s\n",
> + ((retval == SUCCESS) ? "SUCCESS" : "FAILED"));
again, () can be removed
> return retval;
> }
>
> diff --git a/drivers/scsi/ncr53c8xx.c b/drivers/scsi/ncr53c8xx.c
> index 35869b4f9329..59efe064b304 100644
> --- a/drivers/scsi/ncr53c8xx.c
> +++ b/drivers/scsi/ncr53c8xx.c
> @@ -7936,9 +7936,9 @@ static void ncr53c8xx_timeout(struct timer_list *t)
> ncr_flush_done_cmds(done_list);
> }
>
> -static int ncr53c8xx_bus_reset(struct scsi_cmnd *cmd)
> +static int ncr53c8xx_bus_reset(struct Scsi_Host *host, int channel)
> {
> - struct ncb *np = ((struct host_data *) cmd->device->host->hostdata)->ncb;
> + struct ncb *np = ((struct host_data *) host->hostdata)->ncb;
> int sts;
> unsigned long flags;
> struct scsi_cmnd *done_list;
> diff --git a/drivers/scsi/pcmcia/nsp_cs.c b/drivers/scsi/pcmcia/nsp_cs.c
> index 104647a6170e..c709bb955d05 100644
> --- a/drivers/scsi/pcmcia/nsp_cs.c
> +++ b/drivers/scsi/pcmcia/nsp_cs.c
> @@ -1486,11 +1486,11 @@ static int nsp_bus_reset(nsp_hw_data *data)
> return SUCCESS;
> }
>
> -static int nsp_eh_bus_reset(struct scsi_cmnd *SCpnt)
> +static int nsp_eh_bus_reset(struct Scsi_Host *host, int channel)
> {
> - nsp_hw_data *data = (nsp_hw_data *)SCpnt->device->host->hostdata;
> + nsp_hw_data *data = (nsp_hw_data *)host->hostdata;
>
> - nsp_dbg(NSP_DEBUG_BUSRESET, "SCpnt=0x%p", SCpnt);
> + nsp_dbg(NSP_DEBUG_BUSRESET, "channel=0x%d", channel);
0x%x, and I am not sure that is even better than printing in decimal
>
> return nsp_bus_reset(data);
> }
> diff --git a/drivers/scsi/pcmcia/nsp_cs.h b/drivers/scsi/pcmcia/nsp_cs.h
> index f532adb5f166..8636f0053c02 100644
> --- a/drivers/scsi/pcmcia/nsp_cs.h
> +++ b/drivers/scsi/pcmcia/nsp_cs.h
> @@ -299,7 +299,7 @@ static int nsp_queuecommand(struct Scsi_Host *h, struct scsi_cmnd *SCpnt);
...
> return rc;
> }
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index bb0323d05640..a9d9d0a9abd7 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -1588,7 +1588,7 @@ qla2xxx_eh_target_reset(struct scsi_cmnd *cmd)
> * commands.
> *
> * Input:
> -* cmd = Linux SCSI command packet of the command that cause the
> +* host = Linux SCSI command packet of the command that cause the
> * bus reset.
"Linux SCSI command packet " - really?
> *
> * Returns:
> @@ -1596,12 +1596,10 @@ qla2xxx_eh_target_reset(struct scsi_cmnd *cmd)
> *
> **************************************************************************/
...
>
> - scmd_printk(KERN_INFO, cmd, "SCSI Bus reset\n");
> + shost_printk(KERN_INFO, host, "SCSI Bus reset\n");
how about this the channel #?
>
> /*
> * We don't want to queue new requests for this bus after
> diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
> index 42134f9510d5..bc48152e74b8 100644
> --- a/drivers/scsi/wd719x.c
> +++ b/drivers/scsi/wd719x.c
> @@ -484,11 +484,11 @@ static int wd719x_abort(struct scsi_cmnd *cmd)
> return SUCCESS;
> }
> /* Simulate a SCSI bus reset by resetting the device's USB port. */
> -static int bus_reset(struct scsi_cmnd *srb)
> +static int bus_reset(struct Scsi_Host *shost, int channel)
> {
> - struct us_data *us = host_to_us(srb->device->host);
> + struct us_data *us = host_to_us(shost);
Incidentally host_to_us() seems a copy of shost_priv()
> int result;
>
> usb_stor_dbg(us, "%s called\n", __func__);
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 9e3ec411cdc6..32ef595db028 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -152,7 +152,7 @@ struct scsi_host_template {
> int (* eh_abort_handler)(struct scsi_cmnd *);
> int (* eh_device_reset_handler)(struct scsi_cmnd *);
> int (* eh_target_reset_handler)(struct scsi_cmnd *);
> - int (* eh_bus_reset_handler)(struct scsi_cmnd *);
> + int (* eh_bus_reset_handler)(struct Scsi_Host *, int);
> int (* eh_host_reset_handler)(struct Scsi_Host *);
>
> /*
next prev parent reply other threads:[~2023-10-05 14:43 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-02 15:59 [PATCHv5 0/7] scsi: EH rework, main part Hannes Reinecke
2023-10-02 15:59 ` [PATCH 1/7] scsi: Use Scsi_Host as argument for eh_host_reset_handler Hannes Reinecke
2023-10-03 17:33 ` Bart Van Assche
2023-10-04 6:32 ` Hannes Reinecke
2023-10-09 14:05 ` Hannes Reinecke
2023-10-09 15:23 ` Bart Van Assche
2023-10-05 11:14 ` John Garry
2023-10-02 15:59 ` [PATCH 2/7] scsi: Use Scsi_Host and channel number as argument for eh_bus_reset_handler() Hannes Reinecke
2023-10-05 11:54 ` John Garry [this message]
2023-10-02 15:59 ` [PATCH 3/7] scsi: Use scsi_target as argument for eh_target_reset_handler() Hannes Reinecke
2023-10-05 12:33 ` John Garry
2023-10-02 15:59 ` [PATCH 4/7] scsi: Use scsi_device as argument to eh_device_reset_handler() Hannes Reinecke
2023-10-05 12:59 ` John Garry
2023-10-10 12:12 ` Wenchao Hao
2023-10-10 12:16 ` Hannes Reinecke
2023-10-02 15:59 ` [PATCH 5/7] scsi: Do not allocate scsi command in scsi_ioctl_reset() Hannes Reinecke
2023-10-03 17:39 ` Bart Van Assche
2023-10-02 15:59 ` [PATCH 6/7] scsi: remove SUBMITTED_BY_SCSI_RESET_IOCTL Hannes Reinecke
2023-10-03 17:34 ` Bart Van Assche
2023-10-02 15:59 ` [PATCH 7/7] scsi_error: streamline scsi_eh_bus_device_reset() Hannes Reinecke
2023-10-03 17:45 ` Bart Van Assche
2023-10-04 6:50 ` Hannes Reinecke
2023-10-12 7:48 ` Wenchao Hao
2023-10-12 7:54 ` Wenchao Hao
2023-10-04 6:20 ` [PATCHv5 0/7] scsi: EH rework, main part Johannes Thumshirn
2023-10-11 16:35 ` Wenchao Hao
2023-10-12 6:19 ` Hannes Reinecke
2023-10-12 7:30 ` Wenchao Hao
-- strict thread matches above, loose matches on Subject: below --
2022-05-02 21:59 [PATCH 0/7] scsi: EH rework " Hannes Reinecke
2022-05-02 21:59 ` [PATCH 2/7] scsi: Use Scsi_Host and channel number as argument for eh_bus_reset_handler() Hannes Reinecke
2022-05-03 21:12 ` Lee Duncan
2022-05-06 11:20 ` Steffen Maier
2022-05-06 15:29 ` Sathya Prakash Veerichetty
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b2182011-258c-cdda-2998-4e98fa510bba@oracle.com \
--to=john.g.garry@oracle.com \
--cc=hare@suse.de \
--cc=hch@lst.de \
--cc=james.bottomley@hansenpartnership.com \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox