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

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]

Date:   Wed, 18 Nov 2020 14:38:56 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Claudiu Manoil <claudiu.manoil@....com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net] enetc: Workaround for MDIO register access issue

On Tue, Nov 17, 2020 at 10:22:20AM +0000, Claudiu Manoil wrote:
> >-----Original Message-----
> >From: Andrew Lunn <andrew@...n.ch>
> >Sent: Tuesday, November 17, 2020 4:45 AM
> >To: Claudiu Manoil <claudiu.manoil@....com>
> >Cc: netdev@...r.kernel.org; Jakub Kicinski <kuba@...nel.org>; David S .
> >Miller <davem@...emloft.net>; Alexandru Marginean
> ><alexandru.marginean@....com>; Vladimir Oltean
> ><vladimir.oltean@....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