| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y/pV6KDgopeiPEPo@corigine.com>
Date: Sat, 25 Feb 2023 19:39:36 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Sean Anderson <seanga2@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next 6/7] net: sunhme: Consolidate mac address
initialization
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@...il.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
>
Powered by blists - more mailing lists