Re: [PATCH 17/24] snic: reserve tag for TMF

[John Garry <john.garry@xxxxxxxxxx>]

On 07/05/2022 08:32, Hannes Reinecke wrote:
> On 5/6/22 06:28, John Garry wrote:
> > > diff --git a/drivers/scsi/snic/snic_main.c 
> > > b/drivers/scsi/snic/snic_main.c
> > > index 29d56396058c..f344cbc27923 100644
> > > --- a/drivers/scsi/snic/snic_main.c
> > > +++ b/drivers/scsi/snic/snic_main.c
> > > @@ -512,6 +512,9 @@ snic_probe(struct pci_dev *pdev, const struct 
> > > pci_device_id *ent)
> > >                        max_t(u32, SNIC_MIN_IO_REQ, max_ios));
> > >       snic->max_tag_id = shost->can_queue;
> > > +    /* Reserve one reset command */
> > > +    shost->can_queue--;
> > > +    snic->tmf_tag_id = shost->can_queue;
> > >       shost->max_lun = snic->config.luns_per_tgt;
> > >       shost->max_id = SNIC_MAX_TARGET;
> > > diff --git a/drivers/scsi/snic/snic_scsi.c 
> > > b/drivers/scsi/snic/snic_scsi.c
> > > index 5f17666f3e1d..e18c8c5e4b46 100644
> > > --- a/drivers/scsi/snic/snic_scsi.c
> > > +++ b/drivers/scsi/snic/snic_scsi.c
> > > @@ -1018,17 +1018,6 @@ snic_hba_reset_cmpl_handler(struct snic *snic, 
> > > struct snic_fw_req *fwreq)
> > >                 "reset_cmpl: type = %x, hdr_stat = %x, cmnd_id = %x, 
> > > hid = %x, ctx = %lx\n",
> > >                 typ, hdr_stat, cmnd_id, hid, ctx);
> > > -    /* spl case, host reset issued through ioctl */
> > > -    if (cmnd_id == SCSI_NO_TAG) {
> > > -        rqi = (struct snic_req_info *) ctx;
> > > -        SNIC_HOST_INFO(snic->shost,
> > > -                   "reset_cmpl:Tag %d ctx %lx cmpl stat %s\n",
> > > -                   cmnd_id, ctx, snic_io_status_to_str(hdr_stat));
> > > -        sc = rqi->sc;
> > > -
> > > -        goto ioctl_hba_rst;
> > > -    }
> > > -
> > >       if (cmnd_id >= snic->max_tag_id) {
> > >           SNIC_HOST_ERR(snic->shost,
> > >                     "reset_cmpl: Tag 0x%x out of Range,HdrStat %s\n",
> > > @@ -1039,7 +1028,6 @@ snic_hba_reset_cmpl_handler(struct snic *snic, 
> > > struct snic_fw_req *fwreq)
> > >       }
> > >       sc = scsi_host_find_tag(snic->shost, cmnd_id);
> > > -ioctl_hba_rst:
> > >       if (!sc) {
> > >           atomic64_inc(&snic->s_stats.io.sc_null);
> > >           SNIC_HOST_ERR(snic->shost,
> > > @@ -1725,7 +1713,7 @@ snic_dr_clean_single_req(struct snic *snic,
> > >   {
> > >       struct snic_req_info *rqi = NULL;
> > >       struct snic_tgt *tgt = NULL;
> > > -    struct scsi_cmnd *sc = NULL;
> > > +    struct scsi_cmnd *sc;
> > >       spinlock_t *io_lock = NULL;
> > >       u32 sv_state = 0, tmf = 0;
> > >       DECLARE_COMPLETION_ONSTACK(tm_done);
> > > @@ -2238,13 +2226,6 @@ snic_issue_hba_reset(struct snic *snic, struct 
> > > scsi_cmnd *sc)
> > >           goto hba_rst_end;
> > >       }
> > > -    if (snic_cmd_tag(sc) == SCSI_NO_TAG) {
> > > -        memset(scsi_cmd_priv(sc), 0,
> > > -            sizeof(struct snic_internal_io_state));
> > > -        SNIC_HOST_INFO(snic->shost, "issu_hr:Host reset thru 
> > > ioctl.\n");
> > > -        rqi->sc = sc;
> > > -    }
> > > -
> > >       req = rqi_to_req(rqi);
> > >       io_lock = snic_io_lock_hash(snic, sc);
> > > @@ -2319,11 +2300,13 @@ snic_issue_hba_reset(struct snic *snic, 
> > > struct scsi_cmnd *sc)
> > >   } /* end of snic_issue_hba_reset */
> > >   int
> > > -snic_reset(struct Scsi_Host *shost, struct scsi_cmnd *sc)
> > > +snic_reset(struct Scsi_Host *shost)
> > >   {
> > >       struct snic *snic = shost_priv(shost);
> > > +    struct scsi_cmnd *sc = NULL;
> > >       enum snic_state sv_state;
> > >       unsigned long flags;
> > > +    u32 start_time  = jiffies;
> > >       int ret = FAILED;
> > >       /* Set snic state as SNIC_FWRESET*/
> > > @@ -2348,6 +2331,18 @@ snic_reset(struct Scsi_Host *shost, struct 
> > > scsi_cmnd *sc)
> > >       while (atomic_read(&snic->ios_inflight))
> > >           schedule_timeout(msecs_to_jiffies(1));
> > > +    sc = scsi_host_find_tag(shost, snic->tmf_tag_id);
> >
> > Maybe I am missing something but this does not seem right. As I see, 
> > blk-mq has driver tags in range [0, snic->tmf_tag_id - 1], so we 
> > cannot call scsi_host_find_tag() -> 
> > blk_mq_unique_tag_to_tag(snic->tmf_tag_id)

Hi Hannes,

> We do decrease 'can_queue' by '1' just at the start of this patch, hence
> the last tag is always available for TMF.

So has this code changed this here:
https://lore.kernel.org/linux-scsi/de4a7479-3dbf-0842-f8f7-5d82b8bd6ea6@xxxxxxx/

At a glance it looks the same.

Thanks,
John