netdev - Re: [PATCH v4 2/7] net: dsa: Add DSA driver for Hirschmann Hellcreek switches

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:   Tue, 1 Sep 2020 17:22:43 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Kurt Kanzenbach <kurt@...utronix.de>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Richard Cochran <richardcochran@...il.com>,
        Kamil Alkhouri <kamil.alkhouri@...offenburg.de>,
        ilias.apalodimas@...aro.org
Subject: Re: [PATCH v4 2/7] net: dsa: Add DSA driver for Hirschmann Hellcreek
 switches

On Tue, Sep 01, 2020 at 04:05:42PM +0200, Kurt Kanzenbach wrote:
> Hi Vladimir,
> 
> On Tue Sep 01 2020, Vladimir Oltean wrote:
> > Hi Kurt,
> >
> > On Tue, Sep 01, 2020 at 02:50:09PM +0200, Kurt Kanzenbach wrote:
> [snip]
> >> +struct hellcreek {
> >> +	const struct hellcreek_platform_data *pdata;
> >> +	struct device *dev;
> >> +	struct dsa_switch *ds;
> >> +	struct hellcreek_port *ports;
> >> +	struct mutex reg_lock;	/* Switch IP register lock */
> >
> > Pardon me asking, but I went back through the previous review comments
> > and I didn't see this being asked.
> 
> It was asked multiple times, why there was a spinlock without interrupts
> being registered (see e.g. [1], [2]). I've used the spinlock variant,
> because the previously used hrtimers act like interrupts. As there are
> no timers anymore, there's no need for spinlocks and mutexes can be
> used.
> 

That, yes, I remember, but not why the reg_lock exists in the first
place.

> Florian Fainelli also asked if the reg lock can be removed
> completely. See below.
> 

Missed your answer on that.

> >
> > What is the register lock protecting against, exactly?
> 
> A lot of the register operations work by:
> 
>  * Select port, priority, vlan or counter
>  * Configure it
> 
> These sequences have to be atomic. That's what I wanted to ensure.
> 

So, let me rephrase. Is there any code path that is broken, even if only
theoretically, if you remove the reg_lock?

> Thanks,
> Kurt
> 
> [1] - https://lkml.kernel.org/netdev/def49ff6-72fe-7ca0-9e00-863c314c1c3d@gmail.com/
> [2] - https://lkml.kernel.org/netdev/20200624130318.GD7247@localhost/

Thanks,
-Vladimir