Messages in this thread | | | Date | Sat, 25 Feb 2023 21:27:05 +0100 | From | Simon Horman <> | 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 <seanga2@gmail.com> > > > > 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.
...
| |