[dpdk-dev] [PATCH BUG 335 1/3] net/dpaa: fix compilation error with 0 headroom

[dpdk-dev] [PATCH BUG 335 1/3] net/dpaa: fix compilation error with 0 headroom

[Hemant Agrawal]

When using RTE_PKTMBUF_HEADROOM as 0, dpaa driver throws compilation error
error "Annotation requirement is more than RTE_PKTMBUF_HEADROOM"

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

Fixes: ff9e112d7870 ("net/dpaa: add NXP DPAA PMD driver skeleton")
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/net/dpaa/dpaa_ethdev.c | 10 ++++++++++
 drivers/net/dpaa/dpaa_ethdev.h |  4 ----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index adc0bd5ac..7154fb9b4 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -1469,6 +1469,16 @@ rte_dpaa_probe(struct rte_dpaa_driver *dpaa_drv __rte_unused,
 
 	PMD_INIT_FUNC_TRACE();
 
+	if ((DPAA_MBUF_HW_ANNOTATION + DPAA_FD_PTA_SIZE) >
+		RTE_PKTMBUF_HEADROOM) {
+		DPAA_PMD_ERR(
+		"RTE_PKTMBUF_HEADROOM(%d) shall be > DPAA Annotation req(%d)",
+		RTE_PKTMBUF_HEADROOM,
+		DPAA_MBUF_HW_ANNOTATION + DPAA_FD_PTA_SIZE);
+
+		return -1;
+	}
+
 	/* In case of secondary process, the device is already configured
 	 * and no further action is required, except portal initialization
 	 * and verifying secondary attachment to port name.
diff --git a/drivers/net/dpaa/dpaa_ethdev.h b/drivers/net/dpaa/dpaa_ethdev.h
index 18bc7dfa8..f63a5f164 100644
--- a/drivers/net/dpaa/dpaa_ethdev.h
+++ b/drivers/net/dpaa/dpaa_ethdev.h
@@ -22,10 +22,6 @@
 #define DPAA_MBUF_HW_ANNOTATION		64
 #define DPAA_FD_PTA_SIZE		64
 
-#if (DPAA_MBUF_HW_ANNOTATION + DPAA_FD_PTA_SIZE) > RTE_PKTMBUF_HEADROOM
-#error "Annotation requirement is more than RTE_PKTMBUF_HEADROOM"
-#endif
-
 /* mbuf->seqn will be used to store event entry index for
  * driver specific usage. For parallel mode queues, invalid
  * index will be set and for atomic mode queues, valid value
-- 
2.17.1

[dpdk-dev] [PATCH BUG 335 2/3] bus/fslmc: fix compilation error with 0 headroom

[Hemant Agrawal]

When using RTE_PKTMBUF_HEADROOM as 0, dpaa driver throws compilation error
error "Annotation requirement is more than RTE_PKTMBUF_HEADROOM"

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

Fixes: beb2a7865dda ("bus/fslmc: define hardware annotation area size")
Cc: stable@dpdk.org

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/fslmc/portal/dpaa2_hw_pvt.h |  4 ----
 drivers/net/dpaa2/dpaa2_ethdev.c        | 10 ++++++++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/fslmc/portal/dpaa2_hw_pvt.h b/drivers/bus/fslmc/portal/dpaa2_hw_pvt.h
index 8644761db..4bb6b26c7 100644
--- a/drivers/bus/fslmc/portal/dpaa2_hw_pvt.h
+++ b/drivers/bus/fslmc/portal/dpaa2_hw_pvt.h
@@ -72,10 +72,6 @@
 #define DPAA2_MBUF_HW_ANNOTATION	64
 #define DPAA2_FD_PTA_SIZE		0
 
-#if (DPAA2_MBUF_HW_ANNOTATION + DPAA2_FD_PTA_SIZE) > RTE_PKTMBUF_HEADROOM
-#error "Annotation requirement is more than RTE_PKTMBUF_HEADROOM"
-#endif
-
 /* we will re-use the HEADROOM for annotation in RX */
 #define DPAA2_HW_BUF_RESERVE	0
 #define DPAA2_PACKET_LAYOUT_ALIGN	64 /*changing from 256 */
diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 03f69599c..dd6a78f9f 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -2319,6 +2319,16 @@ rte_dpaa2_probe(struct rte_dpaa2_driver *dpaa2_drv,
 	struct rte_eth_dev *eth_dev;
 	int diag;
 
+	if ((DPAA2_MBUF_HW_ANNOTATION + DPAA2_FD_PTA_SIZE) >
+		RTE_PKTMBUF_HEADROOM) {
+		DPAA2_PMD_ERR(
+		"RTE_PKTMBUF_HEADROOM(%d) shall be > DPAA2 Annotation req(%d)",
+		RTE_PKTMBUF_HEADROOM,
+		DPAA2_MBUF_HW_ANNOTATION + DPAA2_FD_PTA_SIZE);
+
+		return -1;
+	}
+
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		eth_dev = rte_eth_dev_allocate(dpaa2_dev->device.name);
 		if (!eth_dev)
-- 
2.17.1

[dpdk-dev] [PATCH BUG 335 3/3] net/virtio: fix compilation error with 0 headroom

[Hemant Agrawal]

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

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

Re: [dpdk-dev] [PATCH BUG 335 3/3] net/virtio: fix compilation error with 0 headroom

[Olivier Matz]

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
> 

Re: [dpdk-dev] [PATCH BUG 335 3/3] net/virtio: fix compilation error with 0 headroom

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH BUG 335 3/3] net/virtio: fix compilation error with 0 headroom

[Hemant Agrawal]

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

Re: [dpdk-dev] [PATCH BUG 335 3/3] net/virtio: fix compilation error with 0 headroom

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH BUG 335 3/3] net/virtio: fix compilation error with 0 headroom

[Thomas Monjalon]

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.