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@collabora.com>
---
drivers/hid/hid-playstation.c | 216 ++++++++++++++++++------------------------
1 file changed, 93 insertions(+), 123 deletions(-)
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index a82174a21dcce6523a69a8ec374a72504614cedc..16c576cc74284cc680cc7fc17e1bbe6bbcf84177 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -7,6 +7,7 @@
#include <linux/bitfield.h>
#include <linux/bits.h>
+#include <linux/cleanup.h>
#include <linux/crc32.h>
#include <linux/device.h>
#include <linux/hid.h>
@@ -567,26 +568,25 @@ static int ps_devices_list_add(struct ps_device *dev)
{
struct ps_device *entry;
- mutex_lock(&ps_devices_lock);
+ guard(mutex)(&ps_devices_lock);
+
list_for_each_entry(entry, &ps_devices_list, list) {
if (!memcmp(entry->mac_address, dev->mac_address, sizeof(dev->mac_address))) {
hid_err(dev->hdev, "Duplicate device found for MAC address %pMR.\n",
dev->mac_address);
- mutex_unlock(&ps_devices_lock);
return -EEXIST;
}
}
list_add_tail(&dev->list, &ps_devices_list);
- mutex_unlock(&ps_devices_lock);
return 0;
}
static int ps_devices_list_remove(struct ps_device *dev)
{
- mutex_lock(&ps_devices_lock);
+ guard(mutex)(&ps_devices_lock);
+
list_del(&dev->list);
- mutex_unlock(&ps_devices_lock);
return 0;
}
@@ -650,13 +650,12 @@ static int ps_battery_get_property(struct power_supply *psy,
struct ps_device *dev = power_supply_get_drvdata(psy);
uint8_t battery_capacity;
int battery_status;
- unsigned long flags;
int ret = 0;
- spin_lock_irqsave(&dev->lock, flags);
- battery_capacity = dev->battery_capacity;
- battery_status = dev->battery_status;
- spin_unlock_irqrestore(&dev->lock, flags);
+ scoped_guard(spinlock_irqsave, &dev->lock) {
+ battery_capacity = dev->battery_capacity;
+ battery_status = dev->battery_status;
+ }
switch (psp) {
case POWER_SUPPLY_PROP_STATUS:
@@ -1174,19 +1173,17 @@ static int dualsense_player_led_set_brightness(struct led_classdev *led, enum le
{
struct hid_device *hdev = to_hid_device(led->dev->parent);
struct dualsense *ds = hid_get_drvdata(hdev);
- unsigned long flags;
unsigned int led_index;
- spin_lock_irqsave(&ds->base.lock, flags);
-
- led_index = led - ds->player_leds;
- if (value == LED_OFF)
- ds->player_leds_state &= ~BIT(led_index);
- else
- ds->player_leds_state |= BIT(led_index);
+ scoped_guard(spinlock_irqsave, &ds->base.lock) {
+ led_index = led - ds->player_leds;
+ if (value == LED_OFF)
+ ds->player_leds_state &= ~BIT(led_index);
+ else
+ ds->player_leds_state |= BIT(led_index);
- ds->update_player_leds = true;
- spin_unlock_irqrestore(&ds->base.lock, flags);
+ ds->update_player_leds = true;
+ }
dualsense_schedule_work(ds);
@@ -1235,12 +1232,9 @@ static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_
static inline void dualsense_schedule_work(struct dualsense *ds)
{
- unsigned long flags;
-
- spin_lock_irqsave(&ds->base.lock, flags);
+ guard(spinlock_irqsave)(&ds->base.lock);
if (ds->output_worker_initialized)
schedule_work(&ds->output_worker);
- spin_unlock_irqrestore(&ds->base.lock, flags);
}
/*
@@ -1338,7 +1332,6 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
int battery_status;
uint32_t sensor_timestamp;
bool btn_mic_state;
- unsigned long flags;
int i;
/*
@@ -1400,10 +1393,10 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
*/
btn_mic_state = !!(ds_report->buttons[2] & DS_BUTTONS2_MIC_MUTE);
if (btn_mic_state && !ds->last_btn_mic_state) {
- spin_lock_irqsave(&ps_dev->lock, flags);
- ds->update_mic_mute = true;
- ds->mic_muted = !ds->mic_muted; /* toggle */
- spin_unlock_irqrestore(&ps_dev->lock, flags);
+ scoped_guard(spinlock_irqsave, &ps_dev->lock) {
+ ds->update_mic_mute = true;
+ ds->mic_muted = !ds->mic_muted; /* toggle */
+ }
/* Schedule updating of microphone state at hardware level. */
dualsense_schedule_work(ds);
@@ -1496,10 +1489,10 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
}
- spin_lock_irqsave(&ps_dev->lock, flags);
- ps_dev->battery_capacity = battery_capacity;
- ps_dev->battery_status = battery_status;
- spin_unlock_irqrestore(&ps_dev->lock, flags);
+ scoped_guard(spinlock_irqsave, &ps_dev->lock) {
+ ps_dev->battery_capacity = battery_capacity;
+ ps_dev->battery_status = battery_status;
+ }
return 0;
}
@@ -1508,16 +1501,15 @@ static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_ef
{
struct hid_device *hdev = input_get_drvdata(dev);
struct dualsense *ds = hid_get_drvdata(hdev);
- unsigned long flags;
if (effect->type != FF_RUMBLE)
return 0;
- spin_lock_irqsave(&ds->base.lock, flags);
- ds->update_rumble = true;
- ds->motor_left = effect->u.rumble.strong_magnitude / 256;
- ds->motor_right = effect->u.rumble.weak_magnitude / 256;
- spin_unlock_irqrestore(&ds->base.lock, flags);
+ scoped_guard(spinlock_irqsave, &ds->base.lock) {
+ ds->update_rumble = true;
+ ds->motor_left = effect->u.rumble.strong_magnitude / 256;
+ ds->motor_right = effect->u.rumble.weak_magnitude / 256;
+ }
dualsense_schedule_work(ds);
return 0;
@@ -1526,11 +1518,9 @@ static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_ef
static void dualsense_remove(struct ps_device *ps_dev)
{
struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
- unsigned long flags;
- spin_lock_irqsave(&ds->base.lock, flags);
- ds->output_worker_initialized = false;
- spin_unlock_irqrestore(&ds->base.lock, flags);
+ scoped_guard(spinlock_irqsave, &ds->base.lock)
+ ds->output_worker_initialized = false;
cancel_work_sync(&ds->output_worker);
}
@@ -1562,14 +1552,12 @@ static int dualsense_reset_leds(struct dualsense *ds)
static void dualsense_set_lightbar(struct dualsense *ds, uint8_t red, uint8_t green, uint8_t blue)
{
- unsigned long flags;
-
- spin_lock_irqsave(&ds->base.lock, flags);
- ds->update_lightbar = true;
- ds->lightbar_red = red;
- ds->lightbar_green = green;
- ds->lightbar_blue = blue;
- spin_unlock_irqrestore(&ds->base.lock, flags);
+ scoped_guard(spinlock_irqsave, &ds->base.lock) {
+ ds->update_lightbar = true;
+ ds->lightbar_red = red;
+ ds->lightbar_green = green;
+ ds->lightbar_blue = blue;
+ }
dualsense_schedule_work(ds);
}
@@ -1756,7 +1744,6 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
static void dualshock4_dongle_calibration_work(struct work_struct *work)
{
struct dualshock4 *ds4 = container_of(work, struct dualshock4, dongle_hotplug_worker);
- unsigned long flags;
enum dualshock4_dongle_state dongle_state;
int ret;
@@ -1775,9 +1762,8 @@ static void dualshock4_dongle_calibration_work(struct work_struct *work)
dongle_state = DONGLE_CONNECTED;
}
- spin_lock_irqsave(&ds4->base.lock, flags);
- ds4->dongle_state = dongle_state;
- spin_unlock_irqrestore(&ds4->base.lock, flags);
+ scoped_guard(spinlock_irqsave, &ds4->base.lock)
+ ds4->dongle_state = dongle_state;
}
static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
@@ -2049,26 +2035,23 @@ static int dualshock4_led_set_blink(struct led_classdev *led, unsigned long *del
{
struct hid_device *hdev = to_hid_device(led->dev->parent);
struct dualshock4 *ds4 = hid_get_drvdata(hdev);
- unsigned long flags;
- spin_lock_irqsave(&ds4->base.lock, flags);
+ scoped_guard(spinlock_irqsave, &ds4->base.lock) {
+ if (!*delay_on && !*delay_off) {
+ /* Default to 1 Hz (50 centiseconds on, 50 centiseconds off). */
+ ds4->lightbar_blink_on = 50;
+ ds4->lightbar_blink_off = 50;
+ } else {
+ /* Blink delays in centiseconds. */
+ ds4->lightbar_blink_on = min_t(unsigned long, *delay_on / 10,
+ DS4_LIGHTBAR_MAX_BLINK);
+ ds4->lightbar_blink_off = min_t(unsigned long, *delay_off / 10,
+ DS4_LIGHTBAR_MAX_BLINK);
+ }
- if (!*delay_on && !*delay_off) {
- /* Default to 1 Hz (50 centiseconds on, 50 centiseconds off). */
- ds4->lightbar_blink_on = 50;
- ds4->lightbar_blink_off = 50;
- } else {
- /* Blink delays in centiseconds. */
- ds4->lightbar_blink_on = min_t(unsigned long, *delay_on / 10,
- DS4_LIGHTBAR_MAX_BLINK);
- ds4->lightbar_blink_off = min_t(unsigned long, *delay_off / 10,
- DS4_LIGHTBAR_MAX_BLINK);
+ ds4->update_lightbar_blink = true;
}
- ds4->update_lightbar_blink = true;
-
- spin_unlock_irqrestore(&ds4->base.lock, flags);
-
dualshock4_schedule_work(ds4);
/* Report scaled values back to LED subsystem */
@@ -2082,36 +2065,33 @@ static int dualshock4_led_set_brightness(struct led_classdev *led, enum led_brig
{
struct hid_device *hdev = to_hid_device(led->dev->parent);
struct dualshock4 *ds4 = hid_get_drvdata(hdev);
- unsigned long flags;
unsigned int led_index;
- spin_lock_irqsave(&ds4->base.lock, flags);
-
- led_index = led - ds4->lightbar_leds;
- switch (led_index) {
- case 0:
- ds4->lightbar_red = value;
- break;
- case 1:
- ds4->lightbar_green = value;
- break;
- case 2:
- ds4->lightbar_blue = value;
- break;
- case 3:
- ds4->lightbar_enabled = !!value;
-
- /* brightness = 0 also cancels blinking in Linux. */
- if (!ds4->lightbar_enabled) {
- ds4->lightbar_blink_off = 0;
- ds4->lightbar_blink_on = 0;
- ds4->update_lightbar_blink = true;
+ scoped_guard(spinlock_irqsave, &ds4->base.lock) {
+ led_index = led - ds4->lightbar_leds;
+ switch (led_index) {
+ case 0:
+ ds4->lightbar_red = value;
+ break;
+ case 1:
+ ds4->lightbar_green = value;
+ break;
+ case 2:
+ ds4->lightbar_blue = value;
+ break;
+ case 3:
+ ds4->lightbar_enabled = !!value;
+
+ /* brightness = 0 also cancels blinking in Linux. */
+ if (!ds4->lightbar_enabled) {
+ ds4->lightbar_blink_off = 0;
+ ds4->lightbar_blink_on = 0;
+ ds4->update_lightbar_blink = true;
+ }
}
- }
-
- ds4->update_lightbar = true;
- spin_unlock_irqrestore(&ds4->base.lock, flags);
+ ds4->update_lightbar = true;
+ }
dualshock4_schedule_work(ds4);
@@ -2243,7 +2223,6 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
uint8_t battery_capacity, num_touch_reports, value;
int battery_status, i, j;
uint16_t sensor_timestamp;
- unsigned long flags;
bool is_minimal = false;
/*
@@ -2421,10 +2400,10 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
battery_status = POWER_SUPPLY_STATUS_DISCHARGING;
}
- spin_lock_irqsave(&ps_dev->lock, flags);
- ps_dev->battery_capacity = battery_capacity;
- ps_dev->battery_status = battery_status;
- spin_unlock_irqrestore(&ps_dev->lock, flags);
+ scoped_guard(spinlock_irqsave, &ps_dev->lock) {
+ ps_dev->battery_capacity = battery_capacity;
+ ps_dev->battery_status = battery_status;
+ }
return 0;
}
@@ -2442,7 +2421,6 @@ static int dualshock4_dongle_parse_report(struct ps_device *ps_dev, struct hid_r
*/
if (data[0] == DS4_INPUT_REPORT_USB && size == DS4_INPUT_REPORT_USB_SIZE) {
struct dualshock4_input_report_common *ds4_report = (struct dualshock4_input_report_common *)&data[1];
- unsigned long flags;
connected = ds4_report->status[1] & DS4_STATUS1_DONGLE_STATE ? false : true;
@@ -2451,9 +2429,8 @@ static int dualshock4_dongle_parse_report(struct ps_device *ps_dev, struct hid_r
dualshock4_set_default_lightbar_colors(ds4);
- spin_lock_irqsave(&ps_dev->lock, flags);
- ds4->dongle_state = DONGLE_CALIBRATING;
- spin_unlock_irqrestore(&ps_dev->lock, flags);
+ scoped_guard(spinlock_irqsave, &ps_dev->lock)
+ ds4->dongle_state = DONGLE_CALIBRATING;
schedule_work(&ds4->dongle_hotplug_worker);
@@ -2465,9 +2442,8 @@ static int dualshock4_dongle_parse_report(struct ps_device *ps_dev, struct hid_r
ds4->dongle_state == DONGLE_DISABLED) && !connected) {
hid_info(ps_dev->hdev, "DualShock 4 USB dongle: controller disconnected\n");
- spin_lock_irqsave(&ps_dev->lock, flags);
- ds4->dongle_state = DONGLE_DISCONNECTED;
- spin_unlock_irqrestore(&ps_dev->lock, flags);
+ scoped_guard(spinlock_irqsave, &ps_dev->lock)
+ ds4->dongle_state = DONGLE_DISCONNECTED;
/* Return 0, so hidraw can get the report. */
return 0;
@@ -2489,16 +2465,15 @@ static int dualshock4_play_effect(struct input_dev *dev, void *data, struct ff_e
{
struct hid_device *hdev = input_get_drvdata(dev);
struct dualshock4 *ds4 = hid_get_drvdata(hdev);
- unsigned long flags;
if (effect->type != FF_RUMBLE)
return 0;
- spin_lock_irqsave(&ds4->base.lock, flags);
- ds4->update_rumble = true;
- ds4->motor_left = effect->u.rumble.strong_magnitude / 256;
- ds4->motor_right = effect->u.rumble.weak_magnitude / 256;
- spin_unlock_irqrestore(&ds4->base.lock, flags);
+ scoped_guard(spinlock_irqsave, &ds4->base.lock) {
+ ds4->update_rumble = true;
+ ds4->motor_left = effect->u.rumble.strong_magnitude / 256;
+ ds4->motor_right = effect->u.rumble.weak_magnitude / 256;
+ }
dualshock4_schedule_work(ds4);
return 0;
@@ -2507,11 +2482,9 @@ static int dualshock4_play_effect(struct input_dev *dev, void *data, struct ff_e
static void dualshock4_remove(struct ps_device *ps_dev)
{
struct dualshock4 *ds4 = container_of(ps_dev, struct dualshock4, base);
- unsigned long flags;
- spin_lock_irqsave(&ds4->base.lock, flags);
- ds4->output_worker_initialized = false;
- spin_unlock_irqrestore(&ds4->base.lock, flags);
+ scoped_guard(spinlock_irqsave, &ds4->base.lock)
+ ds4->output_worker_initialized = false;
cancel_work_sync(&ds4->output_worker);
@@ -2521,12 +2494,9 @@ static void dualshock4_remove(struct ps_device *ps_dev)
static inline void dualshock4_schedule_work(struct dualshock4 *ds4)
{
- unsigned long flags;
-
- spin_lock_irqsave(&ds4->base.lock, flags);
+ guard(spinlock_irqsave)(&ds4->base.lock);
if (ds4->output_worker_initialized)
schedule_work(&ds4->output_worker);
- spin_unlock_irqrestore(&ds4->base.lock, flags);
}
static void dualshock4_set_bt_poll_interval(struct dualshock4 *ds4, uint8_t interval)
--
2.49.0Hi Christian,
This patch look fine and does simplify some things. Though in terms of
size the patch is a bit long, so it took some careful looking at the
code.
Thanks,
Roderick
On Mon, May 26, 2025 at 5:53 AM Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> 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@collabora.com>
> ---
> drivers/hid/hid-playstation.c | 216 ++++++++++++++++++------------------------
> 1 file changed, 93 insertions(+), 123 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index a82174a21dcce6523a69a8ec374a72504614cedc..16c576cc74284cc680cc7fc17e1bbe6bbcf84177 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -7,6 +7,7 @@
>
> #include <linux/bitfield.h>
> #include <linux/bits.h>
> +#include <linux/cleanup.h>
> #include <linux/crc32.h>
> #include <linux/device.h>
> #include <linux/hid.h>
> @@ -567,26 +568,25 @@ static int ps_devices_list_add(struct ps_device *dev)
> {
> struct ps_device *entry;
>
> - mutex_lock(&ps_devices_lock);
> + guard(mutex)(&ps_devices_lock);
> +
> list_for_each_entry(entry, &ps_devices_list, list) {
> if (!memcmp(entry->mac_address, dev->mac_address, sizeof(dev->mac_address))) {
> hid_err(dev->hdev, "Duplicate device found for MAC address %pMR.\n",
> dev->mac_address);
> - mutex_unlock(&ps_devices_lock);
> return -EEXIST;
> }
> }
>
> list_add_tail(&dev->list, &ps_devices_list);
> - mutex_unlock(&ps_devices_lock);
> return 0;
> }
>
> static int ps_devices_list_remove(struct ps_device *dev)
> {
> - mutex_lock(&ps_devices_lock);
> + guard(mutex)(&ps_devices_lock);
> +
> list_del(&dev->list);
> - mutex_unlock(&ps_devices_lock);
> return 0;
> }
>
> @@ -650,13 +650,12 @@ static int ps_battery_get_property(struct power_supply *psy,
> struct ps_device *dev = power_supply_get_drvdata(psy);
> uint8_t battery_capacity;
> int battery_status;
> - unsigned long flags;
> int ret = 0;
>
> - spin_lock_irqsave(&dev->lock, flags);
> - battery_capacity = dev->battery_capacity;
> - battery_status = dev->battery_status;
> - spin_unlock_irqrestore(&dev->lock, flags);
> + scoped_guard(spinlock_irqsave, &dev->lock) {
> + battery_capacity = dev->battery_capacity;
> + battery_status = dev->battery_status;
> + }
>
> switch (psp) {
> case POWER_SUPPLY_PROP_STATUS:
> @@ -1174,19 +1173,17 @@ static int dualsense_player_led_set_brightness(struct led_classdev *led, enum le
> {
> struct hid_device *hdev = to_hid_device(led->dev->parent);
> struct dualsense *ds = hid_get_drvdata(hdev);
> - unsigned long flags;
> unsigned int led_index;
>
> - spin_lock_irqsave(&ds->base.lock, flags);
> -
> - led_index = led - ds->player_leds;
> - if (value == LED_OFF)
> - ds->player_leds_state &= ~BIT(led_index);
> - else
> - ds->player_leds_state |= BIT(led_index);
> + scoped_guard(spinlock_irqsave, &ds->base.lock) {
> + led_index = led - ds->player_leds;
> + if (value == LED_OFF)
> + ds->player_leds_state &= ~BIT(led_index);
> + else
> + ds->player_leds_state |= BIT(led_index);
>
> - ds->update_player_leds = true;
> - spin_unlock_irqrestore(&ds->base.lock, flags);
> + ds->update_player_leds = true;
> + }
>
> dualsense_schedule_work(ds);
>
> @@ -1235,12 +1232,9 @@ static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_
>
> static inline void dualsense_schedule_work(struct dualsense *ds)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(&ds->base.lock, flags);
> + guard(spinlock_irqsave)(&ds->base.lock);
> if (ds->output_worker_initialized)
> schedule_work(&ds->output_worker);
> - spin_unlock_irqrestore(&ds->base.lock, flags);
> }
>
> /*
> @@ -1338,7 +1332,6 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> int battery_status;
> uint32_t sensor_timestamp;
> bool btn_mic_state;
> - unsigned long flags;
> int i;
>
> /*
> @@ -1400,10 +1393,10 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> */
> btn_mic_state = !!(ds_report->buttons[2] & DS_BUTTONS2_MIC_MUTE);
> if (btn_mic_state && !ds->last_btn_mic_state) {
> - spin_lock_irqsave(&ps_dev->lock, flags);
> - ds->update_mic_mute = true;
> - ds->mic_muted = !ds->mic_muted; /* toggle */
> - spin_unlock_irqrestore(&ps_dev->lock, flags);
> + scoped_guard(spinlock_irqsave, &ps_dev->lock) {
> + ds->update_mic_mute = true;
> + ds->mic_muted = !ds->mic_muted; /* toggle */
> + }
>
> /* Schedule updating of microphone state at hardware level. */
> dualsense_schedule_work(ds);
> @@ -1496,10 +1489,10 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
> }
>
> - spin_lock_irqsave(&ps_dev->lock, flags);
> - ps_dev->battery_capacity = battery_capacity;
> - ps_dev->battery_status = battery_status;
> - spin_unlock_irqrestore(&ps_dev->lock, flags);
> + scoped_guard(spinlock_irqsave, &ps_dev->lock) {
> + ps_dev->battery_capacity = battery_capacity;
> + ps_dev->battery_status = battery_status;
> + }
>
> return 0;
> }
> @@ -1508,16 +1501,15 @@ static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_ef
> {
> struct hid_device *hdev = input_get_drvdata(dev);
> struct dualsense *ds = hid_get_drvdata(hdev);
> - unsigned long flags;
>
> if (effect->type != FF_RUMBLE)
> return 0;
>
> - spin_lock_irqsave(&ds->base.lock, flags);
> - ds->update_rumble = true;
> - ds->motor_left = effect->u.rumble.strong_magnitude / 256;
> - ds->motor_right = effect->u.rumble.weak_magnitude / 256;
> - spin_unlock_irqrestore(&ds->base.lock, flags);
> + scoped_guard(spinlock_irqsave, &ds->base.lock) {
> + ds->update_rumble = true;
> + ds->motor_left = effect->u.rumble.strong_magnitude / 256;
> + ds->motor_right = effect->u.rumble.weak_magnitude / 256;
> + }
>
> dualsense_schedule_work(ds);
> return 0;
> @@ -1526,11 +1518,9 @@ static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_ef
> static void dualsense_remove(struct ps_device *ps_dev)
> {
> struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
> - unsigned long flags;
>
> - spin_lock_irqsave(&ds->base.lock, flags);
> - ds->output_worker_initialized = false;
> - spin_unlock_irqrestore(&ds->base.lock, flags);
> + scoped_guard(spinlock_irqsave, &ds->base.lock)
> + ds->output_worker_initialized = false;
>
> cancel_work_sync(&ds->output_worker);
> }
> @@ -1562,14 +1552,12 @@ static int dualsense_reset_leds(struct dualsense *ds)
>
> static void dualsense_set_lightbar(struct dualsense *ds, uint8_t red, uint8_t green, uint8_t blue)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(&ds->base.lock, flags);
> - ds->update_lightbar = true;
> - ds->lightbar_red = red;
> - ds->lightbar_green = green;
> - ds->lightbar_blue = blue;
> - spin_unlock_irqrestore(&ds->base.lock, flags);
> + scoped_guard(spinlock_irqsave, &ds->base.lock) {
> + ds->update_lightbar = true;
> + ds->lightbar_red = red;
> + ds->lightbar_green = green;
> + ds->lightbar_blue = blue;
> + }
>
> dualsense_schedule_work(ds);
> }
> @@ -1756,7 +1744,6 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
> static void dualshock4_dongle_calibration_work(struct work_struct *work)
> {
> struct dualshock4 *ds4 = container_of(work, struct dualshock4, dongle_hotplug_worker);
> - unsigned long flags;
> enum dualshock4_dongle_state dongle_state;
> int ret;
>
> @@ -1775,9 +1762,8 @@ static void dualshock4_dongle_calibration_work(struct work_struct *work)
> dongle_state = DONGLE_CONNECTED;
> }
>
> - spin_lock_irqsave(&ds4->base.lock, flags);
> - ds4->dongle_state = dongle_state;
> - spin_unlock_irqrestore(&ds4->base.lock, flags);
> + scoped_guard(spinlock_irqsave, &ds4->base.lock)
> + ds4->dongle_state = dongle_state;
> }
>
> static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
> @@ -2049,26 +2035,23 @@ static int dualshock4_led_set_blink(struct led_classdev *led, unsigned long *del
> {
> struct hid_device *hdev = to_hid_device(led->dev->parent);
> struct dualshock4 *ds4 = hid_get_drvdata(hdev);
> - unsigned long flags;
>
> - spin_lock_irqsave(&ds4->base.lock, flags);
> + scoped_guard(spinlock_irqsave, &ds4->base.lock) {
> + if (!*delay_on && !*delay_off) {
> + /* Default to 1 Hz (50 centiseconds on, 50 centiseconds off). */
> + ds4->lightbar_blink_on = 50;
> + ds4->lightbar_blink_off = 50;
> + } else {
> + /* Blink delays in centiseconds. */
> + ds4->lightbar_blink_on = min_t(unsigned long, *delay_on / 10,
> + DS4_LIGHTBAR_MAX_BLINK);
> + ds4->lightbar_blink_off = min_t(unsigned long, *delay_off / 10,
> + DS4_LIGHTBAR_MAX_BLINK);
> + }
>
> - if (!*delay_on && !*delay_off) {
> - /* Default to 1 Hz (50 centiseconds on, 50 centiseconds off). */
> - ds4->lightbar_blink_on = 50;
> - ds4->lightbar_blink_off = 50;
> - } else {
> - /* Blink delays in centiseconds. */
> - ds4->lightbar_blink_on = min_t(unsigned long, *delay_on / 10,
> - DS4_LIGHTBAR_MAX_BLINK);
> - ds4->lightbar_blink_off = min_t(unsigned long, *delay_off / 10,
> - DS4_LIGHTBAR_MAX_BLINK);
> + ds4->update_lightbar_blink = true;
> }
>
> - ds4->update_lightbar_blink = true;
> -
> - spin_unlock_irqrestore(&ds4->base.lock, flags);
> -
> dualshock4_schedule_work(ds4);
>
> /* Report scaled values back to LED subsystem */
> @@ -2082,36 +2065,33 @@ static int dualshock4_led_set_brightness(struct led_classdev *led, enum led_brig
> {
> struct hid_device *hdev = to_hid_device(led->dev->parent);
> struct dualshock4 *ds4 = hid_get_drvdata(hdev);
> - unsigned long flags;
> unsigned int led_index;
>
> - spin_lock_irqsave(&ds4->base.lock, flags);
> -
> - led_index = led - ds4->lightbar_leds;
> - switch (led_index) {
> - case 0:
> - ds4->lightbar_red = value;
> - break;
> - case 1:
> - ds4->lightbar_green = value;
> - break;
> - case 2:
> - ds4->lightbar_blue = value;
> - break;
> - case 3:
> - ds4->lightbar_enabled = !!value;
> -
> - /* brightness = 0 also cancels blinking in Linux. */
> - if (!ds4->lightbar_enabled) {
> - ds4->lightbar_blink_off = 0;
> - ds4->lightbar_blink_on = 0;
> - ds4->update_lightbar_blink = true;
> + scoped_guard(spinlock_irqsave, &ds4->base.lock) {
> + led_index = led - ds4->lightbar_leds;
> + switch (led_index) {
> + case 0:
> + ds4->lightbar_red = value;
> + break;
> + case 1:
> + ds4->lightbar_green = value;
> + break;
> + case 2:
> + ds4->lightbar_blue = value;
> + break;
> + case 3:
> + ds4->lightbar_enabled = !!value;
> +
> + /* brightness = 0 also cancels blinking in Linux. */
> + if (!ds4->lightbar_enabled) {
> + ds4->lightbar_blink_off = 0;
> + ds4->lightbar_blink_on = 0;
> + ds4->update_lightbar_blink = true;
> + }
> }
> - }
> -
> - ds4->update_lightbar = true;
>
> - spin_unlock_irqrestore(&ds4->base.lock, flags);
> + ds4->update_lightbar = true;
> + }
>
> dualshock4_schedule_work(ds4);
>
> @@ -2243,7 +2223,6 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
> uint8_t battery_capacity, num_touch_reports, value;
> int battery_status, i, j;
> uint16_t sensor_timestamp;
> - unsigned long flags;
> bool is_minimal = false;
>
> /*
> @@ -2421,10 +2400,10 @@ static int dualshock4_parse_report(struct ps_device *ps_dev, struct hid_report *
> battery_status = POWER_SUPPLY_STATUS_DISCHARGING;
> }
>
> - spin_lock_irqsave(&ps_dev->lock, flags);
> - ps_dev->battery_capacity = battery_capacity;
> - ps_dev->battery_status = battery_status;
> - spin_unlock_irqrestore(&ps_dev->lock, flags);
> + scoped_guard(spinlock_irqsave, &ps_dev->lock) {
> + ps_dev->battery_capacity = battery_capacity;
> + ps_dev->battery_status = battery_status;
> + }
>
> return 0;
> }
> @@ -2442,7 +2421,6 @@ static int dualshock4_dongle_parse_report(struct ps_device *ps_dev, struct hid_r
> */
> if (data[0] == DS4_INPUT_REPORT_USB && size == DS4_INPUT_REPORT_USB_SIZE) {
> struct dualshock4_input_report_common *ds4_report = (struct dualshock4_input_report_common *)&data[1];
> - unsigned long flags;
>
> connected = ds4_report->status[1] & DS4_STATUS1_DONGLE_STATE ? false : true;
>
> @@ -2451,9 +2429,8 @@ static int dualshock4_dongle_parse_report(struct ps_device *ps_dev, struct hid_r
>
> dualshock4_set_default_lightbar_colors(ds4);
>
> - spin_lock_irqsave(&ps_dev->lock, flags);
> - ds4->dongle_state = DONGLE_CALIBRATING;
> - spin_unlock_irqrestore(&ps_dev->lock, flags);
> + scoped_guard(spinlock_irqsave, &ps_dev->lock)
> + ds4->dongle_state = DONGLE_CALIBRATING;
>
> schedule_work(&ds4->dongle_hotplug_worker);
>
> @@ -2465,9 +2442,8 @@ static int dualshock4_dongle_parse_report(struct ps_device *ps_dev, struct hid_r
> ds4->dongle_state == DONGLE_DISABLED) && !connected) {
> hid_info(ps_dev->hdev, "DualShock 4 USB dongle: controller disconnected\n");
>
> - spin_lock_irqsave(&ps_dev->lock, flags);
> - ds4->dongle_state = DONGLE_DISCONNECTED;
> - spin_unlock_irqrestore(&ps_dev->lock, flags);
> + scoped_guard(spinlock_irqsave, &ps_dev->lock)
> + ds4->dongle_state = DONGLE_DISCONNECTED;
>
> /* Return 0, so hidraw can get the report. */
> return 0;
> @@ -2489,16 +2465,15 @@ static int dualshock4_play_effect(struct input_dev *dev, void *data, struct ff_e
> {
> struct hid_device *hdev = input_get_drvdata(dev);
> struct dualshock4 *ds4 = hid_get_drvdata(hdev);
> - unsigned long flags;
>
> if (effect->type != FF_RUMBLE)
> return 0;
>
> - spin_lock_irqsave(&ds4->base.lock, flags);
> - ds4->update_rumble = true;
> - ds4->motor_left = effect->u.rumble.strong_magnitude / 256;
> - ds4->motor_right = effect->u.rumble.weak_magnitude / 256;
> - spin_unlock_irqrestore(&ds4->base.lock, flags);
> + scoped_guard(spinlock_irqsave, &ds4->base.lock) {
> + ds4->update_rumble = true;
> + ds4->motor_left = effect->u.rumble.strong_magnitude / 256;
> + ds4->motor_right = effect->u.rumble.weak_magnitude / 256;
> + }
>
> dualshock4_schedule_work(ds4);
> return 0;
> @@ -2507,11 +2482,9 @@ static int dualshock4_play_effect(struct input_dev *dev, void *data, struct ff_e
> static void dualshock4_remove(struct ps_device *ps_dev)
> {
> struct dualshock4 *ds4 = container_of(ps_dev, struct dualshock4, base);
> - unsigned long flags;
>
> - spin_lock_irqsave(&ds4->base.lock, flags);
> - ds4->output_worker_initialized = false;
> - spin_unlock_irqrestore(&ds4->base.lock, flags);
> + scoped_guard(spinlock_irqsave, &ds4->base.lock)
> + ds4->output_worker_initialized = false;
>
> cancel_work_sync(&ds4->output_worker);
>
> @@ -2521,12 +2494,9 @@ static void dualshock4_remove(struct ps_device *ps_dev)
>
> static inline void dualshock4_schedule_work(struct dualshock4 *ds4)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(&ds4->base.lock, flags);
> + guard(spinlock_irqsave)(&ds4->base.lock);
> if (ds4->output_worker_initialized)
> schedule_work(&ds4->output_worker);
> - spin_unlock_irqrestore(&ds4->base.lock, flags);
> }
>
> static void dualshock4_set_bt_poll_interval(struct dualshock4 *ds4, uint8_t interval)
>
> --
> 2.49.0
>
>
Hi Roderick, On 6/10/25 7:28 AM, Roderick Colenbrander wrote: > Hi Christian, > > This patch look fine and does simplify some things. Though in terms of > size the patch is a bit long, so it took some careful looking at the > code. Yeah, sorry for the long list of changes - I had a final round of inspection before sending the patch out just to make sure I haven't introduced a regression or something. But that's not a guaranty, obviously, hence thanks a lot for checking this out! Regards, Cristian
© 2016 - 2025 Red Hat, Inc.