| Message ID | 20220503200704.88003-1-hare@suse.de |
|---|---|
| Headers | show |
| Series | scsi: EH rework prep patches, part 1 | expand |
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@suse.de> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > 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 (...) ...