diff mbox series

[BUG,335,3/3] net/virtio: fix compilation error with 0 headroom

Message ID 20190725110645.8817-3-hemant.agrawal@nxp.com
State New
Headers show
Series [BUG,335,1/3] net/dpaa: fix compilation error with 0 headroom | expand

Commit Message

Hemant Agrawal July 25, 2019, 11:06 a.m. UTC 
When using RTE_PKTMBUF_HEADROOM as 0, virito ethdev driver throws
compilation error
virtio_ethdev.c:1851:2: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM
	< sizeof(struct virtio_net_hdr_mrg_rxbuf));

This patch change it into run-time check.
Reported as: https://bugs.dpdk.org/show_bug.cgi?id=335

Fixes: 198ab33677c9 ("net/virtio: move device initialization in a function")
Cc: stable@dpdk.org
Cc: Olivier Matz <olivier.matz@6wind.com>

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

---
 drivers/net/virtio/virtio_ethdev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Olivier Matz July 26, 2019, 12:25 p.m. UTC | #1 
Hi,

On Thu, Jul 25, 2019 at 04:36:45PM +0530, Hemant Agrawal wrote:
> When using RTE_PKTMBUF_HEADROOM as 0, virito ethdev driver throws

> compilation error

> virtio_ethdev.c:1851:2: note: in expansion of macro ‘RTE_BUILD_BUG_ON’

> RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM

> 	< sizeof(struct virtio_net_hdr_mrg_rxbuf));

> 

> This patch change it into run-time check.

> Reported as: https://bugs.dpdk.org/show_bug.cgi?id=335

> 

> Fixes: 198ab33677c9 ("net/virtio: move device initialization in a function")


I think the proper commit is:
Fixes: dec08c28c0b3 ("virtio: check packet headroom at compile time")

It looks this patch more or less reverts this old commit.
+CC Stephen


> Cc: stable@dpdk.org

> Cc: Olivier Matz <olivier.matz@6wind.com>

> 

> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

> ---

>  drivers/net/virtio/virtio_ethdev.c | 9 ++++++++-

>  1 file changed, 8 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c

> index 62c827443..3e2e8bd2a 100644

> --- a/drivers/net/virtio/virtio_ethdev.c

> +++ b/drivers/net/virtio/virtio_ethdev.c

> @@ -1848,7 +1848,14 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)

>  	struct virtio_hw *hw = eth_dev->data->dev_private;

>  	int ret;

>  

> -	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));

> +	if (sizeof(struct virtio_net_hdr_mrg_rxbuf) > RTE_PKTMBUF_HEADROOM) {

> +		PMD_INIT_LOG(ERR,

> +			"Not sufficient headroom required = %d, avail = %d",

> +			(int)sizeof(struct virtio_net_hdr_mrg_rxbuf),

> +			RTE_PKTMBUF_HEADROOM);

> +

> +		return -1;

> +	}

>  

>  	eth_dev->dev_ops = &virtio_eth_dev_ops;

>  

> -- 

> 2.17.1

>
Maxime Coquelin July 30, 2019, 1:35 p.m. UTC | #2 
On 7/26/19 2:25 PM, Olivier Matz wrote:
> Hi,

> 

> On Thu, Jul 25, 2019 at 04:36:45PM +0530, Hemant Agrawal wrote:

>> When using RTE_PKTMBUF_HEADROOM as 0, virito ethdev driver throws

>> compilation error

>> virtio_ethdev.c:1851:2: note: in expansion of macro ‘RTE_BUILD_BUG_ON’

>> RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM

>> 	< sizeof(struct virtio_net_hdr_mrg_rxbuf));

>>

>> This patch change it into run-time check.

>> Reported as: https://bugs.dpdk.org/show_bug.cgi?id=335

>>

>> Fixes: 198ab33677c9 ("net/virtio: move device initialization in a function")

> 

> I think the proper commit is:

> Fixes: dec08c28c0b3 ("virtio: check packet headroom at compile time")


Indeed.

> It looks this patch more or less reverts this old commit.

> +CC Stephen


I wonder whether we could have a warning at build time so that the one
who builds DPDK is aware some driver may not be usable, in addition to
the below patch that fails virtio-net init.

Any thoughts?

Thanks,
Maxime

>> Cc: stable@dpdk.org

>> Cc: Olivier Matz <olivier.matz@6wind.com>

>>

>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

>> ---

>>   drivers/net/virtio/virtio_ethdev.c | 9 ++++++++-

>>   1 file changed, 8 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c

>> index 62c827443..3e2e8bd2a 100644

>> --- a/drivers/net/virtio/virtio_ethdev.c

>> +++ b/drivers/net/virtio/virtio_ethdev.c

>> @@ -1848,7 +1848,14 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)

>>   	struct virtio_hw *hw = eth_dev->data->dev_private;

>>   	int ret;

>>   

>> -	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));

>> +	if (sizeof(struct virtio_net_hdr_mrg_rxbuf) > RTE_PKTMBUF_HEADROOM) {

>> +		PMD_INIT_LOG(ERR,

>> +			"Not sufficient headroom required = %d, avail = %d",

>> +			(int)sizeof(struct virtio_net_hdr_mrg_rxbuf),

>> +			RTE_PKTMBUF_HEADROOM);

>> +

>> +		return -1;

>> +	}

>>   

>>   	eth_dev->dev_ops = &virtio_eth_dev_ops;

>>   

>> -- 

>> 2.17.1

>>

>
Hemant Agrawal Aug. 1, 2019, 8:09 a.m. UTC | #3 
HI,
> On 7/26/19 2:25 PM, Olivier Matz wrote:

> > Hi,

> >

> > On Thu, Jul 25, 2019 at 04:36:45PM +0530, Hemant Agrawal wrote:

> >> When using RTE_PKTMBUF_HEADROOM as 0, virito ethdev driver throws

> >> compilation error

> >> virtio_ethdev.c:1851:2: note: in expansion of macro ‘RTE_BUILD_BUG_ON’

> >> RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM

> >> 	< sizeof(struct virtio_net_hdr_mrg_rxbuf));

> >>

> >> This patch change it into run-time check.

> >> Reported as:

> >>

> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbug

> >>

> s.dpdk.org%2Fshow_bug.cgi%3Fid%3D335&amp;data=02%7C01%7Chemant.

> agrawa

> >>

> l%40nxp.com%7C303b7755f6ff4c44721a08d714f2cbb5%7C686ea1d3bc2b4c6

> fa92c

> >>

> d99c5c301635%7C0%7C0%7C637000905339982654&amp;sdata=oQ4b4TZ36x

> WiHOtKE

> >> %2BwCZfy2ND%2FgP%2FESfGL%2FEdoJbYI%3D&amp;reserved=0

> >>

> >> Fixes: 198ab33677c9 ("net/virtio: move device initialization in a

> >> function")

> >

> > I think the proper commit is:

> > Fixes: dec08c28c0b3 ("virtio: check packet headroom at compile time")

> 

> Indeed.

> 

> > It looks this patch more or less reverts this old commit.

> > +CC Stephen

> 

> I wonder whether we could have a warning at build time so that the one who

> builds DPDK is aware some driver may not be usable, in addition to the

> below patch that fails virtio-net init.


[Hemant] I will also prefer compile time check instead of run-time check for any non-default configs.
If someone is modifying the config, he can very well disable the drivers, which don't like those settings. 

However, earlier discussion w.r.t this bug moved in other direction to make DPDK compliable for all cases and allow regress testing.

> 

> Any thoughts?

> 

> Thanks,

> Maxime

> 

> >> Cc: stable@dpdk.org

> >> Cc: Olivier Matz <olivier.matz@6wind.com>

> >>

> >> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

> >> ---

> >>   drivers/net/virtio/virtio_ethdev.c | 9 ++++++++-

> >>   1 file changed, 8 insertions(+), 1 deletion(-)

> >>

> >> diff --git a/drivers/net/virtio/virtio_ethdev.c

> >> b/drivers/net/virtio/virtio_ethdev.c

> >> index 62c827443..3e2e8bd2a 100644

> >> --- a/drivers/net/virtio/virtio_ethdev.c

> >> +++ b/drivers/net/virtio/virtio_ethdev.c

> >> @@ -1848,7 +1848,14 @@ eth_virtio_dev_init(struct rte_eth_dev

> *eth_dev)

> >>   	struct virtio_hw *hw = eth_dev->data->dev_private;

> >>   	int ret;

> >>

> >> -	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct

> virtio_net_hdr_mrg_rxbuf));

> >> +	if (sizeof(struct virtio_net_hdr_mrg_rxbuf) >

> RTE_PKTMBUF_HEADROOM) {

> >> +		PMD_INIT_LOG(ERR,

> >> +			"Not sufficient headroom required = %d, avail = %d",

> >> +			(int)sizeof(struct virtio_net_hdr_mrg_rxbuf),

> >> +			RTE_PKTMBUF_HEADROOM);

> >> +

> >> +		return -1;

> >> +	}

> >>

> >>   	eth_dev->dev_ops = &virtio_eth_dev_ops;

> >>

> >> --

> >> 2.17.1

> >>

> >
Maxime Coquelin Aug. 5, 2019, 1:07 p.m. UTC | #4 
On 8/1/19 10:09 AM, Hemant Agrawal wrote:
> HI,

>> On 7/26/19 2:25 PM, Olivier Matz wrote:

>>> Hi,

>>>

>>> On Thu, Jul 25, 2019 at 04:36:45PM +0530, Hemant Agrawal wrote:

>>>> When using RTE_PKTMBUF_HEADROOM as 0, virito ethdev driver throws

>>>> compilation error

>>>> virtio_ethdev.c:1851:2: note: in expansion of macro ‘RTE_BUILD_BUG_ON’

>>>> RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM

>>>> 	< sizeof(struct virtio_net_hdr_mrg_rxbuf));

>>>>

>>>> This patch change it into run-time check.

>>>> Reported as:

>>>>

>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbug

>>>>

>> s.dpdk.org%2Fshow_bug.cgi%3Fid%3D335&amp;data=02%7C01%7Chemant.

>> agrawa

>>>>

>> l%40nxp.com%7C303b7755f6ff4c44721a08d714f2cbb5%7C686ea1d3bc2b4c6

>> fa92c

>>>>

>> d99c5c301635%7C0%7C0%7C637000905339982654&amp;sdata=oQ4b4TZ36x

>> WiHOtKE

>>>> %2BwCZfy2ND%2FgP%2FESfGL%2FEdoJbYI%3D&amp;reserved=0

>>>>

>>>> Fixes: 198ab33677c9 ("net/virtio: move device initialization in a

>>>> function")

>>>

>>> I think the proper commit is:

>>> Fixes: dec08c28c0b3 ("virtio: check packet headroom at compile time")

>>

>> Indeed.

>>

>>> It looks this patch more or less reverts this old commit.

>>> +CC Stephen

>>

>> I wonder whether we could have a warning at build time so that the one who

>> builds DPDK is aware some driver may not be usable, in addition to the

>> below patch that fails virtio-net init.

> 

> [Hemant] I will also prefer compile time check instead of run-time check for any non-default configs.

> If someone is modifying the config, he can very well disable the drivers, which don't like those settings. 

> 

> However, earlier discussion w.r.t this bug moved in other direction to make DPDK compliable for all cases and allow regress testing.


Ok, I don't have a strong opinion on this, so feel free to apply this
patch as is. We can add a build-ime warning later if we find it useful.

Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>


Thanks,
Maxime

> 

>>

>> Any thoughts?

>>

>> Thanks,

>> Maxime

>>

>>>> Cc: stable@dpdk.org

>>>> Cc: Olivier Matz <olivier.matz@6wind.com>

>>>>

>>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

>>>> ---

>>>>   drivers/net/virtio/virtio_ethdev.c | 9 ++++++++-

>>>>   1 file changed, 8 insertions(+), 1 deletion(-)

>>>>

>>>> diff --git a/drivers/net/virtio/virtio_ethdev.c

>>>> b/drivers/net/virtio/virtio_ethdev.c

>>>> index 62c827443..3e2e8bd2a 100644

>>>> --- a/drivers/net/virtio/virtio_ethdev.c

>>>> +++ b/drivers/net/virtio/virtio_ethdev.c

>>>> @@ -1848,7 +1848,14 @@ eth_virtio_dev_init(struct rte_eth_dev

>> *eth_dev)

>>>>   	struct virtio_hw *hw = eth_dev->data->dev_private;

>>>>   	int ret;

>>>>

>>>> -	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct

>> virtio_net_hdr_mrg_rxbuf));

>>>> +	if (sizeof(struct virtio_net_hdr_mrg_rxbuf) >

>> RTE_PKTMBUF_HEADROOM) {

>>>> +		PMD_INIT_LOG(ERR,

>>>> +			"Not sufficient headroom required = %d, avail = %d",

>>>> +			(int)sizeof(struct virtio_net_hdr_mrg_rxbuf),

>>>> +			RTE_PKTMBUF_HEADROOM);

>>>> +

>>>> +		return -1;

>>>> +	}

>>>>

>>>>   	eth_dev->dev_ops = &virtio_eth_dev_ops;

>>>>

>>>> --

>>>> 2.17.1

>>>>

>>>
Thomas Monjalon Aug. 5, 2019, 5:26 p.m. UTC | #5 
05/08/2019 15:07, Maxime Coquelin:
> On 8/1/19 10:09 AM, Hemant Agrawal wrote:

> >> On 7/26/19 2:25 PM, Olivier Matz wrote:

> >>> On Thu, Jul 25, 2019 at 04:36:45PM +0530, Hemant Agrawal wrote:

> >>>> When using RTE_PKTMBUF_HEADROOM as 0, virito ethdev driver throws

> >>>> compilation error

> >>>> virtio_ethdev.c:1851:2: note: in expansion of macro ‘RTE_BUILD_BUG_ON’

> >>>> RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM

> >>>> 	< sizeof(struct virtio_net_hdr_mrg_rxbuf));

> >>>>

> >>>> This patch change it into run-time check.

[...]
> >>>>

> >>>> Fixes: 198ab33677c9 ("net/virtio: move device initialization in a

> >>>> function")

> >>>

> >>> I think the proper commit is:

> >>> Fixes: dec08c28c0b3 ("virtio: check packet headroom at compile time")

> >>

> >> Indeed.

> >>

> >>> It looks this patch more or less reverts this old commit.

> >>> +CC Stephen

> >>

> >> I wonder whether we could have a warning at build time so that the one who

> >> builds DPDK is aware some driver may not be usable, in addition to the

> >> below patch that fails virtio-net init.

> > 

> > [Hemant] I will also prefer compile time check instead of run-time check for any non-default configs.

> > If someone is modifying the config, he can very well disable the drivers, which don't like those settings. 

> > 

> > However, earlier discussion w.r.t this bug moved in other direction to make DPDK compliable for all cases and allow regress testing.

> 

> Ok, I don't have a strong opinion on this, so feel free to apply this

> patch as is. We can add a build-ime warning later if we find it useful.

> 

> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>


Series applied, thanks.

I think the right fix should be a check in meson.
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 62c827443..3e2e8bd2a 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1848,7 +1848,14 @@  eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	struct virtio_hw *hw = eth_dev->data->dev_private;
 	int ret;
 
-	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr_mrg_rxbuf));
+	if (sizeof(struct virtio_net_hdr_mrg_rxbuf) > RTE_PKTMBUF_HEADROOM) {
+		PMD_INIT_LOG(ERR,
+			"Not sufficient headroom required = %d, avail = %d",
+			(int)sizeof(struct virtio_net_hdr_mrg_rxbuf),
+			RTE_PKTMBUF_HEADROOM);
+
+		return -1;
+	}
 
 	eth_dev->dev_ops = &virtio_eth_dev_ops;