On Tue, Nov 17, 2020 at 10:22:20AM +0000, Claudiu Manoil wrote:
> >-----Original Message-----
> >From: Andrew Lunn <[email protected]>
> >Sent: Tuesday, November 17, 2020 4:45 AM
> >To: Claudiu Manoil <[email protected]>
> >Cc: [email protected]; Jakub Kicinski <[email protected]>; David S .
> >Miller <[email protected]>; Alexandru Marginean
> ><[email protected]>; Vladimir Oltean
> ><[email protected]>
> >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
