diff mbox series

[3/6] HID: hid-playstation: Disable touchpad reporting when hidraw open

Message ID 20220427224526.35657-3-vi@endrift.com
State New
Headers show
Series [1/6] HID: hid-playstation: Allow removal of touchpad | expand

Commit Message

Vicki Pfau April 27, 2022, 10:45 p.m. UTC
When using the hidraw node directly, disable the touchpad endpoint to prevent
it from sending separate mouse-like reports. This is accomplished in the same
way that the hid-steam driver does it, by creating and attaching an input_dev
with a custom low-level transport driver, which monitors and reports when the
hidraw node is opened or closed. Reports sent by the real device are reported
to the "fake" device, and the real device is prevented from creating a hidraw
node. This "fake" device is connected with only a hidraw node, and is exposed
with identifying information that is identical to the original device, so the
"fake" device's hidraw node appears as the node associated with the dev.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
 1 file changed, 141 insertions(+), 3 deletions(-)

Comments

Benjamin Tissoires May 5, 2022, 8:57 a.m. UTC | #1
On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>
> When using the hidraw node directly, disable the touchpad endpoint to prevent
> it from sending separate mouse-like reports. This is accomplished in the same
> way that the hid-steam driver does it, by creating and attaching an input_dev
> with a custom low-level transport driver, which monitors and reports when the
> hidraw node is opened or closed. Reports sent by the real device are reported
> to the "fake" device, and the real device is prevented from creating a hidraw
> node. This "fake" device is connected with only a hidraw node, and is exposed
> with identifying information that is identical to the original device, so the
> "fake" device's hidraw node appears as the node associated with the dev.

Besides the "we should have a generic way of doing this", why do we
only prevent touchpad events from being reported?

[looking at the code further down]

So it seems you are entirely disabling the input nodes, which is what
I would have expected, so the commit description needs some changes.
Or am I reading this wrong?

Cheers,
Benjamin

>
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
>  drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
>  1 file changed, 141 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index ad0da4470615..3746c9c550d6 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -30,9 +30,10 @@ static bool touchpad_mouse = true;
>  /* Base class for playstation devices. */
>  struct ps_device {
>         struct list_head list;
> -       struct hid_device *hdev;
> +       struct hid_device *hdev, *client_hdev;
>         struct mutex mutex;
>         spinlock_t lock;
> +       bool client_opened;
>
>         uint32_t player_id;
>
> @@ -643,6 +644,102 @@ static const struct attribute_group ps_device_attribute_group = {
>         .attrs = ps_device_attributes,
>  };
>
> +static int ps_client_ll_parse(struct hid_device *hdev)
> +{
> +       struct ps_device *dev = hdev->driver_data;
> +
> +       return hid_parse_report(hdev, dev->hdev->dev_rdesc,
> +                       dev->hdev->dev_rsize);
> +}
> +
> +static int ps_client_ll_start(struct hid_device *hdev)
> +{
> +       return 0;
> +}
> +
> +static void ps_client_ll_stop(struct hid_device *hdev)
> +{
> +}
> +
> +static int ps_client_ll_open(struct hid_device *hdev)
> +{
> +       struct ps_device *dev = hdev->driver_data;
> +       struct dualsense *ds;
> +
> +       mutex_lock(&dev->mutex);
> +       dev->client_opened = true;
> +       mutex_unlock(&dev->mutex);
> +
> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> +               ds = container_of(dev, struct dualsense, base);
> +               dualsense_unregister_touchpad(ds);
> +       }
> +
> +       return 0;
> +}
> +
> +static void ps_client_ll_close(struct hid_device *hdev)
> +{
> +       struct ps_device *dev = hdev->driver_data;
> +       struct dualsense *ds;
> +       struct input_dev *touchpad;
> +
> +       mutex_lock(&dev->mutex);
> +       dev->client_opened = false;
> +       mutex_unlock(&dev->mutex);
> +
> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> +               ds = container_of(dev, struct dualsense, base);
> +               touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> +               if (IS_ERR(touchpad))
> +                       return;
> +               rcu_assign_pointer(ds->touchpad, touchpad);
> +       }
> +}
> +
> +static int ps_client_ll_raw_request(struct hid_device *hdev,
> +                               unsigned char reportnum, u8 *buf,
> +                               size_t count, unsigned char report_type,
> +                               int reqtype)
> +{
> +       struct ps_device *dev = hdev->driver_data;
> +
> +       return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
> +                       report_type, reqtype);
> +}
> +
> +static struct hid_ll_driver ps_client_ll_driver = {
> +       .parse = ps_client_ll_parse,
> +       .start = ps_client_ll_start,
> +       .stop = ps_client_ll_stop,
> +       .open = ps_client_ll_open,
> +       .close = ps_client_ll_close,
> +       .raw_request = ps_client_ll_raw_request,
> +};
> +
> +static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
> +{
> +       struct hid_device *client_hdev;
> +
> +       client_hdev = hid_allocate_device();
> +       if (IS_ERR(client_hdev))
> +               return client_hdev;
> +
> +       client_hdev->ll_driver = &ps_client_ll_driver;
> +       client_hdev->dev.parent = hdev->dev.parent;
> +       client_hdev->bus = hdev->bus;
> +       client_hdev->vendor = hdev->vendor;
> +       client_hdev->product = hdev->product;
> +       client_hdev->version = hdev->version;
> +       client_hdev->type = hdev->type;
> +       client_hdev->country = hdev->country;
> +       strlcpy(client_hdev->name, hdev->name,
> +                       sizeof(client_hdev->name));
> +       strlcpy(client_hdev->phys, hdev->phys,
> +                       sizeof(client_hdev->phys));
> +       return client_hdev;
> +}
> +
>  static int dualsense_get_calibration_data(struct dualsense *ds)
>  {
>         short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> @@ -1190,6 +1287,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>         INIT_WORK(&ds->output_worker, dualsense_output_worker);
>         hid_set_drvdata(hdev, ds);
>
> +       ps_dev->client_hdev = ps_create_client_hid(hdev);
> +       if (IS_ERR(ps_dev->client_hdev))
> +               return ERR_CAST(ps_dev->client_hdev);
> +       ps_dev->client_hdev->driver_data = ps_dev;
> +
>         max_output_report_size = sizeof(struct dualsense_output_report_bt);
>         ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
>         if (!ds->output_report_dmabuf)
> @@ -1280,8 +1382,20 @@ static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
>                 u8 *data, int size)
>  {
>         struct ps_device *dev = hid_get_drvdata(hdev);
> +       int ret = 0;
> +
> +       if (!dev)
> +               return 0;
>
> -       if (dev && dev->parse_report)
> +       if (dev->client_opened) {
> +               ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
> +               if (ret) {
> +                       hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       if (dev->parse_report)
>                 return dev->parse_report(dev, report, data, size);
>
>         return 0;
> @@ -1291,6 +1405,7 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>         struct ps_device *dev;
>         int ret;
> +       unsigned int connect_mask = 0;
>
>         ret = hid_parse(hdev);
>         if (ret) {
> @@ -1298,12 +1413,22 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 return ret;
>         }
>
> -       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +       if (hdev->ll_driver == &ps_client_ll_driver)
> +               connect_mask = HID_CONNECT_HIDRAW;
> +
> +       ret = hid_hw_start(hdev, connect_mask);
>         if (ret) {
>                 hid_err(hdev, "Failed to start HID device\n");
>                 return ret;
>         }
>
> +       /*
> +        * The virtual client_dev is only used for hidraw. Since we've already
> +        * started the hw, return early to avoid the recursive probe.
> +        */
> +       if (hdev->ll_driver == &ps_client_ll_driver)
> +               return ret;
> +
>         ret = hid_hw_open(hdev);
>         if (ret) {
>                 hid_err(hdev, "Failed to open HID device\n");
> @@ -1325,9 +1450,19 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 goto err_close;
>         }
>
> +       if (dev->client_hdev)
> +               ret = hid_add_device(dev->client_hdev);
> +       if (ret) {
> +               hid_err(hdev, "Failed to start client device failed\n");
> +               goto err_close;
> +       }
> +
>         return ret;
>
>  err_close:
> +       if (dev->client_hdev)
> +               hid_destroy_device(dev->client_hdev);
> +
>         hid_hw_close(hdev);
>  err_stop:
>         hid_hw_stop(hdev);
> @@ -1341,6 +1476,9 @@ static void ps_remove(struct hid_device *hdev)
>         ps_devices_list_remove(dev);
>         ps_device_release_player_id(dev);
>
> +       if (dev->client_hdev)
> +               hid_destroy_device(dev->client_hdev);
> +
>         hid_hw_close(hdev);
>         hid_hw_stop(hdev);
>  }
> --
> 2.36.0
>
Vicki Pfau May 5, 2022, 10:03 p.m. UTC | #2
Hello,

On 5/5/22 01:57, Benjamin Tissoires wrote:
> On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
>>
>> When using the hidraw node directly, disable the touchpad endpoint to prevent
>> it from sending separate mouse-like reports. This is accomplished in the same
>> way that the hid-steam driver does it, by creating and attaching an input_dev
>> with a custom low-level transport driver, which monitors and reports when the
>> hidraw node is opened or closed. Reports sent by the real device are reported
>> to the "fake" device, and the real device is prevented from creating a hidraw
>> node. This "fake" device is connected with only a hidraw node, and is exposed
>> with identifying information that is identical to the original device, so the
>> "fake" device's hidraw node appears as the node associated with the dev.
> 
> Besides the "we should have a generic way of doing this", why do we
> only prevent touchpad events from being reported?

As, from what I can tell, most windowing systems treat it as a mouse, it's the most prone to "double input" problems when prodded by both the hidraw and the windowing system. Windowing systems generally don't do anything with the other exposed inputs, as far as I can tell.

> 
> [looking at the code further down]
> 
> So it seems you are entirely disabling the input nodes, which is what
> I would have expected, so the commit description needs some changes.
> Or am I reading this wrong?

I am unsure where this discrepancy lies in the description. Can you be more specific?

Thanks,
Vicki

> 
> Cheers,
> Benjamin

>>
>> Signed-off-by: Vicki Pfau <vi@endrift.com>
>> ---
>>  drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 141 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
>> index ad0da4470615..3746c9c550d6 100644
>> --- a/drivers/hid/hid-playstation.c
>> +++ b/drivers/hid/hid-playstation.c
>> @@ -30,9 +30,10 @@ static bool touchpad_mouse = true;
>>  /* Base class for playstation devices. */
>>  struct ps_device {
>>         struct list_head list;
>> -       struct hid_device *hdev;
>> +       struct hid_device *hdev, *client_hdev;
>>         struct mutex mutex;
>>         spinlock_t lock;
>> +       bool client_opened;
>>
>>         uint32_t player_id;
>>
>> @@ -643,6 +644,102 @@ static const struct attribute_group ps_device_attribute_group = {
>>         .attrs = ps_device_attributes,
>>  };
>>
>> +static int ps_client_ll_parse(struct hid_device *hdev)
>> +{
>> +       struct ps_device *dev = hdev->driver_data;
>> +
>> +       return hid_parse_report(hdev, dev->hdev->dev_rdesc,
>> +                       dev->hdev->dev_rsize);
>> +}
>> +
>> +static int ps_client_ll_start(struct hid_device *hdev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static void ps_client_ll_stop(struct hid_device *hdev)
>> +{
>> +}
>> +
>> +static int ps_client_ll_open(struct hid_device *hdev)
>> +{
>> +       struct ps_device *dev = hdev->driver_data;
>> +       struct dualsense *ds;
>> +
>> +       mutex_lock(&dev->mutex);
>> +       dev->client_opened = true;
>> +       mutex_unlock(&dev->mutex);
>> +
>> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
>> +               ds = container_of(dev, struct dualsense, base);
>> +               dualsense_unregister_touchpad(ds);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void ps_client_ll_close(struct hid_device *hdev)
>> +{
>> +       struct ps_device *dev = hdev->driver_data;
>> +       struct dualsense *ds;
>> +       struct input_dev *touchpad;
>> +
>> +       mutex_lock(&dev->mutex);
>> +       dev->client_opened = false;
>> +       mutex_unlock(&dev->mutex);
>> +
>> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
>> +               ds = container_of(dev, struct dualsense, base);
>> +               touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
>> +               if (IS_ERR(touchpad))
>> +                       return;
>> +               rcu_assign_pointer(ds->touchpad, touchpad);
>> +       }
>> +}
>> +
>> +static int ps_client_ll_raw_request(struct hid_device *hdev,
>> +                               unsigned char reportnum, u8 *buf,
>> +                               size_t count, unsigned char report_type,
>> +                               int reqtype)
>> +{
>> +       struct ps_device *dev = hdev->driver_data;
>> +
>> +       return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
>> +                       report_type, reqtype);
>> +}
>> +
>> +static struct hid_ll_driver ps_client_ll_driver = {
>> +       .parse = ps_client_ll_parse,
>> +       .start = ps_client_ll_start,
>> +       .stop = ps_client_ll_stop,
>> +       .open = ps_client_ll_open,
>> +       .close = ps_client_ll_close,
>> +       .raw_request = ps_client_ll_raw_request,
>> +};
>> +
>> +static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
>> +{
>> +       struct hid_device *client_hdev;
>> +
>> +       client_hdev = hid_allocate_device();
>> +       if (IS_ERR(client_hdev))
>> +               return client_hdev;
>> +
>> +       client_hdev->ll_driver = &ps_client_ll_driver;
>> +       client_hdev->dev.parent = hdev->dev.parent;
>> +       client_hdev->bus = hdev->bus;
>> +       client_hdev->vendor = hdev->vendor;
>> +       client_hdev->product = hdev->product;
>> +       client_hdev->version = hdev->version;
>> +       client_hdev->type = hdev->type;
>> +       client_hdev->country = hdev->country;
>> +       strlcpy(client_hdev->name, hdev->name,
>> +                       sizeof(client_hdev->name));
>> +       strlcpy(client_hdev->phys, hdev->phys,
>> +                       sizeof(client_hdev->phys));
>> +       return client_hdev;
>> +}
>> +
>>  static int dualsense_get_calibration_data(struct dualsense *ds)
>>  {
>>         short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
>> @@ -1190,6 +1287,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>>         INIT_WORK(&ds->output_worker, dualsense_output_worker);
>>         hid_set_drvdata(hdev, ds);
>>
>> +       ps_dev->client_hdev = ps_create_client_hid(hdev);
>> +       if (IS_ERR(ps_dev->client_hdev))
>> +               return ERR_CAST(ps_dev->client_hdev);
>> +       ps_dev->client_hdev->driver_data = ps_dev;
>> +
>>         max_output_report_size = sizeof(struct dualsense_output_report_bt);
>>         ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
>>         if (!ds->output_report_dmabuf)
>> @@ -1280,8 +1382,20 @@ static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
>>                 u8 *data, int size)
>>  {
>>         struct ps_device *dev = hid_get_drvdata(hdev);
>> +       int ret = 0;
>> +
>> +       if (!dev)
>> +               return 0;
>>
>> -       if (dev && dev->parse_report)
>> +       if (dev->client_opened) {
>> +               ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
>> +               if (ret) {
>> +                       hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       if (dev->parse_report)
>>                 return dev->parse_report(dev, report, data, size);
>>
>>         return 0;
>> @@ -1291,6 +1405,7 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  {
>>         struct ps_device *dev;
>>         int ret;
>> +       unsigned int connect_mask = 0;
>>
>>         ret = hid_parse(hdev);
>>         if (ret) {
>> @@ -1298,12 +1413,22 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>                 return ret;
>>         }
>>
>> -       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>> +       if (hdev->ll_driver == &ps_client_ll_driver)
>> +               connect_mask = HID_CONNECT_HIDRAW;
>> +
>> +       ret = hid_hw_start(hdev, connect_mask);
>>         if (ret) {
>>                 hid_err(hdev, "Failed to start HID device\n");
>>                 return ret;
>>         }
>>
>> +       /*
>> +        * The virtual client_dev is only used for hidraw. Since we've already
>> +        * started the hw, return early to avoid the recursive probe.
>> +        */
>> +       if (hdev->ll_driver == &ps_client_ll_driver)
>> +               return ret;
>> +
>>         ret = hid_hw_open(hdev);
>>         if (ret) {
>>                 hid_err(hdev, "Failed to open HID device\n");
>> @@ -1325,9 +1450,19 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>                 goto err_close;
>>         }
>>
>> +       if (dev->client_hdev)
>> +               ret = hid_add_device(dev->client_hdev);
>> +       if (ret) {
>> +               hid_err(hdev, "Failed to start client device failed\n");
>> +               goto err_close;
>> +       }
>> +
>>         return ret;
>>
>>  err_close:
>> +       if (dev->client_hdev)
>> +               hid_destroy_device(dev->client_hdev);
>> +
>>         hid_hw_close(hdev);
>>  err_stop:
>>         hid_hw_stop(hdev);
>> @@ -1341,6 +1476,9 @@ static void ps_remove(struct hid_device *hdev)
>>         ps_devices_list_remove(dev);
>>         ps_device_release_player_id(dev);
>>
>> +       if (dev->client_hdev)
>> +               hid_destroy_device(dev->client_hdev);
>> +
>>         hid_hw_close(hdev);
>>         hid_hw_stop(hdev);
>>  }
>> --
>> 2.36.0
>>
>
Roderick Colenbrander May 6, 2022, 6 a.m. UTC | #3
On Thu, May 5, 2022 at 3:15 PM Vicki Pfau <vi@endrift.com> wrote:
>
> Hello,
>
> On 5/5/22 01:57, Benjamin Tissoires wrote:
> > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> >>
> >> When using the hidraw node directly, disable the touchpad endpoint to prevent
> >> it from sending separate mouse-like reports. This is accomplished in the same
> >> way that the hid-steam driver does it, by creating and attaching an input_dev
> >> with a custom low-level transport driver, which monitors and reports when the
> >> hidraw node is opened or closed. Reports sent by the real device are reported
> >> to the "fake" device, and the real device is prevented from creating a hidraw
> >> node. This "fake" device is connected with only a hidraw node, and is exposed
> >> with identifying information that is identical to the original device, so the
> >> "fake" device's hidraw node appears as the node associated with the dev.
> >
> > Besides the "we should have a generic way of doing this", why do we
> > only prevent touchpad events from being reported?
>
> As, from what I can tell, most windowing systems treat it as a mouse, it's the most prone to "double input" problems when prodded by both the hidraw and the windowing system. Windowing systems generally don't do anything with the other exposed inputs, as far as I can tell.
>

I agree with Benjamin, if we did some disabling it needs to be for everything.

I'm not convinced though that kernel side we can make the best
decision for each use case. Maybe it does make sense to hide for
example input devices when hidraw is open, which helps some use cases.
Other use cases may want to have both around.

I really like the 'inhibit' idea from Dmitry in the other email. It
would solve this use case as well can handle others.

> >
> > [looking at the code further down]
> >
> > So it seems you are entirely disabling the input nodes, which is what
> > I would have expected, so the commit description needs some changes.
> > Or am I reading this wrong?
>
> I am unsure where this discrepancy lies in the description. Can you be more specific?
>
> Thanks,
> Vicki
>
> >
> > Cheers,
> > Benjamin

Thanks,
Roderick
>
> >>
> >> Signed-off-by: Vicki Pfau <vi@endrift.com>
> >> ---
> >>  drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
> >>  1 file changed, 141 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> >> index ad0da4470615..3746c9c550d6 100644
> >> --- a/drivers/hid/hid-playstation.c
> >> +++ b/drivers/hid/hid-playstation.c
> >> @@ -30,9 +30,10 @@ static bool touchpad_mouse = true;
> >>  /* Base class for playstation devices. */
> >>  struct ps_device {
> >>         struct list_head list;
> >> -       struct hid_device *hdev;
> >> +       struct hid_device *hdev, *client_hdev;
> >>         struct mutex mutex;
> >>         spinlock_t lock;
> >> +       bool client_opened;
> >>
> >>         uint32_t player_id;
> >>
> >> @@ -643,6 +644,102 @@ static const struct attribute_group ps_device_attribute_group = {
> >>         .attrs = ps_device_attributes,
> >>  };
> >>
> >> +static int ps_client_ll_parse(struct hid_device *hdev)
> >> +{
> >> +       struct ps_device *dev = hdev->driver_data;
> >> +
> >> +       return hid_parse_report(hdev, dev->hdev->dev_rdesc,
> >> +                       dev->hdev->dev_rsize);
> >> +}
> >> +
> >> +static int ps_client_ll_start(struct hid_device *hdev)
> >> +{
> >> +       return 0;
> >> +}
> >> +
> >> +static void ps_client_ll_stop(struct hid_device *hdev)
> >> +{
> >> +}
> >> +
> >> +static int ps_client_ll_open(struct hid_device *hdev)
> >> +{
> >> +       struct ps_device *dev = hdev->driver_data;
> >> +       struct dualsense *ds;
> >> +
> >> +       mutex_lock(&dev->mutex);
> >> +       dev->client_opened = true;
> >> +       mutex_unlock(&dev->mutex);
> >> +
> >> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> >> +               ds = container_of(dev, struct dualsense, base);
> >> +               dualsense_unregister_touchpad(ds);
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static void ps_client_ll_close(struct hid_device *hdev)
> >> +{
> >> +       struct ps_device *dev = hdev->driver_data;
> >> +       struct dualsense *ds;
> >> +       struct input_dev *touchpad;
> >> +
> >> +       mutex_lock(&dev->mutex);
> >> +       dev->client_opened = false;
> >> +       mutex_unlock(&dev->mutex);
> >> +
> >> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> >> +               ds = container_of(dev, struct dualsense, base);
> >> +               touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> >> +               if (IS_ERR(touchpad))
> >> +                       return;
> >> +               rcu_assign_pointer(ds->touchpad, touchpad);
> >> +       }
> >> +}
> >> +
> >> +static int ps_client_ll_raw_request(struct hid_device *hdev,
> >> +                               unsigned char reportnum, u8 *buf,
> >> +                               size_t count, unsigned char report_type,
> >> +                               int reqtype)
> >> +{
> >> +       struct ps_device *dev = hdev->driver_data;
> >> +
> >> +       return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
> >> +                       report_type, reqtype);
> >> +}
> >> +
> >> +static struct hid_ll_driver ps_client_ll_driver = {
> >> +       .parse = ps_client_ll_parse,
> >> +       .start = ps_client_ll_start,
> >> +       .stop = ps_client_ll_stop,
> >> +       .open = ps_client_ll_open,
> >> +       .close = ps_client_ll_close,
> >> +       .raw_request = ps_client_ll_raw_request,
> >> +};
> >> +
> >> +static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
> >> +{
> >> +       struct hid_device *client_hdev;
> >> +
> >> +       client_hdev = hid_allocate_device();
> >> +       if (IS_ERR(client_hdev))
> >> +               return client_hdev;
> >> +
> >> +       client_hdev->ll_driver = &ps_client_ll_driver;
> >> +       client_hdev->dev.parent = hdev->dev.parent;
> >> +       client_hdev->bus = hdev->bus;
> >> +       client_hdev->vendor = hdev->vendor;
> >> +       client_hdev->product = hdev->product;
> >> +       client_hdev->version = hdev->version;
> >> +       client_hdev->type = hdev->type;
> >> +       client_hdev->country = hdev->country;
> >> +       strlcpy(client_hdev->name, hdev->name,
> >> +                       sizeof(client_hdev->name));
> >> +       strlcpy(client_hdev->phys, hdev->phys,
> >> +                       sizeof(client_hdev->phys));
> >> +       return client_hdev;
> >> +}
> >> +
> >>  static int dualsense_get_calibration_data(struct dualsense *ds)
> >>  {
> >>         short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> >> @@ -1190,6 +1287,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
> >>         INIT_WORK(&ds->output_worker, dualsense_output_worker);
> >>         hid_set_drvdata(hdev, ds);
> >>
> >> +       ps_dev->client_hdev = ps_create_client_hid(hdev);
> >> +       if (IS_ERR(ps_dev->client_hdev))
> >> +               return ERR_CAST(ps_dev->client_hdev);
> >> +       ps_dev->client_hdev->driver_data = ps_dev;
> >> +
> >>         max_output_report_size = sizeof(struct dualsense_output_report_bt);
> >>         ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
> >>         if (!ds->output_report_dmabuf)
> >> @@ -1280,8 +1382,20 @@ static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
> >>                 u8 *data, int size)
> >>  {
> >>         struct ps_device *dev = hid_get_drvdata(hdev);
> >> +       int ret = 0;
> >> +
> >> +       if (!dev)
> >> +               return 0;
> >>
> >> -       if (dev && dev->parse_report)
> >> +       if (dev->client_opened) {
> >> +               ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
> >> +               if (ret) {
> >> +                       hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
> >> +                       return ret;
> >> +               }
> >> +       }
> >> +
> >> +       if (dev->parse_report)
> >>                 return dev->parse_report(dev, report, data, size);
> >>
> >>         return 0;
> >> @@ -1291,6 +1405,7 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>  {
> >>         struct ps_device *dev;
> >>         int ret;
> >> +       unsigned int connect_mask = 0;
> >>
> >>         ret = hid_parse(hdev);
> >>         if (ret) {
> >> @@ -1298,12 +1413,22 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>                 return ret;
> >>         }
> >>
> >> -       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> >> +       if (hdev->ll_driver == &ps_client_ll_driver)
> >> +               connect_mask = HID_CONNECT_HIDRAW;
> >> +
> >> +       ret = hid_hw_start(hdev, connect_mask);
> >>         if (ret) {
> >>                 hid_err(hdev, "Failed to start HID device\n");
> >>                 return ret;
> >>         }
> >>
> >> +       /*
> >> +        * The virtual client_dev is only used for hidraw. Since we've already
> >> +        * started the hw, return early to avoid the recursive probe.
> >> +        */
> >> +       if (hdev->ll_driver == &ps_client_ll_driver)
> >> +               return ret;
> >> +
> >>         ret = hid_hw_open(hdev);
> >>         if (ret) {
> >>                 hid_err(hdev, "Failed to open HID device\n");
> >> @@ -1325,9 +1450,19 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>                 goto err_close;
> >>         }
> >>
> >> +       if (dev->client_hdev)
> >> +               ret = hid_add_device(dev->client_hdev);
> >> +       if (ret) {
> >> +               hid_err(hdev, "Failed to start client device failed\n");
> >> +               goto err_close;
> >> +       }
> >> +
> >>         return ret;
> >>
> >>  err_close:
> >> +       if (dev->client_hdev)
> >> +               hid_destroy_device(dev->client_hdev);
> >> +
> >>         hid_hw_close(hdev);
> >>  err_stop:
> >>         hid_hw_stop(hdev);
> >> @@ -1341,6 +1476,9 @@ static void ps_remove(struct hid_device *hdev)
> >>         ps_devices_list_remove(dev);
> >>         ps_device_release_player_id(dev);
> >>
> >> +       if (dev->client_hdev)
> >> +               hid_destroy_device(dev->client_hdev);
> >> +
> >>         hid_hw_close(hdev);
> >>         hid_hw_stop(hdev);
> >>  }
> >> --
> >> 2.36.0
> >>
> >
Benjamin Tissoires May 6, 2022, 6:54 a.m. UTC | #4
On Fri, May 6, 2022 at 8:00 AM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> On Thu, May 5, 2022 at 3:15 PM Vicki Pfau <vi@endrift.com> wrote:
> >
> > Hello,
> >
> > On 5/5/22 01:57, Benjamin Tissoires wrote:
> > > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> > >>
> > >> When using the hidraw node directly, disable the touchpad endpoint to prevent
> > >> it from sending separate mouse-like reports. This is accomplished in the same
> > >> way that the hid-steam driver does it, by creating and attaching an input_dev
> > >> with a custom low-level transport driver, which monitors and reports when the
> > >> hidraw node is opened or closed. Reports sent by the real device are reported
> > >> to the "fake" device, and the real device is prevented from creating a hidraw
> > >> node. This "fake" device is connected with only a hidraw node, and is exposed
> > >> with identifying information that is identical to the original device, so the
> > >> "fake" device's hidraw node appears as the node associated with the dev.
> > >
> > > Besides the "we should have a generic way of doing this", why do we
> > > only prevent touchpad events from being reported?
> >
> > As, from what I can tell, most windowing systems treat it as a mouse, it's the most prone to "double input" problems when prodded by both the hidraw and the windowing system. Windowing systems generally don't do anything with the other exposed inputs, as far as I can tell.
> >
>
> I agree with Benjamin, if we did some disabling it needs to be for everything.
>
> I'm not convinced though that kernel side we can make the best
> decision for each use case. Maybe it does make sense to hide for
> example input devices when hidraw is open, which helps some use cases.
> Other use cases may want to have both around.

And with that said... How about eBPF to do that instead of sysfs???? :)
(I know I have a nice hammer so I see nails everywhere).

>
> I really like the 'inhibit' idea from Dmitry in the other email. It
> would solve this use case as well can handle others.

The only problem of the sysfs approach is how do we give steam/SDL the
access to this sysfs file.
Steam for example ships an ugly udev rule that gives uaccess to all
hidraw nodes on gamepads, which is worrying for a security reason.

There are a couple of systemd PR trying to solve that
(https://github.com/systemd/systemd/pull/23140 for an ioctl approach),
but we realized with Bastien and Peter that BPF was the best way
forward because we can have a central piece that makes such decisions
without changing current applications.

In this particular case, we could also use the sysfs to inhibit the
device by a daemon that would:
- monitor each hidraw_open/hidraw_close
- when a hidraw_openm comes in, it notifies userspace
- user space then calls inhibit in the sysfs

But we could also put it one step forward by adding a hook that makes
all of this logic directly in ebpf (see
https://gitlab.freedesktop.org/bentiss/logind-hidraw for such an
approach for the hidraw ioctl-like). And given that we are working
towards adding this eBPF capability to logind, we could also implement
that logic in logind directly (based on some user-defined or
session-defined rules).

Cheers,
Benjamin

>
> > >
> > > [looking at the code further down]
> > >
> > > So it seems you are entirely disabling the input nodes, which is what
> > > I would have expected, so the commit description needs some changes.
> > > Or am I reading this wrong?
> >
> > I am unsure where this discrepancy lies in the description. Can you be more specific?
> >
> > Thanks,
> > Vicki
> >
> > >
> > > Cheers,
> > > Benjamin
>
> Thanks,
> Roderick
> >
> > >>
> > >> Signed-off-by: Vicki Pfau <vi@endrift.com>
> > >> ---
> > >>  drivers/hid/hid-playstation.c | 144 +++++++++++++++++++++++++++++++++-
> > >>  1 file changed, 141 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > >> index ad0da4470615..3746c9c550d6 100644
> > >> --- a/drivers/hid/hid-playstation.c
> > >> +++ b/drivers/hid/hid-playstation.c
> > >> @@ -30,9 +30,10 @@ static bool touchpad_mouse = true;
> > >>  /* Base class for playstation devices. */
> > >>  struct ps_device {
> > >>         struct list_head list;
> > >> -       struct hid_device *hdev;
> > >> +       struct hid_device *hdev, *client_hdev;
> > >>         struct mutex mutex;
> > >>         spinlock_t lock;
> > >> +       bool client_opened;
> > >>
> > >>         uint32_t player_id;
> > >>
> > >> @@ -643,6 +644,102 @@ static const struct attribute_group ps_device_attribute_group = {
> > >>         .attrs = ps_device_attributes,
> > >>  };
> > >>
> > >> +static int ps_client_ll_parse(struct hid_device *hdev)
> > >> +{
> > >> +       struct ps_device *dev = hdev->driver_data;
> > >> +
> > >> +       return hid_parse_report(hdev, dev->hdev->dev_rdesc,
> > >> +                       dev->hdev->dev_rsize);
> > >> +}
> > >> +
> > >> +static int ps_client_ll_start(struct hid_device *hdev)
> > >> +{
> > >> +       return 0;
> > >> +}
> > >> +
> > >> +static void ps_client_ll_stop(struct hid_device *hdev)
> > >> +{
> > >> +}
> > >> +
> > >> +static int ps_client_ll_open(struct hid_device *hdev)
> > >> +{
> > >> +       struct ps_device *dev = hdev->driver_data;
> > >> +       struct dualsense *ds;
> > >> +
> > >> +       mutex_lock(&dev->mutex);
> > >> +       dev->client_opened = true;
> > >> +       mutex_unlock(&dev->mutex);
> > >> +
> > >> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> > >> +               ds = container_of(dev, struct dualsense, base);
> > >> +               dualsense_unregister_touchpad(ds);
> > >> +       }
> > >> +
> > >> +       return 0;
> > >> +}
> > >> +
> > >> +static void ps_client_ll_close(struct hid_device *hdev)
> > >> +{
> > >> +       struct ps_device *dev = hdev->driver_data;
> > >> +       struct dualsense *ds;
> > >> +       struct input_dev *touchpad;
> > >> +
> > >> +       mutex_lock(&dev->mutex);
> > >> +       dev->client_opened = false;
> > >> +       mutex_unlock(&dev->mutex);
> > >> +
> > >> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> > >> +               ds = container_of(dev, struct dualsense, base);
> > >> +               touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> > >> +               if (IS_ERR(touchpad))
> > >> +                       return;
> > >> +               rcu_assign_pointer(ds->touchpad, touchpad);
> > >> +       }
> > >> +}
> > >> +
> > >> +static int ps_client_ll_raw_request(struct hid_device *hdev,
> > >> +                               unsigned char reportnum, u8 *buf,
> > >> +                               size_t count, unsigned char report_type,
> > >> +                               int reqtype)
> > >> +{
> > >> +       struct ps_device *dev = hdev->driver_data;
> > >> +
> > >> +       return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
> > >> +                       report_type, reqtype);
> > >> +}
> > >> +
> > >> +static struct hid_ll_driver ps_client_ll_driver = {
> > >> +       .parse = ps_client_ll_parse,
> > >> +       .start = ps_client_ll_start,
> > >> +       .stop = ps_client_ll_stop,
> > >> +       .open = ps_client_ll_open,
> > >> +       .close = ps_client_ll_close,
> > >> +       .raw_request = ps_client_ll_raw_request,
> > >> +};
> > >> +
> > >> +static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
> > >> +{
> > >> +       struct hid_device *client_hdev;
> > >> +
> > >> +       client_hdev = hid_allocate_device();
> > >> +       if (IS_ERR(client_hdev))
> > >> +               return client_hdev;
> > >> +
> > >> +       client_hdev->ll_driver = &ps_client_ll_driver;
> > >> +       client_hdev->dev.parent = hdev->dev.parent;
> > >> +       client_hdev->bus = hdev->bus;
> > >> +       client_hdev->vendor = hdev->vendor;
> > >> +       client_hdev->product = hdev->product;
> > >> +       client_hdev->version = hdev->version;
> > >> +       client_hdev->type = hdev->type;
> > >> +       client_hdev->country = hdev->country;
> > >> +       strlcpy(client_hdev->name, hdev->name,
> > >> +                       sizeof(client_hdev->name));
> > >> +       strlcpy(client_hdev->phys, hdev->phys,
> > >> +                       sizeof(client_hdev->phys));
> > >> +       return client_hdev;
> > >> +}
> > >> +
> > >>  static int dualsense_get_calibration_data(struct dualsense *ds)
> > >>  {
> > >>         short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> > >> @@ -1190,6 +1287,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
> > >>         INIT_WORK(&ds->output_worker, dualsense_output_worker);
> > >>         hid_set_drvdata(hdev, ds);
> > >>
> > >> +       ps_dev->client_hdev = ps_create_client_hid(hdev);
> > >> +       if (IS_ERR(ps_dev->client_hdev))
> > >> +               return ERR_CAST(ps_dev->client_hdev);
> > >> +       ps_dev->client_hdev->driver_data = ps_dev;
> > >> +
> > >>         max_output_report_size = sizeof(struct dualsense_output_report_bt);
> > >>         ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
> > >>         if (!ds->output_report_dmabuf)
> > >> @@ -1280,8 +1382,20 @@ static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
> > >>                 u8 *data, int size)
> > >>  {
> > >>         struct ps_device *dev = hid_get_drvdata(hdev);
> > >> +       int ret = 0;
> > >> +
> > >> +       if (!dev)
> > >> +               return 0;
> > >>
> > >> -       if (dev && dev->parse_report)
> > >> +       if (dev->client_opened) {
> > >> +               ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
> > >> +               if (ret) {
> > >> +                       hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
> > >> +                       return ret;
> > >> +               }
> > >> +       }
> > >> +
> > >> +       if (dev->parse_report)
> > >>                 return dev->parse_report(dev, report, data, size);
> > >>
> > >>         return 0;
> > >> @@ -1291,6 +1405,7 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >>  {
> > >>         struct ps_device *dev;
> > >>         int ret;
> > >> +       unsigned int connect_mask = 0;
> > >>
> > >>         ret = hid_parse(hdev);
> > >>         if (ret) {
> > >> @@ -1298,12 +1413,22 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >>                 return ret;
> > >>         }
> > >>
> > >> -       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> > >> +       if (hdev->ll_driver == &ps_client_ll_driver)
> > >> +               connect_mask = HID_CONNECT_HIDRAW;
> > >> +
> > >> +       ret = hid_hw_start(hdev, connect_mask);
> > >>         if (ret) {
> > >>                 hid_err(hdev, "Failed to start HID device\n");
> > >>                 return ret;
> > >>         }
> > >>
> > >> +       /*
> > >> +        * The virtual client_dev is only used for hidraw. Since we've already
> > >> +        * started the hw, return early to avoid the recursive probe.
> > >> +        */
> > >> +       if (hdev->ll_driver == &ps_client_ll_driver)
> > >> +               return ret;
> > >> +
> > >>         ret = hid_hw_open(hdev);
> > >>         if (ret) {
> > >>                 hid_err(hdev, "Failed to open HID device\n");
> > >> @@ -1325,9 +1450,19 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >>                 goto err_close;
> > >>         }
> > >>
> > >> +       if (dev->client_hdev)
> > >> +               ret = hid_add_device(dev->client_hdev);
> > >> +       if (ret) {
> > >> +               hid_err(hdev, "Failed to start client device failed\n");
> > >> +               goto err_close;
> > >> +       }
> > >> +
> > >>         return ret;
> > >>
> > >>  err_close:
> > >> +       if (dev->client_hdev)
> > >> +               hid_destroy_device(dev->client_hdev);
> > >> +
> > >>         hid_hw_close(hdev);
> > >>  err_stop:
> > >>         hid_hw_stop(hdev);
> > >> @@ -1341,6 +1476,9 @@ static void ps_remove(struct hid_device *hdev)
> > >>         ps_devices_list_remove(dev);
> > >>         ps_device_release_player_id(dev);
> > >>
> > >> +       if (dev->client_hdev)
> > >> +               hid_destroy_device(dev->client_hdev);
> > >> +
> > >>         hid_hw_close(hdev);
> > >>         hid_hw_stop(hdev);
> > >>  }
> > >> --
> > >> 2.36.0
> > >>
> > >
>
Benjamin Tissoires May 6, 2022, 6:57 a.m. UTC | #5
On Fri, May 6, 2022 at 12:04 AM Vicki Pfau <vi@endrift.com> wrote:
>
> Hello,
>
> On 5/5/22 01:57, Benjamin Tissoires wrote:
> > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@endrift.com> wrote:
> >>
> >> When using the hidraw node directly, disable the touchpad endpoint to prevent
> >> it from sending separate mouse-like reports. This is accomplished in the same
> >> way that the hid-steam driver does it, by creating and attaching an input_dev
> >> with a custom low-level transport driver, which monitors and reports when the
> >> hidraw node is opened or closed. Reports sent by the real device are reported
> >> to the "fake" device, and the real device is prevented from creating a hidraw
> >> node. This "fake" device is connected with only a hidraw node, and is exposed
> >> with identifying information that is identical to the original device, so the
> >> "fake" device's hidraw node appears as the node associated with the dev.
> >
> > Besides the "we should have a generic way of doing this", why do we
> > only prevent touchpad events from being reported?
>
> As, from what I can tell, most windowing systems treat it as a mouse, it's the most prone to "double input" problems when prodded by both the hidraw and the windowing system. Windowing systems generally don't do anything with the other exposed inputs, as far as I can tell.
>
> >
> > [looking at the code further down]
> >
> > So it seems you are entirely disabling the input nodes, which is what
> > I would have expected, so the commit description needs some changes.
> > Or am I reading this wrong?
>
> I am unsure where this discrepancy lies in the description. Can you be more specific?
>

When I read the code, I was under the impression that when an hidraw
node was opened, it was disabling all other input nodes (joysticks,
buttons and touchpad).
But maybe this is just disabling the touchpad, which still leaves you
with duplicated events on the gamepad side.
So either we need to also disable those other events, or the code is
doing just that and the description is not correct :)

Cheersm
Benjamin
diff mbox series

Patch

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index ad0da4470615..3746c9c550d6 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -30,9 +30,10 @@  static bool touchpad_mouse = true;
 /* Base class for playstation devices. */
 struct ps_device {
 	struct list_head list;
-	struct hid_device *hdev;
+	struct hid_device *hdev, *client_hdev;
 	struct mutex mutex;
 	spinlock_t lock;
+	bool client_opened;
 
 	uint32_t player_id;
 
@@ -643,6 +644,102 @@  static const struct attribute_group ps_device_attribute_group = {
 	.attrs = ps_device_attributes,
 };
 
+static int ps_client_ll_parse(struct hid_device *hdev)
+{
+	struct ps_device *dev = hdev->driver_data;
+
+	return hid_parse_report(hdev, dev->hdev->dev_rdesc,
+			dev->hdev->dev_rsize);
+}
+
+static int ps_client_ll_start(struct hid_device *hdev)
+{
+	return 0;
+}
+
+static void ps_client_ll_stop(struct hid_device *hdev)
+{
+}
+
+static int ps_client_ll_open(struct hid_device *hdev)
+{
+	struct ps_device *dev = hdev->driver_data;
+	struct dualsense *ds;
+
+	mutex_lock(&dev->mutex);
+	dev->client_opened = true;
+	mutex_unlock(&dev->mutex);
+
+	if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
+		ds = container_of(dev, struct dualsense, base);
+		dualsense_unregister_touchpad(ds);
+	}
+
+	return 0;
+}
+
+static void ps_client_ll_close(struct hid_device *hdev)
+{
+	struct ps_device *dev = hdev->driver_data;
+	struct dualsense *ds;
+	struct input_dev *touchpad;
+
+	mutex_lock(&dev->mutex);
+	dev->client_opened = false;
+	mutex_unlock(&dev->mutex);
+
+	if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
+		ds = container_of(dev, struct dualsense, base);
+		touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
+		if (IS_ERR(touchpad))
+			return;
+		rcu_assign_pointer(ds->touchpad, touchpad);
+	}
+}
+
+static int ps_client_ll_raw_request(struct hid_device *hdev,
+				unsigned char reportnum, u8 *buf,
+				size_t count, unsigned char report_type,
+				int reqtype)
+{
+	struct ps_device *dev = hdev->driver_data;
+
+	return hid_hw_raw_request(dev->hdev, reportnum, buf, count,
+			report_type, reqtype);
+}
+
+static struct hid_ll_driver ps_client_ll_driver = {
+	.parse = ps_client_ll_parse,
+	.start = ps_client_ll_start,
+	.stop = ps_client_ll_stop,
+	.open = ps_client_ll_open,
+	.close = ps_client_ll_close,
+	.raw_request = ps_client_ll_raw_request,
+};
+
+static struct hid_device *ps_create_client_hid(struct hid_device *hdev)
+{
+	struct hid_device *client_hdev;
+
+	client_hdev = hid_allocate_device();
+	if (IS_ERR(client_hdev))
+		return client_hdev;
+
+	client_hdev->ll_driver = &ps_client_ll_driver;
+	client_hdev->dev.parent = hdev->dev.parent;
+	client_hdev->bus = hdev->bus;
+	client_hdev->vendor = hdev->vendor;
+	client_hdev->product = hdev->product;
+	client_hdev->version = hdev->version;
+	client_hdev->type = hdev->type;
+	client_hdev->country = hdev->country;
+	strlcpy(client_hdev->name, hdev->name,
+			sizeof(client_hdev->name));
+	strlcpy(client_hdev->phys, hdev->phys,
+			sizeof(client_hdev->phys));
+	return client_hdev;
+}
+
 static int dualsense_get_calibration_data(struct dualsense *ds)
 {
 	short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
@@ -1190,6 +1287,11 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
 	INIT_WORK(&ds->output_worker, dualsense_output_worker);
 	hid_set_drvdata(hdev, ds);
 
+	ps_dev->client_hdev = ps_create_client_hid(hdev);
+	if (IS_ERR(ps_dev->client_hdev))
+		return ERR_CAST(ps_dev->client_hdev);
+	ps_dev->client_hdev->driver_data = ps_dev;
+
 	max_output_report_size = sizeof(struct dualsense_output_report_bt);
 	ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
 	if (!ds->output_report_dmabuf)
@@ -1280,8 +1382,20 @@  static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *data, int size)
 {
 	struct ps_device *dev = hid_get_drvdata(hdev);
+	int ret = 0;
+
+	if (!dev)
+		return 0;
 
-	if (dev && dev->parse_report)
+	if (dev->client_opened) {
+		ret = hid_input_report(dev->client_hdev, HID_INPUT_REPORT, data, size, 0);
+		if (ret) {
+			hid_err(hdev, "can't send input report to client hdev: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (dev->parse_report)
 		return dev->parse_report(dev, report, data, size);
 
 	return 0;
@@ -1291,6 +1405,7 @@  static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct ps_device *dev;
 	int ret;
+	unsigned int connect_mask = 0;
 
 	ret = hid_parse(hdev);
 	if (ret) {
@@ -1298,12 +1413,22 @@  static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (hdev->ll_driver == &ps_client_ll_driver)
+		connect_mask = HID_CONNECT_HIDRAW;
+
+	ret = hid_hw_start(hdev, connect_mask);
 	if (ret) {
 		hid_err(hdev, "Failed to start HID device\n");
 		return ret;
 	}
 
+	/*
+	 * The virtual client_dev is only used for hidraw. Since we've already
+	 * started the hw, return early to avoid the recursive probe.
+	 */
+	if (hdev->ll_driver == &ps_client_ll_driver)
+		return ret;
+
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev, "Failed to open HID device\n");
@@ -1325,9 +1450,19 @@  static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_close;
 	}
 
+	if (dev->client_hdev)
+		ret = hid_add_device(dev->client_hdev);
+	if (ret) {
+		hid_err(hdev, "Failed to start client device failed\n");
+		goto err_close;
+	}
+
 	return ret;
 
 err_close:
+	if (dev->client_hdev)
+		hid_destroy_device(dev->client_hdev);
+
 	hid_hw_close(hdev);
 err_stop:
 	hid_hw_stop(hdev);
@@ -1341,6 +1476,9 @@  static void ps_remove(struct hid_device *hdev)
 	ps_devices_list_remove(dev);
 	ps_device_release_player_id(dev);
 
+	if (dev->client_hdev)
+		hid_destroy_device(dev->client_hdev);
+
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 }