net: gianfar: use alloc_ethdev_mqs

[PATCH net-next] net: gianfar: use alloc_ethdev_mqs

Posted by Rosen Penev 1 month, 2 weeks ago

From looking at git history, mqs was introduced after mq and after this
code was written. Having said that, mqs can be used as there is already
an RX queue variable in place. Not only that, mqs already sets the
num_xx_queues members. No need to open code this.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 3271de5844f8..7b47c7c49c08 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -669,7 +669,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 		return -EINVAL;
 	}
 
-	*pdev = alloc_etherdev_mq(sizeof(*priv), num_tx_qs);
+	*pdev = alloc_etherdev_mqs(sizeof(*priv), num_tx_qs, num_rx_qs);
 	dev = *pdev;
 	if (NULL == dev)
 		return -ENOMEM;
@@ -679,10 +679,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 
 	priv->mode = mode;
 
-	priv->num_tx_queues = num_tx_qs;
-	netif_set_real_num_rx_queues(dev, num_rx_qs);
-	priv->num_rx_queues = num_rx_qs;
-
 	err = gfar_alloc_tx_queues(priv);
 	if (err)
 		goto tx_alloc_failed;
-- 
2.54.0

Re: [PATCH net-next] net: gianfar: use alloc_ethdev_mqs

Posted by Andrew Lunn 1 month, 2 weeks ago

On Tue, Apr 28, 2026 at 03:30:58PM -0700, Rosen Penev wrote:
> >From looking at git history, mqs was introduced after mq and after this
> code was written. Having said that, mqs can be used as there is already
> an RX queue variable in place. Not only that, mqs already sets the
> num_xx_queues members. No need to open code this.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/freescale/gianfar.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 3271de5844f8..7b47c7c49c08 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -669,7 +669,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
>  		return -EINVAL;
>  	}
>  
> -	*pdev = alloc_etherdev_mq(sizeof(*priv), num_tx_qs);
> +	*pdev = alloc_etherdev_mqs(sizeof(*priv), num_tx_qs, num_rx_qs);
>  	dev = *pdev;
>  	if (NULL == dev)
>  		return -ENOMEM;
> @@ -679,10 +679,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
>  
>  	priv->mode = mode;
>  
> -	priv->num_tx_queues = num_tx_qs;
> -	netif_set_real_num_rx_queues(dev, num_rx_qs);
> -	priv->num_rx_queues = num_rx_qs;

Please add to the commit message an explanation of why these two
assignments can be removed, because it is not obvious.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next] net: gianfar: use alloc_ethdev_mqs

Posted by Rosen Penev 1 month, 2 weeks ago

On Tue, Apr 28, 2026 at 4:13 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Apr 28, 2026 at 03:30:58PM -0700, Rosen Penev wrote:
> > >From looking at git history, mqs was introduced after mq and after this
> > code was written. Having said that, mqs can be used as there is already
> > an RX queue variable in place. Not only that, mqs already sets the
> > num_xx_queues members. No need to open code this.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/ethernet/freescale/gianfar.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > index 3271de5844f8..7b47c7c49c08 100644
> > --- a/drivers/net/ethernet/freescale/gianfar.c
> > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > @@ -669,7 +669,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> >               return -EINVAL;
> >       }
> >
> > -     *pdev = alloc_etherdev_mq(sizeof(*priv), num_tx_qs);
> > +     *pdev = alloc_etherdev_mqs(sizeof(*priv), num_tx_qs, num_rx_qs);
> >       dev = *pdev;
> >       if (NULL == dev)
> >               return -ENOMEM;
> > @@ -679,10 +679,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> >
> >       priv->mode = mode;
> >
> > -     priv->num_tx_queues = num_tx_qs;
> > -     netif_set_real_num_rx_queues(dev, num_rx_qs);
> > -     priv->num_rx_queues = num_rx_qs;
>
> Please add to the commit message an explanation of why these two
> assignments can be removed, because it is not obvious.
I didn't explain that _mqs sets them?
>
>     Andrew
>
> ---
> pw-bot: cr

Re: [PATCH net-next] net: gianfar: use alloc_ethdev_mqs

Posted by Andrew Lunn 1 month, 2 weeks ago

> > > +     *pdev = alloc_etherdev_mqs(sizeof(*priv), num_tx_qs, num_rx_qs);
> > >       dev = *pdev;
> > >       if (NULL == dev)
> > >               return -ENOMEM;
> > > @@ -679,10 +679,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> > >
> > >       priv->mode = mode;
> > >
> > > -     priv->num_tx_queues = num_tx_qs;
> > > -     netif_set_real_num_rx_queues(dev, num_rx_qs);
> > > -     priv->num_rx_queues = num_rx_qs;
> >
> > Please add to the commit message an explanation of why these two
> > assignments can be removed, because it is not obvious.
> I didn't explain that _mqs sets them?

How can alloc_etherdev_mqs() set them? priv is opaque to the core. All
the core knows is the size of struct gfar_private, but nothing about
its layout, where num_tx_queues and num_rx_queues are within priv.

	Andrew

Re: [PATCH net-next] net: gianfar: use alloc_ethdev_mqs

Posted by Rosen Penev 1 month, 2 weeks ago

On Tue, Apr 28, 2026 at 5:41 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > > +     *pdev = alloc_etherdev_mqs(sizeof(*priv), num_tx_qs, num_rx_qs);
> > > >       dev = *pdev;
> > > >       if (NULL == dev)
> > > >               return -ENOMEM;
> > > > @@ -679,10 +679,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> > > >
> > > >       priv->mode = mode;
> > > >
> > > > -     priv->num_tx_queues = num_tx_qs;
> > > > -     netif_set_real_num_rx_queues(dev, num_rx_qs);
> > > > -     priv->num_rx_queues = num_rx_qs;
> > >
> > > Please add to the commit message an explanation of why these two
> > > assignments can be removed, because it is not obvious.
> > I didn't explain that _mqs sets them?
>
> How can alloc_etherdev_mqs() set them? priv is opaque to the core. All
> the core knows is the size of struct gfar_private, but nothing about
> its layout, where num_tx_queues and num_rx_queues are within priv.
I see what you mean. Will respin this.
>
>         Andrew

Re: [PATCH net-next] net: gianfar: use alloc_ethdev_mqs

Posted by Andrew Lunn 1 month, 2 weeks ago

> I see what you mean. Will respin this.

This is why the netdev FAQ says:

  Netdev discourages patches which perform simple clean-ups, which are
  not in the context of other work.

  ...

  This is because it is felt that the churn that such changes produce
  comes at a greater cost than the value of such clean-ups.

Does changing alloc_netdev_mq() to alloc_netdev_mqs() bring enough
gain to offset the work needed to find the new bugs added at the same
time?

I know i would prefer looking at patches adding new drivers, new
infrastructure, new features, than broken cleanups for very old
drivers with very few users.

	Andrew