From mboxrd@z Thu Jan  1 00:00:00 1970
From: Roger Quadros <rogerq@ti.com>
Date: Tue, 14 Mar 2017 12:52:13 +0200
Subject: [U-Boot] [u-boot PATCH v4 04/10] ti: common: board_detect:
 commodify ethaddr environment setting code
In-Reply-To: <87a88pwc5p.fsf@linux.intel.com>
References: <1489410273-10159-1-git-send-email-rogerq@ti.com>
 <1489410273-10159-5-git-send-email-rogerq@ti.com>
 <87a88pwc5p.fsf@linux.intel.com>
Message-ID: <cb0e31b2-112e-6065-33de-50967b5301ae@ti.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 13/03/17 15:15, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> +void board_ti_set_ethaddr(int index)
>> +{
>> +	uint8_t mac_addr[6];
>> +	int i;
>> +	u64 mac1, mac2;
>> +	u8 mac_addr1[6], mac_addr2[6];
>> +	int num_macs;
>> +	/*
>> +	 * Export any Ethernet MAC addresses from EEPROM.
>> +	 * The 2 MAC addresses in EEPROM define the address range.
>> +	 */
>> +	board_ti_get_eth_mac_addr(0, mac_addr1);
>> +	board_ti_get_eth_mac_addr(1, mac_addr2);
>> +
>> +	if (is_valid_ethaddr(mac_addr1) && is_valid_ethaddr(mac_addr2)) {
>> +		mac1 = mac_to_u64(mac_addr1);
>> +		mac2 = mac_to_u64(mac_addr2);
>> +
>> +		/* must contain an address range */
>> +		num_macs = mac2 - mac1 + 1;
>> +		/* <= 50 to protect against user programming error */
>> +		if (num_macs > 0 && num_macs <= 50) {
> 
> if user programs a range > 50, then you do nothing. How about allowing
> up to 50 addresses?
> 
> Something like:
> 
> 	if (num_macs < 0)
>         	bail_out();
> 
> 	if (num_macs > 50) {
> 		printf("Too many addresses. %d > 50\n", num_macs);
>                 num_macs = 50;
> 	}
> 
> this also mean you can reduce indentation level for the for loop.
> 

Good idea.

-- 
cheers,
-roger