| Message ID | 20161128112626.GA31859@gnr743-HP-ZBook-15 (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
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? > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "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, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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];
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(-)