[dpdk-stable] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[dpdk-stable] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[David Christensen]

The function eth_dev_pci_specific_init is missing a typecast to
(struct rte_pci_device *) for the input argument bus_device.

Cc: stable@dpdk.org

Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
---
 lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
index 23257e9..a325311 100644
--- a/lib/librte_ethdev/rte_ethdev_pci.h
+++ b/lib/librte_ethdev/rte_ethdev_pci.h
@@ -72,7 +72,7 @@
 
 static inline int
 eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
-	struct rte_pci_device *pci_dev = bus_device;
+	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
 
 	if (!pci_dev)
 		return -ENODEV;
-- 
1.8.3.1

Re: [dpdk-stable] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Thomas Monjalon]

10/04/2019 22:16, David Christensen:
> The function eth_dev_pci_specific_init is missing a typecast to
> (struct rte_pci_device *) for the input argument bus_device.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> Tested-by: Radhika Chirra <radhika.chirra@ibm.com>

This is a duplicate of this patch:

https://patches.dpdk.org/patch/52505/

You are from 2 different Linux teams at IBM,
you hit the same issue at the same time,
you both miss to give an explanation,
funny.

About collaborating inside IBM, please check this email:
http://mails.dpdk.org/archives/dev/2019-April/129915.html

Re: [dpdk-stable] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[David Christensen]

> This is a duplicate of this patch:
> 
> https://patches.dpdk.org/patch/52505/
> 
> You are from 2 different Linux teams at IBM,
> you hit the same issue at the same time,
> you both miss to give an explanation,
> funny.

We both work for infrastructure teams who are supporting a project team 
that is developing a DPDK application that runs across multiple CPU 
architectures and encountered the error.

The error comes from g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0:

[CXX]       g++ -o utils/pcap_handle.o -c utils/pcap_handle.cc -g3 
-ggdb3 -mcpu=native -mtune=native -isystem 
/home/rchirra/gen/io/ppc64include -std=gnu++11 -flax-vector-conversions 
-Werror -isystem /home/rchirra/gen/io

In file included from drivers/pmd.cc:7:0:
/home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:
In function ‘int eth_dev_pci_specific_init(rte_eth_dev*, void*)’:
/home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:75:35: 
error: invalid conversion from ‘void*’ to ‘rte_pci_device*’ [-fpermissive]
   struct rte_pci_device *pci_dev = bus_device;
                                    ^~~~~~~~~~
make[1]: *** [drivers/pmd.o] Error 1

Do you believe that more permissive compiler flags are the better answer?

Dave

Re: [dpdk-stable] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Thomas Monjalon]

10/04/2019 22:58, David Christensen:
> > This is a duplicate of this patch:
> > 
> > https://patches.dpdk.org/patch/52505/
> > 
> > You are from 2 different Linux teams at IBM,
> > you hit the same issue at the same time,
> > you both miss to give an explanation,
> > funny.
> 
> We both work for infrastructure teams who are supporting a project team 
> that is developing a DPDK application that runs across multiple CPU 
> architectures and encountered the error.
> 
> The error comes from g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0:
> 
> [CXX]       g++ -o utils/pcap_handle.o -c utils/pcap_handle.cc -g3 
> -ggdb3 -mcpu=native -mtune=native -isystem 
> /home/rchirra/gen/io/ppc64include -std=gnu++11 -flax-vector-conversions 
> -Werror -isystem /home/rchirra/gen/io
> 
> In file included from drivers/pmd.cc:7:0:
> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:
> In function ‘int eth_dev_pci_specific_init(rte_eth_dev*, void*)’:
> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:75:35: 
> error: invalid conversion from ‘void*’ to ‘rte_pci_device*’ [-fpermissive]
>    struct rte_pci_device *pci_dev = bus_device;
>                                     ^~~~~~~~~~
> make[1]: *** [drivers/pmd.o] Error 1
> 
> Do you believe that more permissive compiler flags are the better answer?

No, we should not force any compiler flag of the application.
We are even trying to support pedantic flags in the app.
So it seems we must fix it.

We just need to change the title to mention it fixes the build for C++,
and add the error message in the body.

Re: [dpdk-stable] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Ferruh Yigit]

On 4/10/2019 10:14 PM, Thomas Monjalon wrote:
> 10/04/2019 22:58, David Christensen:
>>> This is a duplicate of this patch:
>>>
>>> https://patches.dpdk.org/patch/52505/
>>>
>>> You are from 2 different Linux teams at IBM,
>>> you hit the same issue at the same time,
>>> you both miss to give an explanation,
>>> funny.
>>
>> We both work for infrastructure teams who are supporting a project team 
>> that is developing a DPDK application that runs across multiple CPU 
>> architectures and encountered the error.
>>
>> The error comes from g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0:
>>
>> [CXX]       g++ -o utils/pcap_handle.o -c utils/pcap_handle.cc -g3 
>> -ggdb3 -mcpu=native -mtune=native -isystem 
>> /home/rchirra/gen/io/ppc64include -std=gnu++11 -flax-vector-conversions 
>> -Werror -isystem /home/rchirra/gen/io
>>
>> In file included from drivers/pmd.cc:7:0:
>> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:
>> In function ‘int eth_dev_pci_specific_init(rte_eth_dev*, void*)’:
>> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:75:35: 
>> error: invalid conversion from ‘void*’ to ‘rte_pci_device*’ [-fpermissive]
>>    struct rte_pci_device *pci_dev = bus_device;
>>                                     ^~~~~~~~~~
>> make[1]: *** [drivers/pmd.o] Error 1
>>
>> Do you believe that more permissive compiler flags are the better answer?
> 
> No, we should not force any compiler flag of the application.
> We are even trying to support pedantic flags in the app.
> So it seems we must fix it.
> 
> We just need to change the title to mention it fixes the build for C++,
> and add the error message in the body.
> 

I agree our public headers should be usable from c++ applications, but this
header is not a public header.

Is the DPDK driver compiled with c++ in this case?
If so do we support (or have a target for support) that DPDK library itself can
be compiled by c++ compiler?

Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Bruce Richardson]

On Wed, Apr 10, 2019 at 03:16:16PM -0500, David Christensen wrote:
> The function eth_dev_pci_specific_init is missing a typecast to
> (struct rte_pci_device *) for the input argument bus_device.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
> ---
>  lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> index 23257e9..a325311 100644
> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> @@ -72,7 +72,7 @@
>  
>  static inline int
>  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
> -	struct rte_pci_device *pci_dev = bus_device;
> +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
>

Is this needed for building some C++ apps that are including the header
file (directly, or indirectly), because for pure C, "void *" types should
be assignable to any other pointer type without casting?

/Bruce

Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Stephen Hemminger]

On Wed, 10 Apr 2019 22:00:18 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Wed, Apr 10, 2019 at 03:16:16PM -0500, David Christensen wrote:
> > The function eth_dev_pci_specific_init is missing a typecast to
> > (struct rte_pci_device *) for the input argument bus_device.
> > 
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> > Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
> > ---
> >  lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> > index 23257e9..a325311 100644
> > --- a/lib/librte_ethdev/rte_ethdev_pci.h
> > +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> > @@ -72,7 +72,7 @@
> >  
> >  static inline int
> >  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
> > -	struct rte_pci_device *pci_dev = bus_device;
> > +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
> >  
> 
> Is this needed for building some C++ apps that are including the header
> file (directly, or indirectly), because for pure C, "void *" types should
> be assignable to any other pointer type without casting?
> 
> /Bruce

Another example of Why the Hell is this inline?

Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Ferruh Yigit]

On 4/11/2019 12:08 AM, Stephen Hemminger wrote:
> On Wed, 10 Apr 2019 22:00:18 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
>> On Wed, Apr 10, 2019 at 03:16:16PM -0500, David Christensen wrote:
>>> The function eth_dev_pci_specific_init is missing a typecast to
>>> (struct rte_pci_device *) for the input argument bus_device.
>>>
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
>>> Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
>>> ---
>>>  lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
>>> index 23257e9..a325311 100644
>>> --- a/lib/librte_ethdev/rte_ethdev_pci.h
>>> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
>>> @@ -72,7 +72,7 @@
>>>  
>>>  static inline int
>>>  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
>>> -	struct rte_pci_device *pci_dev = bus_device;
>>> +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
>>>  
>>
>> Is this needed for building some C++ apps that are including the header
>> file (directly, or indirectly), because for pure C, "void *" types should
>> be assignable to any other pointer type without casting?
>>
>> /Bruce
> 
> Another example of Why the Hell is this inline?
> 

It has been done inline intentionally at the time as far as remember, this
header is for drivers not for applications, it has helper functions.

The common code from drivers related to the bus put into header files, so the
code itself belongs to drivers not ethdev and reduces duplicates in them.

Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Friday, April 12, 2019 6:09 PM
> To: Stephen Hemminger <stephen@networkplumber.org>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: David Christensen <drc@linux.vnet.ibm.com>; thomas@monjalon.net; arybchenko@solarflare.com; dev@dpdk.org;
> radhika.chirra@ibm.com; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init
> 
> On 4/11/2019 12:08 AM, Stephen Hemminger wrote:
> > On Wed, 10 Apr 2019 22:00:18 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> >
> >> On Wed, Apr 10, 2019 at 03:16:16PM -0500, David Christensen wrote:
> >>> The function eth_dev_pci_specific_init is missing a typecast to
> >>> (struct rte_pci_device *) for the input argument bus_device.
> >>>
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> >>> Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
> >>> ---
> >>>  lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> >>> index 23257e9..a325311 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> >>> @@ -72,7 +72,7 @@
> >>>
> >>>  static inline int
> >>>  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
> >>> -	struct rte_pci_device *pci_dev = bus_device;
> >>> +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
> >>>
> >>
> >> Is this needed for building some C++ apps that are including the header
> >> file (directly, or indirectly), because for pure C, "void *" types should
> >> be assignable to any other pointer type without casting?
> >>
> >> /Bruce
> >
> > Another example of Why the Hell is this inline?
> >
> 
> It has been done inline intentionally at the time as far as remember, this
> header is for drivers not for applications, it has helper functions.
> 
> The common code from drivers related to the bus put into header files, so the
> code itself belongs to drivers not ethdev and reduces duplicates in them.

Ok that's the common code used by the drivers...
But why it still can't be in .c file?
Konstantin

Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Ferruh Yigit]

On 4/12/2019 6:15 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>> Sent: Friday, April 12, 2019 6:09 PM
>> To: Stephen Hemminger <stephen@networkplumber.org>; Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: David Christensen <drc@linux.vnet.ibm.com>; thomas@monjalon.net; arybchenko@solarflare.com; dev@dpdk.org;
>> radhika.chirra@ibm.com; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init
>>
>> On 4/11/2019 12:08 AM, Stephen Hemminger wrote:
>>> On Wed, 10 Apr 2019 22:00:18 +0100
>>> Bruce Richardson <bruce.richardson@intel.com> wrote:
>>>
>>>> On Wed, Apr 10, 2019 at 03:16:16PM -0500, David Christensen wrote:
>>>>> The function eth_dev_pci_specific_init is missing a typecast to
>>>>> (struct rte_pci_device *) for the input argument bus_device.
>>>>>
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
>>>>> Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
>>>>> ---
>>>>>  lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
>>>>> index 23257e9..a325311 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev_pci.h
>>>>> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
>>>>> @@ -72,7 +72,7 @@
>>>>>
>>>>>  static inline int
>>>>>  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
>>>>> -	struct rte_pci_device *pci_dev = bus_device;
>>>>> +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
>>>>>
>>>>
>>>> Is this needed for building some C++ apps that are including the header
>>>> file (directly, or indirectly), because for pure C, "void *" types should
>>>> be assignable to any other pointer type without casting?
>>>>
>>>> /Bruce
>>>
>>> Another example of Why the Hell is this inline?
>>>
>>
>> It has been done inline intentionally at the time as far as remember, this
>> header is for drivers not for applications, it has helper functions.
>>
>> The common code from drivers related to the bus put into header files, so the
>> code itself belongs to drivers not ethdev and reduces duplicates in them.
> 
> Ok that's the common code used by the drivers...
> But why it still can't be in .c file?

When it is in .c file, it will be either in ethdev library, single location in
.c file and binary file, but location is not exactly right, because code belongs
to drivers.
Or code should be in .c files of each drivers, this will be code duplication.

Having in .h file makes code in single place, but when compiled code will be in
each driver object file/ library.

Of course it works when put into a .c file in ehtdev, but bus (pci and vdev)
related code are not belongs to ethdev library and I believe shouldn't be part
of ethdev binary. And those bus helper headers are only for drivers to include,
so having inline shouldn't be a problem at all because there is not stability
concern in that interface.

Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Ferruh Yigit]

On 4/12/2019 6:25 PM, Ferruh Yigit wrote:
> On 4/12/2019 6:15 PM, Ananyev, Konstantin wrote:
>>
>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>>> Sent: Friday, April 12, 2019 6:09 PM
>>> To: Stephen Hemminger <stephen@networkplumber.org>; Richardson, Bruce <bruce.richardson@intel.com>
>>> Cc: David Christensen <drc@linux.vnet.ibm.com>; thomas@monjalon.net; arybchenko@solarflare.com; dev@dpdk.org;
>>> radhika.chirra@ibm.com; stable@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init
>>>
>>> On 4/11/2019 12:08 AM, Stephen Hemminger wrote:
>>>> On Wed, 10 Apr 2019 22:00:18 +0100
>>>> Bruce Richardson <bruce.richardson@intel.com> wrote:
>>>>
>>>>> On Wed, Apr 10, 2019 at 03:16:16PM -0500, David Christensen wrote:
>>>>>> The function eth_dev_pci_specific_init is missing a typecast to
>>>>>> (struct rte_pci_device *) for the input argument bus_device.
>>>>>>
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
>>>>>> Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
>>>>>> ---
>>>>>>  lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
>>>>>> index 23257e9..a325311 100644
>>>>>> --- a/lib/librte_ethdev/rte_ethdev_pci.h
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
>>>>>> @@ -72,7 +72,7 @@
>>>>>>
>>>>>>  static inline int
>>>>>>  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
>>>>>> -	struct rte_pci_device *pci_dev = bus_device;
>>>>>> +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
>>>>>>
>>>>>
>>>>> Is this needed for building some C++ apps that are including the header
>>>>> file (directly, or indirectly), because for pure C, "void *" types should
>>>>> be assignable to any other pointer type without casting?
>>>>>
>>>>> /Bruce
>>>>
>>>> Another example of Why the Hell is this inline?
>>>>
>>>
>>> It has been done inline intentionally at the time as far as remember, this
>>> header is for drivers not for applications, it has helper functions.
>>>
>>> The common code from drivers related to the bus put into header files, so the
>>> code itself belongs to drivers not ethdev and reduces duplicates in them.
>>
>> Ok that's the common code used by the drivers...
>> But why it still can't be in .c file?
> 
> When it is in .c file, it will be either in ethdev library, single location in
> .c file and binary file, but location is not exactly right, because code belongs
> to drivers.
> Or code should be in .c files of each drivers, this will be code duplication.
> 
> Having in .h file makes code in single place, but when compiled code will be in
> each driver object file/ library.
> 
> Of course it works when put into a .c file in ehtdev, but bus (pci and vdev)
> related code are not belongs to ethdev library and I believe shouldn't be part
> of ethdev binary. And those bus helper headers are only for drivers to include,
> so having inline shouldn't be a problem at all because there is not stability
> concern in that interface.
> 

btw, if you put those into .c file in ethdev, you will be creating a dependency
from ethdev to bus code, to all available buses which will make impossible to
disable any bus type if you use ethdev.

Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Stephen Hemminger]

On Fri, 12 Apr 2019 18:29:46 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 4/12/2019 6:25 PM, Ferruh Yigit wrote:
> > On 4/12/2019 6:15 PM, Ananyev, Konstantin wrote:  
> >>
> >>  
> >>> -----Original Message-----
> >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> >>> Sent: Friday, April 12, 2019 6:09 PM
> >>> To: Stephen Hemminger <stephen@networkplumber.org>; Richardson, Bruce <bruce.richardson@intel.com>
> >>> Cc: David Christensen <drc@linux.vnet.ibm.com>; thomas@monjalon.net; arybchenko@solarflare.com; dev@dpdk.org;
> >>> radhika.chirra@ibm.com; stable@dpdk.org
> >>> Subject: Re: [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init
> >>>
> >>> On 4/11/2019 12:08 AM, Stephen Hemminger wrote:  
> >>>> On Wed, 10 Apr 2019 22:00:18 +0100
> >>>> Bruce Richardson <bruce.richardson@intel.com> wrote:
> >>>>  
> >>>>> On Wed, Apr 10, 2019 at 03:16:16PM -0500, David Christensen wrote:  
> >>>>>> The function eth_dev_pci_specific_init is missing a typecast to
> >>>>>> (struct rte_pci_device *) for the input argument bus_device.
> >>>>>>
> >>>>>> Cc: stable@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> >>>>>> Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
> >>>>>> ---
> >>>>>>  lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> >>>>>> index 23257e9..a325311 100644
> >>>>>> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> >>>>>> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> >>>>>> @@ -72,7 +72,7 @@
> >>>>>>
> >>>>>>  static inline int
> >>>>>>  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
> >>>>>> -	struct rte_pci_device *pci_dev = bus_device;
> >>>>>> +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
> >>>>>>  
> >>>>>
> >>>>> Is this needed for building some C++ apps that are including the header
> >>>>> file (directly, or indirectly), because for pure C, "void *" types should
> >>>>> be assignable to any other pointer type without casting?
> >>>>>
> >>>>> /Bruce  
> >>>>
> >>>> Another example of Why the Hell is this inline?
> >>>>  
> >>>
> >>> It has been done inline intentionally at the time as far as remember, this
> >>> header is for drivers not for applications, it has helper functions.
> >>>
> >>> The common code from drivers related to the bus put into header files, so the
> >>> code itself belongs to drivers not ethdev and reduces duplicates in them.  
> >>
> >> Ok that's the common code used by the drivers...
> >> But why it still can't be in .c file?  
> > 
> > When it is in .c file, it will be either in ethdev library, single location in
> > .c file and binary file, but location is not exactly right, because code belongs
> > to drivers.
> > Or code should be in .c files of each drivers, this will be code duplication.
> > 
> > Having in .h file makes code in single place, but when compiled code will be in
> > each driver object file/ library.
> > 
> > Of course it works when put into a .c file in ehtdev, but bus (pci and vdev)
> > related code are not belongs to ethdev library and I believe shouldn't be part
> > of ethdev binary. And those bus helper headers are only for drivers to include,
> > so having inline shouldn't be a problem at all because there is not stability
> > concern in that interface.
> >   
> 
> btw, if you put those into .c file in ethdev, you will be creating a dependency
> from ethdev to bus code, to all available buses which will make impossible to
> disable any bus type if you use ethdev.

The problem I see is rte_ethdev_pci.h, it should be headers only and then put
code rte_ethdev_pci.c

Re: [dpdk-stable] [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init

[Ferruh Yigit]

On 4/12/2019 10:31 PM, Stephen Hemminger wrote:
> On Fri, 12 Apr 2019 18:29:46 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 4/12/2019 6:25 PM, Ferruh Yigit wrote:
>>> On 4/12/2019 6:15 PM, Ananyev, Konstantin wrote:  
>>>>
>>>>  
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>>>>> Sent: Friday, April 12, 2019 6:09 PM
>>>>> To: Stephen Hemminger <stephen@networkplumber.org>; Richardson, Bruce <bruce.richardson@intel.com>
>>>>> Cc: David Christensen <drc@linux.vnet.ibm.com>; thomas@monjalon.net; arybchenko@solarflare.com; dev@dpdk.org;
>>>>> radhika.chirra@ibm.com; stable@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: missing typecast from void in eth_dev_pci_specific_init
>>>>>
>>>>> On 4/11/2019 12:08 AM, Stephen Hemminger wrote:  
>>>>>> On Wed, 10 Apr 2019 22:00:18 +0100
>>>>>> Bruce Richardson <bruce.richardson@intel.com> wrote:
>>>>>>  
>>>>>>> On Wed, Apr 10, 2019 at 03:16:16PM -0500, David Christensen wrote:  
>>>>>>>> The function eth_dev_pci_specific_init is missing a typecast to
>>>>>>>> (struct rte_pci_device *) for the input argument bus_device.
>>>>>>>>
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
>>>>>>>> Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
>>>>>>>> ---
>>>>>>>>  lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
>>>>>>>> index 23257e9..a325311 100644
>>>>>>>> --- a/lib/librte_ethdev/rte_ethdev_pci.h
>>>>>>>> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
>>>>>>>> @@ -72,7 +72,7 @@
>>>>>>>>
>>>>>>>>  static inline int
>>>>>>>>  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
>>>>>>>> -	struct rte_pci_device *pci_dev = bus_device;
>>>>>>>> +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
>>>>>>>>  
>>>>>>>
>>>>>>> Is this needed for building some C++ apps that are including the header
>>>>>>> file (directly, or indirectly), because for pure C, "void *" types should
>>>>>>> be assignable to any other pointer type without casting?
>>>>>>>
>>>>>>> /Bruce  
>>>>>>
>>>>>> Another example of Why the Hell is this inline?
>>>>>>  
>>>>>
>>>>> It has been done inline intentionally at the time as far as remember, this
>>>>> header is for drivers not for applications, it has helper functions.
>>>>>
>>>>> The common code from drivers related to the bus put into header files, so the
>>>>> code itself belongs to drivers not ethdev and reduces duplicates in them.  
>>>>
>>>> Ok that's the common code used by the drivers...
>>>> But why it still can't be in .c file?  
>>>
>>> When it is in .c file, it will be either in ethdev library, single location in
>>> .c file and binary file, but location is not exactly right, because code belongs
>>> to drivers.
>>> Or code should be in .c files of each drivers, this will be code duplication.
>>>
>>> Having in .h file makes code in single place, but when compiled code will be in
>>> each driver object file/ library.
>>>
>>> Of course it works when put into a .c file in ehtdev, but bus (pci and vdev)
>>> related code are not belongs to ethdev library and I believe shouldn't be part
>>> of ethdev binary. And those bus helper headers are only for drivers to include,
>>> so having inline shouldn't be a problem at all because there is not stability
>>> concern in that interface.
>>>   
>>
>> btw, if you put those into .c file in ethdev, you will be creating a dependency
>> from ethdev to bus code, to all available buses which will make impossible to
>> disable any bus type if you use ethdev.
> 
> The problem I see is rte_ethdev_pci.h, it should be headers only and then put
> code rte_ethdev_pci.c
> 

Where this 'rte_ethdev_pci.c' should be? Because of reasons explained above,
ehtdev is not good place.
Perhaps a 'common' folder for net drivers may work, create a 'rte_ethdev_pci.o'
and link it with relevant drivers.?

[dpdk-stable] [PATCH v2] ethdev: missing typecast causes C++ build error

[David Christensen]

The function eth_dev_pci_specific_init is missing a typecast to
(struct rte_pci_device *) for the input argument bus_device.
This causes build issues in the GNU C++ compiler.

[CXX]       g++ -o utils/pcap_handle.o -c utils/pcap_handle.cc -g3·
-ggdb3 -mcpu=native -mtune=native -isystem·
/home/rchirra/gen/io/ppc64include -std=gnu++11 -flax-vector-conversions·
-Werror -isystem /home/rchirra/gen/io

In file included from drivers/pmd.cc:7:0:
/home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:
In function ‘int eth_dev_pci_specific_init(rte_eth_dev*, void*)’:
/home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:75:35:·
error: invalid conversion from ‘void*’ to ‘rte_pci_device*’
[-fpermissive]
  struct rte_pci_device *pci_dev = bus_device;
                                   ^~~~~~~~~~
make[1]: *** [drivers/pmd.o] Error 1

$ g++ --version
g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0

Cc: stable@dpdk.org

Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
---
v2:
* Mmodified subject line to indicate a C++ compiler issue
* Added error output from the compiler

 lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
index 23257e9..a325311 100644
--- a/lib/librte_ethdev/rte_ethdev_pci.h
+++ b/lib/librte_ethdev/rte_ethdev_pci.h
@@ -72,7 +72,7 @@
 
 static inline int
 eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
-	struct rte_pci_device *pci_dev = bus_device;
+	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
 
 	if (!pci_dev)
 		return -ENODEV;
-- 
1.8.3.1

Re: [dpdk-stable] [PATCH v2] ethdev: missing typecast causes C++ build error

[Ferruh Yigit]

On 4/10/2019 10:36 PM, David Christensen wrote:
> The function eth_dev_pci_specific_init is missing a typecast to
> (struct rte_pci_device *) for the input argument bus_device.
> This causes build issues in the GNU C++ compiler.
> 
> [CXX]       g++ -o utils/pcap_handle.o -c utils/pcap_handle.cc -g3·
> -ggdb3 -mcpu=native -mtune=native -isystem·
> /home/rchirra/gen/io/ppc64include -std=gnu++11 -flax-vector-conversions·
> -Werror -isystem /home/rchirra/gen/io
> 
> In file included from drivers/pmd.cc:7:0:
> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:
> In function ‘int eth_dev_pci_specific_init(rte_eth_dev*, void*)’:
> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:75:35:·
> error: invalid conversion from ‘void*’ to ‘rte_pci_device*’
> [-fpermissive]
>   struct rte_pci_device *pci_dev = bus_device;
>                                    ^~~~~~~~~~
> make[1]: *** [drivers/pmd.o] Error 1
> 
> $ g++ --version
> g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0

This problem is while building a driver with c++ compiler, but since the
solution is a simple casting, which doesn't have any side affect, I am for
getting the patch, any objection?

> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> Tested-by: Radhika Chirra <radhika.chirra@ibm.com>
> ---
> v2:
> * Mmodified subject line to indicate a C++ compiler issue
> * Added error output from the compiler
> 
>  lib/librte_ethdev/rte_ethdev_pci.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> index 23257e9..a325311 100644
> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> @@ -72,7 +72,7 @@
>  
>  static inline int
>  eth_dev_pci_specific_init(struct rte_eth_dev *eth_dev, void *bus_device) {
> -	struct rte_pci_device *pci_dev = bus_device;
> +	struct rte_pci_device *pci_dev = (struct rte_pci_device *)bus_device;
>  
>  	if (!pci_dev)
>  		return -ENODEV;
> 

Re: [dpdk-stable] [PATCH v2] ethdev: missing typecast causes C++ build error

[Andrew Rybchenko]

On 4/16/19 7:31 PM, Ferruh Yigit wrote:
> On 4/10/2019 10:36 PM, David Christensen wrote:
>> The function eth_dev_pci_specific_init is missing a typecast to
>> (struct rte_pci_device *) for the input argument bus_device.
>> This causes build issues in the GNU C++ compiler.
>>
>> [CXX]       g++ -o utils/pcap_handle.o -c utils/pcap_handle.cc -g3·
>> -ggdb3 -mcpu=native -mtune=native -isystem·
>> /home/rchirra/gen/io/ppc64include -std=gnu++11 -flax-vector-conversions·
>> -Werror -isystem /home/rchirra/gen/io
>>
>> In file included from drivers/pmd.cc:7:0:
>> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:
>> In function ‘int eth_dev_pci_specific_init(rte_eth_dev*, void*)’:
>> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:75:35:·
>> error: invalid conversion from ‘void*’ to ‘rte_pci_device*’
>> [-fpermissive]
>>    struct rte_pci_device *pci_dev = bus_device;
>>                                     ^~~~~~~~~~
>> make[1]: *** [drivers/pmd.o] Error 1
>>
>> $ g++ --version
>> g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0
> This problem is while building a driver with c++ compiler, but since the
> solution is a simple casting, which doesn't have any side affect, I am for
> getting the patch, any objection?

It should be a decision that we support PMDs in C++ (if I understand
the usage correctly) and it will require all headers to be compatible and
ideally it should be checked by build automation (otherwise we will break
and fix it pretty often).

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] ethdev: missing typecast causes C++ build error

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andrew Rybchenko
> Sent: Tuesday, April 16, 2019 5:40 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; David Christensen <drc@linux.vnet.ibm.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; radhika.chirra@ibm.com; stable@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v2] ethdev: missing typecast causes C++ build error
> 
> On 4/16/19 7:31 PM, Ferruh Yigit wrote:
> > On 4/10/2019 10:36 PM, David Christensen wrote:
> >> The function eth_dev_pci_specific_init is missing a typecast to
> >> (struct rte_pci_device *) for the input argument bus_device.
> >> This causes build issues in the GNU C++ compiler.
> >>
> >> [CXX]       g++ -o utils/pcap_handle.o -c utils/pcap_handle.cc -g3·
> >> -ggdb3 -mcpu=native -mtune=native -isystem·
> >> /home/rchirra/gen/io/ppc64include -std=gnu++11 -flax-vector-conversions·
> >> -Werror -isystem /home/rchirra/gen/io
> >>
> >> In file included from drivers/pmd.cc:7:0:
> >> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:
> >> In function ‘int eth_dev_pci_specific_init(rte_eth_dev*, void*)’:
> >> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:75:35:·
> >> error: invalid conversion from ‘void*’ to ‘rte_pci_device*’
> >> [-fpermissive]
> >>    struct rte_pci_device *pci_dev = bus_device;
> >>                                     ^~~~~~~~~~
> >> make[1]: *** [drivers/pmd.o] Error 1
> >>
> >> $ g++ --version
> >> g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0
> > This problem is while building a driver with c++ compiler, but since the
> > solution is a simple casting, which doesn't have any side affect, I am for
> > getting the patch, any objection?
> 
> It should be a decision that we support PMDs in C++ (if I understand
> the usage correctly) and it will require all headers to be compatible and
> ideally it should be checked by build automation (otherwise we will break
> and fix it pretty often).

Or as alternative, we probably can claim that PMDs in C++ are not supported,
and if people like to do that - they have to deal with it on their own
(create a C wrapper file, or so).
Konstantin

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] ethdev: missing typecast causes C++ build error

[Stephen Hemminger]

On Tue, 16 Apr 2019 16:46:14 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andrew Rybchenko
> > Sent: Tuesday, April 16, 2019 5:40 PM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>; David Christensen <drc@linux.vnet.ibm.com>; thomas@monjalon.net
> > Cc: dev@dpdk.org; radhika.chirra@ibm.com; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v2] ethdev: missing typecast causes C++ build error
> > 
> > On 4/16/19 7:31 PM, Ferruh Yigit wrote:  
> > > On 4/10/2019 10:36 PM, David Christensen wrote:  
> > >> The function eth_dev_pci_specific_init is missing a typecast to
> > >> (struct rte_pci_device *) for the input argument bus_device.
> > >> This causes build issues in the GNU C++ compiler.
> > >>
> > >> [CXX]       g++ -o utils/pcap_handle.o -c utils/pcap_handle.cc -g3·
> > >> -ggdb3 -mcpu=native -mtune=native -isystem·
> > >> /home/rchirra/gen/io/ppc64include -std=gnu++11 -flax-vector-conversions·
> > >> -Werror -isystem /home/rchirra/gen/io
> > >>
> > >> In file included from drivers/pmd.cc:7:0:
> > >> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:
> > >> In function ‘int eth_dev_pci_specific_init(rte_eth_dev*, void*)’:
> > >> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:75:35:·
> > >> error: invalid conversion from ‘void*’ to ‘rte_pci_device*’
> > >> [-fpermissive]
> > >>    struct rte_pci_device *pci_dev = bus_device;
> > >>                                     ^~~~~~~~~~
> > >> make[1]: *** [drivers/pmd.o] Error 1
> > >>
> > >> $ g++ --version
> > >> g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0  
> > > This problem is while building a driver with c++ compiler, but since the
> > > solution is a simple casting, which doesn't have any side affect, I am for
> > > getting the patch, any objection?  
> > 
> > It should be a decision that we support PMDs in C++ (if I understand
> > the usage correctly) and it will require all headers to be compatible and
> > ideally it should be checked by build automation (otherwise we will break
> > and fix it pretty often).  
> 
> Or as alternative, we probably can claim that PMDs in C++ are not supported,
> and if people like to do that - they have to deal with it on their own
> (create a C wrapper file, or so).
> Konstantin
> 


+1 no drivers or other parts of EAL in C++

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] ethdev: missing typecast causes C++ build error

[David Christensen]

>>>>> In file included from drivers/pmd.cc:7:0:
>>>>> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:
>>>>> In function ‘int eth_dev_pci_specific_init(rte_eth_dev*, void*)’:
>>>>> /home/rchirra/gen/io/deps/dpdk/build/include/rte_ethdev_pci.h:75:35:·
>>>>> error: invalid conversion from ‘void*’ to ‘rte_pci_device*’
>>>>> [-fpermissive]
>>>>>     struct rte_pci_device *pci_dev = bus_device;
>>>>>                                      ^~~~~~~~~~
>>>>> make[1]: *** [drivers/pmd.o] Error 1
>>>>>
>>>>> $ g++ --version
>>>>> g++ (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0
>>>> This problem is while building a driver with c++ compiler, but since the
>>>> solution is a simple casting, which doesn't have any side affect, I am for
>>>> getting the patch, any objection?
>>>
>>> It should be a decision that we support PMDs in C++ (if I understand
>>> the usage correctly) and it will require all headers to be compatible and
>>> ideally it should be checked by build automation (otherwise we will break
>>> and fix it pretty often).
>>
>> Or as alternative, we probably can claim that PMDs in C++ are not supported,
>> and if people like to do that - they have to deal with it on their own
>> (create a C wrapper file, or so).
>> Konstantin
>>
> +1 no drivers or other parts of EAL in C++
> 
I've learned more about the intended usage in this case and it turns out 
not to be a PMD at all, it's an application that's reaching into the EAL 
in what I consider an inappropriate way.  As a result I'm withdrawing 
the patch request and I'll work with the developers to find an alternate 
solution to their problem.

Dave