[dpdk-dev] [PATCH] igb_uio: cast private data to correct struct type

[dpdk-dev] [PATCH] igb_uio: cast private data to correct struct type

[Ferruh Yigit]

Fixes: af75078fece3 ("first public release")

This was working fine because addresses of two structs are same:

struct A {
	struct B b;
} a;

As above sample "a" and "b" has same address.

Now casting private data back to the correct struct type, to the one
stored.

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

diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index f5617d2..3374e44 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -561,24 +561,17 @@ fail_free:
 static void
 igbuio_pci_remove(struct pci_dev *dev)
 {
-	struct uio_info *info = pci_get_drvdata(dev);
-	struct rte_uio_pci_dev *udev;
-
-	if (info->priv == NULL) {
-		pr_notice("Not igbuio device\n");
-		return;
-	}
-	udev = info->priv;
+	struct rte_uio_pci_dev *udev = pci_get_drvdata(dev);
 
 	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
-	uio_unregister_device(info);
-	igbuio_pci_release_iomem(info);
+	uio_unregister_device(&udev->info);
+	igbuio_pci_release_iomem(&udev->info);
 	if (udev->mode == RTE_INTR_MODE_MSIX)
 		pci_disable_msix(dev);
 	pci_release_regions(dev);
 	pci_disable_device(dev);
 	pci_set_drvdata(dev, NULL);
-	kfree(info);
+	kfree(udev);
 }
 
 static int
-- 
2.5.0

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

[Ferruh Yigit]

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)
 		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

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

[Ananyev, Konstantin]

> -----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

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

[Ferruh Yigit]

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

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

[Ananyev, Konstantin]

> -----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

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

[Ferruh Yigit]

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

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

[Ferruh Yigit]

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..a3ad912 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 >= ARRAY_SIZE(info->mem))
 		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 >= ARRAY_SIZE(info->port))
 		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

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

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Friday, March 04, 2016 11:21 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] 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>
> ---

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Re: [dpdk-dev] [PATCH] igb_uio: cast private data to correct struct type

[Ananyev, Konstantin]

> Subject: [dpdk-dev] [PATCH] igb_uio: cast private data to correct struct type
> 
> Fixes: af75078fece3 ("first public release")
> 
> This was working fine because addresses of two structs are same:
> 
> struct A {
> 	struct B b;
> } a;
> 
> As above sample "a" and "b" has same address.
> 
> Now casting private data back to the correct struct type, to the one
> stored.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> --
> 2.5.0

Re: [dpdk-dev] [PATCH] igb_uio: cast private data to correct struct type

[Thomas Monjalon]

2016-03-04 13:17, Ananyev, Konstantin:
> > Subject: [dpdk-dev] [PATCH] igb_uio: cast private data to correct struct type
> > 
> > Fixes: af75078fece3 ("first public release")
> > 
> > This was working fine because addresses of two structs are same:
> > 
> > struct A {
> > 	struct B b;
> > } a;
> > 
> > As above sample "a" and "b" has same address.
> > 
> > Now casting private data back to the correct struct type, to the one
> > stored.
> > 
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks

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

[Thomas Monjalon]

> > Minor code cleanup.
> > Remove array size calculations and remove unnecessary assignment.
> > 
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks