netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] net: ti: icssg-prueth: Fix skb handling for XDP_PASS
@ 2025-07-21 12:49 Meghana Malladi
  2025-07-21 22:02 ` Jacob Keller
  2025-07-24  0:19 ` Jakub Kicinski
  0 siblings, 2 replies; 4+ messages in thread
From: Meghana Malladi @ 2025-07-21 12:49 UTC (permalink / raw)
  To: namcao, jacob.e.keller, m-malladi, sdf, john.fastabend, hawk,
	daniel, ast, pabeni, kuba, edumazet, davem, andrew+netdev
  Cc: bpf, linux-kernel, netdev, linux-arm-kernel, srk,
	Vignesh Raghavendra, Roger Quadros, danishanwar

emac_rx_packet() is a common function for handling traffic
for both xdp and non-xdp use cases. Use common logic for
handling skb with or without xdp to prevent any incorrect
packet processing. This patch fixes ping working with
XDP_PASS for icssg driver.

Fixes: 62aa3246f4623 ("net: ti: icssg-prueth: Add XDP support")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_common.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index 12f25cec6255..a0e7def33e8e 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_common.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
@@ -757,15 +757,12 @@ static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id, u32 *xdp_state)
 		xdp_prepare_buff(&xdp, pa, PRUETH_HEADROOM, pkt_len, false);
 
 		*xdp_state = emac_run_xdp(emac, &xdp, page, &pkt_len);
-		if (*xdp_state == ICSSG_XDP_PASS)
-			skb = xdp_build_skb_from_buff(&xdp);
-		else
+		if (*xdp_state != ICSSG_XDP_PASS)
 			goto requeue;
-	} else {
-		/* prepare skb and send to n/w stack */
-		skb = napi_build_skb(pa, PAGE_SIZE);
 	}
 
+	/* prepare skb and send to n/w stack */
+	skb = napi_build_skb(pa, PAGE_SIZE);
 	if (!skb) {
 		ndev->stats.rx_dropped++;
 		page_pool_recycle_direct(pool, page);

base-commit: 81e0db8e839822b8380ce4716cd564a593ccbfc5
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net: ti: icssg-prueth: Fix skb handling for XDP_PASS
  2025-07-21 12:49 [PATCH net] net: ti: icssg-prueth: Fix skb handling for XDP_PASS Meghana Malladi
@ 2025-07-21 22:02 ` Jacob Keller
  2025-07-24  0:19 ` Jakub Kicinski
  1 sibling, 0 replies; 4+ messages in thread
From: Jacob Keller @ 2025-07-21 22:02 UTC (permalink / raw)
  To: Meghana Malladi, namcao, sdf, john.fastabend, hawk, daniel, ast,
	pabeni, kuba, edumazet, davem, andrew+netdev
  Cc: bpf, linux-kernel, netdev, linux-arm-kernel, srk,
	Vignesh Raghavendra, Roger Quadros, danishanwar


[-- Attachment #1.1: Type: text/plain, Size: 1636 bytes --]



On 7/21/2025 5:49 AM, Meghana Malladi wrote:
> emac_rx_packet() is a common function for handling traffic
> for both xdp and non-xdp use cases. Use common logic for
> handling skb with or without xdp to prevent any incorrect
> packet processing. This patch fixes ping working with
> XDP_PASS for icssg driver.
> 
> Fixes: 62aa3246f4623 ("net: ti: icssg-prueth: Add XDP support")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_common.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index 12f25cec6255..a0e7def33e8e 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -757,15 +757,12 @@ static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id, u32 *xdp_state)
>  		xdp_prepare_buff(&xdp, pa, PRUETH_HEADROOM, pkt_len, false);
>  
>  		*xdp_state = emac_run_xdp(emac, &xdp, page, &pkt_len);
> -		if (*xdp_state == ICSSG_XDP_PASS)
> -			skb = xdp_build_skb_from_buff(&xdp);
> -		else
> +		if (*xdp_state != ICSSG_XDP_PASS)
>  			goto requeue;
> -	} else {
> -		/* prepare skb and send to n/w stack */
> -		skb = napi_build_skb(pa, PAGE_SIZE);
>  	}
>  
> +	/* prepare skb and send to n/w stack */
> +	skb = napi_build_skb(pa, PAGE_SIZE);
>  	if (!skb) {
>  		ndev->stats.rx_dropped++;
>  		page_pool_recycle_direct(pool, page);
> 
> base-commit: 81e0db8e839822b8380ce4716cd564a593ccbfc5

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net: ti: icssg-prueth: Fix skb handling for XDP_PASS
  2025-07-21 12:49 [PATCH net] net: ti: icssg-prueth: Fix skb handling for XDP_PASS Meghana Malladi
  2025-07-21 22:02 ` Jacob Keller
@ 2025-07-24  0:19 ` Jakub Kicinski
  2025-07-30 15:14   ` Meghana Malladi
  1 sibling, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2025-07-24  0:19 UTC (permalink / raw)
  To: Meghana Malladi
  Cc: namcao, jacob.e.keller, sdf, john.fastabend, hawk, daniel, ast,
	pabeni, edumazet, davem, andrew+netdev, bpf, linux-kernel, netdev,
	linux-arm-kernel, srk, Vignesh Raghavendra, Roger Quadros,
	danishanwar

On Mon, 21 Jul 2025 18:19:18 +0530 Meghana Malladi wrote:
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index 12f25cec6255..a0e7def33e8e 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -757,15 +757,12 @@ static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id, u32 *xdp_state)
>  		xdp_prepare_buff(&xdp, pa, PRUETH_HEADROOM, pkt_len, false);
>  
>  		*xdp_state = emac_run_xdp(emac, &xdp, page, &pkt_len);
> -		if (*xdp_state == ICSSG_XDP_PASS)
> -			skb = xdp_build_skb_from_buff(&xdp);
> -		else
> +		if (*xdp_state != ICSSG_XDP_PASS)
>  			goto requeue;
> -	} else {
> -		/* prepare skb and send to n/w stack */
> -		skb = napi_build_skb(pa, PAGE_SIZE);
>  	}
>  
> +	/* prepare skb and send to n/w stack */
> +	skb = napi_build_skb(pa, PAGE_SIZE);
>  	if (!skb) {
>  		ndev->stats.rx_dropped++;
>  		page_pool_recycle_direct(pool, page);

I'm not sure this is correct. We seem to hardcode headroom to
PRUETH_HEADROOM lower in this function. If XDP adds or removes
network headers and then returns XDP_PASS we'll look for the packet 
at the wrong offset.

We just merged some XDP tests, could you try running 
tools/testing/selftests/drivers/net/xdp.py ? 
Some general instructions can be found here:
https://github.com/linux-netdev/nipa/wiki/Running-driver-tests

Not sure how stable the test is for all NICs but I think the 
 xdp.test_xdp_native_adjst_head_grow_data
test case will exercise what I'm suspecting will fail.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net: ti: icssg-prueth: Fix skb handling for XDP_PASS
  2025-07-24  0:19 ` Jakub Kicinski
@ 2025-07-30 15:14   ` Meghana Malladi
  0 siblings, 0 replies; 4+ messages in thread
From: Meghana Malladi @ 2025-07-30 15:14 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: namcao, jacob.e.keller, sdf, john.fastabend, hawk, daniel, ast,
	pabeni, edumazet, davem, andrew+netdev, bpf, linux-kernel, netdev,
	linux-arm-kernel, srk, Vignesh Raghavendra, Roger Quadros,
	danishanwar

Hi Jakub,

On 7/24/25 05:49, Jakub Kicinski wrote:
> On Mon, 21 Jul 2025 18:19:18 +0530 Meghana Malladi wrote:
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
>> index 12f25cec6255..a0e7def33e8e 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
>> @@ -757,15 +757,12 @@ static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id, u32 *xdp_state)
>>   		xdp_prepare_buff(&xdp, pa, PRUETH_HEADROOM, pkt_len, false);
>>   
>>   		*xdp_state = emac_run_xdp(emac, &xdp, page, &pkt_len);
>> -		if (*xdp_state == ICSSG_XDP_PASS)
>> -			skb = xdp_build_skb_from_buff(&xdp);
>> -		else
>> +		if (*xdp_state != ICSSG_XDP_PASS)
>>   			goto requeue;
>> -	} else {
>> -		/* prepare skb and send to n/w stack */
>> -		skb = napi_build_skb(pa, PAGE_SIZE);
>>   	}
>>   
>> +	/* prepare skb and send to n/w stack */
>> +	skb = napi_build_skb(pa, PAGE_SIZE);
>>   	if (!skb) {
>>   		ndev->stats.rx_dropped++;
>>   		page_pool_recycle_direct(pool, page);
> 
> I'm not sure this is correct. We seem to hardcode headroom to
> PRUETH_HEADROOM lower in this function. If XDP adds or removes
> network headers and then returns XDP_PASS we'll look for the packet
> at the wrong offset.
> 

Yes that makes sense. Thanks for pointing it out. I think I have the 
right fix in mind. Will post it shortly as v2.

> We just merged some XDP tests, could you try running
> tools/testing/selftests/drivers/net/xdp.py ?
> Some general instructions can be found here:
> https://github.com/linux-netdev/nipa/wiki/Running-driver-tests
> 
> Not sure how stable the test is for all NICs but I think the
>   xdp.test_xdp_native_adjst_head_grow_data
> test case will exercise what I'm suspecting will fail.

It took me some time to install all the dependencies and get this tool 
running with the fix I mentioned above [1]. But I am not sure the error 
logs I got [2] are the one which you were expecting or some stability 
issue with the test. Can you please take a look at it.

[1]: 
https://gist.github.com/MeghanaMalladiTI/ce6a440f05fbdfb2d4363f672296d7d8
[2]:https://gist.github.com/MeghanaMalladiTI/52fe9d06c114562be08105d73f91ba62 


If the fix I attached here [1] makes sense, I will go ahead and post 
this as v2.

Thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-07-30 15:15 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-21 12:49 [PATCH net] net: ti: icssg-prueth: Fix skb handling for XDP_PASS Meghana Malladi
2025-07-21 22:02 ` Jacob Keller
2025-07-24  0:19 ` Jakub Kicinski
2025-07-30 15:14   ` Meghana Malladi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).