by Simon Horman

[permalink] [raw]

Subject: Re: [RFC PATCH net-next 6/7] net: sunhme: Consolidate mac address initialization

On Sat, Feb 25, 2023 at 01:59:33PM -0500, Sean Anderson wrote:
> On 2/25/23 13:39, Simon Horman wrote:
> > On Wed, Feb 22, 2023 at 04:03:54PM -0500, Sean Anderson wrote:
> > > The mac address initialization is braodly the same between PCI and SBUS,
> > > and one was clearly copied from the other. Consolidate them. We still have
> > > to have some ifdefs because pci_(un)map_rom is only implemented for PCI,
> > > and idprom is only implemented for SPARC.
> > >
> > > Signed-off-by: Sean Anderson <[email protected]>
> >
> > Overall this looks to correctly move code around as suggest.
> > Some minor nits and questions inline.
> >
> > > ---
> > >
> > > drivers/net/ethernet/sun/sunhme.c | 284 ++++++++++++++----------------
> > > 1 file changed, 135 insertions(+), 149 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> > > index 75993834729a..9b55adbe61df 100644
> > > --- a/drivers/net/ethernet/sun/sunhme.c
> > > +++ b/drivers/net/ethernet/sun/sunhme.c
> > > @@ -34,6 +34,7 @@
> > > #include <linux/mm.h>
> > > #include <linux/module.h>
> > > #include <linux/netdevice.h>
> > > +#include <linux/of.h>
> > > #include <linux/random.h>
> > > #include <linux/skbuff.h>
> > > #include <linux/slab.h>
> > > @@ -47,7 +48,6 @@
> > > #include <asm/oplib.h>
> > > #include <asm/prom.h>
> > > #include <linux/of_device.h>
> > > -#include <linux/of.h>
> > > #endif
> > > #include <linux/uaccess.h>
> >
> > nit: The above hunks don't seem related to the rest of this patch.
>
> I think I originally included this because I referenced some of_ thing from non-sparc
> code. But it seems like that got refactored out.
>
> > > @@ -2313,6 +2313,133 @@ static const struct net_device_ops hme_netdev_ops = {
> > > .ndo_validate_addr = eth_validate_addr,
> > > };
> > > +#ifdef CONFIG_PCI
> > > +static int is_quattro_p(struct pci_dev *pdev)
> >
> > nit: I know you are moving code around here,
> > and likewise for many of my other comments.
> > But I think bool would be a better return type for this function.
>
> I agree. I will address these sorts of things in a separate patch.

Thanks.

> > > +{
> > > + struct pci_dev *busdev = pdev->bus->self;
> > > + struct pci_dev *this_pdev;
> > > + int n_hmes;
> > > +
> > > + if (!busdev || busdev->vendor != PCI_VENDOR_ID_DEC ||
> > > + busdev->device != PCI_DEVICE_ID_DEC_21153)
> > > + return 0;
> > > +
> > > + n_hmes = 0;
> > > + list_for_each_entry(this_pdev, &pdev->bus->devices, bus_list) {
> > > + if (this_pdev->vendor == PCI_VENDOR_ID_SUN &&
> > > + this_pdev->device == PCI_DEVICE_ID_SUN_HAPPYMEAL)
> > > + n_hmes++;
> > > + }
> > > +
> > > + if (n_hmes != 4)
> > > + return 0;
> > > +
> > > + return 1;
> > > +}
> > > +
> > > +/* Fetch MAC address from vital product data of PCI ROM. */
> > > +static int find_eth_addr_in_vpd(void __iomem *rom_base, int len, int index, unsigned char *dev_addr)
> >
> > nit: At some point it might be better
> > to update this function to return 0 on success and
> > an error otherwise.
> >
> > > +{
> > > + int this_offset;
> > > +
> > > + for (this_offset = 0x20; this_offset < len; this_offset++) {
> > > + void __iomem *p = rom_base + this_offset;
> > > +
> > > + if (readb(p + 0) != 0x90 ||
> > > + readb(p + 1) != 0x00 ||
> > > + readb(p + 2) != 0x09 ||
> > > + readb(p + 3) != 0x4e ||
> > > + readb(p + 4) != 0x41 ||
> > > + readb(p + 5) != 0x06)
> > > + continue;
> > > +
> > > + this_offset += 6;
> > > + p += 6;
> > > +
> > > + if (index == 0) {
> > > + int i;
> > > +
> > > + for (i = 0; i < 6; i++)
> >
> > nit: This could be,
> >
> > for (int i = 0; i < 6; i++)
>
> That's kosher now?

I can't vouch for all architectures (e.g. sparc).
But in general, yes, I think so.

> > > + /* Sun MAC prefix then 3 random bytes. */
> > > + dev_addr[0] = 0x08;
> > > + dev_addr[1] = 0x00;
> > > + dev_addr[2] = 0x20;
> > > + get_random_bytes(&dev_addr[3], 3);
> > > +}
> > > +#endif /* !(CONFIG_SPARC) */
> >
> > Should this be CONFIG_PCI ?
>
> No idea. I think I will just remove it...

Yes, that would remove the problem quite nicely.

> > > @@ -2346,34 +2472,11 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
> > > return -ENOMEM;
> > > SET_NETDEV_DEV(dev, &op->dev);
> > > - /* If user did not specify a MAC address specifically, use
> > > - * the Quattro local-mac-address property...
> > > - */
> > > - for (i = 0; i < 6; i++) {
> > > - if (macaddr[i] != 0)
> > > - break;
> > > - }
> > > - if (i < 6) { /* a mac address was given */
> > > - for (i = 0; i < 6; i++)
> > > - addr[i] = macaddr[i];
> > > - eth_hw_addr_set(dev, addr);
> > > - macaddr[5]++;
> > > - } else {
> > > - const unsigned char *addr;
> > > - int len;
> > > -
> > > - addr = of_get_property(dp, "local-mac-address", &len);
> > > -
> > > - if (qfe_slot != -1 && addr && len == ETH_ALEN)
> > > - eth_hw_addr_set(dev, addr);
> > > - else
> > > - eth_hw_addr_set(dev, idprom->id_ethaddr);
> > > - }
> > > -
> > > hp = netdev_priv(dev);
> > > -
> > > + hp->dev = dev;
> >
> > I'm not clear how this change relates to the rest of the patch.
>
> This mirrors the initialization on the PCI side. Makes their equivalence
> more obvious.

Thanks, understood.

> > > hp->happy_dev = op;
> > > hp->dma_dev = &op->dev;
> > > + happy_meal_addr_init(hp, dp, qfe_slot);
> > > spin_lock_init(&hp->happy_lock);
> > > @@ -2451,7 +2554,6 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
> > > timer_setup(&hp->happy_timer, happy_meal_timer, 0);
> > > - hp->dev = dev;
> > > dev->netdev_ops = &hme_netdev_ops;
> > > dev->watchdog_timeo = 5*HZ;
> > > dev->ethtool_ops = &hme_ethtool_ops;
> >
> > ...
> >
> > > static int happy_meal_pci_probe(struct pci_dev *pdev,
> > > const struct pci_device_id *ent)
> > > {
> > > struct quattro *qp = NULL;
> > > -#ifdef CONFIG_SPARC
> > > - struct device_node *dp;
> >
> > Was dp not being initialised previously a bug?
>
> No. All uses are protected by CONFIG_SPARC. But passing garbage to other
> functions is bad form.

Thanks, agreed.

> > > -#endif
> > > + struct device_node *dp = NULL;
> >
> > nit: I think it would be good to move towards, rather than away from,
> > reverse xmas tree here.
>
> Which is why this line comes first ;)

Yes, silly me.

> But I am not a fan of introducing churn just to organize line lengths. So the
> following will stay as-is until it needs to be reworked further.
>

Yes, no objections there.
I just misread the diff. Sorry.

...

2023-02-25 21:08:09

by Simon Horman

[permalink] [raw]

Subject: Re: [RFC PATCH net-next 4/7] net: sunhme: Alphabetize includes

On Wed, Feb 22, 2023 at 04:03:52PM -0500, Sean Anderson wrote:
> Alphabetize includes to make it clearer where to add new ones.
>
> Signed-off-by: Sean Anderson <[email protected]>
> ---
>
> drivers/net/ethernet/sun/sunhme.c | 45 +++++++++++++++----------------
> 1 file changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index 127253c67c59..ab39b555d9f7 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c
> @@ -14,41 +14,40 @@
> * argument : macaddr=0x00,0x10,0x20,0x30,0x40,0x50
> */
>
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/types.h>
> +#include <asm/byteorder.h>
> +#include <asm/dma.h>
> +#include <linux/bitops.h>
> +#include <linux/crc32.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/errno.h>
> +#include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
> #include <linux/fcntl.h>
> -#include <linux/interrupt.h>
> -#include <linux/ioport.h>
> #include <linux/in.h>
> -#include <linux/slab.h>
> -#include <linux/string.h>
> -#include <linux/delay.h>
> #include <linux/init.h>
> -#include <linux/ethtool.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> #include <linux/mii.h>
> -#include <linux/crc32.h>
> -#include <linux/random.h>
> -#include <linux/errno.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> #include <linux/netdevice.h>
> -#include <linux/etherdevice.h>
> +#include <linux/random.h>
> #include <linux/skbuff.h>
> -#include <linux/mm.h>
> -#include <linux/bitops.h>
> -#include <linux/dma-mapping.h>
> -
> -#include <asm/io.h>
> -#include <asm/dma.h>
> -#include <asm/byteorder.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
>
> #ifdef CONFIG_SPARC
> -#include <linux/of.h>
> -#include <linux/of_device.h>
> +#include <asm/auxio.h>
> #include <asm/idprom.h>
> #include <asm/openprom.h>
> #include <asm/oplib.h>
> #include <asm/prom.h>
> -#include <asm/auxio.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>

A nice cleanup, IMHO.
But can linux/ includes also moved out of the ifdefs.

> #endif
> #include <linux/uaccess.h>

And, perhaps it would be nice to further move things around so there are
three groups of includes:

* <asm/*.h>
- non #ifdef CONFIG_SPARC; and
- #ifdef CONFIG_SPARC
* <linux/*>
* "*.h"