Message-ID: <12926ee8-ee35-791f-d7a5-943cdf744d46@gmail.com>
Date:   Sat, 18 Mar 2023 11:31:17 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH net-next v3 6/9] net: sunhme: Consolidate mac address
 initialization
Content-Language: en-US
To:     Simon Horman <simon.horman@corigine.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230314003613.3874089-1-seanga2@gmail.com>
 <20230314003613.3874089-7-seanga2@gmail.com> <ZBV9M28EhKFYrHnc@corigine.com>
From:   Sean Anderson <seanga2@gmail.com>
In-Reply-To: <ZBV9M28EhKFYrHnc@corigine.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/18/23 04:58, 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@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.

Yes, I would like to come back to this later.

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

This is why I dislike this style...

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