Re: [PATCH v2 2/3] s390/pci: Rework__zpci_event_availability() to remove conditional locking

Heiko Carstens <[email protected]>
Newsgroups org.kernel.vger.linux-s390,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
On Wed, Aug 05, 2026 at 01:32:10PM +0200, Niklas Schnelle wrote:
> On Mon, 2026-08-03 at 16:29 +0200, Heiko Carstens wrote:
> > Clang's compiler based static context analysis does not work with locks
> > that are conditionally taken like in __zpci_event_availability():
> > 
> > arch/s390/pci/pci_event.c:402:10: warning: mutex 'get_zdev_by_fid(ccdf->fid).state_lock'
> >   is not held on every path through here [-Wthread-safety-analysis]
> > 
> > Given that code which takes locks conditionally can be considered
> > suboptimal rework __zpci_event_availability() to get rid of this.
> > 
> > Signed-off-by: Heiko Carstens <[email protected]>
> > ---
> >  arch/s390/pci/pci_event.c | 108 +++++++++++++++++++-------------------
> >  1 file changed, 53 insertions(+), 55 deletions(-)

...

> Personally I think I'd put the 0x0306 and each of the two switches in
> helper functions to improve readability as this is getting awfully
> long. Maybe something like zpci_event_avail_any_device() (0x0306),
> zpci_event_avail_new_device() and zpci_event_avail_existing_device().
> 
> If you prefer and since I'm doing a follow up for the missing locking
> in zpci_reserved_devices() I can also do that in a separate patch. Also
> just to clarify Sashiko is right in that the missing locking is a pre-
> existing issue as the state_lock was already not taken in that case,
> it's just more obvious now.
> 
> Either way, functionality looks good to me so feel free to add:
> 
> Reviewed-by: Niklas Schnelle <[email protected]>

Thanks, I'll address the two nits for patch 1 and 2, and will send a
new version. However I prefer if you do additional code refactoring
with addon patches, so the result looks exactly like you want it.
Otherwise we will go back and forth :)
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.