netdev - Re: [PATCH] net: macb: do not disable MDIO bus at open/close time

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6b9e6b74-374b-8ad1-cae6-7d00548a88a5@microchip.com>
Date:   Wed, 22 Aug 2018 18:33:58 +0300
From:   Claudiu Beznea <Claudiu.Beznea@...rochip.com>
To:     Anssi Hannula <anssi.hannula@...wise.fi>, <netdev@...r.kernel.org>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH] net: macb: do not disable MDIO bus at open/close time



On 20.08.2018 17:55, Anssi Hannula wrote:
> macb_reset_hw() is called from macb_close() and indirectly from
> macb_open(). macb_reset_hw() zeroes the 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 remains registered at macb_reset_hw() time, until
> macb_init_hw() is called from macb_open() which sets the MPE bit again.
> 
> I.e. currently the MDIO bus has a short disruption at open time and is
> disabled at close time until the interface is opened again.
> 
> Fix that by only touching the RE and TE bits when enabling and disabling
> RX/TX.
> 
> Fixes: 6c36a7074436 ("macb: Use generic PHY layer")
> Signed-off-by: Anssi Hannula <anssi.hannula@...wise.fi>
> ---
> 
> Claudiu Beznea wrote:
>> On 10.08.2018 09:22, Anssi Hannula wrote:
>>>
>>> macb_reset_hw() is called in init path too,
>>
>> I only see it in macb_close() and macb_open() called from macb_init_hw().
> 
> Yeah, macb_init_hw() is what I meant :)
> 
> 
>  drivers/net/ethernet/cadence/macb_main.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index dc09f9a8a49b..6501e9b3785a 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2028,14 +2028,17 @@ static void macb_reset_hw(struct macb *bp)
>  {
>  	struct macb_queue *queue;
>  	unsigned int q;
> +	u32 ctrl = macb_readl(bp, NCR);
>  
>  	/* Disable RX and TX (XXX: Should we halt the transmission
>  	 * more gracefully?)
>  	 */
> -	macb_writel(bp, NCR, 0);
> +	ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
>  
>  	/* Clear the stats registers (XXX: Update stats first?) */
> -	macb_writel(bp, NCR, MACB_BIT(CLRSTAT));
> +	ctrl |= MACB_BIT(CLRSTAT);
> +
> +	macb_writel(bp, NCR, ctrl);
>  
>  	/* Clear all status flags */
>  	macb_writel(bp, TSR, -1);
> @@ -2170,6 +2173,7 @@ static void macb_init_hw(struct macb *bp)
>  	unsigned int q;
>  
>  	u32 config;
> +	u32 ctrl;
>  
>  	macb_reset_hw(bp);
>  	macb_set_hwaddr(bp);
> @@ -2223,7 +2227,9 @@ static void macb_init_hw(struct macb *bp)
>  	}
>  
>  	/* Enable TX and RX */
> -	macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
> +	ctrl = macb_readl(bp, NCR);
> +	ctrl |= MACB_BIT(RE) | MACB_BIT(TE);
> +	macb_writel(bp, NCR, ctrl);

I would keep it as:

	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));

>  }
>  
>  /* The hash address register is 64 bits long and takes up two
>