| Message ID | 20240910152254.21238-1-qianqiang.liu@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ag71xx: remove dead code path | expand |
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 > >
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 > >
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
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?
On Thu, 12 Sep 2024 23:46:18 +0800 Qianqiang Liu wrote:
> Could you please review this patch?
My preference is to combine the removal with Oleksij's suggestion
of adding the drop/error increment at the right spot.
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;
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(-)