Re: [PATCH net] enetc: Workaround for MDIO register access issue

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Nov 17, 2020 at 10:22:20AM +0000, Claudiu Manoil wrote:
> >-----Original Message-----
> >From: Andrew Lunn <andrew@lunn.ch>
> >Sent: Tuesday, November 17, 2020 4:45 AM
> >To: Claudiu Manoil <claudiu.manoil@nxp.com>
> >Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>; David S .
> >Miller <davem@davemloft.net>; Alexandru Marginean
> ><alexandru.marginean@nxp.com>; Vladimir Oltean
> ><vladimir.oltean@nxp.com>
> >Subject: Re: [PATCH net] enetc: Workaround for MDIO register access issue
> >
> >> +static inline void enetc_lock_mdio(void)
> >> +{
> >> +	read_lock(&enetc_mdio_lock);
> >> +}
> >> +
> >
> >> +static inline u32 _enetc_rd_mdio_reg_wa(void __iomem *reg)
> >> +{
> >> +	unsigned long flags;
> >> +	u32 val;
> >> +
> >> +	write_lock_irqsave(&enetc_mdio_lock, flags);
> >> +	val = ioread32(reg);
> >> +	write_unlock_irqrestore(&enetc_mdio_lock, flags);
> >> +
> >> +	return val;
> >> +}
> >
> >Can you mix read_lock() with write_lock_irqsave()?  Normal locks you
> >should not mix, so i assume read/writes also cannot be mixed?
> >
> 
> Not sure I understand your concerns, but this is the readers-writers locking
> scheme. The readers (read_lock) are "lightweight", they get the most calls,
> can be taken from any context including interrupt context, and compete only
> with the writers (write_lock). The writers can take the lock only when there are
> no readers holding it, and the writer must insure that it doesn't get preempted
> (by interrupts etc.) when holding the lock (irqsave). The good part is that mdio
> operations are not frequent. Also, we had this code out of the tree for quite some
> time, it's well exercised.

Hi CLaidiu

Thanks for the explanation. I don't think i've every reviewed a driver
using read/write locks like this. But thinking it through, it does
seem O.K.

     Andrew