Re: [PATCH 1/4] drm/i915/gt: add sparse lock annotation to avoid warnings

[Matt Roper <matthew.d.roper@xxxxxxxxx>]

On Tue, Feb 07, 2023 at 02:40:23PM +0200, Jani Nikula wrote:
> Annotate intel_gt_mcr_lock() and intel_gt_mcr_unlock() to fix sparse
> warnings:
> 
> drivers/gpu/drm/i915/gt/intel_gt_mcr.c:397:9: warning: context imbalance in 'intel_gt_mcr_lock' - wrong count at exit
> drivers/gpu/drm/i915/gt/intel_gt_mcr.c:412:6: warning: context imbalance in 'intel_gt_mcr_unlock' - unexpected unlock
> 
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> index 169393a7ad88..a4a8b8bc5737 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> @@ -364,6 +364,7 @@ static u32 rw_with_mcr_steering(struct intel_gt *gt,
>   *          function call.
>   */
>  void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags)
> +	__acquires(&gt->mcr_lock)

Are these annotations supposed to take the lock itself or a pointer to
the lock as you're doing here?  Or does it matter?  It seems like
there's a mix of both forms elsewhere in the kernel and sparse.rst
doesn't really specify which is expected.

Assuming it's correct to provide a pointer to the lock in the
annotation,

Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>

>  {
>  	unsigned long __flags;
>  	int err = 0;
> @@ -410,6 +411,7 @@ void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags)
>   * Context: Releases gt->mcr_lock
>   */
>  void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags)
> +	__releases(&gt->mcr_lock)
>  {
>  	spin_unlock_irqrestore(&gt->mcr_lock, flags);
>  
> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation