| Message ID | 1551702395-15526-1-git-send-email-sreekanth.reddy@broadcom.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | c2fe742ff6e77c5b4fe4ad273191ddf28fdea25e |
| Headers | show |
| Series | mpt3sas: Fix kernel panic occurs during expander reset | expand |
Hannes & Christoph: Please comment on Sreekanth's proposed approach. > 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. Driver loops from tag value zero > to hba queue depth to obtain the outstanding scmds. But when blk-mq is > enabled then Kernel’s block layer may return stale entry for one or > more requests. 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/ > > Fix: Instead of calling scsi_host_find_tag() API for each and every > smid(smid is tag +1) from one to shost->can_queue, now driver will > call this API (to obtain the outstanding scmd) for only those smid's > which are outstanding at the driver level. > > Driver will determine whether this smid is outstanding at driver level > by looking into it's corresponding MPI request frame, if it's MPI > request frame is empty then it means that this smid is free and no > need to call scsi_host_find_tag() API for this smid. By doing this > driver will invoke scsi_host_find_tag() for only those tags which are > outstanding at the driver level. > > Driver will check whether particular MPI request frame is empty or not > by looking into the "DevHandle" field. If this field is zero then it > means that this MPI request is empty. For active MPI request DevHandle > must be non-zero. > > Also driver will memset the MPI request frame once the corresponding > scmd is processed (i.e. just before calling scmd->done function).
On Wed, Mar 06, 2019 at 12:49:55PM -0500, Martin K. Petersen wrote: > > Hannes & Christoph: Please comment on Sreekanth's proposed approach. Iterating over all tags from the driver is always wrong. We've been though this a few times.
On Fri, Mar 8, 2019 at 6:52 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Mar 06, 2019 at 12:49:55PM -0500, Martin K. Petersen wrote: > > > > Hannes & Christoph: Please comment on Sreekanth's proposed approach. > > Iterating over all tags from the driver is always wrong. We've been > though this a few times. Current issue is very easy to be reproduced and it is widely impacted. We proposed this approach i.e. invoking scsi_host_find_tag() for only those tags which are outstanding at the driver level; as this has very minimal code changes without impacting any design and also it will work in both non-mq + mq mode. We can rework on those code sections where driver is iterating over all tags. I understood from your reply that - "Low level driver should not have any requirement to loop outstanding IOs". Not sure if such things can be done without SML support. AFAIK, similar issue is very generic and many low level scsi driver has similar requirement. Can we go with current solution assuming any new interface as you requested can be done as separate activity? Thanks, Sreekanth
On 3/11/19 10:17 AM, Sreekanth Reddy wrote: > On Fri, Mar 8, 2019 at 6:52 PM Christoph Hellwig <hch@infradead.org> wrote: >> >> On Wed, Mar 06, 2019 at 12:49:55PM -0500, Martin K. Petersen wrote: >>> >>> Hannes & Christoph: Please comment on Sreekanth's proposed approach. >> >> Iterating over all tags from the driver is always wrong. We've been >> though this a few times. > > Current issue is very easy to be reproduced and it is widely impacted. > We proposed this approach i.e. invoking scsi_host_find_tag() for only > those tags which are outstanding at the driver level; as this has > very minimal code changes without impacting any design and also it > will work in both non-mq + mq mode. > We can rework on those code sections where driver is iterating over > all tags. I understood from your reply that - "Low level driver should > not have any requirement to loop outstanding IOs". Not sure if such > things can be done without SML support. AFAIK, similar issue is very > generic and many low level scsi driver has similar requirement. > > Can we go with current solution assuming any new interface as you > requested can be done as separate activity? > > Thanks, > Sreekanth > In context of this issue (in my case kernel panics on shutdown that I mentioned in another mail some time ago) - which patch should I be using (even if temporarily) ? Currently I'm on https://patchwork.kernel.org/patch/10829927/ .
On Tue, Mar 12, 2019 at 3:17 PM Michal Soltys <soltys@ziu.info> wrote: > > On 3/11/19 10:17 AM, Sreekanth Reddy wrote: > > On Fri, Mar 8, 2019 at 6:52 PM Christoph Hellwig <hch@infradead.org> wrote: > >> > >> On Wed, Mar 06, 2019 at 12:49:55PM -0500, Martin K. Petersen wrote: > >>> > >>> Hannes & Christoph: Please comment on Sreekanth's proposed approach. > >> > >> Iterating over all tags from the driver is always wrong. We've been > >> though this a few times. > > > > Current issue is very easy to be reproduced and it is widely impacted. > > We proposed this approach i.e. invoking scsi_host_find_tag() for only > > those tags which are outstanding at the driver level; as this has > > very minimal code changes without impacting any design and also it > > will work in both non-mq + mq mode. > > We can rework on those code sections where driver is iterating over > > all tags. I understood from your reply that - "Low level driver should > > not have any requirement to loop outstanding IOs". Not sure if such > > things can be done without SML support. AFAIK, similar issue is very > > generic and many low level scsi driver has similar requirement. > > > > Can we go with current solution assuming any new interface as you > > requested can be done as separate activity? > > > > Thanks, > > Sreekanth > > > > In context of this issue (in my case kernel panics on shutdown that I > mentioned in another mail some time ago) - which patch should I be using > (even if temporarily) ? Currently I'm on > https://patchwork.kernel.org/patch/10829927/ . Please use below patch, https://patchwork.kernel.org/patch/10837777/ Chris, Hannes, Just a gentle ping.. This patch will just fix this kernel panic which are observed during expander resets, system shutdown or driver unload operation. It has a very minimal code change without impacting any design. Many customers are observing this issue. Please consider this patch. As I mentioned in the above mail that we can rework on those code sections where driver is iterating over all tags. Thanks, Sreekanth >
On Tue, Mar 12, 2019 at 3:46 PM Sreekanth Reddy <sreekanth.reddy@broadcom.com> wrote: > > On Tue, Mar 12, 2019 at 3:17 PM Michal Soltys <soltys@ziu.info> wrote: > > > > On 3/11/19 10:17 AM, Sreekanth Reddy wrote: > > > On Fri, Mar 8, 2019 at 6:52 PM Christoph Hellwig <hch@infradead.org> wrote: > > >> > > >> On Wed, Mar 06, 2019 at 12:49:55PM -0500, Martin K. Petersen wrote: > > >>> > > >>> Hannes & Christoph: Please comment on Sreekanth's proposed approach. > > >> > > >> Iterating over all tags from the driver is always wrong. We've been > > >> though this a few times. > > > > > > Current issue is very easy to be reproduced and it is widely impacted. > > > We proposed this approach i.e. invoking scsi_host_find_tag() for only > > > those tags which are outstanding at the driver level; as this has > > > very minimal code changes without impacting any design and also it > > > will work in both non-mq + mq mode. > > > We can rework on those code sections where driver is iterating over > > > all tags. I understood from your reply that - "Low level driver should > > > not have any requirement to loop outstanding IOs". Not sure if such > > > things can be done without SML support. AFAIK, similar issue is very > > > generic and many low level scsi driver has similar requirement. > > > > > > Can we go with current solution assuming any new interface as you > > > requested can be done as separate activity? > > > > > > Thanks, > > > Sreekanth > > > > > > > In context of this issue (in my case kernel panics on shutdown that I > > mentioned in another mail some time ago) - which patch should I be using > > (even if temporarily) ? Currently I'm on > > https://patchwork.kernel.org/patch/10829927/ . > > Please use below patch, > https://patchwork.kernel.org/patch/10837777/ > > Chris, Hannes, > Just a gentle ping.. > This patch will just fix this kernel panic which are observed during > expander resets, system shutdown or driver unload operation. > It has a very minimal code change without impacting any design. > Many customers are observing this issue. Please consider this patch. > > As I mentioned in the above mail that we can rework on those code > sections where driver is iterating over all tags. Just a gentle ping.. Regards, Sreekanth > > Thanks, > Sreekanth > > >
On Wed, Mar 13, 2019 at 10:43 PM Sreekanth Reddy <sreekanth.reddy@broadcom.com> wrote: > > On Tue, Mar 12, 2019 at 3:46 PM Sreekanth Reddy > <sreekanth.reddy@broadcom.com> wrote: > > > > On Tue, Mar 12, 2019 at 3:17 PM Michal Soltys <soltys@ziu.info> wrote: > > > > > > On 3/11/19 10:17 AM, Sreekanth Reddy wrote: > > > > On Fri, Mar 8, 2019 at 6:52 PM Christoph Hellwig <hch@infradead.org> wrote: > > > >> > > > >> On Wed, Mar 06, 2019 at 12:49:55PM -0500, Martin K. Petersen wrote: > > > >>> > > > >>> Hannes & Christoph: Please comment on Sreekanth's proposed approach. > > > >> > > > >> Iterating over all tags from the driver is always wrong. We've been > > > >> though this a few times. > > > > > > > > Current issue is very easy to be reproduced and it is widely impacted. > > > > We proposed this approach i.e. invoking scsi_host_find_tag() for only > > > > those tags which are outstanding at the driver level; as this has > > > > very minimal code changes without impacting any design and also it > > > > will work in both non-mq + mq mode. > > > > We can rework on those code sections where driver is iterating over > > > > all tags. I understood from your reply that - "Low level driver should > > > > not have any requirement to loop outstanding IOs". Not sure if such > > > > things can be done without SML support. AFAIK, similar issue is very > > > > generic and many low level scsi driver has similar requirement. > > > > > > > > Can we go with current solution assuming any new interface as you > > > > requested can be done as separate activity? > > > > > > > > Thanks, > > > > Sreekanth > > > > > > > > > > In context of this issue (in my case kernel panics on shutdown that I > > > mentioned in another mail some time ago) - which patch should I be using > > > (even if temporarily) ? Currently I'm on > > > https://patchwork.kernel.org/patch/10829927/ . > > > > Please use below patch, > > https://patchwork.kernel.org/patch/10837777/ > > > > Chris, Hannes, > > Just a gentle ping.. > > This patch will just fix this kernel panic which are observed during > > expander resets, system shutdown or driver unload operation. > > It has a very minimal code change without impacting any design. > > Many customers are observing this issue. Please consider this patch. > > > > As I mentioned in the above mail that we can rework on those code > > sections where driver is iterating over all tags. > > Just a gentle ping.. Hi All, Any update here. > > Regards, > Sreekanth > > > > > Thanks, > > Sreekanth > > > > >
> > > > >>> Hannes & Christoph: Please comment on Sreekanth's proposed > approach. > > > > >> > > > > >> Iterating over all tags from the driver is always wrong. We've > > > > >> been though this a few times. > > > > > > > > > > Current issue is very easy to be reproduced and it is widely > > > > > impacted. > > > > > We proposed this approach i.e. invoking scsi_host_find_tag() for > > > > > only those tags which are outstanding at the driver level; as > > > > > this has very minimal code changes without impacting any design > > > > > and also it will work in both non-mq + mq mode. > > > > > We can rework on those code sections where driver is iterating > > > > > over all tags. I understood from your reply that - "Low level > > > > > driver should not have any requirement to loop outstanding IOs". > > > > > Not sure if such things can be done without SML support. AFAIK, > > > > > similar issue is very generic and many low level scsi driver has > > > > > similar > requirement. > > > > > > > > > > Can we go with current solution assuming any new interface as > > > > > you requested can be done as separate activity? Hi Martin, Christopher, Can you please consider latest fix since it is a multiple field issue and it is critical. We can work on further improvement as Christopher mentioned in his last comment. Driver finding total firmware outstanding command is very common in lots of SCSI driver, so we may have to figure out what best mid layer can provide which can avoid driver work. Kashyap > > > > > > > > > > Thanks, > > > > > Sreekanth > > > > > > > > > > > > > In context of this issue (in my case kernel panics on shutdown > > > > that I mentioned in another mail some time ago) - which patch > > > > should I be using (even if temporarily) ? Currently I'm on > > > > https://patchwork.kernel.org/patch/10829927/ . > > > > > > Please use below patch, > > > https://patchwork.kernel.org/patch/10837777/ > > > > > > Chris, Hannes, > > > Just a gentle ping.. > > > This patch will just fix this kernel panic which are observed during > > > expander resets, system shutdown or driver unload operation. > > > It has a very minimal code change without impacting any design. > > > Many customers are observing this issue. Please consider this patch. > > > > > > As I mentioned in the above mail that we can rework on those code > > > sections where driver is iterating over all tags. > > > > Just a gentle ping.. > > Hi All, Any update here. > > > > > Regards, > > Sreekanth > > > > > > > > Thanks, > > > Sreekanth > > > > > > >
On 3/21/19 7:25 PM, Kashyap Desai wrote: >>>>>>>> Hannes & Christoph: Please comment on Sreekanth's proposed >> approach. >>>>>>> >>>>>>> Iterating over all tags from the driver is always wrong. We've >>>>>>> been though this a few times. >>>>>> >>>>>> Current issue is very easy to be reproduced and it is widely >>>>>> impacted. >>>>>> We proposed this approach i.e. invoking scsi_host_find_tag() for >>>>>> only those tags which are outstanding at the driver level; as >>>>>> this has very minimal code changes without impacting any design >>>>>> and also it will work in both non-mq + mq mode. >>>>>> We can rework on those code sections where driver is iterating >>>>>> over all tags. I understood from your reply that - "Low level >>>>>> driver should not have any requirement to loop outstanding IOs". >>>>>> Not sure if such things can be done without SML support. AFAIK, >>>>>> similar issue is very generic and many low level scsi driver has >>>>>> similar >> requirement. >>>>>> >>>>>> Can we go with current solution assuming any new interface as >>>>>> you requested can be done as separate activity? > > After reconsideration, and seeing that the required interface (namely blk_mq_tagset_busy_iter()) is undergoing changes currently, I'm fine with the patch. Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c index e577744..1d8c584 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_base.c +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -3281,12 +3281,18 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, if (smid < ioc->hi_priority_smid) { struct scsiio_tracker *st; + void *request; st = _get_st_from_smid(ioc, smid); if (!st) { _base_recovery_check(ioc); return; } + + /* Clear MPI request frame */ + request = mpt3sas_base_get_msg_frame(ioc, smid); + memset(request, 0, ioc->request_sz); + mpt3sas_base_clear_st(ioc, st); _base_recovery_check(ioc); return; diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c index 8bb5b8f..1ccfbc7 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c @@ -1462,11 +1462,23 @@ struct scsi_cmnd * { struct scsi_cmnd *scmd = NULL; struct scsiio_tracker *st; + Mpi25SCSIIORequest_t *mpi_request; if (smid > 0 && smid <= ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT) { u32 unique_tag = smid - 1; + mpi_request = mpt3sas_base_get_msg_frame(ioc, smid); + + /* + * If SCSI IO request is outstanding at driver level then + * DevHandle filed must be non-zero. If DevHandle is zero + * then it means that this smid is free at driver level, + * so return NULL. + */ + if (!mpi_request->DevHandle) + return scmd; + scmd = scsi_host_find_tag(ioc->shost, unique_tag); if (scmd) { st = scsi_cmd_priv(scmd);
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. Driver loops from tag value zero to hba queue depth to obtain the outstanding scmds. But when blk-mq is enabled then Kernel’s block layer may return stale entry for one or more requests. 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/ Fix: Instead of calling scsi_host_find_tag() API for each and every smid(smid is tag +1) from one to shost->can_queue, now driver will call this API (to obtain the outstanding scmd) for only those smid's which are outstanding at the driver level. Driver will determine whether this smid is outstanding at driver level by looking into it's corresponding MPI request frame, if it's MPI request frame is empty then it means that this smid is free and no need to call scsi_host_find_tag() API for this smid. By doing this driver will invoke scsi_host_find_tag() for only those tags which are outstanding at the driver level. Driver will check whether particular MPI request frame is empty or not by looking into the "DevHandle" field. If this field is zero then it means that this MPI request is empty. For active MPI request DevHandle must be non-zero. Also driver will memset the MPI request frame once the corresponding scmd is processed (i.e. just before calling scmd->done function). Signed-off-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> --- drivers/scsi/mpt3sas/mpt3sas_base.c | 6 ++++++ drivers/scsi/mpt3sas/mpt3sas_scsih.c | 12 ++++++++++++ 2 files changed, 18 insertions(+)