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

[Simon Horman]

On Sat, Mar 18, 2023 at 09:58:35AM +0100, Simon Horman wrote:
> 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@xxxxxxxxx>
> 
> Hi Sean,
> 
> Nits aside, this looks good to me.
> 
> Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>
> 
> > 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);

Oops, I now see that you got this in patch 7/9.
Thanks!

> > +
> > +	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. */
> 
> ...