Re: [PATCH net-next v3 6/9] net: sunhme: Consolidate mac address initialization

Simon Horman
2023-03-18T08:58:49Z

On Mon, Mar 13, 2023 at 08:36:10PM -0400, 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>

Hi Sean,

Nits aside, this looks good to me.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index 3072578c334a..c2737f26afbe 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c

...

> +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 - longest line to shortest - would be nice here.

void __iomem *p;
size_t size;

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);

nit: Maybe as a follow-up using eth_hw_addr_random() could be considered here.

> +}
> +#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;

nit: if dp was added above qp then then
things would move closer to reverse xmas tree.

> 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;

nit: if qfe_slot was added below prom_name[64] then then
things would move closer to reverse xmas tree.

> char prom_name[64];
> - u8 addr[ETH_ALEN];
> int err;
>
> /* Now make sure pci_dev cookie is there. */

...