Re: [PATCH net-next] dibs: Avoid inconsistent lockstate warning in dibs_lo_move_data()

[Alexandra Winter]

On 08.09.26 14:45, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> - [Medium] The patch fixes an observable lockdep "inconsistent lock
>   state" splat produced by the plain spin_lock(&dibs->lock) in…
> 
> --
> 
> Here is the report.
> 
> dibs: Avoid inconsistent lockstate warning in dibs_lo_move_data()
> 
> This converts the plain spin_lock()/spin_unlock() around the
> dibs->subs[]->ops->handle_irq() call in dibs_lo_move_data() to
> spin_lock_irqsave()/spin_unlock_irqrestore(), because the same lock is
> also taken from hardirq context in ism_handle_irq().  The commit message
> notes this is not a real deadlock since dibs_lo devices have no interrupt
> context.
> 
>> diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
>> index 649e4e375be33..44a2e74c2efcb 100644
>> --- a/drivers/dibs/dibs_loopback.c
>> +++ b/drivers/dibs/dibs_loopback.c
> 
> [ ... ]
> 
>> @@ -267,12 +268,12 @@ static int dibs_lo_move_data(struct dibs_dev *dibs, u64 dmb_tok,
>>  	if (!sf)
>>  		return 0;
>>  
>> -	spin_lock(&dibs->lock);
>> +	spin_lock_irqsave(&dibs->lock, flags);
>>  	client_id = dibs->dmb_clientid_arr[sba_idx];
>>  	s_mask = ror16(0x1000, idx);
>>  	if (likely(client_id != NO_DIBS_CLIENT && dibs->subs[client_id]))
>>  		dibs->subs[client_id]->ops->handle_irq(dibs, sba_idx, s_mask);
>> -	spin_unlock(&dibs->lock);
>> +	spin_unlock_irqrestore(&dibs->lock, flags);
> 
> [Severity: Medium]
> Should this carry a Fixes: tag?  The lockdep splat quoted in the commit
> message is reproducible on shipped kernels, and git blame attributes the
> converted spin_lock()/spin_unlock() pair in dibs_lo_move_data() to:
> 
> cc21191b584c6f ("dibs: Move data path to dibs layer")
> 
> The hardirq-side acquisition of the same lock is in ism_handle_irq()
> (drivers/s390/net/ism_drv.c):
> 
> 	spin_lock(&dibs->lock);
> 
> which matches the {IN-HARDIRQ-W} registration in the report, so the
> warning can be hit by ordinary SMC traffic through
> smc_close_active()->smcd_cdc_msg_send()->smcd_tx_ism_write()->dibs_lo_move_data()
> on a lockdep-enabled kernel.
> 
> The splat also ends with "INFO: lockdep is turned off", so further lock
> checking is lost for the rest of that boot on affected kernels.
> 
> Would adding the following help backporters and stable tooling identify
> which trees need this change?
> 
> Fixes: cc21191b584c6f ("dibs: Move data path to dibs layer")
> 


As mentioned in the commit message the paths reported by lockdep belong to two
different device types (dibs-lo and ism). So not a real deadlock.
So unless lockdep-warnings by themselves warrant a Fixes-tag (?), I think net-next is correct here.

Paolo applied this already to net-next.
For the records: Sahiko identified the right patch to blame.