[PATCH v2] scsi: core: Rework the DEF_SCSI_QCMD() implementation

[PATCH v2] scsi: core: Rework the DEF_SCSI_QCMD() implementation

[Bart Van Assche]

In DEF_SCSI_QCMD(), make the following changes:
 - Assign cmd->device->host to shost because func_name##_lck()
   implementations will be annotated with
   __must_hold(&cmd->device->host->host_lock). This change prepares for
   enabling lock context annotations for SCSI drivers.
 - Do not save and restore the IRQ flags. Since support for the legacy
   block layer has been removed, .queue_rq() implementations are always
   called from thread context and with interrupts enabled. Hence, saving
   and restoring the interrupt state is not necessary.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---

Changes compared to v1:
 - Fixed a use-after-free bug.
 - Added __maybe_unused to the shost argument.

 include/scsi/scsi_host.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 16243ec376cd..d62f1e1fbd0c 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -528,15 +528,22 @@ struct scsi_host_template {
  *
  */
 #define DEF_SCSI_QCMD(func_name) \
-	enum scsi_qc_status func_name(struct Scsi_Host *shost,		\
+	enum scsi_qc_status func_name(struct Scsi_Host *shost		\
+				      __maybe_unused,			\
 				      struct scsi_cmnd *cmd)		\
 	{								\
-		unsigned long irq_flags;				\
 		enum scsi_qc_status rc;					\
 									\
-		spin_lock_irqsave(shost->host_lock, irq_flags);		\
+		/*							\
+		 * Help compiler context analysis by using		\
+		 * cmd->device->host instead of the shost argument	\
+		 * for the func_name##_lck() implementations annotated	\
+		 * with __must_hold(cmd->device->host->host_lock).	\
+		 */							\
+		shost = cmd->device->host;				\
+		spin_lock_irq(shost->host_lock);			\
 		rc = func_name##_lck(cmd);				\
-		spin_unlock_irqrestore(shost->host_lock, irq_flags);	\
+		spin_unlock_irq(shost->host_lock);			\
 		return rc;						\
 	}
 

Re: [PATCH v2] scsi: core: Rework the DEF_SCSI_QCMD() implementation

[John Garry]

On 31/08/2026 23:19, Bart Van Assche wrote:
> In DEF_SCSI_QCMD(), make the following changes:
>    - Assign cmd->device->host to shost because func_name##_lck()
>      implementations will be annotated with
>      __must_hold(&cmd->device->host->host_lock). This change prepares for

Why not annotate with __must_hold(&host->host_lock)? That is what we do 
in scsi_host_set_state().

>      enabling lock context annotations for SCSI drivers.
>    - Do not save and restore the IRQ flags. Since support for the legacy
>      block layer has been removed, .queue_rq() implementations are always
>      called from thread context and with interrupts enabled. Hence, saving
>      and restoring the interrupt state is not necessary.

That is really a separate change.

> 
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>

It would help the maintainer to mention that this conflicts with my 
host_lock patch

> ---
> 
> Changes compared to v1:
>    - Fixed a use-after-free bug.
>    - Added __maybe_unused to the shost argument.
> 
>    include/scsi/scsi_host.h | 15 +++++++++++----
>    1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 16243ec376cd..d62f1e1fbd0c 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -528,15 +528,22 @@ struct scsi_host_template {
>     *
>     */
>    #define DEF_SCSI_QCMD(func_name) \
> -	enum scsi_qc_status func_name(struct Scsi_Host *shost,		\
> +	enum scsi_qc_status func_name(struct Scsi_Host *shost		\
> +				      __maybe_unused,			\
>    				      struct scsi_cmnd *cmd)		\
>    	{								\
> -		unsigned long irq_flags;				\
>    		enum scsi_qc_status rc;					\
>    									\
> -		spin_lock_irqsave(shost->host_lock, irq_flags);		\
> +		/*							\
> +		 * Help compiler context analysis by using		\
> +		 * cmd->device->host instead of the shost argument	\
> +		 * for the func_name##_lck() implementations annotated	\
> +		 * with __must_hold(cmd->device->host->host_lock).	\
> +		 */							\
> +		shost = cmd->device->host;				\
> +		spin_lock_irq(shost->host_lock);			\
>    		rc = func_name##_lck(cmd);				\
> -		spin_unlock_irqrestore(shost->host_lock, irq_flags);	\
> +		spin_unlock_irq(shost->host_lock);			\
>    		return rc;						\
>    	}
>    

Re: [PATCH v2] scsi: core: Rework the DEF_SCSI_QCMD() implementation

[Bart Van Assche]

On 9/1/26 1:04 AM, John Garry wrote:
> On 31/08/2026 23:19, Bart Van Assche wrote:
>> In DEF_SCSI_QCMD(), make the following changes:
>>    - Assign cmd->device->host to shost because func_name##_lck()
>>      implementations will be annotated with
>>      __must_hold(&cmd->device->host->host_lock). This change prepares for
> 
> Why not annotate with __must_hold(&host->host_lock)? That is what we do 
> in scsi_host_set_state().

Because the host pointer is not in the argument list of functions like
blogic_qcmd_lck(). Do you perhaps want me to pass the SCSI host pointer
as an argument to blogic_qcmd_lck()? This could be done by open-coding
DEF_SCSI_QCMD() in the BusLogic driver.

> It would help the maintainer to mention that this conflicts with my 
> host_lock patch

I will wait with posting a new version of this patch until your
host_lock patch has been merged.

Thanks,

Bart.

Re: [PATCH v2] scsi: core: Rework the DEF_SCSI_QCMD() implementation

[John Garry]

On 9/1/26 19:17, Bart Van Assche wrote:
> On 9/1/26 1:04 AM, John Garry wrote:
>> On 31/08/2026 23:19, Bart Van Assche wrote:
>>> In DEF_SCSI_QCMD(), make the following changes:
>>>    - Assign cmd->device->host to shost because func_name##_lck()
>>>      implementations will be annotated with
>>>      __must_hold(&cmd->device->host->host_lock). This change prepares 
>>> for
>>
>> Why not annotate with __must_hold(&host->host_lock)? That is what we 
>> do in scsi_host_set_state().
> 
> Because the host pointer is not in the argument list of functions like
> blogic_qcmd_lck(). 

Yes, silly me.

> Do you perhaps want me to pass the SCSI host pointer
> as an argument to blogic_qcmd_lck()? This could be done by open-coding
> DEF_SCSI_QCMD() in the BusLogic driver.

How would that look? All users of DEF_SCSI_QCMD would need a similar 
change, right?