RE: [PATCH 13/47] megaraid: pass in NULL scb for host reset

2017-06-29 Thread Sumit Saxena 
>-Original Message-
>From: Hannes Reinecke [mailto:h...@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-ow...@vger.kernel.org [mailto:linux-scsi-
>>> ow...@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-ow...@vger.kernel.org [mailto:linux-scsi-
>>>>> ow...@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
>h...@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)

Re: [PATCH 13/47] megaraid: pass in NULL scb for host reset

2017-06-28 Thread Hannes Reinecke 
On 06/28/2017 07:40 PM, Kashyap Desai wrote:
>> -Original Message-
>> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
>> ow...@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-ow...@vger.kernel.org [mailto:linux-scsi-
>>>> ow...@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
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@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)

Re: [PATCH 13/47] megaraid: pass in NULL scb for host reset

2017-06-28 Thread Christoph Hellwig 
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.

RE: [PATCH 13/47] megaraid: pass in NULL scb for host reset

2017-06-28 Thread Kashyap Desai 
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@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-ow...@vger.kernel.org [mailto:linux-scsi-
> >> ow...@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 <h...@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(>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(>dev->dev, "%s cmd=%x

Re: [PATCH 13/47] megaraid: pass in NULL scb for host reset

2017-06-28 Thread Hannes Reinecke 
On 06/28/2017 03:41 PM, Sumit Saxena wrote:
>> -Original Message-
>> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
>> ow...@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 
>> ---
>> 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(>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(>dev->dev, "%s cmd=%x

RE: [PATCH 13/47] megaraid: pass in NULL scb for host reset

2017-06-28 Thread Sumit Saxena 
>-Original Message-
>From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
>ow...@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 
>---
> 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(>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(>dev->dev, "%s cmd=%x

Re: [PATCH 13/47] megaraid: pass in NULL scb for host reset

2017-06-28 Thread Johannes Thumshirn 

Reviewed-by: Johannes Thumshirn 
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850