netdev - Re: [PATCH] net: macb: do not disable MDIO bus when closing interface

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: <3c3a606e-b3e1-41fc-fcd8-7b98968fd06b@bitwise.fi>
Date:   Thu, 9 Aug 2018 17:54:27 +0300
From:   Anssi Hannula <anssi.hannula@...wise.fi>
To:     Claudiu Beznea <Claudiu.Beznea@...rochip.com>
Cc:     Nicolas Ferre <nicolas.ferre@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: macb: do not disable MDIO bus when closing interface

On 9.8.2018 11:26, Claudiu Beznea wrote:
> On 08.08.2018 15:19, Anssi Hannula wrote:
>> macb_close() calls macb_reset_hw() which zeroes NCR register, including
>> the MPE (Management Port Enable) bit.
>>
>> This will prevent accessing any other PHYs for other Ethernet MACs on
>> the MDIO bus which is still registered.
>>
>> Fix that by keeping the MPE bit set.
>>
>> Signed-off-by: Anssi Hannula <anssi.hannula@...wise.fi>
>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index dc09f9a8a49b..3ca98fc32144 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -2030,12 +2030,13 @@ static void macb_reset_hw(struct macb *bp)
>>  	unsigned int q;
>>  
>>  	/* Disable RX and TX (XXX: Should we halt the transmission
>> -	 * more gracefully?)
>> +	 * more gracefully?) but keep management port open since there
>> +	 * may be other users of the mdio bus
>>  	 */
>> -	macb_writel(bp, NCR, 0);
>> +	macb_writel(bp, NCR, MACB_BIT(MPE));
> Would be better to read the NCR and clear only RX and TX bits, something like:
> 	val = macb_readl(bp, NCR);
>
> 	/* Disable TX and RX. */
> 	val &= ~(MACB_BIT(TE) | MACB_BIT(RE));
>
> 	/* Clear statistics */
> 	val |= MACB_BIT(CLRSTAT);
>
> 	macb_writel(bp, NCR, val);
>
> MPE should have been enabled by previous operations.

macb_reset_hw() is called in init path too, though, so maybe clearing
all bits is intentional / wanted to get the controller to a known state,
even though the comment only mentions TX/RX?

If so, maybe the below would be better? But if you think just clearing
TE/RE is better I'll just do it like you suggested.

    val = macb_readl(bp, NCR);

    /* Keep only MDIO port active */
    val &= MACB_BIT(MPE);

    /* Clear statistics */
    val |= MACB_BIT(CLRSTAT);

    macb_writel(bp, NCR, val);



> Thank you,
> Claudiu Beznea
>
>>  
>>  	/* Clear the stats registers (XXX: Update stats first?) */
>> -	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
>> +	macb_writel(bp, NCR, MACB_BIT(CLRSTAT) | MACB_BIT(MPE));
>>  
>>  	/* Clear all status flags */
>>  	macb_writel(bp, TSR, -1);
>>

-- 
Anssi Hannula / Bitwise Oy
+358 503803997