CC-ing linux-sparse We are seeing a lock context imbalance warning which we can't get rid of after applying a patch moving locking around across function boundaries. For context see: https://lore.kernel.org/linux-cifs/20200702164411.108672-1-paul@xxxxxxxxxxxxxx/T/#u Paul Aurich <paul@xxxxxxxxxxxxxx> writes: > On 2020-07-06 10:30:27 +0200, Aurélien Aptel wrote: >>Paul Aurich <paul@xxxxxxxxxxxxxx> writes: >>> Changes since v2: >>> - address sparse lock warnings by inlining smb2_tcon_has_lease and >>> hardcoding some return values (seems to help sparse's analysis) >> >>Ah, I think the issue is not the inlining but rather you need to >>instruct sparse that smb2_tcon_hash_lease is expected to release the >>lock. >> >>https://www.kernel.org/doc/html/v4.12/dev-tools/sparse.html#using-sparse-for-lock-checking >> >>Probably with __releases somewhere in the func prototype. > > I tried various iterations of that without finding one that seems to work. > I suspect it's because the unlocking is _conditional_. Hm could be it... > w/o the inline and with __releases(&cifs_tcp_ses_lock): > > fs/cifs/smb2misc.c:508:1: warning: context imbalance in 'smb2_tcon_has_lease' > - different lock contexts for basic block > fs/cifs/smb2misc.c:612:25: warning: context imbalance in > 'smb2_is_valid_lease_break'- different lock contexts for basic block > > > Digging further, I found __acquire and __release (not plural), which can be > used in individual blocks. The following seems to silence the sparse warnings > - does this seem valid? To be honnest I'm not sure, these seem counterproductive. If you are indicating you are acquiring X but lock Y the next line it feels like we are fighting the tool instead of letting it help us. > @@ -504,7 +504,7 @@ cifs_ses_oplock_break(struct work_struct *work) > kfree(lw); > } > > -static inline bool > +static bool > smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp) > { > bool found; > @@ -521,6 +521,7 @@ smb2_tcon_has_lease(struct cifs_tcon *tcon, struct smb2_lease_break *rsp) > > lease_state = le32_to_cpu(rsp->NewLeaseState); > > + __acquire(cifs_tcp_ses_lock); Should it have a "&" here? Cheers, -- Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
