| Message ID | 20230222210355.2741485-7-seanga2@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sunhme: Probe/IRQ cleanups | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 100 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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. > @@ -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. > +{ > + 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++) > + dev_addr[i] = readb(p + i); > + return 1; > + } > + index--; > + } nit: blank line > + return 0; > +} > + > +static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev, > + unsigned char *dev_addr) > +{ > + size_t size; > + void __iomem *p = pci_map_rom(pdev, &size); nit: reverse xmas tree. > + > + if (p) { > + int index = 0; > + int found; > + > + if (is_quattro_p(pdev)) > + index = PCI_SLOT(pdev->devfn); > + > + found = readb(p) == 0x55 && > + readb(p + 1) == 0xaa && > + find_eth_addr_in_vpd(p, (64 * 1024), index, dev_addr); > + pci_unmap_rom(pdev, p); > + if (found) > + return; > + } > + > + /* 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 ? ... > @@ -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. > 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? > -#endif > + struct device_node *dp = NULL; nit: I think it would be good to move towards, rather than away from, reverse xmas tree here. > struct happy_meal *hp; > struct net_device *dev; > void __iomem *hpreg_base; > struct resource *hpreg_res; > - int i, qfe_slot = -1; > + int qfe_slot = -1; > char prom_name[64]; > - u8 addr[ETH_ALEN]; > int err; > > /* Now make sure pci_dev cookie is there. */ ... > @@ -2680,35 +2695,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, > goto err_out_clear_quattro; > } > > - 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 { > -#ifdef CONFIG_SPARC > - const unsigned char *addr; > - int len; > - > - if (qfe_slot != -1 && > - (addr = of_get_property(dp, "local-mac-address", &len)) > - != NULL && > - len == 6) { > - eth_hw_addr_set(dev, addr); > - } else { > - eth_hw_addr_set(dev, idprom->id_ethaddr); > - } > -#else > - u8 addr[ETH_ALEN]; > - > - get_hme_mac_nonsparc(pdev, addr); > - eth_hw_addr_set(dev, addr); > -#endif > - } > + happy_meal_addr_init(hp, dp, qfe_slot); > > /* Layout registers. */ > hp->gregs = (hpreg_base + 0x0000UL); > @@ -2757,7 +2744,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, > timer_setup(&hp->happy_timer, happy_meal_timer, 0); > > hp->irq = pdev->irq; > - hp->dev = dev; > dev->netdev_ops = &hme_netdev_ops; > dev->watchdog_timeo = 5*HZ; > dev->ethtool_ops = &hme_ethtool_ops; > -- > 2.37.1 >
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. >> +{ >> + 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? >> + dev_addr[i] = readb(p + i); >> + return 1; >> + } >> + index--; >> + } > > nit: blank line OK >> + return 0; >> +} >> + >> +static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev, >> + unsigned char *dev_addr) >> +{ >> + size_t size; >> + void __iomem *p = pci_map_rom(pdev, &size); > > nit: reverse xmas tree. OK >> + >> + if (p) { >> + int index = 0; >> + int found; >> + >> + if (is_quattro_p(pdev)) >> + index = PCI_SLOT(pdev->devfn); >> + >> + found = readb(p) == 0x55 && >> + readb(p + 1) == 0xaa && >> + find_eth_addr_in_vpd(p, (64 * 1024), index, dev_addr); >> + pci_unmap_rom(pdev, p); >> + if (found) >> + return; >> + } >> + >> + /* 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... >> @@ -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. >> 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. >> -#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 ;) 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. >> struct happy_meal *hp; >> struct net_device *dev; >> void __iomem *hpreg_base; >> struct resource *hpreg_res; >> - int i, qfe_slot = -1; >> + int qfe_slot = -1; >> char prom_name[64]; >> - u8 addr[ETH_ALEN]; >> int err; >> >> /* Now make sure pci_dev cookie is there. */ > > ... > >> @@ -2680,35 +2695,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, >> goto err_out_clear_quattro; >> } >> >> - 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 { >> -#ifdef CONFIG_SPARC >> - const unsigned char *addr; >> - int len; >> - >> - if (qfe_slot != -1 && >> - (addr = of_get_property(dp, "local-mac-address", &len)) >> - != NULL && >> - len == 6) { >> - eth_hw_addr_set(dev, addr); >> - } else { >> - eth_hw_addr_set(dev, idprom->id_ethaddr); >> - } >> -#else >> - u8 addr[ETH_ALEN]; >> - >> - get_hme_mac_nonsparc(pdev, addr); >> - eth_hw_addr_set(dev, addr); >> -#endif >> - } >> + happy_meal_addr_init(hp, dp, qfe_slot); >> >> /* Layout registers. */ >> hp->gregs = (hpreg_base + 0x0000UL); >> @@ -2757,7 +2744,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, >> timer_setup(&hp->happy_timer, happy_meal_timer, 0); >> >> hp->irq = pdev->irq; >> - hp->dev = dev; >> dev->netdev_ops = &hme_netdev_ops; >> dev->watchdog_timeo = 5*HZ; >> dev->ethtool_ops = &hme_ethtool_ops; >> -- >> 2.37.1 >> --Sean
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. ...
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> @@ -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) +{ + 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) +{ + 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++) + dev_addr[i] = readb(p + i); + return 1; + } + index--; + } + return 0; +} + +static void __maybe_unused get_hme_mac_nonsparc(struct pci_dev *pdev, + unsigned char *dev_addr) +{ + size_t size; + void __iomem *p = pci_map_rom(pdev, &size); + + if (p) { + int index = 0; + int found; + + if (is_quattro_p(pdev)) + index = PCI_SLOT(pdev->devfn); + + found = readb(p) == 0x55 && + readb(p + 1) == 0xaa && + find_eth_addr_in_vpd(p, (64 * 1024), index, dev_addr); + pci_unmap_rom(pdev, p); + if (found) + return; + } + + /* 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) */ + +static void happy_meal_addr_init(struct happy_meal *hp, + struct device_node *dp, int qfe_slot) +{ + int i; + + for (i = 0; i < 6; i++) { + if (macaddr[i] != 0) + break; + } + + if (i < 6) { /* a mac address was given */ + u8 addr[ETH_ALEN]; + + for (i = 0; i < 6; i++) + addr[i] = macaddr[i]; + eth_hw_addr_set(hp->dev, addr); + macaddr[5]++; + } else { +#ifdef CONFIG_SPARC + const unsigned char *addr; + int len; + + /* If user did not specify a MAC address specifically, use + * the Quattro local-mac-address property... + */ + if (qfe_slot != -1) { + addr = of_get_property(dp, "local-mac-address", &len); + if (addr && len == 6) { + eth_hw_addr_set(hp->dev, addr); + return; + } + } + + eth_hw_addr_set(hp->dev, idprom->id_ethaddr); +#else + u8 addr[ETH_ALEN]; + + get_hme_mac_nonsparc(hp->happy_dev, addr); + eth_hw_addr_set(hp->dev, addr); +#endif + } +} + #ifdef CONFIG_SBUS static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe) { @@ -2320,8 +2447,7 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe) struct quattro *qp = NULL; struct happy_meal *hp; struct net_device *dev; - int i, qfe_slot = -1; - u8 addr[ETH_ALEN]; + int qfe_slot = -1; int err; sbus_dp = op->dev.parent->of_node; @@ -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; 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; @@ -2504,104 +2606,17 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe) #endif #ifdef CONFIG_PCI -#ifndef CONFIG_SPARC -static int is_quattro_p(struct pci_dev *pdev) -{ - struct pci_dev *busdev = pdev->bus->self; - struct pci_dev *this_pdev; - int n_hmes; - - if (busdev == NULL || - 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) -{ - 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++) - dev_addr[i] = readb(p + i); - return 1; - } - index--; - } - return 0; -} - -static void get_hme_mac_nonsparc(struct pci_dev *pdev, unsigned char *dev_addr) -{ - size_t size; - void __iomem *p = pci_map_rom(pdev, &size); - - if (p) { - int index = 0; - int found; - - if (is_quattro_p(pdev)) - index = PCI_SLOT(pdev->devfn); - - found = readb(p) == 0x55 && - readb(p + 1) == 0xaa && - find_eth_addr_in_vpd(p, (64 * 1024), index, dev_addr); - pci_unmap_rom(pdev, p); - if (found) - return; - } - - /* 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) */ - 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; -#endif + struct device_node *dp = NULL; struct happy_meal *hp; struct net_device *dev; void __iomem *hpreg_base; struct resource *hpreg_res; - int i, qfe_slot = -1; + int qfe_slot = -1; char prom_name[64]; - u8 addr[ETH_ALEN]; int err; /* Now make sure pci_dev cookie is there. */ @@ -2644,7 +2659,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, SET_NETDEV_DEV(dev, &pdev->dev); hp = netdev_priv(dev); - + hp->dev = dev; hp->happy_dev = pdev; hp->dma_dev = &pdev->dev; @@ -2680,35 +2695,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, goto err_out_clear_quattro; } - 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 { -#ifdef CONFIG_SPARC - const unsigned char *addr; - int len; - - if (qfe_slot != -1 && - (addr = of_get_property(dp, "local-mac-address", &len)) - != NULL && - len == 6) { - eth_hw_addr_set(dev, addr); - } else { - eth_hw_addr_set(dev, idprom->id_ethaddr); - } -#else - u8 addr[ETH_ALEN]; - - get_hme_mac_nonsparc(pdev, addr); - eth_hw_addr_set(dev, addr); -#endif - } + happy_meal_addr_init(hp, dp, qfe_slot); /* Layout registers. */ hp->gregs = (hpreg_base + 0x0000UL); @@ -2757,7 +2744,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, timer_setup(&hp->happy_timer, happy_meal_timer, 0); hp->irq = pdev->irq; - hp->dev = dev; dev->netdev_ops = &hme_netdev_ops; dev->watchdog_timeo = 5*HZ; dev->ethtool_ops = &hme_ethtool_ops;
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> --- drivers/net/ethernet/sun/sunhme.c | 284 ++++++++++++++---------------- 1 file changed, 135 insertions(+), 149 deletions(-)