Hello, On 5/5/22 01:50, Benjamin Tissoires wrote: > Hi Vicki, > > On Thu, Apr 28, 2022 at 12:52 AM Vicki Pfau <vi@xxxxxxxxxxx> 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@xxxxxxxxxxx/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@xxxxxxxxxxx> >> --- >> 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
