[U-Boot] [PATCH 01/27] Blackfin: bfin_mac: force board_get_enetaddr() usage

[Ben Warren <biggerbadderben@gmail.com>]

Mike Frysinger wrote:
> On Thursday 29 January 2009 01:01:41 Ben Warren wrote:
>   
>> Mike Frysinger wrote:
>>     
>>> On Thursday 29 January 2009 00:43:31 Ben Warren wrote:
>>>       
>>>> Mike Frysinger wrote:
>>>>         
>>>>> --- a/drivers/net/bfin_mac.c
>>>>> +++ b/drivers/net/bfin_mac.c
>>>>>
>>>>>  	eth_register(dev);
>>>>>
>>>>> +	ethaddr = getenv("ethaddr");
>>>>> +#ifndef CONFIG_ETHADDR
>>>>>           
>>>> I know this was there before, but CONFIG_ETHADDR is kinda deprecated.
>>>> We don't allow it in in-tree config files, so as far as I'm concerned we
>>>> should pretend it doesn't exist.  Boards should get their MAC address
>>>> from an EEPROM or from the environment.
>>>>         
>>> that's news to me.  i see no mention of deprecation in the README file
>>> (quite the opposite ... looks fully supported there), and i see a ton of
>>> board configs defining it:
>>> $ grep 'CONFIG_ETH.*ADDR' include/configs/*.h | wc -l
>>> 257
>>>
>>> so what's up ?
>>>       
>> Hence the 'kinda'.  It's one of those things that doesn't make sense
>> (MAC addresses are supposed to be unique, so why would you hard code?)
>> and for at least the past year we've been rejecting config files that
>> use it.
>>     
>
> if that's the case, shouldnt we at least mark the README as "these options are 
> discouraged / deprecated" ?
>
> i dont have a problem removing the ifndef here as it accomplishes two things:
>  - no board_get_enetaddr() reference
>  - slightly smaller code
>
> it will break a Blackfin platform or two, but i can fix them up pretty easily.
>
>   
I'll patch README to make this more official and clear
>>>>> +	if (ethaddr == NULL) {
>>>>> +		char nid[20];
>>>>> +		board_get_enetaddr(bd->bi_enetaddr);
>>>>> +		sprintf(nid, "%02X:%02X:%02X:%02X:%02X:%02X",
>>>>>           
>>>> How about snprintf()
>>>>         
>>> when would the limit actually be violated ?  bi_enetaddr is unsigned char
>>> which means it is impossible for it to be represented as more than two
>>> chars. so storage would always be exactly 2 * 6 + 5 + 1 (17 bytes).
>>>       
>> You're right, not a strong opinion on my part.  I guess I've been beaten
>> to submission by Coverity at work and so always use the 'n' functions.
>>     
>
> i have no problem with proactive coding when it makes sense, but i hate some 
> of the BSD-ish policies where they try and cram the "n" versions down your 
> throat all the time.
>   
amen
> -mike
>   
regards,
Ben