netdev - Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <83b3fb27-97dd-a9ea-e0db-017d616f93fe@egil-hjelmeland.no>
Date:   Mon, 31 Jul 2017 16:09:10 +0200
From:   Egil Hjelmeland <privat@...l-hjelmeland.no>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...gutronix.de
Subject: Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify
 lan9303_xxx_packet_processing() usage

On 31. juli 2017 15:46, Andrew Lunn wrote:
> On Mon, Jul 31, 2017 at 01:33:55PM +0200, Egil Hjelmeland wrote:
>> Simplify usage of lan9303_enable_packet_processing,
>> lan9303_disable_packet_processing()
>>
>> Signed-off-by: Egil Hjelmeland <privat@...l-hjelmeland.no>
>> ---
>>   drivers/net/dsa/lan9303-core.c | 24 ++++++++++--------------
>>   1 file changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index 4d2bb8144c15..705267a1d2ba 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -559,15 +559,16 @@ static int lan9303_handle_reset(struct lan9303 *chip)
>>   /* stop processing packets for all ports */
>>   static int lan9303_disable_processing(struct lan9303 *chip)
>>   {
>> -	int ret;
>> +	int p;
>>   
>> -	ret = lan9303_disable_packet_processing(chip, 0);
>> -	if (ret)
>> -		return ret;
>> -	ret = lan9303_disable_packet_processing(chip, 1);
>> -	if (ret)
>> -		return ret;
>> -	return lan9303_disable_packet_processing(chip, 2);
>> +	for (p = 0; p <= 2; p++) {
>> +		int ret;
>> +
>> +		ret = lan9303_disable_packet_processing(chip, p);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	return 0;
>>   }
>>   
>>   static int lan9303_check_device(struct lan9303 *chip)
>> @@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>>   	/* enable internal packet processing */
>>   	switch (port) {
>>   	case 1:
>> -		return lan9303_enable_packet_processing(chip, port);
>>   	case 2:
>>   		return lan9303_enable_packet_processing(chip, port);
>>   	default:
>> @@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>>   	/* disable internal packet processing */
>>   	switch (port) {
>>   	case 1:
>> -		lan9303_disable_packet_processing(chip, port);
>> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
>> -				  MII_BMCR, BMCR_PDOWN);
>> -		break;
>>   	case 2:
>>   		lan9303_disable_packet_processing(chip, port);
>> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
>> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
>>   				  MII_BMCR, BMCR_PDOWN);
>>   		break;
>>   	default:
> 
> :-)
> 
> So maybe i would squash this part into the previous patch. You were
> touching the functions anyway, and the change is obvious, so easy to
> review.
> 
> But it is also O.K. this way. The cover note could of helped. You
> could of said something like: "Changes made in the first patch allow
> some simplifications to be made in the same code in the second patch.
> 
> Breaking changes up is hard, and you cannot please everybody, all the
> time.
> 
> Reviewed-by: Andrew Lunn <andrew@...n.ch>
> 
>      Andrew
> 

Hi Andrew

This time I took the extra effort to split my  original patch...

Your lan9303_write_switch_port suggestion (in previous reply) is fine.
And I can improve the coverletter.

So I will do a v2 of the patch. But what is your advice:
Should I squash the patch?

Egil