| Message ID | 1498638793-44672-14-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
>-----Original Message----- >From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi- >owner@vger.kernel.org] On Behalf Of Hannes Reinecke >Sent: Wednesday, June 28, 2017 2:03 PM >To: Christoph Hellwig >Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; Hannes >Reinecke; Hannes Reinecke >Subject: [PATCH 13/47] megaraid: pass in NULL scb for host reset > >When calling a host reset we shouldn't rely on the command triggering the >reset, so allow megaraid_abort_and_reset() to be called with a NULL scb. >And drop the pointless 'bus_reset' and 'target_reset' handlers, which just call >the same function as host_reset. If this patch address any functional issue, then we should consider this. If it's code optimization, can we ignore this as this is being very old driver and no more maintained by Broadcom/LSI ? > >Signed-off-by: Hannes Reinecke <hare@suse.com> >--- > drivers/scsi/megaraid.c | 42 ++++++++++++++++-------------------------- > 1 file changed, 16 insertions(+), 26 deletions(-) > >diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c index >3c63c29..7e504d3 100644 >--- a/drivers/scsi/megaraid.c >+++ b/drivers/scsi/megaraid.c >@@ -1909,7 +1909,7 @@ static DEF_SCSI_QCMD(megaraid_queue) > > spin_lock_irq(&adapter->lock); > >- rval = megaraid_abort_and_reset(adapter, cmd, SCB_RESET); >+ rval = megaraid_abort_and_reset(adapter, NULL, SCB_RESET); If cmd=NULL is passed, it will crash inside function megaraid_abort_and_reset() while dereferencing "cmd" pointer. Below is the code of function megaraid_abort_and_reset() where it will crash- static int megaraid_abort_and_reset(adapter_t *adapter, Scsi_Cmnd *cmd, int aor) { struct list_head *pos, *next; scb_t *scb; dev_warn(&adapter->dev->dev, "%s cmd=%x <c=%d t=%d l=%d>\n", (aor == SCB_ABORT)? "ABORTING":"RESET", cmd->cmnd[0], cmd->device->channel,>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>it should crash here cmd->device->id, (u32)cmd->device->lun); Please correct if I am missing something here. > > /* > * This is required here to complete any completed requests @@ - >1948,7 +1948,7 @@ static DEF_SCSI_QCMD(megaraid_queue) > > scb = list_entry(pos, scb_t, list); > >- if (scb->cmd == cmd) { /* Found command */ >+ if (!cmd || scb->cmd == cmd) { /* Found command */ > > scb->state |= aor; > >@@ -1967,31 +1967,23 @@ static DEF_SCSI_QCMD(megaraid_queue) > > return FAILED; > } >- else { >- >- /* >- * Not yet issued! Remove from the pending >- * list >- */ >- dev_warn(&adapter->dev->dev, >- "%s-[%x], driver owner\n", >- (aor==SCB_ABORT) ? >"ABORTING":"RESET", >- scb->idx); >- >- mega_free_scb(adapter, scb); >- >- if( aor == SCB_ABORT ) { >- cmd->result = (DID_ABORT << 16); >- } >- else { >- cmd->result = (DID_RESET << 16); >- } >+ /* >+ * Not yet issued! Remove from the pending >+ * list >+ */ >+ dev_warn(&adapter->dev->dev, >+ "%s-[%x], driver owner\n", >+ (cmd) ? "ABORTING":"RESET", >+ scb->idx); >+ mega_free_scb(adapter, scb); > >+ if (cmd) { >+ cmd->result = (DID_ABORT << 16); > list_add_tail(SCSI_LIST(cmd), >- &adapter->completed_list); >- >- return SUCCESS; >+ &adapter->completed_list); > } >+ >+ return SUCCESS; > } > } > >@@ -4180,8 +4172,6 @@ static inline void mega_create_proc_entry(int index, >struct proc_dir_entry *pare > .cmd_per_lun = DEF_CMD_PER_LUN, > .use_clustering = ENABLE_CLUSTERING, > .eh_abort_handler = megaraid_abort, >- .eh_device_reset_handler = megaraid_reset, >- .eh_bus_reset_handler = megaraid_reset, > .eh_host_reset_handler = megaraid_reset, > .no_write_same = 1, > }; >-- >1.8.5.6
On 06/28/2017 03:41 PM, Sumit Saxena wrote: >> -----Original Message----- >> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi- >> owner@vger.kernel.org] On Behalf Of Hannes Reinecke >> Sent: Wednesday, June 28, 2017 2:03 PM >> To: Christoph Hellwig >> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; > Hannes >> Reinecke; Hannes Reinecke >> Subject: [PATCH 13/47] megaraid: pass in NULL scb for host reset >> >> When calling a host reset we shouldn't rely on the command triggering the >> reset, so allow megaraid_abort_and_reset() to be called with a NULL scb. >> And drop the pointless 'bus_reset' and 'target_reset' handlers, which > just call >> the same function as host_reset. > > If this patch address any functional issue, then we should consider this. > If it's code optimization, can we ignore this as this is being very old > driver > and no more maintained by Broadcom/LSI ? > Sadly, ignoring is not an option. I'm planning to update the calling convention for SCSI EH, to resolve the long-standing problem with sg_reset ioctls. sg_reset ioctl will allocate an out-of-band SCSI command, which does no longer work with the new command allocation scheme in multiqueue. So it's not possible to just 'ignore' it, as then SCSI EH will cease to function with that driver. Sorry. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> drivers/scsi/megaraid.c | 42 ++++++++++++++++-------------------------- >> 1 file changed, 16 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c index >> 3c63c29..7e504d3 100644 >> --- a/drivers/scsi/megaraid.c >> +++ b/drivers/scsi/megaraid.c >> @@ -1909,7 +1909,7 @@ static DEF_SCSI_QCMD(megaraid_queue) >> >> spin_lock_irq(&adapter->lock); >> >> - rval = megaraid_abort_and_reset(adapter, cmd, SCB_RESET); >> + rval = megaraid_abort_and_reset(adapter, NULL, SCB_RESET); > > If cmd=NULL is passed, it will crash inside function > megaraid_abort_and_reset() while dereferencing "cmd" pointer. > Below is the code of function megaraid_abort_and_reset() where it will > crash- > > static int > megaraid_abort_and_reset(adapter_t *adapter, Scsi_Cmnd *cmd, int aor) > { > struct list_head *pos, *next; > scb_t *scb; > > dev_warn(&adapter->dev->dev, "%s cmd=%x <c=%d t=%d l=%d>\n", > (aor == SCB_ABORT)? "ABORTING":"RESET", > cmd->cmnd[0], > cmd->device->channel,>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>it should crash > here > cmd->device->id, (u32)cmd->device->lun); > > Please correct if I am missing something here.Ah, correct. Will be fixing it up. Cheers, Hannes
> -----Original Message----- > From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi- > owner@vger.kernel.org] On Behalf Of Hannes Reinecke > Sent: Wednesday, June 28, 2017 9:00 PM > To: Sumit Saxena; Christoph Hellwig > Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; > Hannes > Reinecke > Subject: Re: [PATCH 13/47] megaraid: pass in NULL scb for host reset > > On 06/28/2017 03:41 PM, Sumit Saxena wrote: > >> -----Original Message----- > >> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi- > >> owner@vger.kernel.org] On Behalf Of Hannes Reinecke > >> Sent: Wednesday, June 28, 2017 2:03 PM > >> To: Christoph Hellwig > >> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; > > Hannes > >> Reinecke; Hannes Reinecke > >> Subject: [PATCH 13/47] megaraid: pass in NULL scb for host reset > >> > >> When calling a host reset we shouldn't rely on the command triggering > >> the reset, so allow megaraid_abort_and_reset() to be called with a NULL > scb. > >> And drop the pointless 'bus_reset' and 'target_reset' handlers, which > > just call > >> the same function as host_reset. > > > > If this patch address any functional issue, then we should consider > > this. > > If it's code optimization, can we ignore this as this is being very > > old driver and no more maintained by Broadcom/LSI ? > > > Sadly, ignoring is not an option. > I'm planning to update the calling convention for SCSI EH, to resolve the > long- > standing problem with sg_reset ioctls. > sg_reset ioctl will allocate an out-of-band SCSI command, which does no > longer > work with the new command allocation scheme in multiqueue. > So it's not possible to just 'ignore' it, as then SCSI EH will cease to > function with > that driver. Hannes - We are in process of sending megaraid and 3ware driver removal as LSI/Broadcom stopped supporting those products. I agree we should review this closely, but lack of test coverage and end of life cycle of product is requesting us to know the rational. For now, let's consider NACK for this patch. We will be removing old megaraid (mbox) driver and 3Ware drivers soon. > > Sorry. > > >> > >> Signed-off-by: Hannes Reinecke <hare@suse.com> > >> --- > >> drivers/scsi/megaraid.c | 42 > >> ++++++++++++++++-------------------------- > >> 1 file changed, 16 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c index > >> 3c63c29..7e504d3 100644 > >> --- a/drivers/scsi/megaraid.c > >> +++ b/drivers/scsi/megaraid.c > >> @@ -1909,7 +1909,7 @@ static DEF_SCSI_QCMD(megaraid_queue) > >> > >> spin_lock_irq(&adapter->lock); > >> > >> - rval = megaraid_abort_and_reset(adapter, cmd, SCB_RESET); > >> + rval = megaraid_abort_and_reset(adapter, NULL, SCB_RESET); > > > > If cmd=NULL is passed, it will crash inside function > > megaraid_abort_and_reset() while dereferencing "cmd" pointer. > > Below is the code of function megaraid_abort_and_reset() where it > > will > > crash- > > > > static int > > megaraid_abort_and_reset(adapter_t *adapter, Scsi_Cmnd *cmd, int aor) > > { > > struct list_head *pos, *next; > > scb_t *scb; > > > > dev_warn(&adapter->dev->dev, "%s cmd=%x <c=%d t=%d l=%d>\n", > > (aor == SCB_ABORT)? "ABORTING":"RESET", > > cmd->cmnd[0], > > cmd->device->channel,>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>it should > > cmd->device->crash > > here > > cmd->device->id, (u32)cmd->device->lun); > > > > Please correct if I am missing something here.Ah, correct. Will be > > fixing it up. > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Teamlead Storage & Networking > hare@suse.de +49 911 74053 688 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 > (AG > Nürnberg)
On Wed, Jun 28, 2017 at 11:38:22AM -0700, adam radford wrote: > LSI/Broadcom isn't the maintainer of the 3ware drivers. See 'MAINTAINERS'. > Your attempts > to remove them may get a NACK from the Maintainer since I believe there are > still users of these controllers. Same for the legacy megaraid driver, but we can just mark that as orphan for now.
On 06/28/2017 07:40 PM, Kashyap Desai wrote: >> -----Original Message----- >> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi- >> owner@vger.kernel.org] On Behalf Of Hannes Reinecke >> Sent: Wednesday, June 28, 2017 9:00 PM >> To: Sumit Saxena; Christoph Hellwig >> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; >> Hannes >> Reinecke >> Subject: Re: [PATCH 13/47] megaraid: pass in NULL scb for host reset >> >> On 06/28/2017 03:41 PM, Sumit Saxena wrote: >>>> -----Original Message----- >>>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi- >>>> owner@vger.kernel.org] On Behalf Of Hannes Reinecke >>>> Sent: Wednesday, June 28, 2017 2:03 PM >>>> To: Christoph Hellwig >>>> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; >>> Hannes >>>> Reinecke; Hannes Reinecke >>>> Subject: [PATCH 13/47] megaraid: pass in NULL scb for host reset >>>> >>>> When calling a host reset we shouldn't rely on the command triggering >>>> the reset, so allow megaraid_abort_and_reset() to be called with a NULL >> scb. >>>> And drop the pointless 'bus_reset' and 'target_reset' handlers, which >>> just call >>>> the same function as host_reset. >>> >>> If this patch address any functional issue, then we should consider >>> this. >>> If it's code optimization, can we ignore this as this is being very >>> old driver and no more maintained by Broadcom/LSI ? >>> >> Sadly, ignoring is not an option. >> I'm planning to update the calling convention for SCSI EH, to resolve the >> long- >> standing problem with sg_reset ioctls. >> sg_reset ioctl will allocate an out-of-band SCSI command, which does no >> longer >> work with the new command allocation scheme in multiqueue. >> So it's not possible to just 'ignore' it, as then SCSI EH will cease to >> function with >> that driver. > > Hannes - We are in process of sending megaraid and 3ware driver removal as > LSI/Broadcom stopped supporting those products. > I agree we should review this closely, but lack of test coverage and end of > life cycle of product is requesting us to know the rational. > For now, let's consider NACK for this patch. We will be removing old > megaraid (mbox) driver and 3Ware drivers soon. > Hmm. Can't we do the removal now? There is not a lot of testing involved with _that_, surely? I'd be happy to do a patch if you like ... Cheers, Hannes
>-----Original Message----- >From: Hannes Reinecke [mailto:hare@suse.de] >Sent: Thursday, June 29, 2017 11:23 AM >To: Kashyap Desai; Sumit Saxena; Christoph Hellwig >Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; Hannes >Reinecke >Subject: Re: [PATCH 13/47] megaraid: pass in NULL scb for host reset > >On 06/28/2017 07:40 PM, Kashyap Desai wrote: >>> -----Original Message----- >>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi- >>> owner@vger.kernel.org] On Behalf Of Hannes Reinecke >>> Sent: Wednesday, June 28, 2017 9:00 PM >>> To: Sumit Saxena; Christoph Hellwig >>> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; >>> Hannes Reinecke >>> Subject: Re: [PATCH 13/47] megaraid: pass in NULL scb for host reset >>> >>> On 06/28/2017 03:41 PM, Sumit Saxena wrote: >>>>> -----Original Message----- >>>>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi- >>>>> owner@vger.kernel.org] On Behalf Of Hannes Reinecke >>>>> Sent: Wednesday, June 28, 2017 2:03 PM >>>>> To: Christoph Hellwig >>>>> Cc: Martin K. Petersen; James Bottomley; >>>>> linux-scsi@vger.kernel.org; >>>> Hannes >>>>> Reinecke; Hannes Reinecke >>>>> Subject: [PATCH 13/47] megaraid: pass in NULL scb for host reset >>>>> >>>>> When calling a host reset we shouldn't rely on the command >>>>> triggering the reset, so allow megaraid_abort_and_reset() to be >>>>> called with a NULL >>> scb. >>>>> And drop the pointless 'bus_reset' and 'target_reset' handlers, >>>>> which >>>> just call >>>>> the same function as host_reset. >>>> >>>> If this patch address any functional issue, then we should consider >>>> this. >>>> If it's code optimization, can we ignore this as this is being very >>>> old driver and no more maintained by Broadcom/LSI ? >>>> >>> Sadly, ignoring is not an option. >>> I'm planning to update the calling convention for SCSI EH, to resolve >>> the >>> long- >>> standing problem with sg_reset ioctls. >>> sg_reset ioctl will allocate an out-of-band SCSI command, which does >>> no longer work with the new command allocation scheme in multiqueue. >>> So it's not possible to just 'ignore' it, as then SCSI EH will cease >>> to function with that driver. >> >> Hannes - We are in process of sending megaraid and 3ware driver >> removal as LSI/Broadcom stopped supporting those products. >> I agree we should review this closely, but lack of test coverage and >> end of life cycle of product is requesting us to know the rational. >> For now, let's consider NACK for this patch. We will be removing old >> megaraid (mbox) driver and 3Ware drivers soon. >> >Hmm. >Can't we do the removal now? >There is not a lot of testing involved with _that_, surely? > >I'd be happy to do a patch if you like ... We are fine with you submitting the patch to remove megaraid and megaraid_mm drivers. 3ware drivers are maintained by Adam Radford so we can not remove them. > >Cheers, > >Hannes >-- >Dr. Hannes Reinecke Teamlead Storage & Networking >hare@suse.de +49 911 74053 688 >SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg >GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 >(AG Nürnberg)
diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c index 3c63c29..7e504d3 100644 --- a/drivers/scsi/megaraid.c +++ b/drivers/scsi/megaraid.c @@ -1909,7 +1909,7 @@ static DEF_SCSI_QCMD(megaraid_queue) spin_lock_irq(&adapter->lock); - rval = megaraid_abort_and_reset(adapter, cmd, SCB_RESET); + rval = megaraid_abort_and_reset(adapter, NULL, SCB_RESET); /* * This is required here to complete any completed requests @@ -1948,7 +1948,7 @@ static DEF_SCSI_QCMD(megaraid_queue) scb = list_entry(pos, scb_t, list); - if (scb->cmd == cmd) { /* Found command */ + if (!cmd || scb->cmd == cmd) { /* Found command */ scb->state |= aor; @@ -1967,31 +1967,23 @@ static DEF_SCSI_QCMD(megaraid_queue) return FAILED; } - else { - - /* - * Not yet issued! Remove from the pending - * list - */ - dev_warn(&adapter->dev->dev, - "%s-[%x], driver owner\n", - (aor==SCB_ABORT) ? "ABORTING":"RESET", - scb->idx); - - mega_free_scb(adapter, scb); - - if( aor == SCB_ABORT ) { - cmd->result = (DID_ABORT << 16); - } - else { - cmd->result = (DID_RESET << 16); - } + /* + * Not yet issued! Remove from the pending + * list + */ + dev_warn(&adapter->dev->dev, + "%s-[%x], driver owner\n", + (cmd) ? "ABORTING":"RESET", + scb->idx); + mega_free_scb(adapter, scb); + if (cmd) { + cmd->result = (DID_ABORT << 16); list_add_tail(SCSI_LIST(cmd), - &adapter->completed_list); - - return SUCCESS; + &adapter->completed_list); } + + return SUCCESS; } } @@ -4180,8 +4172,6 @@ static inline void mega_create_proc_entry(int index, struct proc_dir_entry *pare .cmd_per_lun = DEF_CMD_PER_LUN, .use_clustering = ENABLE_CLUSTERING, .eh_abort_handler = megaraid_abort, - .eh_device_reset_handler = megaraid_reset, - .eh_bus_reset_handler = megaraid_reset, .eh_host_reset_handler = megaraid_reset, .no_write_same = 1, };
When calling a host reset we shouldn't rely on the command triggering the reset, so allow megaraid_abort_and_reset() to be called with a NULL scb. And drop the pointless 'bus_reset' and 'target_reset' handlers, which just call the same function as host_reset. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/scsi/megaraid.c | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-)