Re: [PATCH] scsi: ufs: core: Avoid sleeping in hard interrupt context when PREEMP_RT is enabled.

[Gregory CLEMENT]

Hello,

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] The static function `ufshcd_intr` becomes unused when `CONFIG_PREEMPT_RT` is enabled, causing a compiler warning.
> - [Low] Typo in the newly added source code comment.

Both of Sachiko's comments are valid. I am addressing these issues in
the next version.

Gregory

> --
>
> commit 6afdc0b243918ce89eb55cea6349f608516d9d03
> Author: Gregory CLEMENT <gregory.clement@xxxxxxxxxxx>
>
> scsi: ufs: core: Avoid sleeping in hard interrupt context when PREEMP_RT is enabled.
>
> This commit addresses a sleeping-in-hard-irq context bug under PREEMPT_RT. 
> It mitigates the issue by bypassing the hard IRQ handler and directly 
> registering the threaded interrupt handler when PREEMPT_RT is enabled.
>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index d3044a3089b53..6d82658a1a66b 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -11235,9 +11235,17 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>>  	 */
>>  	ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
>>  
>> -	/* IRQ registration */
>> +	/* IRQ registration
>> +	 * In the case of PREMMP_RT, directly use the threaded
>
> [Severity: Low]
> This isn't a bug, but there is a typo in this comment ("PREMMP_RT" instead
> of "PREEMPT_RT"). The commit subject also has a small typo ("PREEMP_RT").
>
>> +	 * interrupt to avoid using a spinlock (which could sleep)
>> +	 * in the hard IRQ handler.
>> +	 */
>> +#ifdef CONFIG_PREEMPT_RT
>> +	err = devm_request_irq(dev, irq, ufshcd_threaded_intr, IRQF_SHARED, UFSHCD, hba);
>> +#else
>>  	err = devm_request_threaded_irq(dev, irq, ufshcd_intr, ufshcd_threaded_intr,
>>  					IRQF_ONESHOT | IRQF_SHARED, UFSHCD, hba);
>> +#endif
>
> [Severity: Low]
> Will this cause a compiler warning for an unused function when building with
> CONFIG_PREEMPT_RT=y? 
>
> Because the reference to ufshcd_intr() is compiled out in the #else block, 
> but its static definition remains unconditionally compiled earlier in the 
> file:
>
> static irqreturn_t ufshcd_intr(int irq, void *__hba)
> {
>     ...
> }
>
> Could this break builds that have CONFIG_WERROR=y enabled due to the 
> -Wunused-function warning?
>
>>  	if (err) {
>>  		dev_err(hba->dev, "request irq failed\n");
>>  		goto out_disable;
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260630-ufshcd-spinlock-sleep-fix-v1-1-339b05a1c6f4@xxxxxxxxxxx?part=1

-- 
Grégory CLEMENT, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com