* Re: [PATCH] net: ag71xx: remove dead code path
2024-09-10 15:22 [PATCH] net: ag71xx: remove dead code path Qianqiang Liu
@ 2024-09-10 15:58 ` Andrew Lunn
2024-09-11 7:53 ` Oleksij Rempel
2024-09-10 18:22 ` Rosen Penev
2024-09-12 15:46 ` Qianqiang Liu
2 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2024-09-10 15:58 UTC (permalink / raw)
To: Qianqiang Liu
Cc: chris.snook, davem, edumazet, kuba, pabeni, netdev, linux-kernel,
Oleksij Rempel
On Tue, Sep 10, 2024 at 11:22:54PM +0800, Qianqiang Liu wrote:
> The 'err' is always zero, so the following branch can never be executed:
> if (err) {
> ndev->stats.rx_dropped++;
> kfree_skb(skb);
> }
> Therefore, the 'if' statement can be removed.
This code was added by Oleksij Rempel <o.rempel@pengutronix.de>. It is
good to Cc: him, he might have useful comments.
Your changed does look correct, but maybe ret was actually supposed to
be set somewhere? Is there an actual bug hiding here somewhere?
Andrew
>
> Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
> ---
> drivers/net/ethernet/atheros/ag71xx.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 96a6189cc31e..5477f3f87e10 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -1616,7 +1616,6 @@ static int ag71xx_rx_packets(struct ag71xx *ag, int limit)
> unsigned int i = ring->curr & ring_mask;
> struct ag71xx_desc *desc = ag71xx_ring_desc(ring, i);
> int pktlen;
> - int err = 0;
>
> if (ag71xx_desc_empty(desc))
> break;
> @@ -1646,14 +1645,9 @@ static int ag71xx_rx_packets(struct ag71xx *ag, int limit)
> skb_reserve(skb, offset);
> skb_put(skb, pktlen);
>
> - if (err) {
> - ndev->stats.rx_dropped++;
> - kfree_skb(skb);
> - } else {
> - skb->dev = ndev;
> - skb->ip_summed = CHECKSUM_NONE;
> - list_add_tail(&skb->list, &rx_list);
> - }
> + skb->dev = ndev;
> + skb->ip_summed = CHECKSUM_NONE;
> + list_add_tail(&skb->list, &rx_list);
>
> next:
> ring->buf[i].rx.rx_buf = NULL;
> --
> 2.39.2
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: ag71xx: remove dead code path
2024-09-10 15:58 ` Andrew Lunn
@ 2024-09-11 7:53 ` Oleksij Rempel
0 siblings, 0 replies; 6+ messages in thread
From: Oleksij Rempel @ 2024-09-11 7:53 UTC (permalink / raw)
To: Andrew Lunn
Cc: Qianqiang Liu, chris.snook, davem, edumazet, kuba, pabeni, netdev,
linux-kernel
On Tue, Sep 10, 2024 at 05:58:22PM +0200, Andrew Lunn wrote:
> On Tue, Sep 10, 2024 at 11:22:54PM +0800, Qianqiang Liu wrote:
> > The 'err' is always zero, so the following branch can never be executed:
> > if (err) {
> > ndev->stats.rx_dropped++;
> > kfree_skb(skb);
> > }
> > Therefore, the 'if' statement can be removed.
>
> This code was added by Oleksij Rempel <o.rempel@pengutronix.de>. It is
> good to Cc: him, he might have useful comments.
Yes, please.
> Your changed does look correct, but maybe ret was actually supposed to
> be set somewhere? Is there an actual bug hiding here somewhere?
Hm, let's see... this issue existed in the openwrt code and I didn't
spotted it by upstreaming.
The only place which may fail in this part is napi_build_skb(), we will
need to count it probably with rx_errors++.
I'm ok with this patch, it can be reworked to use rx_errors++ on
napi_build_skb() instead or this can be done in a separate patch.
If you like to keep this patch as is, here is my:
Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
Thank you!
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ag71xx: remove dead code path
2024-09-10 15:22 [PATCH] net: ag71xx: remove dead code path Qianqiang Liu
2024-09-10 15:58 ` Andrew Lunn
@ 2024-09-10 18:22 ` Rosen Penev
2024-09-12 15:46 ` Qianqiang Liu
2 siblings, 0 replies; 6+ messages in thread
From: Rosen Penev @ 2024-09-10 18:22 UTC (permalink / raw)
To: Qianqiang Liu
Cc: chris.snook, davem, edumazet, kuba, pabeni, netdev, linux-kernel
On Tue, Sep 10, 2024 at 8:30 AM Qianqiang Liu <qianqiang.liu@163.com> wrote:
>
> The 'err' is always zero, so the following branch can never be executed:
> if (err) {
> ndev->stats.rx_dropped++;
> kfree_skb(skb);
> }
> Therefore, the 'if' statement can be removed.
>
> Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
Reviewed-by: Rosen Penev <rosenp@gmail.com>
err was used downstream in OpenWrt when platform data was used. In the
transition to OF, the function was removed. Which was back in 2019 at
this point.
> ---
> drivers/net/ethernet/atheros/ag71xx.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 96a6189cc31e..5477f3f87e10 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -1616,7 +1616,6 @@ static int ag71xx_rx_packets(struct ag71xx *ag, int limit)
> unsigned int i = ring->curr & ring_mask;
> struct ag71xx_desc *desc = ag71xx_ring_desc(ring, i);
> int pktlen;
> - int err = 0;
>
> if (ag71xx_desc_empty(desc))
> break;
> @@ -1646,14 +1645,9 @@ static int ag71xx_rx_packets(struct ag71xx *ag, int limit)
> skb_reserve(skb, offset);
> skb_put(skb, pktlen);
>
> - if (err) {
> - ndev->stats.rx_dropped++;
> - kfree_skb(skb);
> - } else {
> - skb->dev = ndev;
> - skb->ip_summed = CHECKSUM_NONE;
> - list_add_tail(&skb->list, &rx_list);
> - }
> + skb->dev = ndev;
> + skb->ip_summed = CHECKSUM_NONE;
> + list_add_tail(&skb->list, &rx_list);
>
> next:
> ring->buf[i].rx.rx_buf = NULL;
> --
> 2.39.2
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: ag71xx: remove dead code path
2024-09-10 15:22 [PATCH] net: ag71xx: remove dead code path Qianqiang Liu
2024-09-10 15:58 ` Andrew Lunn
2024-09-10 18:22 ` Rosen Penev
@ 2024-09-12 15:46 ` Qianqiang Liu
2024-09-13 0:36 ` Jakub Kicinski
2 siblings, 1 reply; 6+ messages in thread
From: Qianqiang Liu @ 2024-09-12 15:46 UTC (permalink / raw)
To: kuba; +Cc: andrew, o.rempel, rosenp, netdev, linux-kernel
On Tue, Sep 10, 2024 at 11:22:54PM +0800, Qianqiang Liu wrote:
> The 'err' is always zero, so the following branch can never be executed:
> if (err) {
> ndev->stats.rx_dropped++;
> kfree_skb(skb);
> }
> Therefore, the 'if' statement can be removed.
>
> Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
> ---
> drivers/net/ethernet/atheros/ag71xx.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 96a6189cc31e..5477f3f87e10 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -1616,7 +1616,6 @@ static int ag71xx_rx_packets(struct ag71xx *ag, int limit)
> unsigned int i = ring->curr & ring_mask;
> struct ag71xx_desc *desc = ag71xx_ring_desc(ring, i);
> int pktlen;
> - int err = 0;
>
> if (ag71xx_desc_empty(desc))
> break;
> @@ -1646,14 +1645,9 @@ static int ag71xx_rx_packets(struct ag71xx *ag, int limit)
> skb_reserve(skb, offset);
> skb_put(skb, pktlen);
>
> - if (err) {
> - ndev->stats.rx_dropped++;
> - kfree_skb(skb);
> - } else {
> - skb->dev = ndev;
> - skb->ip_summed = CHECKSUM_NONE;
> - list_add_tail(&skb->list, &rx_list);
> - }
> + skb->dev = ndev;
> + skb->ip_summed = CHECKSUM_NONE;
> + list_add_tail(&skb->list, &rx_list);
>
> next:
> ring->buf[i].rx.rx_buf = NULL;
> --
> 2.39.2
Hi Jakub,
Could you please review this patch?
--
Best,
Qianqiang Liu
^ permalink raw reply [flat|nested] 6+ messages in thread