linux-kernel - Re: [PATCH v2 03/11] HID: playstation: Simplify locking with guard() and scoped

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e9c9685a-6c8c-4f2b-a89c-e20cbbff370f@collabora.com>
Date: Tue, 23 Sep 2025 00:04:59 +0300
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Benjamin Tissoires <bentiss@...nel.org>
Cc: Roderick Colenbrander <roderick.colenbrander@...y.com>,
 Jiri Kosina <jikos@...nel.org>, Henrik Rydberg <rydberg@...math.org>,
 kernel@...labora.com, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 03/11] HID: playstation: Simplify locking with guard()
 and scoped_guard()

Hi Benjamin,

Thank you for reviewing the series and sorry for my late reply, I've just
returned from vacation.

On 9/17/25 5:21 PM, Benjamin Tissoires wrote:
> On Sep 17 2025, Benjamin Tissoires wrote:
>> On Jun 25 2025, Cristian Ciocaltea wrote:
>>> Use guard() and scoped_guard() infrastructure instead of explicitly
>>> acquiring and releasing spinlocks and mutexes to simplify the code and
>>> ensure that all locks are released properly.
>>>
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>>
>> It looks like the patch is now creating sparse errors:
>>
>> https://gitlab.freedesktop.org/bentiss/hid/-/jobs/84636162
>>
>> with:
>>
>> drivers/hid/hid-playstation.c:1187:32: warning: context imbalance in 'dualsense_player_led_set_brightness' - wrong count at exit
>> drivers/hid/hid-playstation.c:1403:9: warning: context imbalance in 'dualsense_parse_report' - different lock contexts for basic block
>> drivers/hid/hid-playstation.c:1499:12: warning: context imbalance in 'dualsense_play_effect' - different lock contexts for basic block
>> drivers/hid/hid-playstation.c:1552:13: warning: context imbalance in 'dualsense_set_lightbar' - wrong count at exit
>> drivers/hid/hid-playstation.c:1564:13: warning: context imbalance in 'dualsense_set_player_leds' - wrong count at exit
>> drivers/hid/hid-playstation.c:2054:33: warning: context imbalance in 'dualshock4_led_set_blink' - wrong count at exit
>> drivers/hid/hid-playstation.c:2095:33: warning: context imbalance in 'dualshock4_led_set_brightness' - wrong count at exit
>> drivers/hid/hid-playstation.c:2463:12: warning: context imbalance in 'dualshock4_play_effect' - different lock contexts for basic block
>> drivers/hid/hid-playstation.c:2501:13: warning: context imbalance in 'dualshock4_set_bt_poll_interval' - wrong count at exit
>> drivers/hid/hid-playstation.c:2509:13: warning: context imbalance in 'dualshock4_set_default_lightbar_colors' - wrong count at exit
>>
>> (the artifacts are going to be removed in 4 hours, so better document
>> the line numbers here).
>>
>> I am under the impression that it's because the 2 *_output_worker
>> functions are not using scoped guarding, but it could very well be
>> something entirely different. Do you mind taking a look as well?
> 
> Turns out it's the mix of guard and scoped_guard that confuses spare:
> https://lkml.org/lkml/2025/6/8/74
> 
> the following shuts down all of the warnings:

Indeed, I think that would be the best workaround for now.  At least I couldn't
find anything better while experimenting with latest sparse commit 0196afe16a50
("Merge branch 'riscv'") - which is still almost 2 years old btw.

> 
> ---
> 
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index d2bee1a314b1..36f3ac044fdc 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -1274,9 +1274,9 @@ static void dualsense_init_output_report(struct dualsense *ds,
>  
>  static inline void dualsense_schedule_work(struct dualsense *ds)
>  {
> -       guard(spinlock_irqsave)(&ds->base.lock);
> -       if (ds->output_worker_initialized)
> -               schedule_work(&ds->output_worker);
> +       scoped_guard(spinlock_irqsave, &ds->base.lock)
> +               if (ds->output_worker_initialized)
> +                       schedule_work(&ds->output_worker);
>  }
>  
>  /*
> @@ -2626,9 +2626,9 @@ static void dualshock4_remove(struct ps_device *ps_dev)
>  
>  static inline void dualshock4_schedule_work(struct dualshock4 *ds4)
>  {
> -       guard(spinlock_irqsave)(&ds4->base.lock);
> -       if (ds4->output_worker_initialized)
> -               schedule_work(&ds4->output_worker);
> +       scoped_guard(spinlock_irqsave, &ds4->base.lock)
> +               if (ds4->output_worker_initialized)
> +                       schedule_work(&ds4->output_worker);
>  }
>  
>  static void dualshock4_set_bt_poll_interval(struct dualshock4 *ds4, u8 interval)
> 
> ---
> 
> There are also a couple more of manual spin_unlock_irqrestore which
> would benefit from the scoped guard mechanism.

Oh, my bad, I somehow missed to mention in the commit description that I
intentionally omitted those residing in {dualsense|dualshock4}_output_worker()
as the functions do already contain plenty of long statements and adding yet
another level of indentation would affect readability in a negative way.

I've just checked it again and it doesn't really look that bad.  Consistency
should be more important, anyway, hence let's convert them as well.

Regards,
Cristian