On Tue, Feb 26, 2019 at 12:38 AM Hannes Reinecke <hare@xxxxxxx> wrote: > > On 2/21/19 1:11 PM, Sreekanth Reddy wrote: > > During expander reset handling, the driver invokes kernel function > > scsi_host_find_tag() to obtain outstanding requests associated with > > the scsi host managed by the driver. Kernel’s block layer may return > > stale entry for one or more outstanding requests if blk-mq is enabled. > > This may lead to Kernel panic if the returned value is inaccessible or > > the memory pointed by the returned value is reused. > > > > Reference of upstream discussion - > > https://patchwork.kernel.org/patch/10734933/ > > > > So to fix this issue, driver will use scsi lookup table to track > > outstanding IOs at driver level and it avoids using scsi_host_find_tag(). > > > > Have done following changes in this patch, > > * Allocated & initialized scsi_lookup table of type > > (struct scsiio_tracker) and of depth host's can_queue at driver load time > > and it will be deallocated at driver unload time. > > > > * Once scmd is received, driver will take scsiio_tracker at entry > > corresponding to scmd's tag value from scsi_lookup table. Then this > > scsiio_tracker entry is initialized with proper smid, scmd, cb_idx etc. > > And this scsiio_tracker entry contents are cleared before driver > > calling scsi_done callback function. > > > > * scmd's host_scribble variable is used to save the corresponding > > scsiio_tracker address, later at any time driver can easily retrieve the > > scmd's corresponding scsiio_tacker using this host_scribble variable. > > > > * Whenever driver wants to get the outstanding IOs at the driver level > > then driver can go through this scsi_lookup table and if it observe > > any entry with non-null scmd then it means that scmd is outstanding > > at the driver level. > > > > v1 change set: > > Updated the patch description. > > > > Cc: <stable@xxxxxxxxxxxxxxx> > > Signed-off-by: Sreekanth Reddy <sreekanth.reddy@xxxxxxxxxxxx> > > --- > > drivers/scsi/mpt3sas/mpt3sas_base.c | 45 +++++++++++++++++++++++++++++--- > > drivers/scsi/mpt3sas/mpt3sas_base.h | 10 +++++++ > > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 2 +- > > drivers/scsi/mpt3sas/mpt3sas_scsih.c | 16 +++++------- > > drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 2 +- > > 5 files changed, 60 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c > > index 0a6cb8f..8e3d0d5 100644 > > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > > @@ -1301,7 +1301,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) > > > > cmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid); > > if (cmd) > > - return scsi_cmd_priv(cmd); > > + return mpt3sas_get_st_from_scmd(cmd); > > > > return NULL; > > } > > @@ -1709,7 +1709,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) > > struct scsi_cmnd *scmd) > > { > > struct chain_tracker *chain_req; > > - struct scsiio_tracker *st = scsi_cmd_priv(scmd); > > + struct scsiio_tracker *st = mpt3sas_get_st_from_scmd(scmd); > > u16 smid = st->smid; > > u8 chain_offset = > > atomic_read(&ioc->chain_lookup[smid - 1].chain_offset); > > @@ -3203,14 +3203,18 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) > > mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx, > > struct scsi_cmnd *scmd) > > { > > - struct scsiio_tracker *request = scsi_cmd_priv(scmd); > > + struct scsiio_tracker *request; > > unsigned int tag = scmd->request->tag; > > u16 smid; > > > > + scmd->host_scribble = (unsigned char *)(&ioc->scsi_lookup[tag]); > > + request = mpt3sas_get_st_from_scmd(scmd); > > + > > smid = tag + 1; > > request->cb_idx = cb_idx; > > request->msix_io = _base_get_msix_index(ioc); > > request->smid = smid; > > + request->scmd = scmd; > > INIT_LIST_HEAD(&request->chain_list); > > return smid; > > } > > @@ -3264,6 +3268,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > > return; > > st->cb_idx = 0xFF; > > st->direct_io = 0; > > + st->scmd = NULL; > > atomic_set(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); > > st->smid = 0; > > } > > @@ -4252,6 +4257,11 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > > ioc->config_page, ioc->config_page_dma); > > } > > > > + if (ioc->scsi_lookup) { > > + free_pages((ulong)ioc->scsi_lookup, ioc->scsi_lookup_pages); > > + ioc->scsi_lookup = NULL; > > + } > > + > > kfree(ioc->hpr_lookup); > > kfree(ioc->internal_lookup); > > if (ioc->chain_lookup) { > > @@ -4377,6 +4387,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > > max_request_credit = min_t(u16, facts->RequestCredit, > > MAX_HBA_QUEUE_DEPTH); > > > > +retry: > > /* Firmware maintains additional facts->HighPriorityCredit number of > > * credits for HiPriprity Request messages, so hba queue depth will be > > * sum of max_request_credit and high priority queue depth. > > @@ -4581,9 +4592,29 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > > (unsigned long long)ioc->request_dma)); > > total_sz += sz; > > > > + sz = ioc->shost->can_queue * sizeof(struct scsiio_tracker); > > + ioc->scsi_lookup_pages = get_order(sz); > > + ioc->scsi_lookup = (struct scsiio_tracker *)__get_free_pages( > > + GFP_KERNEL, ioc->scsi_lookup_pages); > > + if (!ioc->scsi_lookup) { > > + /* Retry allocating memory by reducing the queue depth */ > > + if ((max_request_credit - 64) > > > + (ioc->internal_depth + INTERNAL_SCSIIO_CMDS_COUNT)) { > > + max_request_credit -= 64; > > + _base_release_memory_pools(ioc); > > + goto retry; > > + } else { > > + ioc_err(ioc, > > + "scsi_lookup: get_free_pages failed, sz(%d)\n", > > + (int)sz); > > + goto out; > > + } > > + } > > + > > dinitprintk(ioc, > > ioc_info(ioc, "scsiio(0x%p): depth(%d)\n", > > ioc->request, ioc->scsiio_depth)); > > + total_sz += sz; > > > > ioc->chain_depth = min_t(u32, ioc->chain_depth, MAX_CHAIN_DEPTH); > > sz = ioc->scsiio_depth * sizeof(struct chain_lookup); > > @@ -6239,6 +6270,14 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > > > > spin_lock_irqsave(&ioc->scsi_lookup_lock, flags); > > > > + smid = 1; > > + for (i = 0; i < ioc->shost->can_queue; i++, smid++) { > > + ioc->scsi_lookup[i].cb_idx = 0xFF; > > + ioc->scsi_lookup[i].smid = smid; > > + ioc->scsi_lookup[i].scmd = NULL; > > + ioc->scsi_lookup[i].direct_io = 0; > > + } > > + > > /* hi-priority queue */ > > INIT_LIST_HEAD(&ioc->hpr_free_list); > > smid = ioc->hi_priority_smid; > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h > > index 19158cb..f8c82f6 100644 > > --- a/drivers/scsi/mpt3sas/mpt3sas_base.h > > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h > > @@ -816,6 +816,7 @@ struct chain_lookup { > > /** > > * struct scsiio_tracker - scsi mf request tracker > > * @smid: system message id > > + * @scmd: scsi request pointer > > * @cb_idx: callback index > > * @direct_io: To indicate whether I/O is direct (WARPDRIVE) > > * @chain_list: list of associated firmware chain tracker > > @@ -823,6 +824,7 @@ struct chain_lookup { > > */ > > struct scsiio_tracker { > > u16 smid; > > + struct scsi_cmnd *scmd; > > u8 cb_idx; > > u8 direct_io; > > struct pcie_sg_list pcie_sg_list; > > @@ -830,6 +832,12 @@ struct scsiio_tracker { > > u16 msix_io; > > }; > > > > +static inline struct scsiio_tracker * > > +mpt3sas_get_st_from_scmd(struct scsi_cmnd *scmd) > > +{ > > + return (struct scsiio_tracker *)scmd->host_scribble; > > +} > > + > > /** > > * struct request_tracker - firmware request tracker > > * @smid: system message id > > @@ -1296,6 +1304,8 @@ struct MPT3SAS_ADAPTER { > > u8 *request; > > dma_addr_t request_dma; > > u32 request_dma_sz; > > + struct scsiio_tracker *scsi_lookup; > > + ulong scsi_lookup_pages; > > struct pcie_sg_list *pcie_sg_lookup; > > spinlock_t scsi_lookup_lock; > > int pending_io_count; > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > index b2bb47c..ad43e60 100644 > > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > @@ -595,7 +595,7 @@ void mpt3sas_ctl_reset_done_handler(struct MPT3SAS_ADAPTER *ioc) > > continue; > > if (priv_data->sas_target->handle != handle) > > continue; > > - st = scsi_cmd_priv(scmd); > > + st = mpt3sas_get_st_from_scmd(scmd); > > tm_request->TaskMID = cpu_to_le16(st->smid); > > found = 1; > > } > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c > > index 8bb5b8f..86d0e3c 100644 > > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c > > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c > > @@ -1465,11 +1465,9 @@ struct scsi_cmnd * > > > > if (smid > 0 && > > smid <= ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT) { > > - u32 unique_tag = smid - 1; > > - > > - scmd = scsi_host_find_tag(ioc->shost, unique_tag); > > + scmd = ioc->scsi_lookup[smid - 1].scmd; > > if (scmd) { > > - st = scsi_cmd_priv(scmd); > > + st = mpt3sas_get_st_from_scmd(scmd); > > if (st->cb_idx == 0xFF || st->smid == 0) > > scmd = NULL; > > } > > @@ -2819,7 +2817,7 @@ int mpt3sas_scsih_issue_locked_tm(struct MPT3SAS_ADAPTER *ioc, u16 handle, > > { > > struct MPT3SAS_ADAPTER *ioc = shost_priv(scmd->device->host); > > struct MPT3SAS_DEVICE *sas_device_priv_data; > > - struct scsiio_tracker *st = scsi_cmd_priv(scmd); > > + struct scsiio_tracker *st = mpt3sas_get_st_from_scmd(scmd); > > u16 handle; > > int r; > > > > @@ -4466,7 +4464,7 @@ static int _scsih_set_satl_pending(struct scsi_cmnd *scmd, bool pending) > > continue; > > count++; > > _scsih_set_satl_pending(scmd, false); > > - st = scsi_cmd_priv(scmd); > > + st = mpt3sas_get_st_from_scmd(scmd); > > mpt3sas_base_clear_st(ioc, st); > > scsi_dma_unmap(scmd); > > if (ioc->pci_error_recovery || ioc->remove_host) > > @@ -5193,7 +5191,7 @@ static int _scsih_set_satl_pending(struct scsi_cmnd *scmd, bool pending) > > * WARPDRIVE: If direct_io is set then it is directIO, > > * the failed direct I/O should be redirected to volume > > */ > > - st = scsi_cmd_priv(scmd); > > + st = mpt3sas_get_st_from_scmd(scmd); > > if (st->direct_io && > > ((ioc_status & MPI2_IOCSTATUS_MASK) > > != MPI2_IOCSTATUS_SCSI_TASK_TERMINATED)) { > > @@ -7335,7 +7333,7 @@ static int _scsih_set_satl_pending(struct scsi_cmnd *scmd, bool pending) > > scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid); > > if (!scmd) > > continue; > > - st = scsi_cmd_priv(scmd); > > + st = mpt3sas_get_st_from_scmd(scmd); > > sdev = scmd->device; > > sas_device_priv_data = sdev->hostdata; > > if (!sas_device_priv_data || !sas_device_priv_data->sas_target) > > @@ -10176,7 +10174,6 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, > > .shost_attrs = mpt3sas_host_attrs, > > .sdev_attrs = mpt3sas_dev_attrs, > > .track_queue_depth = 1, > > - .cmd_size = sizeof(struct scsiio_tracker), > > }; > > > > /* raid transport support for SAS 2.0 HBA devices */ > > @@ -10214,7 +10211,6 @@ static void pcie_device_make_active(struct MPT3SAS_ADAPTER *ioc, > > .shost_attrs = mpt3sas_host_attrs, > > .sdev_attrs = mpt3sas_dev_attrs, > > .track_queue_depth = 1, > > - .cmd_size = sizeof(struct scsiio_tracker), > > }; > > > > /* raid transport support for SAS 3.0 HBA devices */ > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_warpdrive.c b/drivers/scsi/mpt3sas/mpt3sas_warpdrive.c > > index cc07ba4..2a05bf3 100644 > > --- a/drivers/scsi/mpt3sas/mpt3sas_warpdrive.c > > +++ b/drivers/scsi/mpt3sas/mpt3sas_warpdrive.c > > @@ -259,7 +259,7 @@ > > sector_t v_lba, p_lba, stripe_off, column, io_size; > > u32 stripe_sz, stripe_exp; > > u8 num_pds, cmd = scmd->cmnd[0]; > > - struct scsiio_tracker *st = scsi_cmd_priv(scmd); > > + struct scsiio_tracker *st = mpt3sas_get_st_from_scmd(scmd); > > > > if (cmd != READ_10 && cmd != WRITE_10 && > > cmd != READ_16 && cmd != WRITE_16) > > > I still think this is the wrong way of approaching it. > I'd rather using blk_mq_tagset_busy_iter() here; that would avoid the > issue mentioned. > > Let's see if I can come up with a patch. Hannes, This API blk_mq_tagset_busy_iter() is not flexible to accommodate it into mpt3sas driver code. For example driver can’t exit from this API in-between if need, i.e. While processing Async Broadcast primitive event if host reset occurs then driver has to wait for this blk_mq_tagset_busy_iter() API to call the callback function for all the outstanding requests and no way to quit in-between. Thanks, Sreekanth > > Cheers, > > Hannes
