Hi Hannes,
On Tue, 3 May 2022, Hannes Reinecke wrote:
> When sending a device reset we should not take a reference to the
> scsi command.
>
> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
> drivers/scsi/aic7xxx/aic7xxx_osm.c | 102 +++++++++++++++--------------
> 1 file changed, 54 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm.c b/drivers/scsi/aic7xxx/aic7xxx_osm.c
> index 05591650f89f..596d5870b024 100644
> --- a/drivers/scsi/aic7xxx/aic7xxx_osm.c
> +++ b/drivers/scsi/aic7xxx/aic7xxx_osm.c
> @@ -366,7 +366,8 @@ static void ahc_linux_queue_cmd_complete(struct ahc_softc *ahc,
> struct scsi_cmnd *cmd);
> static void ahc_linux_freeze_simq(struct ahc_softc *ahc);
> static void ahc_linux_release_simq(struct ahc_softc *ahc);
> -static int ahc_linux_queue_recovery_cmd(struct scsi_cmnd *cmd, scb_flag flag);
> +static int ahc_linux_queue_recovery_cmd(struct scsi_device *sdev,
> + struct scsi_cmnd *cmd);
> static void ahc_linux_initialize_scsi_bus(struct ahc_softc *ahc);
> static u_int ahc_linux_user_tagdepth(struct ahc_softc *ahc,
> struct ahc_devinfo *devinfo);
> @@ -728,7 +729,7 @@ ahc_linux_abort(struct scsi_cmnd *cmd)
> {
> int error;
>
> - error = ahc_linux_queue_recovery_cmd(cmd, SCB_ABORT);
> + error = ahc_linux_queue_recovery_cmd(cmd->device, cmd);
> if (error != SUCCESS)
> printk("aic7xxx_abort returns 0x%x\n", error);
> return (error);
> @@ -742,7 +743,7 @@ ahc_linux_dev_reset(struct scsi_cmnd *cmd)
> {
> int error;
>
> - error = ahc_linux_queue_recovery_cmd(cmd, SCB_DEVICE_RESET);
> + error = ahc_linux_queue_recovery_cmd(cmd->device, NULL);
> if (error != SUCCESS)
> printk("aic7xxx_dev_reset returns 0x%x\n", error);
> return (error);
> @@ -2036,11 +2037,12 @@ ahc_linux_release_simq(struct ahc_softc *ahc)
> }
>
> static int
> -ahc_linux_queue_recovery_cmd(struct scsi_cmnd *cmd, scb_flag flag)
> +ahc_linux_queue_recovery_cmd(struct scsi_device *sdev,
> + struct scsi_cmnd *cmd)
> {
> struct ahc_softc *ahc;
> struct ahc_linux_device *dev;
> - struct scb *pending_scb;
> + struct scb *pending_scb = NULL, *scb;
> u_int saved_scbptr;
> u_int active_scb_index;
> u_int last_phase;
> @@ -2053,18 +2055,19 @@ ahc_linux_queue_recovery_cmd(struct scsi_cmnd *cmd, scb_flag flag)
> int disconnected;
> unsigned long flags;
>
> - pending_scb = NULL;
> paused = FALSE;
> wait = FALSE;
> - ahc = *(struct ahc_softc **)cmd->device->host->hostdata;
> + ahc = *(struct ahc_softc **)sdev->host->hostdata;
>
> - scmd_printk(KERN_INFO, cmd, "Attempting to queue a%s message\n",
> - flag == SCB_ABORT ? "n ABORT" : " TARGET RESET");
> + sdev_printk(KERN_INFO, sdev, "Attempting to queue a%s message\n",
> + cmd ? "n ABORT" : " TARGET RESET");
>
> - printk("CDB:");
> - for (cdb_byte = 0; cdb_byte < cmd->cmd_len; cdb_byte++)
> - printk(" 0x%x", cmd->cmnd[cdb_byte]);
> - printk("\n");
> + if (cmd) {
> + printk("CDB:");
> + for (cdb_byte = 0; cdb_byte < cmd->cmd_len; cdb_byte++)
> + printk(" 0x%x", cmd->cmnd[cdb_byte]);
> + printk("\n");
> + }
>
> ahc_lock(ahc, &flags);
>
> @@ -2075,7 +2078,7 @@ ahc_linux_queue_recovery_cmd(struct scsi_cmnd *cmd, scb_flag flag)
> * at all, and the system wanted us to just abort the
> * command, return success.
> */
> - dev = scsi_transport_device_data(cmd->device);
> + dev = scsi_transport_device_data(sdev);
>
> if (dev == NULL) {
> /*
> @@ -2083,13 +2086,12 @@ ahc_linux_queue_recovery_cmd(struct scsi_cmnd *cmd, scb_flag flag)
> * so we must not still own the command.
> */
> printk("%s:%d:%d:%d: Is not an active device\n",
> - ahc_name(ahc), cmd->device->channel, cmd->device->id,
> - (u8)cmd->device->lun);
> + ahc_name(ahc), sdev->channel, sdev->id, (u8)sdev->lun);
> retval = SUCCESS;
> goto no_cmd;
> }
>
> - if ((dev->flags & (AHC_DEV_Q_BASIC|AHC_DEV_Q_TAGGED)) == 0
> + if (cmd && (dev->flags & (AHC_DEV_Q_BASIC|AHC_DEV_Q_TAGGED)) == 0
> && ahc_search_untagged_queues(ahc, cmd, cmd->device->id,
> cmd->device->channel + 'A',
> (u8)cmd->device->lun,
> @@ -2104,25 +2106,28 @@ ahc_linux_queue_recovery_cmd(struct scsi_cmnd *cmd, scb_flag flag)
> /*
> * See if we can find a matching cmd in the pending list.
> */
> - LIST_FOREACH(pending_scb, &ahc->pending_scbs, pending_links) {
> - if (pending_scb->io_ctx == cmd)
> + LIST_FOREACH(scb, &ahc->pending_scbs, pending_links) {
> + if (cmd && scb->io_ctx == cmd) {
> + pending_scb = scb;
> break;
> + }
> }
>
> - if (pending_scb == NULL && flag == SCB_DEVICE_RESET) {
> -
> + if (!cmd) {
> /* Any SCB for this device will do for a target reset */
> - LIST_FOREACH(pending_scb, &ahc->pending_scbs, pending_links) {
> - if (ahc_match_scb(ahc, pending_scb, scmd_id(cmd),
> - scmd_channel(cmd) + 'A',
> + LIST_FOREACH(scb, &ahc->pending_scbs, pending_links) {
> + if (ahc_match_scb(ahc, scb, sdev->id,
> + sdev->channel + 'A',
> CAM_LUN_WILDCARD,
> - SCB_LIST_NULL, ROLE_INITIATOR))
> + SCB_LIST_NULL, ROLE_INITIATOR)) {
> + pending_scb = scb;
> break;
> + }
> }
> }
>
A minor style criticism. Wouldn't that be better written as,
if (cmd) {
LIST_FOREACH (...)
...
} else {
LIST_FOREACH (...)
...
}
rather than,
LIST_FOREACH (...)
if (cmd && ...)
...
if (!cmd)
LIST_FOREACH (...)
...
