[PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[Souptick Joarder]

Inside mvs_task_prep(), pci_pool_alloc() followed by memset will be
replaced by pci_pool_zalloc()

Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>
---
 drivers/scsi/mvsas/mv_sas.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 86eb199..681e5f7 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -790,10 +790,9 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf
 	slot->n_elem = n_elem;
 	slot->slot_tag = tag;
 
-	slot->buf = pci_pool_alloc(mvi->dma_pool, GFP_ATOMIC, &slot->buf_dma);
+	slot->buf = pci_pool_zalloc(mvi->dma_pool, GFP_ATOMIC, &slot->buf_dma);
 	if (!slot->buf)
 		goto err_out_tag;
-	memset(slot->buf, 0, MVS_SLOT_BUF_SZ);
 
 	tei.task = task;
 	tei.hdr = &mvi->slot[tag];
-- 
1.9.1

Re: [PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[Souptick Joarder]

On Mon, Nov 28, 2016 at 4:56 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
> Inside mvs_task_prep(), pci_pool_alloc() followed by memset will be
> replaced by pci_pool_zalloc()
>
> Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>
> ---
>  drivers/scsi/mvsas/mv_sas.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
> index 86eb199..681e5f7 100644
> --- a/drivers/scsi/mvsas/mv_sas.c
> +++ b/drivers/scsi/mvsas/mv_sas.c
> @@ -790,10 +790,9 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf
>         slot->n_elem = n_elem;
>         slot->slot_tag = tag;
>
> -       slot->buf = pci_pool_alloc(mvi->dma_pool, GFP_ATOMIC, &slot->buf_dma);
> +       slot->buf = pci_pool_zalloc(mvi->dma_pool, GFP_ATOMIC, &slot->buf_dma);
>         if (!slot->buf)
>                 goto err_out_tag;
> -       memset(slot->buf, 0, MVS_SLOT_BUF_SZ);
>
>         tei.task = task;
>         tei.hdr = &mvi->slot[tag];
> --
> 1.9.1

Any Comment on this patch?
>

Re: [PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[Souptick Joarder]

Hi Martin,

Any comment on this patch?

On Thu, Dec 1, 2016 at 11:34 AM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
> On Mon, Nov 28, 2016 at 4:56 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
>> Inside mvs_task_prep(), pci_pool_alloc() followed by memset will be
>> replaced by pci_pool_zalloc()
>>
>> Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>
>> ---
>>  drivers/scsi/mvsas/mv_sas.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
>> index 86eb199..681e5f7 100644
>> --- a/drivers/scsi/mvsas/mv_sas.c
>> +++ b/drivers/scsi/mvsas/mv_sas.c
>> @@ -790,10 +790,9 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf
>>         slot->n_elem = n_elem;
>>         slot->slot_tag = tag;
>>
>> -       slot->buf = pci_pool_alloc(mvi->dma_pool, GFP_ATOMIC, &slot->buf_dma);
>> +       slot->buf = pci_pool_zalloc(mvi->dma_pool, GFP_ATOMIC, &slot->buf_dma);
>>         if (!slot->buf)
>>                 goto err_out_tag;
>> -       memset(slot->buf, 0, MVS_SLOT_BUF_SZ);
>>
>>         tei.task = task;
>>         tei.hdr = &mvi->slot[tag];
>> --
>> 1.9.1
>
> Any Comment on this patch?
>>

Regards
Souptick

Re: [PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[Martin K. Petersen]

>>>>> "Souptick" == Souptick Joarder <jrdr.linux@gmail.com> writes:

Souptick,

Souptick> Any comment on this patch?

The patch looked OK to me when you posted it. However, you need one
person in addition to me to review it. And you need convince us of the
merit of a presumably untested change to an unmaintained driver.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[Souptick Joarder]

Hi Martin,

On Tue, Dec 6, 2016 at 3:34 AM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Souptick" == Souptick Joarder <jrdr.linux@gmail.com> writes:
>
> Souptick,
>
> Souptick> Any comment on this patch?
>
> The patch looked OK to me when you posted it. However, you need one
> person in addition to me to review it. And you need convince us of the
> merit of a presumably untested change to an unmaintained driver.

pci_pool_zalloc() will perform the same as pci_pool_alloc() and memset combined.

I have send similar patches to other modules like - dmaengine, scsi,
net, gpu . Those
were accepted and merged into respective branches. one similar patch
you have applied
previously.

I am sure this patch will work fine even without testing.

>
> --
> Martin K. Petersen      Oracle Linux Engineering

Regards
Souptick

Re: [PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[Johannes Thumshirn]

On Mon, Nov 28, 2016 at 04:56:26PM +0530, Souptick Joarder wrote:
> Inside mvs_task_prep(), pci_pool_alloc() followed by memset will be
> replaced by pci_pool_zalloc()
> 
> Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>
> ---

FWIW,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@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

Re: [PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[Souptick Joarder]

Hi Martin,

On Tue, Dec 6, 2016 at 6:58 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On Mon, Nov 28, 2016 at 04:56:26PM +0530, Souptick Joarder wrote:
>> Inside mvs_task_prep(), pci_pool_alloc() followed by memset will be
>> replaced by pci_pool_zalloc()
>>
>> Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>
>> ---
>
> FWIW,
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
>
> --
> Johannes Thumshirn                                          Storage
> jthumshirn@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

Any further comment on this ?

Regards
Souptick

Re: [PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[Martin K. Petersen]

>>>>> "Souptick" == Souptick Joarder <jrdr.linux@gmail.com> writes:

Souptick,

Sorry about the delay. Been out for a few days.

>>> Inside mvs_task_prep(), pci_pool_alloc() followed by memset will be
>>> replaced by pci_pool_zalloc()

Souptick> Any further comment on this ?

I took one of your other patches because the driver maintainer acked it,
thus assuming responsibility for testing it and fixing any regressions
it may cause.

The failure rate on these "trivial" patches to old and unmaintained
drivers is very high. And since you are not fixing any bugs and your
change is functionally identical what the code already does, why would
we merge it and risk a regression? (for changes like this, a Tested-by:
from somebody with actual hardware is worth a thousand Reviewed-by:
tags).

Also, I'm not really convinced that this constant churn of new and
"improved" kernel interface helper functions is really solving anything.
Quite the contrary. Real bug fixes for drivers adopting the
pci_pool_zalloc() interface will now potentially be harder to backport
to stable releases predating 4.4 or whenever that call was introduced.

So in summary, I don't see any actual benefits to your proposed
change. It's probably fine, but why would I risk merging it?

Hope that all makes sense...

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: mvsas: Replace pci_pool_alloc by pci_pool_zalloc

[Souptick Joarder]

Martin,

On Wed, Dec 14, 2016 at 7:22 AM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Souptick" == Souptick Joarder <jrdr.linux@gmail.com> writes:
>
> Souptick,
>
> Sorry about the delay. Been out for a few days.
>
>>>> Inside mvs_task_prep(), pci_pool_alloc() followed by memset will be
>>>> replaced by pci_pool_zalloc()
>
> Souptick> Any further comment on this ?
>
> I took one of your other patches because the driver maintainer acked it,
> thus assuming responsibility for testing it and fixing any regressions
> it may cause.
>
> The failure rate on these "trivial" patches to old and unmaintained
> drivers is very high. And since you are not fixing any bugs and your
> change is functionally identical what the code already does, why would
> we merge it and risk a regression? (for changes like this, a Tested-by:
> from somebody with actual hardware is worth a thousand Reviewed-by:
> tags).
>
> Also, I'm not really convinced that this constant churn of new and
> "improved" kernel interface helper functions is really solving anything.
> Quite the contrary. Real bug fixes for drivers adopting the
> pci_pool_zalloc() interface will now potentially be harder to backport
> to stable releases predating 4.4 or whenever that call was introduced.
>
> So in summary, I don't see any actual benefits to your proposed
> change. It's probably fine, but why would I risk merging it?

  I understand the importance of testing this patch on old and
unmaintained driver and
  totally agreed with your point of view now.

  I will drop this patch.
  If possible, can you please let me know what are all the basic
stability test cases are covered
  for SCSI drivers?
>
> Hope that all makes sense...
>
> Thanks!
>
> --
> Martin K. Petersen      Oracle Linux Engineering

Thanks -
Souptick