Re: [PATCH v3 39/57] scsi: megaraid_sas: Prepare for enabling lock context analysis

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incorrect `__must_hold` annotations misrepresent API contracts and mask missing lock acquisitions
--

--- Patch [39]: [PATCH v3 39/57] scsi: megaraid_sas: Prepare for enabling lock context analysis ---
commit e0b92da41c18eebd6dc6b3dd8f685d565a0c3f5f
Author: Bart Van Assche <bvanassche@acm.org>

scsi: megaraid_sas: Prepare for enabling lock context analysis
    
This commit prepares the megaraid_sas driver for lock context analysis
by documenting locking requirements with __must_hold().

> diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
> index 8ee2bfe475715..67713173793a3 100644
> --- a/drivers/scsi/megaraid/megaraid_sas.h
> +++ b/drivers/scsi/megaraid/megaraid_sas.h
> @@ -2696,15 +2696,18 @@ __le16 get_updated_dev_handle(struct megasas_instance *instance,
>  			      struct MR_DRV_RAID_MAP_ALL *drv_map);
>  void mr_update_load_balance_params(struct MR_DRV_RAID_MAP_ALL *map,
>  	struct LD_LOAD_BALANCE_INFO *lbInfo);
> -int megasas_get_ctrl_info(struct megasas_instance *instance);
> +int megasas_get_ctrl_info(struct megasas_instance *instance)
> +	__must_hold(&instance->reset_mutex);

[Severity: Medium]
Is this annotation accurate for all callers?

megasas_get_ctrl_info() is called by un-annotated and unprotected paths 
like megasas_init_fw(). Adding __must_hold() here implies the caller is 
responsible for holding the lock, but the caller doesn't currently acquire 
it. Will this cause false positive warnings when lock context analysis is 
enabled?

[ ... ]

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index ecd365d78ae3c..ccefe5841a17b 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
[ ... ]
> @@ -7789,6 +7796,7 @@ megasas_suspend(struct device *dev)
>   */
>  static int __maybe_unused
>  megasas_resume(struct device *dev)
> +	__must_hold(&((struct megasas_instance *)dev_get_drvdata(dev))->reset_mutex)
>  {
>  	int rval;
>  	struct Scsi_Host *host;

[Severity: Medium]
Does the generic PM core actually hold this driver-private mutex when
invoking the resume callback?

Since megasas_resume() is used as a device power management callback, its
callers outside the driver do not know about or hold reset_mutex. Will
this annotation lead to static analysis errors on the PM core invocation
path?

[ ... ]

> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index 056cbe50e19ed..a55094a31c829 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -1792,6 +1792,7 @@ static inline void megasas_free_ioc_init_cmd(struct megasas_instance *instance)
>   */
>  static u32
>  megasas_init_adapter_fusion(struct megasas_instance *instance)
> +	__must_hold(&instance->reset_mutex)
>  {
>  	struct fusion_context *fusion;
>  	u32 scratch_pad_1;

[Severity: Medium]
Will this annotation trigger analysis errors during adapter initialization?

megasas_init_adapter_fusion() is invoked via the init_adapter function
pointer in megasas_init_fw(). The caller does not appear to hold reset_mutex
before invoking the pointer. Should the lock acquisition be added to the 
callers, or is this annotation overly strict here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785451174.git.bvanassche@acm.org?part=39