[Intel-wired-lan] [PATCH v2] ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

[Intel-wired-lan] [PATCH v2] ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

[Souptick Joarder]

In e100_alloc_cbs(), pci_pool_alloc() followed by memset will be
replaced by pci_pool_zalloc()

Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>
---
v2:
  - Alignment changes

 drivers/net/ethernet/intel/e100.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index 068789e..0cf338f 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -1910,11 +1910,10 @@ static int e100_alloc_cbs(struct nic *nic)
 	nic->cb_to_use = nic->cb_to_send = nic->cb_to_clean = NULL;
 	nic->cbs_avail = 0;

-	nic->cbs = pci_pool_alloc(nic->cbs_pool, GFP_KERNEL,
-				  &nic->cbs_dma_addr);
+	nic->cbs = pci_pool_zalloc(nic->cbs_pool, GFP_KERNEL,
+				   &nic->cbs_dma_addr);
 	if (!nic->cbs)
 		return -ENOMEM;
-	memset(nic->cbs, 0, count * sizeof(struct cb));

 	for (cb = nic->cbs, i = 0; i < count; cb++, i++) {
 		cb->next = (i + 1 < count) ? cb + 1 : nic->cbs;
--
1.9.1

[Intel-wired-lan] [PATCH v2] ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

[Jeff Kirsher]

On Fri, 2016-12-02 at 00:34 +0530, Souptick Joarder wrote:
> In e100_alloc_cbs(), pci_pool_alloc() followed by memset will be
> replaced by pci_pool_zalloc()
> 
> Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>

Why the change?  Is pci_pool_alloc() being deprecated?  I ask because this
is a very old driver and changes will cause a huge regression, where many
of the parts/NICs may not be available to test with.

> ---
> v2:
> ? - Alignment changes
> 
> ?drivers/net/ethernet/intel/e100.c | 5 ++---
> ?1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e100.c
> b/drivers/net/ethernet/intel/e100.c
> index 068789e..0cf338f 100644
> --- a/drivers/net/ethernet/intel/e100.c
> +++ b/drivers/net/ethernet/intel/e100.c
> @@ -1910,11 +1910,10 @@ static int e100_alloc_cbs(struct nic *nic)
> ?	nic->cb_to_use = nic->cb_to_send = nic->cb_to_clean = NULL;
> ?	nic->cbs_avail = 0;
> 
> -	nic->cbs = pci_pool_alloc(nic->cbs_pool, GFP_KERNEL,
> -				??&nic->cbs_dma_addr);
> +	nic->cbs = pci_pool_zalloc(nic->cbs_pool, GFP_KERNEL,
> +				???&nic->cbs_dma_addr);
> ?	if (!nic->cbs)
> ?		return -ENOMEM;
> -	memset(nic->cbs, 0, count * sizeof(struct cb));
> 
> ?	for (cb = nic->cbs, i = 0; i < count; cb++, i++) {
> ?		cb->next = (i + 1 < count) ? cb + 1 : nic->cbs;
> --
> 1.9.1
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20161202/6f139877/attachment-0001.asc>

[Intel-wired-lan] [PATCH v2] ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

[Souptick Joarder]

On Sat, Dec 3, 2016 at 1:14 AM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Fri, 2016-12-02 at 00:34 +0530, Souptick Joarder wrote:
>> In e100_alloc_cbs(), pci_pool_alloc() followed by memset will be
>> replaced by pci_pool_zalloc()
>>
>> Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>
>
> Why the change?  Is pci_pool_alloc() being deprecated?  I ask because this
> is a very old driver and changes will cause a huge regression, where many
> of the parts/NICs may not be available to test with.

pci_pool_alloc is not deprecated. As pci_pool_zalloc can do the same as
pci_pool_alloc/memset, hence this change.

Do we need to go through complete regression to validate this patch?

>
>> ---
>> v2:
>>   - Alignment changes
>>
>>  drivers/net/ethernet/intel/e100.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e100.c
>> b/drivers/net/ethernet/intel/e100.c
>> index 068789e..0cf338f 100644
>> --- a/drivers/net/ethernet/intel/e100.c
>> +++ b/drivers/net/ethernet/intel/e100.c
>> @@ -1910,11 +1910,10 @@ static int e100_alloc_cbs(struct nic *nic)
>>       nic->cb_to_use = nic->cb_to_send = nic->cb_to_clean = NULL;
>>       nic->cbs_avail = 0;
>>
>> -     nic->cbs = pci_pool_alloc(nic->cbs_pool, GFP_KERNEL,
>> -                               &nic->cbs_dma_addr);
>> +     nic->cbs = pci_pool_zalloc(nic->cbs_pool, GFP_KERNEL,
>> +                                &nic->cbs_dma_addr);
>>       if (!nic->cbs)
>>               return -ENOMEM;
>> -     memset(nic->cbs, 0, count * sizeof(struct cb));
>>
>>       for (cb = nic->cbs, i = 0; i < count; cb++, i++) {
>>               cb->next = (i + 1 < count) ? cb + 1 : nic->cbs;
>> --
>> 1.9.1
>>

[Intel-wired-lan] [PATCH v2] ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

[Jeff Kirsher]

On Sat, 2016-12-03 at 12:58 +0530, Souptick Joarder wrote:
> On Sat, Dec 3, 2016 at 1:14 AM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
> > On Fri, 2016-12-02 at 00:34 +0530, Souptick Joarder wrote:
> > > In e100_alloc_cbs(), pci_pool_alloc() followed by memset will be
> > > replaced by pci_pool_zalloc()
> > > 
> > > Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>
> > 
> > Why the change?? Is pci_pool_alloc() being deprecated?? I ask because
> > this
> > is a very old driver and changes will cause a huge regression, where
> > many
> > of the parts/NICs may not be available to test with.
> 
> pci_pool_alloc is not deprecated. As pci_pool_zalloc can do the same as
> pci_pool_alloc/memset, hence this change.
> 
> Do we need to go through complete regression to validate this patch?

Well, I do not think it is good idea to make changes and not test them,
even the "simple" patches need to be tested and verified.

Since pci_pool_zalloc() does the same thing as pci_pool_alloc/memset, then
what is the benefit of the change?  If there is no benefit and just causes
a bunch of regression testing, then I am not seeing the gain.

Currently e100 and e1000 drivers are in bug fix only mode (and have been so
for some time now) so if you looking to kick the dust off these older
drivers for no real good reason other than you see a possible optimization,
which you are not wanting to regression test to verify there are no issues.
 Then I am not going to take your requested change seriously.

Since we are not actively working on the driver, it makes even more sense
that we should thoroughly test any changes to ensure there are no issues.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20161202/c5f6c799/attachment.asc>

[Intel-wired-lan] [PATCH v2] ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

[Souptick Joarder]

On Sat, Dec 3, 2016 at 1:29 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Sat, 2016-12-03 at 12:58 +0530, Souptick Joarder wrote:
>> On Sat, Dec 3, 2016 at 1:14 AM, Jeff Kirsher
>> <jeffrey.t.kirsher@intel.com> wrote:
>> > On Fri, 2016-12-02 at 00:34 +0530, Souptick Joarder wrote:
>> > > In e100_alloc_cbs(), pci_pool_alloc() followed by memset will be
>> > > replaced by pci_pool_zalloc()
>> > >
>> > > Signed-off-by: Souptick joarder <jrdr.linux@gmail.com>
>> >
>> > Why the change?  Is pci_pool_alloc() being deprecated?  I ask because
>> > this
>> > is a very old driver and changes will cause a huge regression, where
>> > many
>> > of the parts/NICs may not be available to test with.
>>
>> pci_pool_alloc is not deprecated. As pci_pool_zalloc can do the same as
>> pci_pool_alloc/memset, hence this change.
>>
>> Do we need to go through complete regression to validate this patch?
>
> Well, I do not think it is good idea to make changes and not test them,
> even the "simple" patches need to be tested and verified.
>
> Since pci_pool_zalloc() does the same thing as pci_pool_alloc/memset, then
> what is the benefit of the change?  If there is no benefit and just causes
> a bunch of regression testing, then I am not seeing the gain.
>
> Currently e100 and e1000 drivers are in bug fix only mode (and have been so
> for some time now) so if you looking to kick the dust off these older
> drivers for no real good reason other than you see a possible optimization,
> which you are not wanting to regression test to verify there are no issues.
>  Then I am not going to take your requested change seriously.

Well , I mean, rather than running entire regression whether we can run minimum
set of test cases to verify it :)

>
> Since we are not actively working on the driver, it makes even more sense
> that we should thoroughly test any changes to ensure there are no issues.

Ok, I understand it's really painful to run entire regression test for
this small
optimization change. I will drop this patch.