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

[Vladimir Oltean <olteanv@gmail.com>]

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