* [PATCH v2 08/14] HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Add a helper to take the 3 hid_hw_start() + hid_hw_open() +
hid_device_io_start() steps which are necessary to be able to
talk to the hw.
This is a preparation patch for moving the connect check to after
restarting IO, in case we miss a connect packet coming in while IO
is disabled during the restart.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 40 ++++++++++++++++++++------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index bbb1c6d8ccc9..45b371e7b9ee 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4391,6 +4391,29 @@ static bool hidpp_application_equals(struct hid_device *hdev,
return report && report->application == application;
}
+static int hidpp_connect_and_start(struct hidpp_device *hidpp, unsigned int connect_mask)
+{
+ struct hid_device *hdev = hidpp->hid_dev;
+ int ret;
+
+ ret = hid_hw_start(hdev, connect_mask);
+ if (ret) {
+ hid_err(hdev, "hw start failed: %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "hw open failed: %d\n", ret);
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ /* Allow incoming packets */
+ hid_device_io_start(hdev);
+ return 0;
+}
+
static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
struct hidpp_device *hidpp;
@@ -4462,21 +4485,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
* Plain USB connections need to actually call start and open
* on the transport driver to allow incoming data.
*/
- ret = hid_hw_start(hdev, 0);
- if (ret) {
- hid_err(hdev, "hw start failed\n");
+ ret = hidpp_connect_and_start(hidpp, 0);
+ if (ret)
goto hid_hw_start_fail;
- }
-
- ret = hid_hw_open(hdev);
- if (ret < 0) {
- dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
- __func__, ret);
- goto hid_hw_open_fail;
- }
-
- /* Allow incoming packets */
- hid_device_io_start(hdev);
/* Get name + serial, store in hdev->name + hdev->uniq */
if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
@@ -4523,7 +4534,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hid_hw_init_fail:
hid_hw_close(hdev);
-hid_hw_open_fail:
hid_hw_stop(hdev);
hid_hw_start_fail:
sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
--
2.41.0
^ permalink raw reply related
* [PATCH v2 11/14] HID: logitech-hidpp: Remove unused connected param from *_connect()
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Remove the unused connected function parameter from wtp_connect(),
m560_send_config_command() and k400_connect().
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index b278e2b8924a..2b0a2ea5da22 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3141,7 +3141,7 @@ static int wtp_allocate(struct hid_device *hdev, const struct hid_device_id *id)
return 0;
};
-static int wtp_connect(struct hid_device *hdev, bool connected)
+static int wtp_connect(struct hid_device *hdev)
{
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
struct wtp_data *wd = hidpp->private_data;
@@ -3203,7 +3203,7 @@ static const u8 m560_config_parameter[] = {0x00, 0xaf, 0x03};
#define M560_SUB_ID 0x0a
#define M560_BUTTON_MODE_REGISTER 0x35
-static int m560_send_config_command(struct hid_device *hdev, bool connected)
+static int m560_send_config_command(struct hid_device *hdev)
{
struct hidpp_report response;
struct hidpp_device *hidpp_dev;
@@ -3398,7 +3398,7 @@ static int k400_allocate(struct hid_device *hdev)
return 0;
};
-static int k400_connect(struct hid_device *hdev, bool connected)
+static int k400_connect(struct hid_device *hdev)
{
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
@@ -4205,15 +4205,15 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
}
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
- ret = wtp_connect(hdev, connected);
+ ret = wtp_connect(hdev);
if (ret)
return;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
- ret = m560_send_config_command(hdev, connected);
+ ret = m560_send_config_command(hdev);
if (ret)
return;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
- ret = k400_connect(hdev, connected);
+ ret = k400_connect(hdev);
if (ret)
return;
}
--
2.41.0
^ permalink raw reply related
* [PATCH v2 05/14] HID: logitech-hidpp: Move get_wireless_feature_index() check to hidpp_connect_event()
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Calling get_wireless_feature_index() from probe() causes
the wireless_feature_index to only get set for unifying devices which
are already connected at probe() time. It does not get set for devices
which connect later.
Fix this by moving get_wireless_feature_index() to hidpp_connect_event(),
this does not make a difference for devices connected at probe() since
probe() will queue the hidpp_connect_event() for those at probe time.
Fixes: 0da0a63b7cba ("HID: logitech-hidpp: Support WirelessDeviceStatus connect events")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 82f9723154f1..fe4c4871ea5c 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1835,15 +1835,14 @@ static int hidpp_battery_get_property(struct power_supply *psy,
/* -------------------------------------------------------------------------- */
#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS 0x1d4b
-static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
+static int hidpp_get_wireless_feature_index(struct hidpp_device *hidpp, u8 *feature_index)
{
u8 feature_type;
int ret;
ret = hidpp_root_get_feature(hidpp,
HIDPP_PAGE_WIRELESS_DEVICE_STATUS,
- &hidpp->wireless_feature_index,
- &feature_type);
+ feature_index, &feature_type);
return ret;
}
@@ -4247,6 +4246,13 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
}
}
+ if (hidpp->protocol_major >= 2) {
+ u8 feature_index;
+
+ if (!hidpp_get_wireless_feature_index(hidpp, &feature_index))
+ hidpp->wireless_feature_index = feature_index;
+ }
+
if (hidpp->name == hdev->name && hidpp->protocol_major >= 2) {
name = hidpp_get_device_name(hidpp);
if (name) {
@@ -4481,14 +4487,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
connected = hidpp_root_get_protocol_version(hidpp) == 0;
atomic_set(&hidpp->connected, connected);
- if (connected && hidpp->protocol_major >= 2) {
- ret = hidpp_set_wireless_feature_index(hidpp);
- if (ret == -ENOENT)
- hidpp->wireless_feature_index = 0;
- else if (ret)
- goto hid_hw_init_fail;
- }
-
if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
ret = wtp_get_config(hidpp);
if (ret)
--
2.41.0
^ permalink raw reply related
* [PATCH v2 00/14] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
Hi Benjamin,
Here is a v2 of my series to fix issues with hidpp_connect_event() running
while restarting IO, which now also fixes the issues you mentioned with
potentially missing connect events.
This series is best explained by a brief sketch of how probe()
looks at the end of the series (1):
Prep work:
1. All code depending on a device being in connected state is moved to
the hidpp_connect_event() workqueue item
2. hidpp_connect_event() now checks the connected state itself by
checking that hidpp_root_get_protocol_version() succeeds, instead
of relying on possibly stale (racy) data in struct hidpp_device
With this in place the new probe() sequence looks like this:
1. enable_connect_event flag starts at 0, this filters out / ignores any
connect-events in hidpp_raw_hidpp_event() avoiding
hidpp_connect_event() getting queued before the IO restart
2. IO is started with connect-mask = 0
this avoids hid-input and hidraw connecting while probe() is setting
hdev->name and hdev->uniq
3. Name and serialnr are retrieved and stored in hdev
4. IO is fully restarted (including hw_open + io_start, not just hw_start)
with the actual connect-mask.
5. enable_connect_event atomic_t is set to 1 to enable processing of
connect events.
6. hidpp_connect_event() is queued + flushed to query the connected state
and do the connect work if the device is connected.
7. probe() now ends with:
/*
* This relies on logi_dj_ll_close() being a no-op so that
* DJ connection events will still be received.
*/
hid_hw_close(hdev);
Since on restarting IO it now is fully restarted so the hid_hw_open()
there needs to be balanced.
This series now obviously is no longer 6.6 material, instead I hope we
can get this rework (and IMHO nice cleanup) into 6.7 .
Regards,
Hans
1) For reviewing you may also want to apply the entire series and look
at the end result to help you understand why various intermediate steps
are taken.
Hans de Goede (14):
HID: logitech-hidpp: Revert "Don't restart communication if not
necessary"
HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect
check
HID: logitech-hidpp: Add hidpp_non_unifying_init() helper
HID: logitech-hidpp: Remove connected check for non-unifying devices
HID: logitech-hidpp: Move get_wireless_feature_index() check to
hidpp_connect_event()
HID: logitech-hidpp: Remove wtp_get_config() call from probe()
HID: logitech-hidpp: Remove connected check for g920_get_config() call
HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
HID: logitech-hidpp: Move the connected check to after restarting IO
HID: logitech-hidpp: Move g920_get_config() to just before
hidpp_ff_init()
HID: logitech-hidpp: Remove unused connected param from *_connect()
HID: logitech-hidpp: Fix connect event race
HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
restarts IO
HID: logitech-hidpp: Drop delayed_work_cb()
drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
1 file changed, 91 insertions(+), 120 deletions(-)
--
2.41.0
^ permalink raw reply
* [PATCH v2 04/14] HID: logitech-hidpp: Remove connected check for non-unifying devices
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Non-unifying devices (USB, Bluetooth) are always connected during
probe(), remove the unnecessary connected check.
This is a preparation patch for moving querying if the device is connected
to after restarting IO, in case we miss a connect packet coming in while
IO is disabled during the restart.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index e4dbbdf46b97..82f9723154f1 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4480,13 +4480,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
connected = hidpp_root_get_protocol_version(hidpp) == 0;
atomic_set(&hidpp->connected, connected);
- if (!(hidpp->quirks & HIDPP_QUIRK_UNIFYING)) {
- if (!connected) {
- ret = -ENODEV;
- hid_err(hdev, "Device not connected");
- goto hid_hw_init_fail;
- }
- }
if (connected && hidpp->protocol_major >= 2) {
ret = hidpp_set_wireless_feature_index(hidpp);
--
2.41.0
^ permalink raw reply related
* [PATCH v2 01/14] HID: logitech-hidpp: Revert "Don't restart communication if not necessary"
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Commit 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")
makes hidpp_probe() first call hid_hw_start(hdev, 0) to allow IO
without connecting any hid subdrivers (hid-input, hidraw).
This is done to allow to retrieve the device's name and serial number
and store these in hdev->name and hdev->uniq.
Then later on IO is stopped and started again with hid_hw_start(hdev,
HID_CONNECT_DEFAULT) connecting hid-input and hidraw after the name
and serial number have been setup.
Commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication
if not necessary") changed the probe() code to only do the start with
a 0 connect-mask + restart later for unifying devices.
But for non unifying devices hdev->name and hdev->uniq are updated too.
So this change re-introduces the problem for which the start with
a 0 connect-mask + restart later behavior was introduced.
Revert the change to limit the restart behavior to unifying devices to
fix hdev->name changing after userspace facing devices have already been
registered.
Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary")
Cc: Bastien Nocera <hadess@hadess.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 32 ++++++++++++--------------------
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index a209d51bd247..09f7723eaabe 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4394,7 +4394,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
bool connected;
unsigned int connect_mask = HID_CONNECT_DEFAULT;
struct hidpp_ff_private_data data;
- bool will_restart = false;
/* report_fixup needs drvdata to be set before we call hid_parse */
hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL);
@@ -4445,10 +4444,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
- if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT ||
- hidpp->quirks & HIDPP_QUIRK_UNIFYING)
- will_restart = true;
-
INIT_WORK(&hidpp->work, delayed_work_cb);
mutex_init(&hidpp->send_mutex);
init_waitqueue_head(&hidpp->wait);
@@ -4463,7 +4458,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
* Plain USB connections need to actually call start and open
* on the transport driver to allow incoming data.
*/
- ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
+ ret = hid_hw_start(hdev, 0);
if (ret) {
hid_err(hdev, "hw start failed\n");
goto hid_hw_start_fail;
@@ -4502,7 +4497,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hidpp->wireless_feature_index = 0;
else if (ret)
goto hid_hw_init_fail;
- ret = 0;
}
if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
@@ -4518,21 +4512,19 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
schedule_work(&hidpp->work);
flush_work(&hidpp->work);
- if (will_restart) {
- /* Reset the HID node state */
- hid_device_io_stop(hdev);
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
+ /* Reset the HID node state */
+ hid_device_io_stop(hdev);
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
- if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
- connect_mask &= ~HID_CONNECT_HIDINPUT;
+ if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
+ connect_mask &= ~HID_CONNECT_HIDINPUT;
- /* Now export the actual inputs and hidraw nodes to the world */
- ret = hid_hw_start(hdev, connect_mask);
- if (ret) {
- hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
- goto hid_hw_start_fail;
- }
+ /* Now export the actual inputs and hidraw nodes to the world */
+ ret = hid_hw_start(hdev, connect_mask);
+ if (ret) {
+ hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
+ goto hid_hw_start_fail;
}
if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
--
2.41.0
^ permalink raw reply related
* [PATCH v2 02/14] HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect check
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Move the hidpp_overwrite_name() call to before the connect check, this
puts it at the same place in the probe() order as hidpp_serial_init()
which seems more logical. This should not make a difference since this
is in the non-unifying path and only unifying devices can be probed
in non-connected state.
This is a preparation patch for moving the connect check to after
restarting IO, in case we miss a connect packet coming in while IO
is disabled during the restart.
Doing this before the connect check requires dropping the protocol
version check since protocol_major is not set yet now. Instead
this relies on hidpp_root_get_feature(HIDPP_PAGE_GET_DEVICE_NAME_TYPE)
failing on older devices, just like how hidpp_get_serial() relies on
hidpp_root_get_feature(HIDPP_PAGE_DEVICE_INFORMATION) failing on older
devices. So this again makes the code more consistent.
Also stop printing an error on failure now, since with the proto
version check gone failures are expected to happen on older devices.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note: The weird unbalanced curly braces in the "else { ... }" go away
in the next patch in the series.
---
drivers/hid/hid-logitech-hidpp.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 09f7723eaabe..b1965b91c5bb 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4136,19 +4136,12 @@ static void hidpp_overwrite_name(struct hid_device *hdev)
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
char *name;
- if (hidpp->protocol_major < 2)
- return;
-
name = hidpp_get_device_name(hidpp);
-
- if (!name) {
- hid_err(hdev, "unable to retrieve the name of the device");
- } else {
+ if (name) {
dbg_hid("HID++: Got name: %s\n", name);
snprintf(hdev->name, sizeof(hdev->name), "%s", name);
+ kfree(name);
}
-
- kfree(name);
}
static int hidpp_input_open(struct input_dev *dev)
@@ -4476,8 +4469,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
hidpp_unifying_init(hidpp);
- else if (hid_is_usb(hidpp->hid_dev))
- hidpp_serial_init(hidpp);
+ else {
+ if (hid_is_usb(hidpp->hid_dev))
+ hidpp_serial_init(hidpp);
+
+ hidpp_overwrite_name(hdev);
+ }
connected = hidpp_root_get_protocol_version(hidpp) == 0;
atomic_set(&hidpp->connected, connected);
@@ -4487,8 +4484,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hid_err(hdev, "Device not connected");
goto hid_hw_init_fail;
}
-
- hidpp_overwrite_name(hdev);
}
if (connected && hidpp->protocol_major >= 2) {
--
2.41.0
^ permalink raw reply related
* [PATCH] Input: synaptics: enable InterTouch for ThinkPad L14 G1
From: José Pekkarinen @ 2023-10-08 8:01 UTC (permalink / raw)
To: dmitry.torokhov, rydberg, skhan
Cc: José Pekkarinen, amandhoot12, rrangel, linux-input,
linux-kernel, linux-kernel-mentees
Observed on dmesg of my laptop I see the following
output:
[ 19.898700] psmouse serio1: synaptics: queried max coordinates: x [..5678], y [..4694]
[ 19.936057] psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1162..]
[ 19.936076] psmouse serio1: synaptics: Your touchpad (PNP: LEN0411 PNP0f13) says it can support a different bus. If i2c-hid and hid-rmi are not used, you might want to try setting psmouse.synaptics_intertouch to 1 and report this to linux-input@vger.kernel.org.
[ 20.008901] psmouse serio1: synaptics: Touchpad model: 1, fw: 10.32, id: 0x1e2a1, caps: 0xf014a3/0x940300/0x12e800/0x500000, board id: 3471, fw id: 2909640
[ 20.008925] psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
[ 20.053344] input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio1/input/input7
[ 20.397608] mousedev: PS/2 mouse device common for all mice
This patch will add its pnp id to the smbus list to
produce the setup of intertouch for the device. After
applying, the ouput will look like:
[ 19.168664] psmouse serio1: synaptics: queried max coordinates: x [..5678], y [..4694]
[ 19.206311] psmouse serio1: synaptics: queried min coordinates: x [1266..], y [1162..]
[ 19.206325] psmouse serio1: synaptics: Trying to set up SMBus access
[ 19.209545] psmouse serio1: synaptics: SMbus companion is not ready yet
[ 19.283845] psmouse serio1: synaptics: Touchpad model: 1, fw: 10.32, id: 0x1e2a1, caps: 0xf014a3/0x940300/0x12e800/0x500000, board id: 3471, fw id: 2909640
[ 19.283863] psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
[ 19.328959] input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio1/input/input8
[ 19.706164] mousedev: PS/2 mouse device common for all mice
Signed-off-by: José Pekkarinen <jose.pekkarinen@foxhound.fi>
---
drivers/input/mouse/synaptics.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index ada299ec5bba..376a041c80b2 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -183,6 +183,7 @@ static const char * const smbus_pnp_ids[] = {
"LEN009b", /* T580 */
"LEN0402", /* X1 Extreme Gen 2 / P1 Gen 2 */
"LEN040f", /* P1 Gen 3 */
+ "LEN0411", /* L14 Gen 1 */
"LEN200f", /* T450s */
"LEN2044", /* L470 */
"LEN2054", /* E480 */
--
2.41.0
^ permalink raw reply related
* Re: [PATCH 2/2] input: add Adafruit Seesaw Gamepad driver
From: Anshul Dalal @ 2023-10-07 16:13 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-input, devicetree
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shuah Khan, linux-kernel-mentees
In-Reply-To: <a49deeb6-728c-4527-8399-57c52214e1d3@linaro.org>
Hello Krzysztof,
Thank you very much for the review, I am planning to address the changes
in my next patch series though I had a question.
On 10/7/23 20:26, Krzysztof Kozlowski wrote:
> On 07/10/2023 16:40, Anshul Dalal wrote:
>> +
>> +static int seesaw_probe(struct i2c_client *client)
>> +{
>> + int err;
>> + struct seesaw_gamepad *private;
>> + unsigned char register_reset[] = { SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_SWRST, 0xFF };
>> + unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
>> +
>> + err = i2c_master_send(client, register_reset, sizeof(register_reset));
>> + if (err < 0)
>> + return err;
>> + if (err != sizeof(register_reset))
>> + return -EIO;
>> + mdelay(10);
>
> Why 10? This should be explained somehow in the code.
The reason for the delay is to ensure the register reset process is
finished before going further. The reference Arduino driver from the
manufacturer also had a delay for the same amount though my hardware
unit worked fine till 5ms delay. Is there some kernel abstraction I
could use to indicate a short delay or will the previous explanation in
a comment above the line do?
Best wishes,
Anshul
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: Add bindings for Adafruit Seesaw Gamepad
From: Rob Herring @ 2023-10-07 15:23 UTC (permalink / raw)
To: Anshul Dalal
Cc: Conor Dooley, linux-kernel-mentees, Shuah Khan,
Krzysztof Kozlowski, linux-input, Dmitry Torokhov, devicetree,
Rob Herring
In-Reply-To: <20231007144052.1535417-1-anshulusr@gmail.com>
On Sat, 07 Oct 2023 20:10:49 +0530, Anshul Dalal wrote:
> A simple driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Steps in reading the gamepad state over i2c:
> 1. Reset the registers
> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> `BUTTON_MASK`: A bit-map for the six digital pins internally
> connected to the joystick buttons.
> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> 5. Poll the device for button and joystick state done by:
> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Tested on RPi Zero 2W
>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
> .../bindings/input/adafruit_seesaw.yaml | 45 +++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/input/adafruit_seesaw.yaml:4:6: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/input/adafruit_seesaw.yaml:5:10: [error] string value is redundantly quoted with any quotes (quoted-strings)
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/input/adafruit_seesaw.example.dts:20.13-26: Warning (reg_format): /example-0/seesaw_gamepad@50:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/input/adafruit_seesaw.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/input/adafruit_seesaw.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/input/adafruit_seesaw.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/input/adafruit_seesaw.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/input/adafruit_seesaw.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/adafruit_seesaw.example.dtb: seesaw_gamepad@50: 'reg' does not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/input/adafruit_seesaw.yaml#
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231007144052.1535417-1-anshulusr@gmail.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply
* Re: [PATCH v3 4/5] clk: twl: add clock driver for TWL6032
From: Andreas Kemnade @ 2023-09-12 18:56 UTC (permalink / raw)
To: Christophe JAILLET
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, linux-input, devicetree,
linux-kernel, linux-omap, linux-clk
In-Reply-To: <a9b646c7-2c02-8a69-a4c8-7e981a630eef@wanadoo.fr>
On Tue, 12 Sep 2023 19:15:54 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> Le 12/09/2023 à 00:13, Andreas Kemnade a écrit :
> > The TWL6032 has some clock outputs which are controlled like
> > fixed-voltage regulators, in some drivers for these chips
> > found in the wild, just the regulator api is abused for controlling
> > them, so simply use something similar to the regulator functions.
> > Due to a lack of hardware available for testing, leave out the
> > TWL6030-specific part of those functions.
> >
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > drivers/clk/Kconfig | 9 ++
> > drivers/clk/Makefile | 1 +
> > drivers/clk/clk-twl.c | 197 ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 207 insertions(+)
> > create mode 100644 drivers/clk/clk-twl.c
> >
>
> ...
>
> > +static int twl_clks_probe(struct platform_device *pdev)
> > +{
> > + struct clk_hw_onecell_data *clk_data;
> > + const struct twl_clks_data *hw_data;
> > +
> > + struct twl_clock_info *cinfo;
> > + int ret;
> > + int i;
> > + int count;
> > +
> > + hw_data = twl6032_clks;
> > + for (count = 0; hw_data[count].init.name; count++)
> > + ;
>
> Nit: does removing the /* sentinel */ and using
> ARRAY_SIZE(twl_clks_data) would make sense and be simpler?
>
well, I would like to have it prepared for different arrays
passed in some device data in the future, so I am choosing that
approach.
Regards,
Andreas
^ permalink raw reply
* Re: [PATCH 2/2] input: add Adafruit Seesaw Gamepad driver
From: Krzysztof Kozlowski @ 2023-10-07 14:56 UTC (permalink / raw)
To: Anshul Dalal, linux-input, devicetree
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shuah Khan, linux-kernel-mentees
In-Reply-To: <20231007144052.1535417-2-anshulusr@gmail.com>
On 07/10/2023 16:40, Anshul Dalal wrote:
> A simple driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Steps in reading the gamepad state over i2c:
> 1. Reset the registers
> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> `BUTTON_MASK`: A bit-map for the six digital pins internally
> connected to the joystick buttons.
> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> 5. Poll the device for button and joystick state done by:
> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Tested on RPi Zero 2W
>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
> MAINTAINERS | 7 +
> drivers/input/joystick/Kconfig | 9 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/adafruit_seesaw.c | 275 +++++++++++++++++++++++
> 4 files changed, 292 insertions(+)
> create mode 100644 drivers/input/joystick/adafruit_seesaw.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 81d5fc0bba68..cd4f9deb77e2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
> W: https://ez.analog.com/linux-software-drivers
> F: drivers/input/touchscreen/ad7879.c
>
> +ADAFRUIT MINI I2C GAMEPAD
> +M: Anshul Dalal <anshulusr@gmail.com>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
> +F: drivers/input/joystick/adafruit_seesaw.c
> +
> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
> M: Jiri Kosina <jikos@kernel.org>
> S: Maintained
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index ac6925ce8366..b8337edc6e22 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
> To compile this driver as a module, choose M here: the
> module will be called sensehat_joystick.
>
> +config JOYSTICK_SEESAW
> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
> + depends on I2C
> + help
> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
This does not look like correct indentation. Just look at other code and
do not do it differently.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called adafruit_seesaw.
> +
> endif
...
> +
> +static int seesaw_probe(struct i2c_client *client)
> +{
> + int err;
> + struct seesaw_gamepad *private;
> + unsigned char register_reset[] = { SEESAW_STATUS_BASE,
> + SEESAW_STATUS_SWRST, 0xFF };
> + unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
> +
> + err = i2c_master_send(client, register_reset, sizeof(register_reset));
> + if (err < 0)
> + return err;
> + if (err != sizeof(register_reset))
> + return -EIO;
> + mdelay(10);
Why 10? This should be explained somehow in the code.
> +
> + private = devm_kzalloc(&client->dev, sizeof(*private), GFP_KERNEL);
> + if (!private)
> + return -ENOMEM;
> +
> + err = i2c_master_send(client, get_hw_id, sizeof(get_hw_id));
> + if (err < 0)
> + return err;
> + if (err != sizeof(get_hw_id))
> + return -EIO;
> + err = i2c_master_recv(client, &private->hardware_id, 1);
> + if (err < 0)
> + return err;
> + if (err != 1)
> + return -EIO;
> +
> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> + private->hardware_id);
> +
> + private->i2c_client = client;
> + scnprintf(private->physical_path, sizeof(private->physical_path),
> + "i2c/%s", dev_name(&client->dev));
> + i2c_set_clientdata(client, private);
> +
> + private->input_dev = devm_input_allocate_device(&client->dev);
> + if (!private->input_dev)
> + return -ENOMEM;
> +
> + private->input_dev->id.bustype = BUS_I2C;
> + private->input_dev->name = "Adafruit Seesaw Gamepad";
> + private->input_dev->phys = private->physical_path;
> + input_set_drvdata(private->input_dev, private);
> + input_set_abs_params(private->input_dev, ABS_X, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_abs_params(private->input_dev, ABS_Y, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_capability(private->input_dev, EV_KEY, BTN_A);
> + input_set_capability(private->input_dev, EV_KEY, BTN_B);
> + input_set_capability(private->input_dev, EV_KEY, BTN_X);
> + input_set_capability(private->input_dev, EV_KEY, BTN_Y);
> + input_set_capability(private->input_dev, EV_KEY, BTN_START);
> + input_set_capability(private->input_dev, EV_KEY, BTN_SELECT);
> +
> + err = input_setup_polling(private->input_dev, seesaw_poll);
> + if (err) {
> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
> + return err;
> + }
> +
> + input_set_poll_interval(private->input_dev,
> + SEESAW_GAMEPAD_POLL_INTERVAL);
> + input_set_max_poll_interval(private->input_dev,
> + SEESAW_GAMEPAD_POLL_MAX);
> + input_set_min_poll_interval(private->input_dev,
> + SEESAW_GAMEPAD_POLL_MIN);
> +
> + err = input_register_device(private->input_dev);
> + if (err) {
> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
> + return err;
> + }
> +
> + /* Set Pin Mode to input and enable pull-up resistors */
> + unsigned char pin_mode[] = { SEESAW_GPIO_BASE, SEESAW_GPIO_DIRCLR_BULK,
> + BUTTON_MASK >> 24, BUTTON_MASK >> 16,
> + BUTTON_MASK >> 8, BUTTON_MASK };
> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
> + pin_mode[1] = SEESAW_GPIO_PULLENSET;
> + err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
> + pin_mode[1] = SEESAW_GPIO_BULK_SET;
> + err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
> + if (err < 0)
> + return err;
> + if (err != sizeof(pin_mode))
> + return -EIO;
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_seesaw_match[] = {
> + {
> + .compatible = "adafruit,seesaw_gamepad",
> + },
> + { /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, of_seesaw_match);
> +#endif /* CONFIG_OF */
> +
> +static const struct i2c_device_id seesaw_id_table[] = { { KBUILD_MODNAME, 0 },
> + { /* Sentinel */ } };
Please format it just like all other drivers have it.
> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> +
> +static struct i2c_driver seesaw_driver = {
> + .driver = {
> + .name = SEESAW_DEVICE_NAME,
> + .of_match_table = of_match_ptr(of_seesaw_match),
> + },
> + .id_table = seesaw_id_table,
> + .probe = seesaw_probe,
Some mixed indentation before =.
> +};
> +module_i2c_driver(seesaw_driver);
> +
> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
> +MODULE_LICENSE("GPL");
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: Add bindings for Adafruit Seesaw Gamepad
From: Krzysztof Kozlowski @ 2023-10-07 14:52 UTC (permalink / raw)
To: Anshul Dalal, linux-input, devicetree
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shuah Khan, linux-kernel-mentees
In-Reply-To: <20231007144052.1535417-1-anshulusr@gmail.com>
On 07/10/2023 16:40, Anshul Dalal wrote:
> A simple driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
Thank you for your patch. There is something to discuss/improve.
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Steps in reading the gamepad state over i2c:
> 1. Reset the registers
> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> `BUTTON_MASK`: A bit-map for the six digital pins internally
> connected to the joystick buttons.
> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> 5. Poll the device for button and joystick state done by:
> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
This describes driver, not bindings or hardware Please instead describe
hardware.
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Tested on RPi Zero 2W
How can you test bindings on RPi Zero 2W? Somehow I don't believe you,
see below.
>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
> .../bindings/input/adafruit_seesaw.yaml | 45 +++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/adafruit_seesaw.yaml b/Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
> new file mode 100644
> index 000000000000..1d00d9da637a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
Use compatible syntax, so vendor-prefix,device-name.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/input/adafruit_seesaw.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
Drop quotes.
It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.
> +
> +title: Adafruit Mini I2C Gamepad with seesaw
> +
> +maintainers:
> + - Anshul Dalal <anshulusr@gmail.com>
> +
> +description: |
> + Bindings for Adafruit Mini I2C Gamepad
Drop "Bindings for"
> +
> + +-----------------------------+
> + | ___ |
> + | / \ (X) |
> + | | S | __ __ (Y) (A) |
> + | \___/ |ST| |SE| (B) |
> + | |
> + +-----------------------------+
> +
> + S -> 10-bit percision bidirectional analog joystick
> + ST -> Start
> + SE -> Select
> + X, A, B, Y -> Digital action buttons
> +
> + Product page: https://www.adafruit.com/product/5743
> + Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw
> +
> +properties:
> + compatible:
> + const: adafruit,seesaw_gamepad
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + seesaw_gamepad@50 {
No underscores, generic node names.
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "adafruit,seesaw_gamepad";
> + reg = <0x50>;
Second hint that you did not test it...
> + };
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH 2/2] input: add Adafruit Seesaw Gamepad driver
From: Anshul Dalal @ 2023-10-07 14:40 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Shuah Khan, linux-kernel-mentees
In-Reply-To: <20231007144052.1535417-1-anshulusr@gmail.com>
A simple driver for a mini gamepad that communicates over i2c, the gamepad
has bidirectional thumb stick input and six buttons.
The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
to transmit the ADC data for the joystick and digital pin state for the
buttons. I have only implemented the functionality required to receive the
thumb stick and button state.
Steps in reading the gamepad state over i2c:
1. Reset the registers
2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
`BUTTON_MASK`: A bit-map for the six digital pins internally
connected to the joystick buttons.
3. Enable internal pullup resistors for the `BUTTON_MASK`
4. Bulk set the pin state HIGH for `BUTTON_MASK`
5. Poll the device for button and joystick state done by:
`seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
Tested on RPi Zero 2W
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
MAINTAINERS | 7 +
drivers/input/joystick/Kconfig | 9 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/adafruit_seesaw.c | 275 +++++++++++++++++++++++
4 files changed, 292 insertions(+)
create mode 100644 drivers/input/joystick/adafruit_seesaw.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 81d5fc0bba68..cd4f9deb77e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
W: https://ez.analog.com/linux-software-drivers
F: drivers/input/touchscreen/ad7879.c
+ADAFRUIT MINI I2C GAMEPAD
+M: Anshul Dalal <anshulusr@gmail.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
+F: drivers/input/joystick/adafruit_seesaw.c
+
ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
M: Jiri Kosina <jikos@kernel.org>
S: Maintained
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index ac6925ce8366..b8337edc6e22 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
To compile this driver as a module, choose M here: the
module will be called sensehat_joystick.
+config JOYSTICK_SEESAW
+ tristate "Adafruit Mini I2C Gamepad with Seesaw"
+ depends on I2C
+ help
+ Say Y here if you want to use the Adafruit Mini I2C Gamepad.
+
+ To compile this driver as a module, choose M here: the module will be
+ called adafruit_seesaw.
+
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 3937535f0098..fdc653209542 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
+obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit_seesaw.o
obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
diff --git a/drivers/input/joystick/adafruit_seesaw.c b/drivers/input/joystick/adafruit_seesaw.c
new file mode 100644
index 000000000000..a04df0469595
--- /dev/null
+++ b/drivers/input/joystick/adafruit_seesaw.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
+ *
+ * Driver for Adafruit Mini I2C Gamepad
+ *
+ * Based on the work of:
+ * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
+ *
+ * Product page: https://www.adafruit.com/product/5743
+ * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
+ */
+
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+/* clang-format off */
+#define SEESAW_DEVICE_NAME "seesaw_gamepad"
+
+#define SEESAW_STATUS_BASE 0
+#define SEESAW_GPIO_BASE 1
+#define SEESAW_ADC_BASE 9
+
+#define SEESAW_GPIO_DIRCLR_BULK 3
+#define SEESAW_GPIO_BULK 4
+#define SEESAW_GPIO_BULK_SET 5
+#define SEESAW_GPIO_PULLENSET 11
+
+#define SEESAW_STATUS_HW_ID 1
+#define SEESAW_STATUS_SWRST 127
+
+#define SEESAW_ADC_OFFSET 7
+
+#define BUTTON_A 5
+#define BUTTON_B 1
+#define BUTTON_X 6
+#define BUTTON_Y 2
+#define BUTTON_START 16
+#define BUTTON_SELECT 0
+
+#define ANALOG_X 14
+#define ANALOG_Y 15
+
+#define SEESAW_JOYSTICK_MAX_AXIS 1023
+#define SEESAW_JOYSTICK_FUZZ 2
+#define SEESAW_JOYSTICK_FLAT 4
+
+#define SEESAW_GAMEPAD_POLL_INTERVAL 16
+#define SEESAW_GAMEPAD_POLL_MIN 8
+#define SEESAW_GAMEPAD_POLL_MAX 32
+/* clang-format on */
+
+u32 BUTTON_MASK = (1UL << BUTTON_A) | (1UL << BUTTON_B) | (1UL << BUTTON_X) |
+ (1UL << BUTTON_Y) | (1UL << BUTTON_START) |
+ (1UL << BUTTON_SELECT);
+
+struct seesaw_gamepad {
+ char physical_path[32];
+ unsigned char hardware_id;
+ struct input_dev *input_dev;
+ struct i2c_client *i2c_client;
+};
+
+struct seesaw_data {
+ __be16 x;
+ __be16 y;
+ u8 button_a, button_b, button_x, button_y, button_start, button_select;
+};
+
+static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
+{
+ int err;
+ unsigned char write_buf[2] = { SEESAW_GPIO_BASE, SEESAW_GPIO_BULK };
+ unsigned char read_buf[4];
+
+ err = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(write_buf))
+ return -EIO;
+ err = i2c_master_recv(client, read_buf, sizeof(read_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(read_buf))
+ return -EIO;
+ u32 result = ((u32)read_buf[0] << 24) | ((u32)read_buf[1] << 16) |
+ ((u32)read_buf[2] << 8) | (u32)read_buf[3];
+ data->button_a = !(result & (1UL << BUTTON_A));
+ data->button_b = !(result & (1UL << BUTTON_B));
+ data->button_x = !(result & (1UL << BUTTON_X));
+ data->button_y = !(result & (1UL << BUTTON_Y));
+ data->button_start = !(result & (1UL << BUTTON_START));
+ data->button_select = !(result & (1UL << BUTTON_SELECT));
+
+ write_buf[0] = SEESAW_ADC_BASE;
+ write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_X;
+ err = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(write_buf))
+ return -EIO;
+ err = i2c_master_recv(client, (char *)&data->x, sizeof(data->x));
+ if (err < 0)
+ return err;
+ if (err != sizeof(data->x))
+ return -EIO;
+ /*
+ * ADC reads left as max and right as 0, must be reversed since kernel
+ * expects reports in opposite order.
+ */
+ data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
+
+ write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_Y;
+ err = i2c_master_send(client, write_buf, 2);
+ if (err < 0)
+ return err;
+ if (err != sizeof(write_buf))
+ return -EIO;
+ err = i2c_master_recv(client, (char *)&data->y, data->y);
+ if (err < 0)
+ return err;
+ if (err != data->y)
+ return -EIO;
+ data->y = be16_to_cpu(data->y);
+
+ return 0;
+}
+
+static void seesaw_poll(struct input_dev *input)
+{
+ struct seesaw_gamepad *private = input_get_drvdata(input);
+ struct seesaw_data data;
+ int err;
+
+ err = seesaw_read_data(private->i2c_client, &data);
+ if (err != 0)
+ return;
+
+ input_report_abs(input, ABS_X, data.x);
+ input_report_abs(input, ABS_Y, data.y);
+ input_report_key(input, BTN_A, data.button_a);
+ input_report_key(input, BTN_B, data.button_b);
+ input_report_key(input, BTN_X, data.button_x);
+ input_report_key(input, BTN_Y, data.button_y);
+ input_report_key(input, BTN_START, data.button_start);
+ input_report_key(input, BTN_SELECT, data.button_select);
+ input_sync(input);
+}
+
+static int seesaw_probe(struct i2c_client *client)
+{
+ int err;
+ struct seesaw_gamepad *private;
+ unsigned char register_reset[] = { SEESAW_STATUS_BASE,
+ SEESAW_STATUS_SWRST, 0xFF };
+ unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
+
+ err = i2c_master_send(client, register_reset, sizeof(register_reset));
+ if (err < 0)
+ return err;
+ if (err != sizeof(register_reset))
+ return -EIO;
+ mdelay(10);
+
+ private = devm_kzalloc(&client->dev, sizeof(*private), GFP_KERNEL);
+ if (!private)
+ return -ENOMEM;
+
+ err = i2c_master_send(client, get_hw_id, sizeof(get_hw_id));
+ if (err < 0)
+ return err;
+ if (err != sizeof(get_hw_id))
+ return -EIO;
+ err = i2c_master_recv(client, &private->hardware_id, 1);
+ if (err < 0)
+ return err;
+ if (err != 1)
+ return -EIO;
+
+ dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
+ private->hardware_id);
+
+ private->i2c_client = client;
+ scnprintf(private->physical_path, sizeof(private->physical_path),
+ "i2c/%s", dev_name(&client->dev));
+ i2c_set_clientdata(client, private);
+
+ private->input_dev = devm_input_allocate_device(&client->dev);
+ if (!private->input_dev)
+ return -ENOMEM;
+
+ private->input_dev->id.bustype = BUS_I2C;
+ private->input_dev->name = "Adafruit Seesaw Gamepad";
+ private->input_dev->phys = private->physical_path;
+ input_set_drvdata(private->input_dev, private);
+ input_set_abs_params(private->input_dev, ABS_X, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ input_set_abs_params(private->input_dev, ABS_Y, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ input_set_capability(private->input_dev, EV_KEY, BTN_A);
+ input_set_capability(private->input_dev, EV_KEY, BTN_B);
+ input_set_capability(private->input_dev, EV_KEY, BTN_X);
+ input_set_capability(private->input_dev, EV_KEY, BTN_Y);
+ input_set_capability(private->input_dev, EV_KEY, BTN_START);
+ input_set_capability(private->input_dev, EV_KEY, BTN_SELECT);
+
+ err = input_setup_polling(private->input_dev, seesaw_poll);
+ if (err) {
+ dev_err(&client->dev, "failed to set up polling: %d\n", err);
+ return err;
+ }
+
+ input_set_poll_interval(private->input_dev,
+ SEESAW_GAMEPAD_POLL_INTERVAL);
+ input_set_max_poll_interval(private->input_dev,
+ SEESAW_GAMEPAD_POLL_MAX);
+ input_set_min_poll_interval(private->input_dev,
+ SEESAW_GAMEPAD_POLL_MIN);
+
+ err = input_register_device(private->input_dev);
+ if (err) {
+ dev_err(&client->dev, "failed to register joystick: %d\n", err);
+ return err;
+ }
+
+ /* Set Pin Mode to input and enable pull-up resistors */
+ unsigned char pin_mode[] = { SEESAW_GPIO_BASE, SEESAW_GPIO_DIRCLR_BULK,
+ BUTTON_MASK >> 24, BUTTON_MASK >> 16,
+ BUTTON_MASK >> 8, BUTTON_MASK };
+ err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
+ pin_mode[1] = SEESAW_GPIO_PULLENSET;
+ err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
+ pin_mode[1] = SEESAW_GPIO_BULK_SET;
+ err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
+ if (err < 0)
+ return err;
+ if (err != sizeof(pin_mode))
+ return -EIO;
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_seesaw_match[] = {
+ {
+ .compatible = "adafruit,seesaw_gamepad",
+ },
+ { /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, of_seesaw_match);
+#endif /* CONFIG_OF */
+
+static const struct i2c_device_id seesaw_id_table[] = { { KBUILD_MODNAME, 0 },
+ { /* Sentinel */ } };
+MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
+
+static struct i2c_driver seesaw_driver = {
+ .driver = {
+ .name = SEESAW_DEVICE_NAME,
+ .of_match_table = of_match_ptr(of_seesaw_match),
+ },
+ .id_table = seesaw_id_table,
+ .probe = seesaw_probe,
+};
+module_i2c_driver(seesaw_driver);
+
+MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
+MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: Add bindings for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-07 14:40 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Shuah Khan, linux-kernel-mentees
A simple driver for a mini gamepad that communicates over i2c, the gamepad
has bidirectional thumb stick input and six buttons.
The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
to transmit the ADC data for the joystick and digital pin state for the
buttons. I have only implemented the functionality required to receive the
thumb stick and button state.
Steps in reading the gamepad state over i2c:
1. Reset the registers
2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
`BUTTON_MASK`: A bit-map for the six digital pins internally
connected to the joystick buttons.
3. Enable internal pullup resistors for the `BUTTON_MASK`
4. Bulk set the pin state HIGH for `BUTTON_MASK`
5. Poll the device for button and joystick state done by:
`seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
Tested on RPi Zero 2W
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
.../bindings/input/adafruit_seesaw.yaml | 45 +++++++++++++++++++
1 file changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
diff --git a/Documentation/devicetree/bindings/input/adafruit_seesaw.yaml b/Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
new file mode 100644
index 000000000000..1d00d9da637a
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/input/adafruit_seesaw.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Adafruit Mini I2C Gamepad with seesaw
+
+maintainers:
+ - Anshul Dalal <anshulusr@gmail.com>
+
+description: |
+ Bindings for Adafruit Mini I2C Gamepad
+
+ +-----------------------------+
+ | ___ |
+ | / \ (X) |
+ | | S | __ __ (Y) (A) |
+ | \___/ |ST| |SE| (B) |
+ | |
+ +-----------------------------+
+
+ S -> 10-bit percision bidirectional analog joystick
+ ST -> Start
+ SE -> Select
+ X, A, B, Y -> Digital action buttons
+
+ Product page: https://www.adafruit.com/product/5743
+ Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw
+
+properties:
+ compatible:
+ const: adafruit,seesaw_gamepad
+
+required:
+ - compatible
+
+additionalProperties: false
+
+examples:
+ - |
+ seesaw_gamepad@50 {
+ compatible = "adafruit,seesaw_gamepad";
+ reg = <0x50>;
+ };
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v4 5/5] ARM: dts: omap4-embt2ws: enable 32K clock on WLAN
From: Tony Lindgren @ 2023-10-07 6:53 UTC (permalink / raw)
To: Andreas Kemnade
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, mturquette, sboyd, linux-input, devicetree,
linux-kernel, linux-omap, linux-clk
In-Reply-To: <20230916100515.1650336-6-andreas@kemnade.info>
* Andreas Kemnade <andreas@kemnade.info> [230916 13:05]:
> WLAN did only work if clock was left enabled by the original system,
> so make it fully enable the needed resources itself.
Seems applying this dts change before the clock patch is applied
would break wlan so please let me know when this is safe to apply.
Regards,
Tony
^ permalink raw reply
* [PATCH] Input: tegra: Use device_get_match_data()
From: Rob Herring @ 2023-10-06 22:44 UTC (permalink / raw)
To: Laxman Dewangan, Dmitry Torokhov, Thierry Reding, Jonathan Hunter
Cc: linux-input, linux-tegra, linux-kernel
Use preferred device_get_match_data() instead of of_match_device() to
get the driver match data. With this, adjust the includes to explicitly
include the correct headers.
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/input/keyboard/tegra-kbc.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index c9a823ea45d0..a1765ed8c825 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -14,7 +14,7 @@
#include <linux/io.h>
#include <linux/interrupt.h>
#include <linux/of.h>
-#include <linux/of_device.h>
+#include <linux/property.h>
#include <linux/clk.h>
#include <linux/slab.h>
#include <linux/input/matrix_keypad.h>
@@ -602,9 +602,6 @@ static int tegra_kbc_probe(struct platform_device *pdev)
unsigned int debounce_cnt;
unsigned int scan_time_rows;
unsigned int keymap_rows;
- const struct of_device_id *match;
-
- match = of_match_device(tegra_kbc_of_match, &pdev->dev);
kbc = devm_kzalloc(&pdev->dev, sizeof(*kbc), GFP_KERNEL);
if (!kbc) {
@@ -613,7 +610,7 @@ static int tegra_kbc_probe(struct platform_device *pdev)
}
kbc->dev = &pdev->dev;
- kbc->hw_support = match->data;
+ kbc->hw_support = device_get_match_data(&pdev->dev);
kbc->max_keys = kbc->hw_support->max_rows *
kbc->hw_support->max_columns;
kbc->num_rows_and_columns = kbc->hw_support->max_rows +
--
2.40.1
^ permalink raw reply related
* Re: [PATCH] Input: Annotate struct ff_device with __counted_by
From: Gustavo A. R. Silva @ 2023-10-06 20:47 UTC (permalink / raw)
To: Kees Cook, Dmitry Torokhov
Cc: Gustavo A. R. Silva, linux-input, linux-hardening,
Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, llvm
In-Reply-To: <20231006201739.work.350-kees@kernel.org>
On 10/6/23 22:17, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for
> array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> As found with Coccinelle[1], add __counted_by for struct ff_device.
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Cc: linux-input@vger.kernel.org
> Cc: linux-hardening@vger.kernel.org
> Link: https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci [1]
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks!
--
Gustavo
> ---
> include/linux/input.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 49790c1bd2c4..de6503c0edb8 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -562,7 +562,7 @@ struct ff_device {
>
> int max_effects;
> struct ff_effect *effects;
> - struct file *effect_owners[];
> + struct file *effect_owners[] __counted_by(max_effects);
> };
>
> int input_ff_create(struct input_dev *dev, unsigned int max_effects);
^ permalink raw reply
* [PATCH] Input: Annotate struct ff_device with __counted_by
From: Kees Cook @ 2023-10-06 20:17 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Kees Cook, Gustavo A. R. Silva, linux-input, linux-hardening,
Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, llvm
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for
array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ff_device.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: linux-input@vger.kernel.org
Cc: linux-hardening@vger.kernel.org
Link: https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci [1]
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/input.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/input.h b/include/linux/input.h
index 49790c1bd2c4..de6503c0edb8 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -562,7 +562,7 @@ struct ff_device {
int max_effects;
struct ff_effect *effects;
- struct file *effect_owners[];
+ struct file *effect_owners[] __counted_by(max_effects);
};
int input_ff_create(struct input_dev *dev, unsigned int max_effects);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 1/2] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-06 17:56 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <c5d79ddb-43ff-2a3d-8577-92fbd52ccb44@redhat.com>
Hi Benjamin,
On 10/6/23 19:24, Hans de Goede wrote:
> Hi,
>
> On 10/6/23 18:28, Benjamin Tissoires wrote:
>> On Oct 06 2023, Hans de Goede wrote:
>
> <snip>
>
>>>>> @@ -4207,36 +4208,39 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>>>>> return;
>>>>> }
>>>>>
>>>>> + /* Avoid probe() restarting IO */
>>>>> + mutex_lock(&hidpp->io_mutex);
>>>>
>>>> I'd put a `__must_hold(&hidpp->io_mutex);` here, not changing any return
>>>> path and forcing any caller to `hidpp_connect_event()` (which will
>>>> eventually only be the work struct) to take the lock.
>>>>
>>>> This should simplify the patch by a lot and also ensure someone doesn't
>>>> forget the `goto out_unlock`.
>>>
>>> Ok, I can add the __must_hold() here and make
>>> delayed_Work_cb take the lock, but that would make it
>>> impossible to implement patch 2/2 in a clean manner and
>>> I do like patch 2/2 since it makes it clear that
>>> hidpp_connect_event must only run from the workqueue
>>> but I guess we could just add a comment for that
>>> instead.
>>
>> In 2/2, just rename this function to __do_hidpp_connect_event(), and
>> have hidpp_connect_event() being the worker, which takes the lock, and
>> calls __do_hidpp_connect_event().
>
> Ok, will do for v2.
>
> <snip>
>
>>>>> @@ -4519,6 +4526,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>>>> flush_work(&hidpp->work);
>>>>>
>>>>> if (will_restart) {
>>>>> + /* Avoid hidpp_connect_event() running while restarting */
>>>>> + mutex_lock(&hidpp->io_mutex);
>>>>> +
>>>>> /* Reset the HID node state */
>>>>> hid_device_io_stop(hdev);
>>>>
>>>> That's the part that makes me raise an eyebrow. Because we lock, then
>>>> release the semaphore to get it back later. Can this induce a dead lock?
>>>>
>>>> Can't we solve that same scenario without a mutex, but forcing either
>>>> the workqueue to not run or to be finished at this point?
>>>
>>> I'm not sure what you are worried about after the mutex_lock
>>> the line above we are 100% guaranteed that hidpp_connect_event()
>>> is not running and since it is not running it will also not
>>> be holding any other locks, so it can not cause any problems.
>>
>> Agree, but my point is that you are not entirely solving the issue:
>> if now, between hid_device_io_stop() and hid_hw_close() we receive a
>> connect notification from the device, hid_input_report() will return
>> -EBUSY, and we will lose it (it will not be stacked in the workqueue).
>>
>> I was thinking at adding a flush_work(&hidpp->work) here, instead of
>> the mutex solution, but yours ensures that any connect event already
>> started will be handled properly, which is a plus.
>>
>> Still if between the mutex lock here we receive a connect event from the
>> device, we still get -EBUSY at the hid-core layer, and so we will lose
>> it. Maybe that's OK because we might re-ask for the device later (I
>> don't remember exactly the code), but my point is that because we add a
>> mutex doesn't mean we will solve all multi-thread problems. So finding a
>> non-mutex solution sometimes is better :)
>>
>> And the fact that we need to think through every preemption case often
>> means that there is something wrong *elsewhere*.
>
> Right, I did consider seeing if we can get rid of the restart
> altogether, as the whole restarting thing is actually the problem
> here. AFAICT this is only really necessary in the WTP path since
> there where we need to know resolution before instantiating the
> input device.
>
> But atm this is also done for all unifying devices, which seems
> unnecessary.
>
> Buy we still need the restart anyways for the WTP case,
> so we need to make it work reliable anyways.
>
> Now that I understand your concern about the missed connect
> packet, which I agree is a real concern I think I know how to
> fix this. I'll prepare a new version of this series tomorrow.
>
> Hmm, thinking more about this, if we normally just create
> the input device right away even for unifying devices and
> we already always delay the creation for WTP even during
> the restart:
>
> if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
> connect_mask &= ~HID_CONNECT_HIDINPUT;
>
> Then I wonder why do we even bother to do the restart
> thing for unifying devices. Do you know what this is based on ?
>
> I guess this might have to do with ensuring the configure
> commands are send before creating the input + hidraw
> devices, but if the connect event comes later on then
> the configuration is already done later on after
> the input device has already been created ?
>
> So maybe we should indeed just remove the whole restart
> thing entirely and also always rely on hidpp_connect_event
> to send the configuration commands, because currently
> those are send twice if the device is already connnected
> at probe() time.
The whole restart thing seems to have been added by you in:
91cf9a98ae4127 ("HID: logitech-hidpp: make .probe usbhid capable")
The commit message says that not starting all the HID
subdrivers right from the start is necessary because:
"It means that any configuration retrieved after the initial hid_hw_start
would not be exposed to user space without races."
I believe this refers to the filling of hdev->name
and hdev->uniq by hidpp_unifying_init()
(or other similar code paths for non unifying).
It seems that has been broken though by:
498ba20690357 ("HID: logitech-hidpp: Don't restart communication if not necessary")
Which causes input-dev registration to happen before
hidpp_unifying_init().
TL;DR:
1. There seems to be a good reason for the restart dance so
we need to fix it rather then remove it.
2. 498ba20690357 ("HID: logitech-hidpp: Don't restart communication if not necessary")
(from Bastien) re-introduces the race which the restart tries
to fix and should be reverted.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 1/2] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-06 17:24 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <zjiang3fdy4o7r3daupwpnx6zesmeeerldpx5fno2adzialpre@cdp7tq4araww>
Hi,
On 10/6/23 18:28, Benjamin Tissoires wrote:
> On Oct 06 2023, Hans de Goede wrote:
<snip>
>>>> @@ -4207,36 +4208,39 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>>>> return;
>>>> }
>>>>
>>>> + /* Avoid probe() restarting IO */
>>>> + mutex_lock(&hidpp->io_mutex);
>>>
>>> I'd put a `__must_hold(&hidpp->io_mutex);` here, not changing any return
>>> path and forcing any caller to `hidpp_connect_event()` (which will
>>> eventually only be the work struct) to take the lock.
>>>
>>> This should simplify the patch by a lot and also ensure someone doesn't
>>> forget the `goto out_unlock`.
>>
>> Ok, I can add the __must_hold() here and make
>> delayed_Work_cb take the lock, but that would make it
>> impossible to implement patch 2/2 in a clean manner and
>> I do like patch 2/2 since it makes it clear that
>> hidpp_connect_event must only run from the workqueue
>> but I guess we could just add a comment for that
>> instead.
>
> In 2/2, just rename this function to __do_hidpp_connect_event(), and
> have hidpp_connect_event() being the worker, which takes the lock, and
> calls __do_hidpp_connect_event().
Ok, will do for v2.
<snip>
>>>> @@ -4519,6 +4526,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>>> flush_work(&hidpp->work);
>>>>
>>>> if (will_restart) {
>>>> + /* Avoid hidpp_connect_event() running while restarting */
>>>> + mutex_lock(&hidpp->io_mutex);
>>>> +
>>>> /* Reset the HID node state */
>>>> hid_device_io_stop(hdev);
>>>
>>> That's the part that makes me raise an eyebrow. Because we lock, then
>>> release the semaphore to get it back later. Can this induce a dead lock?
>>>
>>> Can't we solve that same scenario without a mutex, but forcing either
>>> the workqueue to not run or to be finished at this point?
>>
>> I'm not sure what you are worried about after the mutex_lock
>> the line above we are 100% guaranteed that hidpp_connect_event()
>> is not running and since it is not running it will also not
>> be holding any other locks, so it can not cause any problems.
>
> Agree, but my point is that you are not entirely solving the issue:
> if now, between hid_device_io_stop() and hid_hw_close() we receive a
> connect notification from the device, hid_input_report() will return
> -EBUSY, and we will lose it (it will not be stacked in the workqueue).
>
> I was thinking at adding a flush_work(&hidpp->work) here, instead of
> the mutex solution, but yours ensures that any connect event already
> started will be handled properly, which is a plus.
>
> Still if between the mutex lock here we receive a connect event from the
> device, we still get -EBUSY at the hid-core layer, and so we will lose
> it. Maybe that's OK because we might re-ask for the device later (I
> don't remember exactly the code), but my point is that because we add a
> mutex doesn't mean we will solve all multi-thread problems. So finding a
> non-mutex solution sometimes is better :)
>
> And the fact that we need to think through every preemption case often
> means that there is something wrong *elsewhere*.
Right, I did consider seeing if we can get rid of the restart
altogether, as the whole restarting thing is actually the problem
here. AFAICT this is only really necessary in the WTP path since
there where we need to know resolution before instantiating the
input device.
But atm this is also done for all unifying devices, which seems
unnecessary.
Buy we still need the restart anyways for the WTP case,
so we need to make it work reliable anyways.
Now that I understand your concern about the missed connect
packet, which I agree is a real concern I think I know how to
fix this. I'll prepare a new version of this series tomorrow.
Hmm, thinking more about this, if we normally just create
the input device right away even for unifying devices and
we already always delay the creation for WTP even during
the restart:
if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
connect_mask &= ~HID_CONNECT_HIDINPUT;
Then I wonder why do we even bother to do the restart
thing for unifying devices. Do you know what this is based on ?
I guess this might have to do with ensuring the configure
commands are send before creating the input + hidraw
devices, but if the connect event comes later on then
the configuration is already done later on after
the input device has already been created ?
So maybe we should indeed just remove the whole restart
thing entirely and also always rely on hidpp_connect_event
to send the configuration commands, because currently
those are send twice if the device is already connnected
at probe() time.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH RFT v6 2/6] ARM: pxa: Convert Spitz LEDs to GPIO descriptors
From: Bartosz Golaszewski @ 2023-10-06 17:25 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij, Andy Shevchenko,
Dmitry Torokhov, Mark Brown, linux-arm-kernel, linux-kernel,
linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <20231006-pxa-gpio-v6-2-981b4910d599@skole.hr>
On Fri, Oct 6, 2023 at 3:45 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> Sharp's Spitz board still uses the legacy GPIO interface for configuring
> its two onboard LEDs.
>
> Convert them to use the GPIO descriptor interface.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---
> arch/arm/mach-pxa/spitz.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
> index 535e2b2e997b..1fb4102ea39e 100644
> --- a/arch/arm/mach-pxa/spitz.c
> +++ b/arch/arm/mach-pxa/spitz.c
> @@ -452,16 +452,25 @@ static inline void spitz_keys_init(void) {}
> * LEDs
> ******************************************************************************/
> #if defined(CONFIG_LEDS_GPIO) || defined(CONFIG_LEDS_GPIO_MODULE)
> +static struct gpiod_lookup_table spitz_led_gpio_table = {
> + .dev_id = "leds-gpio",
> + .table = {
> + GPIO_LOOKUP_IDX("pxa-gpio", SPITZ_GPIO_LED_ORANGE, NULL, 0,
> + GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP_IDX("pxa-gpio", SPITZ_GPIO_LED_GREEN, NULL, 1,
> + GPIO_ACTIVE_HIGH),
> + { }
> + }
> +};
> +
> static struct gpio_led spitz_gpio_leds[] = {
> {
> .name = "spitz:amber:charge",
> .default_trigger = "sharpsl-charge",
> - .gpio = SPITZ_GPIO_LED_ORANGE,
> },
> {
> .name = "spitz:green:hddactivity",
> .default_trigger = "disk-activity",
> - .gpio = SPITZ_GPIO_LED_GREEN,
> },
> };
>
> @@ -480,7 +489,14 @@ static struct platform_device spitz_led_device = {
>
> static void __init spitz_leds_init(void)
> {
> + struct gpio_descs *leds;
This should be global, otherwise KASAN will complain.
Bart
> +
> + gpiod_add_lookup_table(&spitz_led_gpio_table);
> platform_device_register(&spitz_led_device);
> + leds = gpiod_get_array_optional(&spitz_led_device.dev,
> + NULL, GPIOD_ASIS);
> + spitz_gpio_leds[0].gpiod = leds->desc[0];
> + spitz_gpio_leds[1].gpiod = leds->desc[1];
> }
> #else
> static inline void spitz_leds_init(void) {}
>
> --
> 2.42.0
>
>
^ permalink raw reply
* Re: [PATCH RFT v6 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors
From: Bartosz Golaszewski @ 2023-10-06 17:24 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij, Andy Shevchenko,
Dmitry Torokhov, Mark Brown, linux-arm-kernel, linux-kernel,
linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <20231006-pxa-gpio-v6-1-981b4910d599@skole.hr>
On Fri, Oct 6, 2023 at 3:45 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> Sharp's Spitz board still uses the legacy GPIO interface for controlling
> a GPIO pin related to the USB host controller.
>
> Convert this function to use the new GPIO descriptor interface.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---
> arch/arm/mach-pxa/spitz.c | 13 ++++++-------
> drivers/usb/host/ohci-pxa27x.c | 7 +++++++
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
> index cc691b199429..535e2b2e997b 100644
> --- a/arch/arm/mach-pxa/spitz.c
> +++ b/arch/arm/mach-pxa/spitz.c
> @@ -649,23 +649,22 @@ static inline void spitz_mmc_init(void) {}
> * USB Host
> ******************************************************************************/
> #if defined(CONFIG_USB_OHCI_HCD) || defined(CONFIG_USB_OHCI_HCD_MODULE)
> +GPIO_LOOKUP_SINGLE(spitz_usb_host_gpio_table, "pxa27x-ohci", "gpio-pxa",
> + SPITZ_GPIO_USB_HOST, "usb-host", GPIO_ACTIVE_LOW);
> +
> static int spitz_ohci_init(struct device *dev)
> {
> - int err;
> -
> - err = gpio_request(SPITZ_GPIO_USB_HOST, "USB_HOST");
> - if (err)
> - return err;
> + gpiod_add_lookup_table(&spitz_usb_host_gpio_table);
>
> /* Only Port 2 is connected, setup USB Port 2 Output Control Register */
> UP2OCR = UP2OCR_HXS | UP2OCR_HXOE | UP2OCR_DPPDE | UP2OCR_DMPDE;
>
> - return gpio_direction_output(SPITZ_GPIO_USB_HOST, 1);
> + return 0;
> }
>
> static void spitz_ohci_exit(struct device *dev)
> {
> - gpio_free(SPITZ_GPIO_USB_HOST);
> + gpiod_remove_lookup_table(&spitz_usb_host_gpio_table);
> }
>
> static struct pxaohci_platform_data spitz_ohci_platform_data = {
> diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
> index 357d9aee38a3..7f04421c80d6 100644
> --- a/drivers/usb/host/ohci-pxa27x.c
> +++ b/drivers/usb/host/ohci-pxa27x.c
> @@ -121,6 +121,7 @@ struct pxa27x_ohci {
> void __iomem *mmio_base;
> struct regulator *vbus[3];
> bool vbus_enabled[3];
> + struct gpio_desc *usb_host;
> };
>
> #define to_pxa27x_ohci(hcd) (struct pxa27x_ohci *)(hcd_to_ohci(hcd)->priv)
> @@ -447,6 +448,10 @@ static int ohci_hcd_pxa27x_probe(struct platform_device *pdev)
> pxa_ohci = to_pxa27x_ohci(hcd);
> pxa_ohci->clk = usb_clk;
> pxa_ohci->mmio_base = (void __iomem *)hcd->regs;
> + pxa_ohci->usb_host = devm_gpiod_get_optional(&pdev->dev, "usb-host", GPIOD_OUT_LOW);
> + if (IS_ERR(pxa_ohci->usb_host))
> + return dev_err_probe(&pdev->dev, PTR_ERR(pxa_ohci->usb_host),
> + "failed to get USB host GPIO\n");
>
> for (i = 0; i < 3; ++i) {
> char name[6];
> @@ -512,6 +517,8 @@ static void ohci_hcd_pxa27x_remove(struct platform_device *pdev)
> for (i = 0; i < 3; ++i)
> pxa27x_ohci_set_vbus_power(pxa_ohci, i, false);
>
> + gpiod_put(pxa_ohci->usb_host);
This is now wrong. Devres APIs are managed by the driver core. You no
longer need this in your remove() callback.
Bart
> +
> usb_put_hcd(hcd);
> }
>
>
> --
> 2.42.0
>
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Benjamin Tissoires @ 2023-10-06 16:28 UTC (permalink / raw)
To: Hans de Goede
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <686e8973-613b-2fb3-efd6-26f3dd21ed9d@redhat.com>
On Oct 06 2023, Hans de Goede wrote:
> Hi Benjamin,
>
> On 10/6/23 15:46, Benjamin Tissoires wrote:
> > Hi Hans,
> >
> > On Oct 06 2023, Hans de Goede wrote:
> >> hidpp_probe() restarts IO after setting things up, if we get a connect
> >> event just before hidpp_probe() stops all IO then hidpp_connect_event()
> >> will see IO errors causing it to fail to setup the connected device.
> >
> > I think I see why you are doing this, but it scares me to be honest.
> >
> >>
> >> Add a new io_mutex which hidpp_probe() locks while restarting IO and
> >> which is also taken by hidpp_connect_event() to avoid these 2 things
> >> from racing.
> >
> > So now we are adding a new mutex to prevent a workqueue to be executed,
> > which is held while there is another semaphore going down/up...
> > It feels error prone to say the least and I'm not sure we are not
> > actually fixing the problem.
> >
> > My guts tells me that the issue is tackled at the wrong time, and that
> > maybe there is a better solution that doesn't involve a new lock in the
> > middle of 2 other locks being actually held...
>
> Since the lock is only taken into 2 places and 1 of them is holding
> no locks when taking it (because workqueue) I don't really see how
> this would be a problem.
>
> Actually introducing a new lock for this, rather then say trying
> to use the send_mutex makes this much easier to reason about,
> more on this below.
>
> > One minor comment in the code.
> >
> >>
> >> Hopefully this will help with the occasional connect errors leading to
> >> e.g. missing battery monitoring.
> >>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> drivers/hid/hid-logitech-hidpp.c | 35 ++++++++++++++++++++++----------
> >> 1 file changed, 24 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> >> index a209d51bd247..33f9cd98485a 100644
> >> --- a/drivers/hid/hid-logitech-hidpp.c
> >> +++ b/drivers/hid/hid-logitech-hidpp.c
> >> @@ -181,6 +181,7 @@ struct hidpp_scroll_counter {
> >> struct hidpp_device {
> >> struct hid_device *hid_dev;
> >> struct input_dev *input;
> >> + struct mutex io_mutex;
> >> struct mutex send_mutex;
> >> void *send_receive_buf;
> >> char *name; /* will never be NULL and should not be freed */
> >> @@ -4207,36 +4208,39 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> >> return;
> >> }
> >>
> >> + /* Avoid probe() restarting IO */
> >> + mutex_lock(&hidpp->io_mutex);
> >
> > I'd put a `__must_hold(&hidpp->io_mutex);` here, not changing any return
> > path and forcing any caller to `hidpp_connect_event()` (which will
> > eventually only be the work struct) to take the lock.
> >
> > This should simplify the patch by a lot and also ensure someone doesn't
> > forget the `goto out_unlock`.
>
> Ok, I can add the __must_hold() here and make
> delayed_Work_cb take the lock, but that would make it
> impossible to implement patch 2/2 in a clean manner and
> I do like patch 2/2 since it makes it clear that
> hidpp_connect_event must only run from the workqueue
> but I guess we could just add a comment for that
> instead.
In 2/2, just rename this function to __do_hidpp_connect_event(), and
have hidpp_connect_event() being the worker, which takes the lock, and
calls __do_hidpp_connect_event().
>
> Either way works for me, with a slight preference
> for the current version even if it introduces
> a bunch of gotos.
>
> >
> >> +
> >> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> >> ret = wtp_connect(hdev, connected);
> >> if (ret)
> >> - return;
> >> + goto out_unlock;
> >> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
> >> ret = m560_send_config_command(hdev, connected);
> >> if (ret)
> >> - return;
> >> + goto out_unlock;
> >> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
> >> ret = k400_connect(hdev, connected);
> >> if (ret)
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_WHEELS) {
> >> ret = hidpp10_wheel_connect(hidpp);
> >> if (ret)
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS) {
> >> ret = hidpp10_extra_mouse_buttons_connect(hidpp);
> >> if (ret)
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
> >> ret = hidpp10_consumer_keys_connect(hidpp);
> >> if (ret)
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> /* the device is already connected, we can ask for its name and
> >> @@ -4245,7 +4249,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> >> ret = hidpp_root_get_protocol_version(hidpp);
> >> if (ret) {
> >> hid_err(hdev, "Can not get the protocol version.\n");
> >> - return;
> >> + goto out_unlock;
> >> }
> >> }
> >>
> >> @@ -4256,7 +4260,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> >> "%s", name);
> >> kfree(name);
> >> if (!devm_name)
> >> - return;
> >> + goto out_unlock;
> >>
> >> hidpp->name = devm_name;
> >> }
> >> @@ -4291,12 +4295,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> >>
> >> if (!(hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) || hidpp->delayed_input)
> >> /* if the input nodes are already created, we can stop now */
> >> - return;
> >> + goto out_unlock;
> >>
> >> input = hidpp_allocate_input(hdev);
> >> if (!input) {
> >> hid_err(hdev, "cannot allocate new input device: %d\n", ret);
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> hidpp_populate_input(hidpp, input);
> >> @@ -4304,10 +4308,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> >> ret = input_register_device(input);
> >> if (ret) {
> >> input_free_device(input);
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> hidpp->delayed_input = input;
> >> +out_unlock:
> >> + mutex_unlock(&hidpp->io_mutex);
> >> }
> >>
> >> static DEVICE_ATTR(builtin_power_supply, 0000, NULL, NULL);
> >> @@ -4450,6 +4456,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >> will_restart = true;
> >>
> >> INIT_WORK(&hidpp->work, delayed_work_cb);
> >> + mutex_init(&hidpp->io_mutex);
> >> mutex_init(&hidpp->send_mutex);
> >> init_waitqueue_head(&hidpp->wait);
> >>
> >> @@ -4519,6 +4526,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >> flush_work(&hidpp->work);
> >>
> >> if (will_restart) {
> >> + /* Avoid hidpp_connect_event() running while restarting */
> >> + mutex_lock(&hidpp->io_mutex);
> >> +
> >> /* Reset the HID node state */
> >> hid_device_io_stop(hdev);
> >
> > That's the part that makes me raise an eyebrow. Because we lock, then
> > release the semaphore to get it back later. Can this induce a dead lock?
> >
> > Can't we solve that same scenario without a mutex, but forcing either
> > the workqueue to not run or to be finished at this point?
>
> I'm not sure what you are worried about after the mutex_lock
> the line above we are 100% guaranteed that hidpp_connect_event()
> is not running and since it is not running it will also not
> be holding any other locks, so it can not cause any problems.
Agree, but my point is that you are not entirely solving the issue:
if now, between hid_device_io_stop() and hid_hw_close() we receive a
connect notification from the device, hid_input_report() will return
-EBUSY, and we will lose it (it will not be stacked in the workqueue).
I was thinking at adding a flush_work(&hidpp->work) here, instead of
the mutex solution, but yours ensures that any connect event already
started will be handled properly, which is a plus.
Still if between the mutex lock here we receive a connect event from the
device, we still get -EBUSY at the hid-core layer, and so we will lose
it. Maybe that's OK because we might re-ask for the device later (I
don't remember exactly the code), but my point is that because we add a
mutex doesn't mean we will solve all multi-thread problems. So finding a
non-mutex solution sometimes is better :)
And the fact that we need to think through every preemption case often
means that there is something wrong *elsewhere*.
>
> The other way around if hidpp_connect_event() is running
> the mutex_lock() above ensures that it will finishes and
> release any locks before we get here.
>
> There is no way that both code paths can run at the same time
> with the new lock. And there thus also is no way that they
> can cause any new not already held locks to be taken while
> the other side is running.
>
> I actually introduced the new lock because in my mind
> introducing the new lock allows to easily reason about
> the impact on other locking (which is none).
>
> I hope this helps explain. As for the making
> hidpp_connect_event()'s caller take the lock thing, let me
> know how you want to resolve that. Either way works for me
> and I guess the less intrusive version of making the caller
> take the lock is easier to backport so we should probably
> go that route.
Again, for that particular point, making the function a private one that
doesn't have to cope with the mutex will be simpler in this patch and in
the future.
Cheers,
Benjamin
>
> Regards,
>
> Hans
>
>
>
> >> hid_hw_close(hdev);
> >> @@ -4529,6 +4539,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>
> >> /* Now export the actual inputs and hidraw nodes to the world */
> >> ret = hid_hw_start(hdev, connect_mask);
> >> + mutex_unlock(&hidpp->io_mutex);
> >> if (ret) {
> >> hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
> >> goto hid_hw_start_fail;
> >> @@ -4553,6 +4564,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >> sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
> >> cancel_work_sync(&hidpp->work);
> >> mutex_destroy(&hidpp->send_mutex);
> >> + mutex_destroy(&hidpp->io_mutex);
> >> return ret;
> >> }
> >>
> >> @@ -4568,6 +4580,7 @@ static void hidpp_remove(struct hid_device *hdev)
> >> hid_hw_stop(hdev);
> >> cancel_work_sync(&hidpp->work);
> >> mutex_destroy(&hidpp->send_mutex);
> >> + mutex_destroy(&hidpp->io_mutex);
> >> }
> >>
> >> #define LDJ_DEVICE(product) \
> >> --
> >> 2.41.0
> >>
> >
> > Cheers,
> > Benjamin
> >
>
>
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-06 15:11 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <iqchunho27bqb6dp24ptfx32gdwbq6f6v654ftfme4kel3hoa6@5t2x4kcms2wk>
Hi Benjamin,
On 10/6/23 15:46, Benjamin Tissoires wrote:
> Hi Hans,
>
> On Oct 06 2023, Hans de Goede wrote:
>> hidpp_probe() restarts IO after setting things up, if we get a connect
>> event just before hidpp_probe() stops all IO then hidpp_connect_event()
>> will see IO errors causing it to fail to setup the connected device.
>
> I think I see why you are doing this, but it scares me to be honest.
>
>>
>> Add a new io_mutex which hidpp_probe() locks while restarting IO and
>> which is also taken by hidpp_connect_event() to avoid these 2 things
>> from racing.
>
> So now we are adding a new mutex to prevent a workqueue to be executed,
> which is held while there is another semaphore going down/up...
> It feels error prone to say the least and I'm not sure we are not
> actually fixing the problem.
>
> My guts tells me that the issue is tackled at the wrong time, and that
> maybe there is a better solution that doesn't involve a new lock in the
> middle of 2 other locks being actually held...
Since the lock is only taken into 2 places and 1 of them is holding
no locks when taking it (because workqueue) I don't really see how
this would be a problem.
Actually introducing a new lock for this, rather then say trying
to use the send_mutex makes this much easier to reason about,
more on this below.
> One minor comment in the code.
>
>>
>> Hopefully this will help with the occasional connect errors leading to
>> e.g. missing battery monitoring.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/hid/hid-logitech-hidpp.c | 35 ++++++++++++++++++++++----------
>> 1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> index a209d51bd247..33f9cd98485a 100644
>> --- a/drivers/hid/hid-logitech-hidpp.c
>> +++ b/drivers/hid/hid-logitech-hidpp.c
>> @@ -181,6 +181,7 @@ struct hidpp_scroll_counter {
>> struct hidpp_device {
>> struct hid_device *hid_dev;
>> struct input_dev *input;
>> + struct mutex io_mutex;
>> struct mutex send_mutex;
>> void *send_receive_buf;
>> char *name; /* will never be NULL and should not be freed */
>> @@ -4207,36 +4208,39 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>> return;
>> }
>>
>> + /* Avoid probe() restarting IO */
>> + mutex_lock(&hidpp->io_mutex);
>
> I'd put a `__must_hold(&hidpp->io_mutex);` here, not changing any return
> path and forcing any caller to `hidpp_connect_event()` (which will
> eventually only be the work struct) to take the lock.
>
> This should simplify the patch by a lot and also ensure someone doesn't
> forget the `goto out_unlock`.
Ok, I can add the __must_hold() here and make
delayed_Work_cb take the lock, but that would make it
impossible to implement patch 2/2 in a clean manner and
I do like patch 2/2 since it makes it clear that
hidpp_connect_event must only run from the workqueue
but I guess we could just add a comment for that
instead.
Either way works for me, with a slight preference
for the current version even if it introduces
a bunch of gotos.
>
>> +
>> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
>> ret = wtp_connect(hdev, connected);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
>> ret = m560_send_config_command(hdev, connected);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
>> ret = k400_connect(hdev, connected);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> }
>>
>> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_WHEELS) {
>> ret = hidpp10_wheel_connect(hidpp);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> }
>>
>> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS) {
>> ret = hidpp10_extra_mouse_buttons_connect(hidpp);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> }
>>
>> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
>> ret = hidpp10_consumer_keys_connect(hidpp);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> }
>>
>> /* the device is already connected, we can ask for its name and
>> @@ -4245,7 +4249,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>> ret = hidpp_root_get_protocol_version(hidpp);
>> if (ret) {
>> hid_err(hdev, "Can not get the protocol version.\n");
>> - return;
>> + goto out_unlock;
>> }
>> }
>>
>> @@ -4256,7 +4260,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>> "%s", name);
>> kfree(name);
>> if (!devm_name)
>> - return;
>> + goto out_unlock;
>>
>> hidpp->name = devm_name;
>> }
>> @@ -4291,12 +4295,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>>
>> if (!(hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) || hidpp->delayed_input)
>> /* if the input nodes are already created, we can stop now */
>> - return;
>> + goto out_unlock;
>>
>> input = hidpp_allocate_input(hdev);
>> if (!input) {
>> hid_err(hdev, "cannot allocate new input device: %d\n", ret);
>> - return;
>> + goto out_unlock;
>> }
>>
>> hidpp_populate_input(hidpp, input);
>> @@ -4304,10 +4308,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>> ret = input_register_device(input);
>> if (ret) {
>> input_free_device(input);
>> - return;
>> + goto out_unlock;
>> }
>>
>> hidpp->delayed_input = input;
>> +out_unlock:
>> + mutex_unlock(&hidpp->io_mutex);
>> }
>>
>> static DEVICE_ATTR(builtin_power_supply, 0000, NULL, NULL);
>> @@ -4450,6 +4456,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> will_restart = true;
>>
>> INIT_WORK(&hidpp->work, delayed_work_cb);
>> + mutex_init(&hidpp->io_mutex);
>> mutex_init(&hidpp->send_mutex);
>> init_waitqueue_head(&hidpp->wait);
>>
>> @@ -4519,6 +4526,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> flush_work(&hidpp->work);
>>
>> if (will_restart) {
>> + /* Avoid hidpp_connect_event() running while restarting */
>> + mutex_lock(&hidpp->io_mutex);
>> +
>> /* Reset the HID node state */
>> hid_device_io_stop(hdev);
>
> That's the part that makes me raise an eyebrow. Because we lock, then
> release the semaphore to get it back later. Can this induce a dead lock?
>
> Can't we solve that same scenario without a mutex, but forcing either
> the workqueue to not run or to be finished at this point?
I'm not sure what you are worried about after the mutex_lock
the line above we are 100% guaranteed that hidpp_connect_event()
is not running and since it is not running it will also not
be holding any other locks, so it can not cause any problems.
The other way around if hidpp_connect_event() is running
the mutex_lock() above ensures that it will finishes and
release any locks before we get here.
There is no way that both code paths can run at the same time
with the new lock. And there thus also is no way that they
can cause any new not already held locks to be taken while
the other side is running.
I actually introduced the new lock because in my mind
introducing the new lock allows to easily reason about
the impact on other locking (which is none).
I hope this helps explain. As for the making
hidpp_connect_event()'s caller take the lock thing, let me
know how you want to resolve that. Either way works for me
and I guess the less intrusive version of making the caller
take the lock is easier to backport so we should probably
go that route.
Regards,
Hans
>> hid_hw_close(hdev);
>> @@ -4529,6 +4539,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>
>> /* Now export the actual inputs and hidraw nodes to the world */
>> ret = hid_hw_start(hdev, connect_mask);
>> + mutex_unlock(&hidpp->io_mutex);
>> if (ret) {
>> hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
>> goto hid_hw_start_fail;
>> @@ -4553,6 +4564,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
>> cancel_work_sync(&hidpp->work);
>> mutex_destroy(&hidpp->send_mutex);
>> + mutex_destroy(&hidpp->io_mutex);
>> return ret;
>> }
>>
>> @@ -4568,6 +4580,7 @@ static void hidpp_remove(struct hid_device *hdev)
>> hid_hw_stop(hdev);
>> cancel_work_sync(&hidpp->work);
>> mutex_destroy(&hidpp->send_mutex);
>> + mutex_destroy(&hidpp->io_mutex);
>> }
>>
>> #define LDJ_DEVICE(product) \
>> --
>> 2.41.0
>>
>
> Cheers,
> Benjamin
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox