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@xxxxxxxxx/
> [2] - https://lkml.kernel.org/netdev/20200624130318.GD7247@localhost/
Thanks,
-Vladimir
