netdev - Re: [PATCH] Use a spinlock to guard `fep->ptp_clk

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] [day] [month] [year] [list]

Message-ID: <YxCjCM78tsk9J3gy@lunn.ch>
Date:   Thu, 1 Sep 2022 14:18:16 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Csókás Bence <csokas.bence@...lan.hu>
Cc:     netdev@...r.kernel.org, Richard Cochran <richardcochran@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, kernel@...gutronix.de,
        Francesco Dolcini <francesco.dolcini@...adex.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>
Subject: Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

On Thu, Sep 01, 2022 at 10:06:03AM +0200, Csókás Bence wrote:
> 
> On 2022. 09. 01. 9:51, Csókás Bence wrote:
> > 
> > On 2022. 08. 31. 18:24, Andrew Lunn wrote:
> >  >>> Please keep to reverse christmas tree
> >  >>
> >  >> checkpatch didn't tell me that was a requirement... Easy to fix though.
> >  >
> >  > checkpatch does not have the smarts to detect this. And it is a netdev
> >  > only thing.
> > 
> > I thought checkpatch also has the per-subsystem rules, too.
> > 
> >  > There is also a different between not being able to sleep, and not
> >  > being able to process an interrupt for some other hardware. You got a
> >  > report about taking a mutex in atomic context. That just means you
> >  > cannot sleep, probably because a spinlock is held. That is very
> >  > different to not being able to handle interrupts. You only need
> >  > spin_lock_irqsave() if the interrupt handler also needs the same spin
> >  > lock to protect it from a thread using the spin lock.
> > 
> > Alright, I will switch to plain `spin_lock()` then.
> 
> By the way, what about `&fep->tmreg_lock`? Should that also be switched to
> `spin_lock()`? If not, how should I handle the nested locking? Calling
> `spin_lock_irqsave(&fep->tmreg_lock)` after `spin_lock(&&fep->ptp_clk_lock)`
> seems pointless. Should I lock with `spin_lock_irqsave(&fep->ptp_clk_lock)`
> there?

Richard was making the point, do you need two locks?

What are the locks protecting? Could you use one lock for both use
cases? Should the other lock also not be an _irqsave()?

       Andrew