diff mbox

[dpdk-dev] igb_uio: use macros for array size calculation

Message ID 1457024900-18245-2-git-send-email-ferruh.yigit@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show

Commit Message

Ferruh Yigit March 3, 2016, 5:08 p.m. UTC
Minor code cleanup.
Remove array size calculations and remove unnecessary assignment.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Konstantin Ananyev March 3, 2016, 5:25 p.m. UTC | #1
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Thursday, March 03, 2016 5:08 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] igb_uio: use macros for array size calculation
> 
> Minor code cleanup.
> Remove array size calculations and remove unnecessary assignment.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> index 3374e44..563c57b 100644
> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> @@ -58,7 +58,7 @@ struct rte_uio_pci_dev {
>  	enum rte_intr_mode mode;
>  };
> 
> -static char *intr_mode = NULL;
> +static char *intr_mode;
>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
> 
>  /* sriov sysfs */
> @@ -332,7 +332,7 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>  	unsigned long addr, len;
>  	void *internal_addr;
> 
> -	if (sizeof(info->mem) / sizeof(info->mem[0]) <= n)
> +	if (n >= MAX_UIO_MAPS)

Why using hardcoded value is better than sizeof()?
As I can see below there is a macro ARRAY_SIZE, why not to use it here then?
Konstantin

>  		return -EINVAL;
> 
>  	addr = pci_resource_start(dev, pci_bar);
> @@ -357,7 +357,7 @@ igbuio_pci_setup_ioport(struct pci_dev *dev, struct uio_info *info,
>  {
>  	unsigned long addr, len;
> 
> -	if (sizeof(info->port) / sizeof(info->port[0]) <= n)
> +	if (n >= MAX_UIO_PORT_REGIONS)
>  		return -EINVAL;
> 
>  	addr = pci_resource_start(dev, pci_bar);
> @@ -402,7 +402,7 @@ igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)
>  	iom = 0;
>  	iop = 0;
> 
> -	for (i = 0; i != sizeof(bar_names) / sizeof(bar_names[0]); i++) {
> +	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
>  		if (pci_resource_len(dev, i) != 0 &&
>  				pci_resource_start(dev, i) != 0) {
>  			flags = pci_resource_flags(dev, i);
> --
> 2.5.0
Ferruh Yigit March 3, 2016, 5:34 p.m. UTC | #2
On 3/3/2016 5:25 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>> Sent: Thursday, March 03, 2016 5:08 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH] igb_uio: use macros for array size calculation
>>
>> Minor code cleanup.
>> Remove array size calculations and remove unnecessary assignment.
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>> index 3374e44..563c57b 100644
>> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>> @@ -58,7 +58,7 @@ struct rte_uio_pci_dev {
>>  	enum rte_intr_mode mode;
>>  };
>>
>> -static char *intr_mode = NULL;
>> +static char *intr_mode;
>>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>>
>>  /* sriov sysfs */
>> @@ -332,7 +332,7 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>>  	unsigned long addr, len;
>>  	void *internal_addr;
>>
>> -	if (sizeof(info->mem) / sizeof(info->mem[0]) <= n)
>> +	if (n >= MAX_UIO_MAPS)
> 
> Why using hardcoded value is better than sizeof()?
> As I can see below there is a macro ARRAY_SIZE, why not to use it here then?

Both are valid, but in uio (uio_driver.h) "mem" array defined as:
 struct uio_mem          mem[MAX_UIO_MAPS];

So we already know the size of the array, and it is exposed to us, why
need to calculate. Is there any benefit of calculating it?

Thanks,
ferruh
Konstantin Ananyev March 3, 2016, 5:45 p.m. UTC | #3
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, March 03, 2016 5:35 PM
> To: Ananyev, Konstantin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] igb_uio: use macros for array size calculation
> 
> On 3/3/2016 5:25 PM, Ananyev, Konstantin wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> >> Sent: Thursday, March 03, 2016 5:08 PM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH] igb_uio: use macros for array size calculation
> >>
> >> Minor code cleanup.
> >> Remove array size calculations and remove unnecessary assignment.
> >>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >> ---
> >>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> >> index 3374e44..563c57b 100644
> >> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> >> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> >> @@ -58,7 +58,7 @@ struct rte_uio_pci_dev {
> >>  	enum rte_intr_mode mode;
> >>  };
> >>
> >> -static char *intr_mode = NULL;
> >> +static char *intr_mode;
> >>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
> >>
> >>  /* sriov sysfs */
> >> @@ -332,7 +332,7 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
> >>  	unsigned long addr, len;
> >>  	void *internal_addr;
> >>
> >> -	if (sizeof(info->mem) / sizeof(info->mem[0]) <= n)
> >> +	if (n >= MAX_UIO_MAPS)
> >
> > Why using hardcoded value is better than sizeof()?
> > As I can see below there is a macro ARRAY_SIZE, why not to use it here then?
> 
> Both are valid, but in uio (uio_driver.h) "mem" array defined as:
>  struct uio_mem          mem[MAX_UIO_MAPS];

Yep, so if both are valid, why to change it a the first place? :)

> 
> So we already know the size of the array, and it is exposed to us, why
> need to calculate. Is there any benefit of calculating it?

if in future definition of the mem[] would change to let say:
struct uio_mem   mem[X]
your code would still be valid, and no need to update it. 
Konstantin

> 
> Thanks,
> ferruh
Ferruh Yigit March 3, 2016, 6:16 p.m. UTC | #4
On 3/3/2016 5:45 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Thursday, March 03, 2016 5:35 PM
>> To: Ananyev, Konstantin; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] igb_uio: use macros for array size calculation
>>
>> On 3/3/2016 5:25 PM, Ananyev, Konstantin wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>>>> Sent: Thursday, March 03, 2016 5:08 PM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH] igb_uio: use macros for array size calculation
>>>>
>>>> Minor code cleanup.
>>>> Remove array size calculations and remove unnecessary assignment.
>>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> ---
>>>>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>>>> index 3374e44..563c57b 100644
>>>> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>>>> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
>>>> @@ -58,7 +58,7 @@ struct rte_uio_pci_dev {
>>>>  	enum rte_intr_mode mode;
>>>>  };
>>>>
>>>> -static char *intr_mode = NULL;
>>>> +static char *intr_mode;
>>>>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>>>>
>>>>  /* sriov sysfs */
>>>> @@ -332,7 +332,7 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>>>>  	unsigned long addr, len;
>>>>  	void *internal_addr;
>>>>
>>>> -	if (sizeof(info->mem) / sizeof(info->mem[0]) <= n)
>>>> +	if (n >= MAX_UIO_MAPS)
>>>
>>> Why using hardcoded value is better than sizeof()?
>>> As I can see below there is a macro ARRAY_SIZE, why not to use it here then?
>>
>> Both are valid, but in uio (uio_driver.h) "mem" array defined as:
>>  struct uio_mem          mem[MAX_UIO_MAPS];
> 
> Yep, so if both are valid, why to change it a the first place? :)
> 
>>
>> So we already know the size of the array, and it is exposed to us, why
>> need to calculate. Is there any benefit of calculating it?
> 
> if in future definition of the mem[] would change to let say:
> struct uio_mem   mem[X]
> your code would still be valid, and no need to update it. 

Since it is the part of uio API, I expect this will remain same,
otherwise igb_uio.c will already have problems  because there is other
piece of code that already rely on this information.

But I don't have a strong opinion on one or other, I will update this to
use ARRAY_SIZE()


Thanks,
ferruh
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index 3374e44..563c57b 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -58,7 +58,7 @@  struct rte_uio_pci_dev {
 	enum rte_intr_mode mode;
 };
 
-static char *intr_mode = NULL;
+static char *intr_mode;
 static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
 
 /* sriov sysfs */
@@ -332,7 +332,7 @@  igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
 	unsigned long addr, len;
 	void *internal_addr;
 
-	if (sizeof(info->mem) / sizeof(info->mem[0]) <= n)
+	if (n >= MAX_UIO_MAPS)
 		return -EINVAL;
 
 	addr = pci_resource_start(dev, pci_bar);
@@ -357,7 +357,7 @@  igbuio_pci_setup_ioport(struct pci_dev *dev, struct uio_info *info,
 {
 	unsigned long addr, len;
 
-	if (sizeof(info->port) / sizeof(info->port[0]) <= n)
+	if (n >= MAX_UIO_PORT_REGIONS)
 		return -EINVAL;
 
 	addr = pci_resource_start(dev, pci_bar);
@@ -402,7 +402,7 @@  igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)
 	iom = 0;
 	iop = 0;
 
-	for (i = 0; i != sizeof(bar_names) / sizeof(bar_names[0]); i++) {
+	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
 		if (pci_resource_len(dev, i) != 0 &&
 				pci_resource_start(dev, i) != 0) {
 			flags = pci_resource_flags(dev, i);