Re: [PATCH 1/6] HID: hid-playstation: Allow removal of touchpad

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

Hi Vicki,

On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>
> This allows the touchpad input_dev to be removed and have the driver remain
> functional without its presence. This will be used to allow the touchpad to
> be disabled, e.g. by a module parameter.

Thanks for the contribution.
I'd like to hear from Roderick, but I have a general comment here:
We had Wii and Steam controllers following this logic. Now we are
adding Sony PS ones... That seems like a lot of duplication, and I
wonder if we should not have something more generic.

We are working on an ioctl to revoke hidraw nodes and I wonder if we
should not take this opportunity to have a more generic mechanism in
HID to also disable input events when the hidraw node is opened...

One comment on the code itself below.

>
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
>  drivers/hid/hid-playstation.c | 60 +++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index ab7c82c2e886..f859a8dd8a2e 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -29,6 +29,7 @@ static DEFINE_IDA(ps_player_id_allocator);
>  struct ps_device {
>         struct list_head list;
>         struct hid_device *hdev;
> +       struct mutex mutex;
>         spinlock_t lock;
>
>         uint32_t player_id;
> @@ -130,7 +131,7 @@ struct dualsense {
>         struct ps_device base;
>         struct input_dev *gamepad;
>         struct input_dev *sensors;
> -       struct input_dev *touchpad;
> +       struct input_dev __rcu *touchpad;
>
>         /* Calibration data for accelerometer and gyroscope. */
>         struct ps_calibration_data accel_calib_data[3];
> @@ -590,6 +591,22 @@ static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width,
>         return touchpad;
>  }
>
> +static void dualsense_unregister_touchpad(struct dualsense *ds)
> +{
> +       struct input_dev *touchpad;
> +
> +       rcu_read_lock();
> +       touchpad = rcu_dereference(ds->touchpad);
> +       rcu_read_unlock();
> +
> +       if (!touchpad)
> +               return;
> +
> +       RCU_INIT_POINTER(ds->touchpad, NULL);
> +       synchronize_rcu();
> +       input_unregister_device(touchpad);
> +}
> +
>  static ssize_t firmware_version_show(struct device *dev,
>                                 struct device_attribute
>                                 *attr, char *buf)
> @@ -888,6 +905,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>         struct hid_device *hdev = ps_dev->hdev;
>         struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
>         struct dualsense_input_report *ds_report;
> +       struct input_dev *touchpad = NULL;
>         uint8_t battery_data, battery_capacity, charging_status, value;
>         int battery_status;
>         uint32_t sensor_timestamp;
> @@ -1002,24 +1020,29 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>         input_event(ds->sensors, EV_MSC, MSC_TIMESTAMP, ds->sensor_timestamp_us);
>         input_sync(ds->sensors);
>
> -       for (i = 0; i < ARRAY_SIZE(ds_report->points); i++) {
> -               struct dualsense_touch_point *point = &ds_report->points[i];
> -               bool active = (point->contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
> +       rcu_read_lock();
> +       touchpad = rcu_dereference(ds->touchpad);
> +       rcu_read_unlock();
> +       if (touchpad) {

Access to touchpad should probably be guarded under RCU (that's what I
understand when I read
https://www.kernel.org/doc/html/latest/RCU/whatisRCU.html#rcu-dereference)

> +               for (i = 0; i < ARRAY_SIZE(ds_report->points); i++) {
> +                       struct dualsense_touch_point *point = &ds_report->points[i];
> +                       bool active = (point->contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
>
> -               input_mt_slot(ds->touchpad, i);
> -               input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
> +                       input_mt_slot(ds->touchpad, i);
> +                       input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
>
> -               if (active) {
> -                       int x = (point->x_hi << 8) | point->x_lo;
> -                       int y = (point->y_hi << 4) | point->y_lo;
> +                       if (active) {
> +                               int x = (point->x_hi << 8) | point->x_lo;
> +                               int y = (point->y_hi << 4) | point->y_lo;
>
> -                       input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
> -                       input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
> +                               input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
> +                               input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
> +                       }
>                 }
> +               input_mt_sync_frame(ds->touchpad);
> +               input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
> +               input_sync(ds->touchpad);
>         }
> -       input_mt_sync_frame(ds->touchpad);
> -       input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
> -       input_sync(ds->touchpad);
>
>         battery_data = ds_report->status & DS_STATUS_BATTERY_CAPACITY;
>         charging_status = (ds_report->status & DS_STATUS_CHARGING) >> DS_STATUS_CHARGING_SHIFT;
> @@ -1141,6 +1164,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>  {
>         struct dualsense *ds;
>         struct ps_device *ps_dev;
> +       struct input_dev *touchpad;
>         uint8_t max_output_report_size;
>         int ret;
>
> @@ -1157,6 +1181,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>         ps_dev = &ds->base;
>         ps_dev->hdev = hdev;
>         spin_lock_init(&ps_dev->lock);
> +       mutex_init(&ps_dev->mutex);

This mutex is never used.

Cheers,
Benjamin

>         ps_dev->battery_capacity = 100; /* initial value until parse_report. */
>         ps_dev->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
>         ps_dev->parse_report = dualsense_parse_report;
> @@ -1204,11 +1229,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>                 goto err;
>         }
>
> -       ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> -       if (IS_ERR(ds->touchpad)) {
> -               ret = PTR_ERR(ds->touchpad);
> +       touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> +       if (IS_ERR(touchpad)) {
> +               ret = PTR_ERR(touchpad);
>                 goto err;
>         }
> +       rcu_assign_pointer(ds->touchpad, touchpad);
>
>         ret = ps_device_register_battery(ps_dev);
>         if (ret)
> --
> 2.36.0
>