| Message ID | 20220427224526.35657-1-vi@endrift.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] HID: hid-playstation: Allow removal of touchpad | expand |
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 >
On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote: > 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. Hmm, if userspace is not interested in data from an input device (and it does not want to simply not open it), it can use inhibit/uninhibit sysfs attributes to silence it. I am not sure we need to build support for destroying input device or introducing module parameters, etc. Thanks.
Hello, On 5/5/22 01:50, Benjamin Tissoires wrote: > 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... I would much rather that. I (attempted) to start a discussion on this a few weeks ago (https://lore.kernel.org/linux-input/b5f229c3-26e5-4fe1-aecb-504aa3c38bee@endrift.com/T/) but no one replied, so I went ahead and implemented what I assumed would be a substandard implementation, if at the very least so my sponsor would be happy, and as an attempt to start the conversation again. > > 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) Ah, I'm new to this so I overlooked that. I'll fix this in a v2, if it's even worth having a v2 given that, as I said, I would really rather not have implemented it this way in the first place. > >> + 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. The mutex is used in both patch 2 and 3, so I put it here in case either 2 or 3 is rejected, but not the other. > > 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 >> > Thanks, Vicki
Hi Vicki, Joining the conversation late as I on a long vacation. On Thu, May 5, 2022 at 12:47 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote: > > 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. > I understand the use case of rejecting input. I wasn't that fond of handling it kernel side also because it would complicate the code a lot more (and some would perhaps want to do the same for accelerometer device). Below Dmitry gives a nice idea about inhibition. Methods I would considered perhaps would have been a custom udev role (it sounds like you have control of the platform). Or another method is using EVIOCGRAB to get exclusive access to an input device to block others from using it again depends on your specific use case. When this topic came up some years ago with Valve it was one of the methods considered there if I recall. > Hmm, if userspace is not interested in data from an input device (and it > does not want to simply not open it), it can use inhibit/uninhibit sysfs > attributes to silence it. > > I am not sure we need to build support for destroying input device or > introducing module parameters, etc. > Inhibition is interesting and could work. I wasn't aware of this feature and we may actually use it for hid-playstation to save some power as there are various HID commands for power saving when the device isn't open. If we were to add that, some care would need to be taken with HIDRAW, which the driver is often unaware of. I guess the end responsibility for making sure the device would work would be with the hidraw user (unless there will be some interop APIs). > Thanks. > > -- > Dmitry Thanks, Roderick
Hello, On 5/5/22 22:21, Roderick Colenbrander wrote: > Hi Vicki, > > Joining the conversation late as I on a long vacation. > > On Thu, May 5, 2022 at 12:47 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: >> >> On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote: >>> 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. >> > > I understand the use case of rejecting input. I wasn't that fond of > handling it kernel side also because it would complicate the code a > lot more (and some would perhaps want to do the same for accelerometer > device). Below Dmitry gives a nice idea about inhibition. > > Methods I would considered perhaps would have been a custom udev role > (it sounds like you have control of the platform). Or another method > is using EVIOCGRAB to get exclusive access to an input device to block > others from using it again depends on your specific use case. When > this topic came up some years ago with Valve it was one of the methods > considered there if I recall. > >> Hmm, if userspace is not interested in data from an input device (and it >> does not want to simply not open it), it can use inhibit/uninhibit sysfs >> attributes to silence it. >> >> I am not sure we need to build support for destroying input device or >> introducing module parameters, etc. >> > > Inhibition is interesting and could work. I wasn't aware of this > feature and we may actually use it for hid-playstation to save some > power as there are various HID commands for power saving when the > device isn't open. If we were to add that, some care would need to be > taken with HIDRAW, which the driver is often unaware of. I guess the > end responsibility for making sure the device would work would be with > the hidraw user (unless there will be some interop APIs). > I would like to know more about these sysfs controls. I wasn't aware of any way to do this from userspace, and paired with udev rules that allow access to these controls (can udev modify sysfs permissions?) to the right users/groups, this might be a viable way to manage this. My sponsor is Valve, so that might be why this is somewhat familiar. They want to be able to have the Steam input daemon disable various reporting from the controllers without requiring root or getting too deep into the system, but I think udev rules that allow Steam itself access to the sysfs inhibition controls may be a good approach if there's a way to do that, especially already in the kernel. I know the kernel they're shipping right now is a few versions old. Thanks, Vicki >> Thanks. >> >> -- >> Dmitry > > > Thanks, > Roderick
On Thu, May 5, 2022 at 12:47 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote: > > 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. > Using this to hook into the thread with Benjamin. It didn't make it to the list (due to HTML I guess) and replying from work email messes things up (Outlook...). There is definitely a need for some type of hidraw / evdev interop. What it should be I'm not fully sure. I think it needs to be some explicit request or something from user space. In case of the Dualsense it is a very complicated device and many features work through HID including many audio and other features, which I hope to support at some point. I feel the need for the driver to be able to 'snoop' what is going through hidraw. The extra node hack allowed for that, though ideally if a hid driver could get some callbacks without having to go through all this extra setup code, would be great. For me the use cases for snooping include: - Keep sysfs nodes in sync e.g. LED nodes with whatever a hidraw user is setting. - Error handling in case of a crashing hidraw client. E.g. a hidraw client may enable rumble. If the application crashes, we probably want the kernel driver to disable rumbling. - Handling of audio features (speaker, microphone, headphone jack settings, ...). One day we will provide a proper audio driver over HID. Many of the control features work over the same HID output report as used for rumble, LEDs etcetera. - When no users (hidraw / evdev) are connected, potentially enable some of the power management features (e.g. disable touchpad / sensors at the hardware level) There are probably some others as well. Thanks, Roderick
On Fri, May 6, 2022 at 8:59 AM Roderick Colenbrander <thunderbird2k@gmail.com> wrote: > > On Thu, May 5, 2022 at 12:47 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > On Thu, May 05, 2022 at 10:50:24AM +0200, Benjamin Tissoires wrote: > > > 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. > > > > Using this to hook into the thread with Benjamin. It didn't make it to > the list (due to HTML I guess) and replying from work email messes > things up (Outlook...). > > There is definitely a need for some type of hidraw / evdev interop. > What it should be I'm not fully sure. I think it needs to be some > explicit request or something from user space. > > In case of the Dualsense it is a very complicated device and many > features work through HID including many audio and other features, > which I hope to support at some point. I feel the need for the driver > to be able to 'snoop' what is going through hidraw. The extra node > hack allowed for that, though ideally if a hid driver could get some > callbacks without having to go through all this extra setup code, > would be great. FWIW, as you probably all know by now, I am implementing eBPF at the HID level, and one of the use cases is to be able to have a HID firewall. The main reason for it right now IMO is that we allow uacess for Steam on the PS controllers, but that also means that anybody can try to initiate a firmware update by talking to the correct endpoints. And even if we trust steam to do that properly or not doing it at all, we do not trust others. And this firewall might help you in some cases: > > For me the use cases for snooping include: > - Keep sysfs nodes in sync e.g. LED nodes with whatever a hidraw user > is setting. BPF might help here, because we can directly snoop on HID reports. We need to add hooks to sync the LED state, but that is doable. A pure kernel implementation would work too though. > - Error handling in case of a crashing hidraw client. E.g. a hidraw > client may enable rumble. If the application crashes, we probably want > the kernel driver to disable rumbling. This is going to happen sooner than you think. With the systemd pull request I mentioned in the other thread, we are going to have the ability to revoke hidraw accesses when the client is not in foreground or if there is a fast user switching. Resetting to a known working state is hard if we don't know the context, so we'll probably need some kind of helper in that situation. > - Handling of audio features (speaker, microphone, headphone jack > settings, ...). One day we will provide a proper audio driver over > HID. Many of the control features work over the same HID output report > as used for rumble, LEDs etcetera. Definitely firewall related, possibly with on the fly modifications of the reports. However, there is a chance we see SDL or Steam saying "I'm going to implement the driver on the userspace, so I can have it cross-platform", and it's going to be hairy :( > - When no users (hidraw / evdev) are connected, potentially enable > some of the power management features (e.g. disable touchpad / sensors > at the hardware level) Shouldn't be too hard to do without bpf, and by just adding the correct callback if any. We should already get a notification at the HID core level when there are no users (hid_hw_open/close IIRC), so maybe we just need a hook into the driver if there is not one already. Cheers, Benjamin > > There are probably some others as well. > > Thanks, > Roderick >
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) { + 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); 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)
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. Signed-off-by: Vicki Pfau <vi@endrift.com> --- drivers/hid/hid-playstation.c | 60 +++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 17 deletions(-)