* [PATCH v3 2/3] HID: i2c-hid: Retry HID descriptor read to wake up STM devices
From: Kenny Levinsen @ 2024-04-26 22:47 UTC (permalink / raw)
To: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki,
Lukasz Majczak
Cc: Kenny Levinsen
In-Reply-To: <20240426225739.2166-1-kl@kl.wtf>
Some STM microcontrollers need 400µs after rising clock edge in order to
come out of their deep sleep state. This in turn means that the first
command sent to them will fail on a bus error.
Retry once on bus error to see if the device came alive, otherwise treat
the error as if no device was present like before.
Link: https://lore.kernel.org/all/20240405102436.3479210-1-lma@chromium.org/#t
Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
Co-developed-by: Lukasz Majczak <lma@chromium.org>
Tested-by: Lukasz Majczak <lma@chromium.org>
Reviewed-by: Lukasz Majczak <lma@chromium.org>
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 6ffa43d245b4..6ac1b11fb675 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -991,8 +991,17 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
struct hid_device *hid = ihid->hid;
int ret;
+ /*
+ * Some STM-based devices need 400µs after a rising clock edge to wake
+ * from deep sleep, which in turn means that our first command will
+ * fail on a bus error. Retry the command in this case.
+ */
ret = i2c_hid_fetch_hid_descriptor(ihid);
- if (ret < 0) {
+ if (ret == -ENXIO) {
+ usleep_range(400, 500);
+ ret = i2c_hid_fetch_hid_descriptor(ihid);
+ }
+ if (ret) {
i2c_hid_dbg(ihid, "failed to fetch HID descriptor: %d\n", ret);
return ret;
}
--
2.44.0
^ permalink raw reply related
* [PATCH v3 3/3] HID: i2c-hid: Align i2c_hid_set_power() retry with HID descriptor read
From: Kenny Levinsen @ 2024-04-26 22:47 UTC (permalink / raw)
To: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki,
Lukasz Majczak
Cc: Kenny Levinsen
In-Reply-To: <20240426225739.2166-1-kl@kl.wtf>
The retry for HID descriptor and for power commands deals with the same
device quirk, so align the two.
Tested-by: Lukasz Majczak <lma@chromium.org>
Reviewed-by: Lukasz Majczak <lma@chromium.org>
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 6ac1b11fb675..4ec12c083714 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -385,25 +385,21 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
i2c_hid_dbg(ihid, "%s\n", __func__);
/*
- * Some devices require to send a command to wakeup before power on.
- * The call will get a return value (EREMOTEIO) but device will be
- * triggered and activated. After that, it goes like a normal device.
+ * Some STM-based devices need 400µs after a rising clock edge to wake
+ * from deep sleep, which in turn means that our first command will
+ * fail on a bus error. Certain Weida Tech devices also need this
+ * wake-up. Retry the command in this case.
*/
- if (power_state == I2C_HID_PWR_ON) {
+ ret = i2c_hid_set_power_command(ihid, power_state);
+ if (ret && power_state == I2C_HID_PWR_ON) {
+ usleep_range(400, 500);
ret = i2c_hid_set_power_command(ihid, I2C_HID_PWR_ON);
-
- /* Device was already activated */
- if (!ret)
- goto set_pwr_exit;
}
- ret = i2c_hid_set_power_command(ihid, power_state);
if (ret)
dev_err(&ihid->client->dev,
"failed to change power setting.\n");
-set_pwr_exit:
-
/*
* The HID over I2C specification states that if a DEVICE needs time
* after the PWR_ON request, it should utilise CLOCK stretching.
--
2.44.0
^ permalink raw reply related
* Re: [PATCH v3 1/3] HID: i2c-hid: Rely on HID descriptor fetch to probe
From: Dmitry Torokhov @ 2024-04-27 3:21 UTC (permalink / raw)
To: Kenny Levinsen
Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki,
Lukasz Majczak
In-Reply-To: <20240426225739.2166-2-kl@kl.wtf>
Hi Kenny, Lukasz,
On Sat, Apr 27, 2024 at 12:47:07AM +0200, Kenny Levinsen wrote:
> To avoid error messages when a device is not present, b3a81b6c4fc6 added
> an initial bus probe using a dummy i2c_smbus_read_byte() call.
>
> Without this probe, i2c_hid_fetch_hid_descriptor() will fail internally
> on a bus error and log. Treat the bus error as a missing device and
> remove the error log so we can do away with the probe.
>
> Tested-by: Lukasz Majczak <lma@chromium.org>
> Reviewed-by: Lukasz Majczak <lma@chromium.org>
> Signed-off-by: Kenny Levinsen <kl@kl.wtf>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index d965382196c6..6ffa43d245b4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -872,12 +872,11 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> ihid->wHIDDescRegister,
> &ihid->hdesc,
> sizeof(ihid->hdesc));
> - if (error) {
> - dev_err(&ihid->client->dev,
> - "failed to fetch HID descriptor: %d\n",
> - error);
> - return -ENODEV;
> - }
> +
> + /* The i2c drivers are a bit inconsistent with their error
> + * codes, so treat everything as -ENXIO for now. */
> + if (error)
> + return -ENXIO;
I really think we should differentiate the cases "we do not know if
there is a device" vs "we do known that there is a device and we have
strong expectation of what that device is, and we do not expect
communication to fail".
Because of that I think we should have separate retry for the original
i2c_smbus_read_byte() (if you want you can make a wrapper around it,
something like i2c_hid_check_device_present()", and if there is a
concern that we will run into similar issue on resume (either from
suspend or from hibernate) then we can have similar retry in
i2c_hid_fetch_hid_descriptor().
But I do think that i2c_hid_fetch_hid_descriptor() should not try to
clobber the error but rather log the true one.
> }
>
> /* Validate the length of HID descriptor, the 4 first bytes:
> @@ -992,17 +991,9 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
> struct hid_device *hid = ihid->hid;
> int ret;
>
> - /* Make sure there is something at this address */
> - ret = i2c_smbus_read_byte(client);
> - if (ret < 0) {
> - i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
> - return -ENXIO;
> - }
> -
> ret = i2c_hid_fetch_hid_descriptor(ihid);
> if (ret < 0) {
> - dev_err(&client->dev,
> - "Failed to fetch the HID Descriptor\n");
> + i2c_hid_dbg(ihid, "failed to fetch HID descriptor: %d\n", ret);
> return ret;
> }
>
> --
> 2.44.0
>
>
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v2] Input: MT - limit max slots
From: Tetsuo Handa @ 2024-04-27 11:15 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg
Cc: syzkaller-bugs, syzbot, akpm, linux-input@vger.kernel.org
In-Reply-To: <03e8c3f0-bbbf-af37-6f52-67547cbd4cde@I-love.SAKURA.ne.jp>
syzbot is reporting too large allocation at input_mt_init_slots(), for
num_slots is supplied from userspace using ioctl(UI_DEV_CREATE).
Since nobody knows possible max slots, this patch chose 1024.
Reported-by: syzbot <syzbot+0122fa359a69694395d5@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v2:
Limit max slots instead of using __GFP_NOWARN.
drivers/input/input-mt.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index 14b53dac1253..6b04a674f832 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -46,6 +46,9 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
return 0;
if (mt)
return mt->num_slots != num_slots ? -EINVAL : 0;
+ /* Arbitrary limit for avoiding too large memory allocation. */
+ if (num_slots > 1024)
+ return -EINVAL;
mt = kzalloc(struct_size(mt, slots, num_slots), GFP_KERNEL);
if (!mt)
--
2.18.4
^ permalink raw reply related
* Re: [PATCH v3 1/3] HID: i2c-hid: Rely on HID descriptor fetch to probe
From: Kenny Levinsen @ 2024-04-27 13:20 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki,
Lukasz Majczak
In-Reply-To: <ZixvUNooESC02cJK@google.com>
On 4/27/24 5:21 AM, Dmitry Torokhov wrote:
> I really think we should differentiate the cases "we do not know if
> there is a device" vs "we do known that there is a device and we have
> strong expectation of what that device is, and we do not expect
> communication to fail".
My reasoning was that there is no difference between looking for address
acknowledge on a probe read vs. a real command. Unfortunately, I ran
into some issues with error code consistency that Doug highlighted...
Considering that the smbus probe bails on *any* error, it's really only
ACK'd address + NACK'd register that remains, and I thought it maybe
wouldn't be too harmful to just always have a debug log as suggested.
However, I would still like *more* good errors by being specific about
the error condition, and I plan to send some patches to get the number
of drivers sending ENXIO up so we can comfortably rely on it in a future
i2c-hid patch.
If you don't think it's acceptable to leave this as a pure debug print
for now, I'll send a patch with just a minor clean-up and Łukasz' delays
- then I'll try this again later when the driver situation has improved.
I've been rapid-firing revisions, so I'll await an ACK this time... :)
---
For some context, I started looking at the i2c-hid driver after a recent
regression around assumed Windows behavior, and found that the actual
behavior differed a lot from our assumptions. Windows gets the job done
notably quicker, with fewer messages and with shorter albeit differently
placed delays.
My plan is to send patches that clean up and aligns our traffic more,
speeding things up and hopefully deprecating some quirks. To that end, I
would also like to get rid of the dummy read once we're comfortable with it.
^ permalink raw reply
* [PATCH] Input: ff-core - prefer struct_size over open coded arithmetic
From: Erick Archer @ 2024-04-27 15:05 UTC (permalink / raw)
To: Dmitry Torokhov, Kees Cook, Gustavo A. R. Silva
Cc: Erick Archer, linux-input, linux-kernel, linux-hardening
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "ff" variable is a pointer to "struct ff_device" and this
structure ends in a flexible array:
struct ff_device {
[...]
struct file *effect_owners[] __counted_by(max_effects);
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() function.
The struct_size() helper returns SIZE_MAX on overflow. So, refactor
the comparison to take advantage of this.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer <erick.archer@outlook.com>
---
Hi,
The Coccinelle script used to detect this code pattern is the following:
virtual report
@rule1@
type t1;
type t2;
identifier i0;
identifier i1;
identifier i2;
identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
position p1;
@@
i0 = sizeof(t1) + sizeof(t2) * i1;
...
i2 = ALLOC@p1(..., i0, ...);
@script:python depends on report@
p1 << rule1.p1;
@@
msg = "WARNING: verify allocation on line %s" % (p1[0].line)
coccilib.report.print_report(p1[0],msg)
Regards,
Erick
---
drivers/input/ff-core.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index 16231fe080b0..609a5f01761b 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -9,8 +9,10 @@
/* #define DEBUG */
#include <linux/input.h>
+#include <linux/limits.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/overflow.h>
#include <linux/sched.h>
#include <linux/slab.h>
@@ -315,9 +317,8 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
return -EINVAL;
}
- ff_dev_size = sizeof(struct ff_device) +
- max_effects * sizeof(struct file *);
- if (ff_dev_size < max_effects) /* overflow */
+ ff_dev_size = struct_size(ff, effect_owners, max_effects);
+ if (ff_dev_size == SIZE_MAX) /* overflow */
return -EINVAL;
ff = kzalloc(ff_dev_size, GFP_KERNEL);
--
2.25.1
^ permalink raw reply related
* [PATCH] Input: try trimming too long modalias strings
From: Dmitry Torokhov @ 2024-04-28 2:06 UTC (permalink / raw)
To: linux-input
Cc: Jason Andryuk, Hans de Goede, Benjamin Tissoires, linux-kernel
If an input device declares too many capability bits then modalias
string for such device may become too long and not fit into uevent
buffer, resulting in failure of sending said uevent. This, in turn,
may prevent userspace from recognizing existence of such devices.
This is typically not a concern for real hardware devices as they have
limited number of keys, but happen with synthetic devices such as
ones created by xen-kbdfront driver, which creates devices as being
capable of delivering all possible keys, since it doesn't know what
keys the backend may produce.
To deal with such devices input core will attempt to trim key data,
in the hope that the rest of modalias string will fit in the given
buffer. When trimming key data it will indicate that it is not
complete by placing "+," sign, resulting in conversions like this:
old: k71,72,73,74,78,7A,7B,7C,7D,8E,9E,A4,AD,E0,E1,E4,F8,174,
new: k71,72,73,74,78,7A,7B,7C,+,
This should allow existing udev rules continue to work with existing
devices, and will also allow writing more complex rules that would
recognize trimmed modalias and check input device characteristics by
other means (for example by parsing KEY= data in uevent or parsing
input device sysfs attributes).
Reported-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/input.c | 85 +++++++++++++++++++++++++++++++++++--------
1 file changed, 70 insertions(+), 15 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index b04bcdeee557..947b514174e4 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1338,19 +1338,19 @@ static int input_print_modalias_bits(char *buf, int size,
char name, const unsigned long *bm,
unsigned int min_bit, unsigned int max_bit)
{
- int len = 0, i;
+ int bit = min_bit;
+ int len = 0;
len += snprintf(buf, max(size, 0), "%c", name);
- for (i = min_bit; i < max_bit; i++)
- if (bm[BIT_WORD(i)] & BIT_MASK(i))
- len += snprintf(buf + len, max(size - len, 0), "%X,", i);
+ for_each_set_bit_from(bit, bm, max_bit)
+ len += snprintf(buf + len, max(size - len, 0), "%X,", bit);
return len;
}
-static int input_print_modalias(char *buf, int size, const struct input_dev *id,
- int add_cr)
+static int input_print_modalias_parts(char *buf, int size, int full_len,
+ const struct input_dev *id)
{
- int len;
+ int len, klen, remainder, space;
len = snprintf(buf, max(size, 0),
"input:b%04Xv%04Xp%04Xe%04X-",
@@ -1359,8 +1359,48 @@ static int input_print_modalias(char *buf, int size, const struct input_dev *id,
len += input_print_modalias_bits(buf + len, size - len,
'e', id->evbit, 0, EV_MAX);
- len += input_print_modalias_bits(buf + len, size - len,
+
+ /*
+ * Calculate the remaining space in the buffer making sure we
+ * have place for the terminating 0.
+ */
+ space = max(size - (len + 1), 0);
+
+ klen = input_print_modalias_bits(buf + len, size - len,
'k', id->keybit, KEY_MIN_INTERESTING, KEY_MAX);
+ len += klen;
+
+ /*
+ * If we have more data than we can fit in the buffer, check
+ * if we can trim key data to fit in the rest. We will indicate
+ * that key data is incomplete by adding "+" sign at the end, like
+ * this: * "k1,2,3,45,+,".
+ *
+ * Note that we shortest key info (if present) is "k+," so we
+ * can only try to trim if key data is longer than that.
+ */
+ if (full_len && size < full_len + 1 && klen > 3) {
+ remainder = full_len - len;
+ /*
+ * We can only trim if we have space for the remainder
+ * and also for at least "k+," which is 3 more characters.
+ */
+ if (remainder <= space - 3) {
+ /*
+ * We are guaranteed to have 'k' in the buffer, so
+ * we need at least 3 additional bytes for storing
+ * "+," in addition to the remainder.
+ */
+ for (int i = size - 1 - remainder - 3; i >= 0; i--) {
+ if (buf[i] == 'k' || buf[i] == ',') {
+ strcpy(buf + i + 1, "+,");
+ len = i + 3; /* Not counting '\0' */
+ break;
+ }
+ }
+ }
+ }
+
len += input_print_modalias_bits(buf + len, size - len,
'r', id->relbit, 0, REL_MAX);
len += input_print_modalias_bits(buf + len, size - len,
@@ -1376,22 +1416,37 @@ static int input_print_modalias(char *buf, int size, const struct input_dev *id,
len += input_print_modalias_bits(buf + len, size - len,
'w', id->swbit, 0, SW_MAX);
- if (add_cr)
- len += snprintf(buf + len, max(size - len, 0), "\n");
-
return len;
}
+static int input_print_modalias(char *buf, int size, const struct input_dev *id)
+{
+ int full_len;
+
+ /*
+ * Printing is done in 2 passes: first one figures out total length
+ * needed for the modalias string, second one will try to trim key
+ * data in case when buffer is too small for the entire modalias.
+ * If the buffer is too small regardless, it will fill as much as it
+ * can (without trimming key data) into the buffer and leave it to
+ * the caller to figure out what to do with the result.
+ */
+ full_len = input_print_modalias_parts(NULL, 0, 0, id);
+ return input_print_modalias_parts(buf, size, full_len, id);
+}
+
static ssize_t input_dev_show_modalias(struct device *dev,
struct device_attribute *attr,
char *buf)
{
struct input_dev *id = to_input_dev(dev);
- ssize_t len;
+ size_t len;
- len = input_print_modalias(buf, PAGE_SIZE, id, 1);
+ len = input_print_modalias(buf, PAGE_SIZE, id);
+ if (len < PAGE_SIZE - 2)
+ len += snprintf(buf + len, PAGE_SIZE - len, "\n");
- return min_t(int, len, PAGE_SIZE);
+ return min(len, PAGE_SIZE);
}
static DEVICE_ATTR(modalias, S_IRUGO, input_dev_show_modalias, NULL);
@@ -1611,7 +1666,7 @@ static int input_add_uevent_modalias_var(struct kobj_uevent_env *env,
len = input_print_modalias(&env->buf[env->buflen - 1],
sizeof(env->buf) - env->buflen,
- dev, 0);
+ dev);
if (len >= (sizeof(env->buf) - env->buflen))
return -ENOMEM;
--
2.44.0.769.g3c40516874-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH v3] Input: xen-kbdfront - drop keys to shrink modalias
From: Dmitry Torokhov @ 2024-04-28 2:10 UTC (permalink / raw)
To: Jason Andryuk
Cc: linux-kernel, Phillip Susi, stable, Mattijs Korpershoek,
linux-input, xen-devel, Juergen Gross
In-Reply-To: <CAKf6xpu+8Uh263NqKm1qFkYG9VzHH-p4UZ=x+Fm+-SHR7J5=wQ@mail.gmail.com>
Hi Jason,
On Sun, Apr 21, 2024 at 07:57:24PM -0400, Jason Andryuk wrote:
> Hi Dmitry,
>
> On Thu, Mar 28, 2024 at 2:05 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Jason,
> >
> > On Wed, Mar 20, 2024 at 01:42:27PM -0400, Jason Andryuk wrote:
> > > Hi Dmitry,
> > >
> > > Do you have any feedback, or can you pick up this patch? It solves a
> > > real issue affecting udev, which crashes the Debian installer and
> > > breaks the mouse for Gnome.
> > >
> > > Or would you be okay if this patch went in via the Xen tree?
> >
> > I'd rather not. Could you please ping me in 2 weeks on this. I promise
> > we find a solution before the next release.
>
> It's been ~3 weeks. Any ideas?
>
> If you think this patch should be pursued in this form, I'd like to
> post a v4 that adds BTN_DPAD_{UP,DOWN,LEFT,RIGHT} on the off chance
> someone wants to use a controller. I dropped them initially since
> they are not keyboard keys, but button presses are delivered through
> the keyboard. Hence, they should be included.
I do not think suppressing random keys in the driver is a good idea,
because we may fill up what you currently consider as gaps, and
people will be confused why certain events are not being delivered.
I just posted a patch (you are CCed) that attempts to trim too long
modalias strings, please take a look and see if that solves your issue.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: try trimming too long modalias strings
From: Jason Andryuk @ 2024-04-28 23:43 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Hans de Goede, Benjamin Tissoires, linux-kernel
In-Reply-To: <Zi2vDUZuVAEh4-yS@google.com>
Hi Dmitry,
On Sat, Apr 27, 2024 at 10:06 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> If an input device declares too many capability bits then modalias
> string for such device may become too long and not fit into uevent
> buffer, resulting in failure of sending said uevent. This, in turn,
> may prevent userspace from recognizing existence of such devices.
>
> This is typically not a concern for real hardware devices as they have
> limited number of keys, but happen with synthetic devices such as
> ones created by xen-kbdfront driver, which creates devices as being
> capable of delivering all possible keys, since it doesn't know what
> keys the backend may produce.
>
> To deal with such devices input core will attempt to trim key data,
> in the hope that the rest of modalias string will fit in the given
> buffer. When trimming key data it will indicate that it is not
> complete by placing "+," sign, resulting in conversions like this:
>
> old: k71,72,73,74,78,7A,7B,7C,7D,8E,9E,A4,AD,E0,E1,E4,F8,174,
> new: k71,72,73,74,78,7A,7B,7C,+,
>
> This should allow existing udev rules continue to work with existing
> devices, and will also allow writing more complex rules that would
> recognize trimmed modalias and check input device characteristics by
> other means (for example by parsing KEY= data in uevent or parsing
> input device sysfs attributes).
I think adding these links may be useful for cross referencing:
[1] https://github.com/systemd/systemd/issues/22944
[2] https://lore.kernel.org/xen-devel/87o8dw52jc.fsf@vps.thesusis.net/T/
> Reported-by: Jason Andryuk <jandryuk@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thank you for looking at this. I think this idea of truncating the
modalias is better than just dropping keys.
cat-ing the individual sysfs files works, but there is still an issue:
# sudo udevadm trigger --action=change
[ 601.379977] ------------[ cut here ]------------
[ 601.395959] add_uevent_var: buffer size too small
[ 601.412009] WARNING: CPU: 0 PID: 630 at lib/kobject_uevent.c:671
add_uevent_var+0x11c/0x130
[ 601.440379] Modules linked in: xen_kbdfront xen_blkfront xen_netfront
[ 601.462078] CPU: 0 PID: 630 Comm: udevadm Tainted: G W
6.8.7+ #2
[ 601.486003] Hardware name: Xen HVM domU, BIOS 4.19-unstable 03/09/2024
[ 601.504867] RIP: 0010:add_uevent_var+0x11c/0x130
[ 601.527988] Code: 5b 41 5c 5d c3 cc cc cc cc 48 c7 c7 c0 3c 4d 9e
e8 49 4c 1c ff 0f 0b b8 f4 ff ff ff eb ce 48 c7 c7 e8 3c 4d 9e e8 34
4c 1c ff <0f> 0b eb e9 e8 eb e0 02 00 66 66 2e 0f 1f 84 00 00 00 00 00
90 90
[ 601.590038] RSP: 0018:ffffadc60053bcf0 EFLAGS: 00010282
[ 601.612133] RAX: 0000000000000000 RBX: ffff96f943c0a000 RCX: 0000000000000000
[ 601.634794] RDX: ffff96f9bd428cd0 RSI: ffff96f9bd41d740 RDI: ffff96f9bd41d740
[ 601.651867] RBP: ffffadc60053bd50 R08: 00000000ffffdfff R09: 0000000000000001
[ 601.677718] R10: 00000000ffffdfff R11: ffffffff9e65cc00 R12: 0000000000000003
[ 601.699194] R13: ffff96f943c0a000 R14: ffffffff9e0db1d0 R15: 0000000000000000
[ 601.718038] FS: 00007fa9a1084d40(0000) GS:ffff96f9bd400000(0000)
knlGS:0000000000000000
[ 601.741494] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 601.761050] CR2: 00007fff40d8bd58 CR3: 0000000002712001 CR4: 00000000000606f0
[ 601.783095] Call Trace:
[ 601.791569] <TASK>
[ 601.798207] ? add_uevent_var+0x11c/0x130
[ 601.810481] ? __warn+0x7c/0x130
[ 601.822437] ? add_uevent_var+0x11c/0x130
[ 601.833016] ? report_bug+0x171/0x1a0
[ 601.848035] ? handle_bug+0x3c/0x80
[ 601.858013] ? exc_invalid_op+0x17/0x70
[ 601.873026] ? asm_exc_invalid_op+0x1a/0x20
[ 601.888058] ? add_uevent_var+0x11c/0x130
[ 601.899042] kobject_uevent_env+0x28e/0x510
[ 601.916043] kobject_synth_uevent+0x326/0x330
[ 601.927373] uevent_store+0x19/0x50
[ 601.940381] kernfs_fop_write_iter+0x122/0x1b0
[ 601.952343] vfs_write+0x299/0x470
[ 601.961853] ksys_write+0x6a/0xf0
[ 601.975611] do_syscall_64+0x52/0x120
[ 601.985363] entry_SYSCALL_64_after_hwframe+0x78/0x80
[ 602.001942] RIP: 0033:0x7fa9a18693b4
[ 602.058258] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f
1f 80 00 00 00 00 48 8d 05 49 53 0d 00 8b 00 85 c0 75 13 b8 01 00 00
00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89
f5 53
[ 602.107044] RSP: 002b:00007ffc204b01e8 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[ 602.125072] RAX: ffffffffffffffda RBX: 0000000000000007 RCX: 00007fa9a18693b4
[ 602.144401] RDX: 0000000000000007 RSI: 00007ffc204b02d0 RDI: 0000000000000003
[ 602.161037] RBP: 00007ffc204b02d0 R08: 00005641874fbcd0 R09: 0000564187578700
[ 602.183535] R10: 0000000000000000 R11: 0000000000000246 R12: 00005641874fbbf0
[ 602.201557] R13: 0000000000000007 R14: 00007fa9a1935760 R15: 0000000000000007
[ 602.220506] </TASK>
[ 602.227408] ---[ end trace 0000000000000000 ]---
[ 602.239544] synth uevent: /devices/virtual/input/input5: failed to
send uevent
[ 602.260848] input input5: uevent: failed to send synthetic uevent: -12
Another path needs to truncate the buffer? Or the problem is that the
total uevent buffer size is what matters - not just the keys modalias?
My other thought is wondering whether the presence of '+' will cause
parsing errors? Has '+' been used before - or will it be an
unexpected character?
Thanks,
Jason
^ permalink raw reply
* [PATCH] Adding quirks for 2024 HP Spectre x360 touchpads
From: Jon Moeller @ 2024-04-29 7:09 UTC (permalink / raw)
To: linux-input; +Cc: Jon Moeller
---
drivers/hid/hid-multitouch.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 04a014cd2a2f..7a7918191628 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -212,6 +212,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
#define MT_CLS_GOOGLE 0x0111
#define MT_CLS_RAZER_BLADE_STEALTH 0x0112
#define MT_CLS_SMART_TECH 0x0113
+#define MT_CLS_HP_SPECTRE_ELAN_HAPTIC 0x0114
#define MT_DEFAULT_MAXCONTACT 10
#define MT_MAX_MAXCONTACT 250
@@ -396,6 +397,13 @@ static const struct mt_class mt_classes[] = {
MT_QUIRK_CONTACT_CNT_ACCURATE |
MT_QUIRK_SEPARATE_APP_REPORT,
},
+ { .name = MT_CLS_HP_SPECTRE_ELAN_HAPTIC,
+ .quirks = MT_QUIRK_ALWAYS_VALID |
+ MT_QUIRK_SLOT_IS_CONTACTID |
+ MT_QUIRK_CONTACT_CNT_ACCURATE |
+ MT_QUIRK_CONFIDENCE |
+ MT_QUIRK_WIN8_PTP_BUTTONS,
+ },
{ }
};
@@ -1992,6 +2000,12 @@ static const struct hid_device_id mt_devices[] = {
HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
USB_VENDOR_ID_ELAN, 0x3148) },
+ { .driver_data = MT_CLS_HP_SPECTRE_ELAN_HAPTIC,
+ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x32c8) },
+
+ { .driver_data = MT_CLS_HP_SPECTRE_ELAN_HAPTIC,
+ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x310a) },
+
/* Elitegroup panel */
{ .driver_data = MT_CLS_SERIAL,
MT_USB_DEVICE(USB_VENDOR_ID_ELITEGROUP,
--
2.44.0
Signed-off-by: Jon Moeller <jon@moeller.io>
^ permalink raw reply related
* [PATCH] HID: intel-ish-hid: ipc: Add check for pci_alloc_irq_vectors
From: Chen Ni @ 2024-04-29 8:54 UTC (permalink / raw)
To: srinivas.pandruvada, jikos, bentiss, even.xu, lixu.zhang,
kai.heng.feng, hongyan.song
Cc: linux-input, linux-kernel, Chen Ni
Add check for the return value of pci_alloc_irq_vectors() and return
the error if it fails in order to catch the error.
Fixes: 74fbc7d371d9 ("HID: intel-ish-hid: add MSI interrupt support")
Signed-off-by: Chen Ni <nichen@iscas.ac.cn>
---
drivers/hid/intel-ish-hid/ipc/pci-ish.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index e79d72f7db2a..9b9bc58f0524 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -174,6 +174,11 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
/* request and enable interrupt */
ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+ if (ret < 0) {
+ dev_err(dev, "ISH: Failed to allocate IRQ vectors\n");
+ return ret;
+ }
+
if (!pdev->msi_enabled && !pdev->msix_enabled)
irq_flag = IRQF_SHARED;
--
2.25.1
^ permalink raw reply related
* [syzbot] Monthly input report (Apr 2024)
From: syzbot @ 2024-04-29 12:34 UTC (permalink / raw)
To: linux-input, linux-kernel, syzkaller-bugs
Hello input maintainers/developers,
This is a 31-day syzbot report for the input subsystem.
All related reports/information can be found at:
https://syzkaller.appspot.com/upstream/s/input
During the period, 3 new issues were detected and 0 were fixed.
In total, 20 issues are still open and 52 have been fixed so far.
Some of the still happening issues:
Ref Crashes Repro Title
<1> 8456 Yes WARNING in input_mt_init_slots
https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5
<2> 952 Yes WARNING in implement
https://syzkaller.appspot.com/bug?extid=38e7237add3712479d65
<3> 553 No possible deadlock in evdev_pass_values (2)
https://syzkaller.appspot.com/bug?extid=13d3cb2a3dc61e6092f5
<4> 375 Yes INFO: task hung in uhid_char_release
https://syzkaller.appspot.com/bug?extid=8fe2d362af0e1cba8735
<5> 250 Yes WARNING in cm109_urb_irq_callback/usb_submit_urb
https://syzkaller.appspot.com/bug?extid=2d6d691af5ab4b7e66df
<6> 1 Yes WARNING in bcm5974_start_traffic/usb_submit_urb (2)
https://syzkaller.appspot.com/bug?extid=b064b5599f18f7ebb1e1
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
To disable reminders for individual bugs, reply with the following command:
#syz set <Ref> no-reminders
To change bug's subsystems, reply with:
#syz set <Ref> subsystems: new-subsystem
You may send multiple commands in a single email message.
^ permalink raw reply
* Re: [PATCH] Input: ff-core - prefer struct_size over open coded arithmetic
From: Kees Cook @ 2024-04-29 17:38 UTC (permalink / raw)
To: Erick Archer
Cc: Dmitry Torokhov, Gustavo A. R. Silva, linux-input, linux-kernel,
linux-hardening
In-Reply-To: <AS8PR02MB72371E646714BAE2E51A6A378B152@AS8PR02MB7237.eurprd02.prod.outlook.com>
On Sat, Apr 27, 2024 at 05:05:56PM +0200, Erick Archer wrote:
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1][2].
>
> As the "ff" variable is a pointer to "struct ff_device" and this
> structure ends in a flexible array:
>
> struct ff_device {
> [...]
> struct file *effect_owners[] __counted_by(max_effects);
> };
>
> the preferred way in the kernel is to use the struct_size() helper to
> do the arithmetic instead of the calculation "size + count * size" in
> the kzalloc() function.
>
> The struct_size() helper returns SIZE_MAX on overflow. So, refactor
> the comparison to take advantage of this.
>
> This way, the code is more readable and safer.
>
> This code was detected with the help of Coccinelle, and audited and
> modified manually.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
> Link: https://github.com/KSPP/linux/issues/160 [2]
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
> ---
> Hi,
>
> The Coccinelle script used to detect this code pattern is the following:
>
> virtual report
>
> @rule1@
> type t1;
> type t2;
> identifier i0;
> identifier i1;
> identifier i2;
> identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
> position p1;
> @@
>
> i0 = sizeof(t1) + sizeof(t2) * i1;
> ...
> i2 = ALLOC@p1(..., i0, ...);
>
> @script:python depends on report@
> p1 << rule1.p1;
> @@
>
> msg = "WARNING: verify allocation on line %s" % (p1[0].line)
> coccilib.report.print_report(p1[0],msg)
>
> Regards,
> Erick
> ---
> drivers/input/ff-core.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
> index 16231fe080b0..609a5f01761b 100644
> --- a/drivers/input/ff-core.c
> +++ b/drivers/input/ff-core.c
> @@ -9,8 +9,10 @@
> /* #define DEBUG */
>
> #include <linux/input.h>
> +#include <linux/limits.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/overflow.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
>
> @@ -315,9 +317,8 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects)
> return -EINVAL;
> }
>
> - ff_dev_size = sizeof(struct ff_device) +
> - max_effects * sizeof(struct file *);
> - if (ff_dev_size < max_effects) /* overflow */
> + ff_dev_size = struct_size(ff, effect_owners, max_effects);
> + if (ff_dev_size == SIZE_MAX) /* overflow */
> return -EINVAL;
>
> ff = kzalloc(ff_dev_size, GFP_KERNEL);
> --
> 2.25.1
>
Yup, thanks. This looks right to me.
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* [PATCH] HID: Add quirk for Logitech Casa touchpad
From: Sean O'Brien @ 2024-04-29 18:08 UTC (permalink / raw)
To: Benjamin Tissoires, linux-input
Cc: Sean O'Brien, Jiri Kosina, linux-kernel
This device sometimes doesn't send touch release signals when moving
from >=4 fingers to <4 fingers. Using MT_QUIRK_NOT_SEEN_MEANS_UP instead
of MT_QUIRK_ALWAYS_VALID makes sure that no touches become stuck.
MT_QUIRK_FORCE_MULTI_INPUT is not necessary for this device, but does no
harm.
Signed-off-by: Sean O'Brien <seobrien@chromium.org>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-multitouch.c | 6 ++++++
2 files changed, 7 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 8376fb5e2d0b4..68b0f39deaa9a 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -823,6 +823,7 @@
#define USB_DEVICE_ID_LOGITECH_AUDIOHUB 0x0a0e
#define USB_DEVICE_ID_LOGITECH_T651 0xb00c
#define USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD 0xb309
+#define USB_DEVICE_ID_LOGITECH_CASA_TOUCHPAD 0xbb00
#define USB_DEVICE_ID_LOGITECH_C007 0xc007
#define USB_DEVICE_ID_LOGITECH_C077 0xc077
#define USB_DEVICE_ID_LOGITECH_RECEIVER 0xc101
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 04a014cd2a2f6..56fc78841f245 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -2081,6 +2081,12 @@ static const struct hid_device_id mt_devices[] = {
USB_VENDOR_ID_LENOVO,
USB_DEVICE_ID_LENOVO_X12_TAB) },
+ /* Logitech devices */
+ { .driver_data = MT_CLS_NSMU,
+ HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_MULTITOUCH_WIN_8,
+ USB_VENDOR_ID_LOGITECH,
+ USB_DEVICE_ID_LOGITECH_CASA_TOUCHPAD) },
+
/* MosArt panels */
{ .driver_data = MT_CLS_CONFIDENCE_MINUS_ONE,
MT_USB_DEVICE(USB_VENDOR_ID_ASUS,
--
2.45.0.rc0.197.gbae5840b3b-goog
^ permalink raw reply related
* [PATCH v2] HID: Add quirk for Logitech Casa touchpad
From: Sean O'Brien @ 2024-04-29 18:20 UTC (permalink / raw)
To: Benjamin Tissoires, linux-input
Cc: Sean O'Brien, Jiri Kosina, linux-kernel
This device sometimes doesn't send touch release signals when moving
from >=4 fingers to <4 fingers. Using MT_QUIRK_NOT_SEEN_MEANS_UP instead
of MT_QUIRK_ALWAYS_VALID makes sure that no touches become stuck.
MT_QUIRK_FORCE_MULTI_INPUT is not necessary for this device, but does no
harm.
Signed-off-by: Sean O'Brien <seobrien@chromium.org>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-multitouch.c | 6 ++++++
2 files changed, 7 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 8376fb5e2d0b4..68b0f39deaa9a 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -823,6 +823,7 @@
#define USB_DEVICE_ID_LOGITECH_AUDIOHUB 0x0a0e
#define USB_DEVICE_ID_LOGITECH_T651 0xb00c
#define USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD 0xb309
+#define USB_DEVICE_ID_LOGITECH_CASA_TOUCHPAD 0xbb00
#define USB_DEVICE_ID_LOGITECH_C007 0xc007
#define USB_DEVICE_ID_LOGITECH_C077 0xc077
#define USB_DEVICE_ID_LOGITECH_RECEIVER 0xc101
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 04a014cd2a2f6..3df9e90693e3a 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -2081,6 +2081,12 @@ static const struct hid_device_id mt_devices[] = {
USB_VENDOR_ID_LENOVO,
USB_DEVICE_ID_LENOVO_X12_TAB) },
+ /* Logitech devices */
+ { .driver_data = MT_CLS_WIN_8_FORCE_MULTI_INPUT_NSMU,
+ HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_MULTITOUCH_WIN_8,
+ USB_VENDOR_ID_LOGITECH,
+ USB_DEVICE_ID_LOGITECH_CASA_TOUCHPAD) },
+
/* MosArt panels */
{ .driver_data = MT_CLS_CONFIDENCE_MINUS_ONE,
MT_USB_DEVICE(USB_VENDOR_ID_ASUS,
--
2.45.0.rc0.197.gbae5840b3b-goog
^ permalink raw reply related
* Re: [PATCH] Input: amimouse - Mark driver struct with __refdata to prevent section mismatch
From: Uwe Kleine-König @ 2024-04-29 20:29 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, kernel, linux-kbuild
In-Reply-To: <ln446wvlcruoglseztao7jwywzxiixyxnx3qxqnm7nmdan6bzt@klbghdi3wwir>
[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]
Hello Dmitry,
On Mon, Apr 15, 2024 at 04:34:53PM +0200, Uwe Kleine-König wrote:
> On Fri, Mar 29, 2024 at 10:54:38PM +0100, Uwe Kleine-König wrote:
> > As described in the added code comment, a reference to .exit.text is ok
> > for drivers registered via module_platform_driver_probe(). Make this
> > explicit to prevent the following section mismatch warning
> >
> > WARNING: modpost: drivers/input/mouse/amimouse: section mismatch in reference: amimouse_driver+0x8 (section: .data) -> amimouse_remove (section: .exit.text)
> >
> > that triggers on an allmodconfig W=1 build.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> I'd like to enable these warnings even for W=0 builds, so it would be
> great to get it into the main line soon.
>
> If you apply it, please notice that I fat-fingered the parameters to git
> send-email and it was sent in a thread. So (assuming you're using b4)
> you'd need:
>
> b4 am -P _ -v1 2e3783106bf6bd9a7bdeb12b706378fb16316471.1711748999.git.u.kleine-koenig@pengutronix.de
Do you have this patch still on your radar? I guess it's to late to get
it into v6.9 now, but do you plan to apply it for v6.10-rc1?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v2] Input: try trimming too long modalias strings
From: Dmitry Torokhov @ 2024-04-29 21:50 UTC (permalink / raw)
To: linux-input
Cc: Jason Andryuk, Hans de Goede, Benjamin Tissoires, Peter Hutterer,
linux-kernel
If an input device declares too many capability bits then modalias
string for such device may become too long and not fit into uevent
buffer, resulting in failure of sending said uevent. This, in turn,
may prevent userspace from recognizing existence of such devices.
This is typically not a concern for real hardware devices as they have
limited number of keys, but happen with synthetic devices such as
ones created by xen-kbdfront driver, which creates devices as being
capable of delivering all possible keys, since it doesn't know what
keys the backend may produce.
To deal with such devices input core will attempt to trim key data,
in the hope that the rest of modalias string will fit in the given
buffer. When trimming key data it will indicate that it is not
complete by placing "+," sign, resulting in conversions like this:
old: k71,72,73,74,78,7A,7B,7C,7D,8E,9E,A4,AD,E0,E1,E4,F8,174,
new: k71,72,73,74,78,7A,7B,7C,+,
This should allow existing udev rules continue to work with existing
devices, and will also allow writing more complex rules that would
recognize trimmed modalias and check input device characteristics by
other means (for example by parsing KEY= data in uevent or parsing
input device sysfs attributes).
Note that the driver core may try adding more uevent environment
variables once input core is done adding its own, so when forming
modalias we can not use the entire available buffer, so we reduce
it by somewhat an arbitrary amount (96 bytes).
Reported-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
v2: do not use entire available buffer when formatting modalias, leave
some space for driver core to add more data.
drivers/input/input.c | 108 +++++++++++++++++++++++++++++++++++-------
1 file changed, 91 insertions(+), 17 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index b04bcdeee557..045f4b62088a 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1338,19 +1338,19 @@ static int input_print_modalias_bits(char *buf, int size,
char name, const unsigned long *bm,
unsigned int min_bit, unsigned int max_bit)
{
- int len = 0, i;
+ int bit = min_bit;
+ int len = 0;
len += snprintf(buf, max(size, 0), "%c", name);
- for (i = min_bit; i < max_bit; i++)
- if (bm[BIT_WORD(i)] & BIT_MASK(i))
- len += snprintf(buf + len, max(size - len, 0), "%X,", i);
+ for_each_set_bit_from(bit, bm, max_bit)
+ len += snprintf(buf + len, max(size - len, 0), "%X,", bit);
return len;
}
-static int input_print_modalias(char *buf, int size, const struct input_dev *id,
- int add_cr)
+static int input_print_modalias_parts(char *buf, int size, int full_len,
+ const struct input_dev *id)
{
- int len;
+ int len, klen, remainder, space;
len = snprintf(buf, max(size, 0),
"input:b%04Xv%04Xp%04Xe%04X-",
@@ -1359,8 +1359,48 @@ static int input_print_modalias(char *buf, int size, const struct input_dev *id,
len += input_print_modalias_bits(buf + len, size - len,
'e', id->evbit, 0, EV_MAX);
- len += input_print_modalias_bits(buf + len, size - len,
+
+ /*
+ * Calculate the remaining space in the buffer making sure we
+ * have place for the terminating 0.
+ */
+ space = max(size - (len + 1), 0);
+
+ klen = input_print_modalias_bits(buf + len, size - len,
'k', id->keybit, KEY_MIN_INTERESTING, KEY_MAX);
+ len += klen;
+
+ /*
+ * If we have more data than we can fit in the buffer, check
+ * if we can trim key data to fit in the rest. We will indicate
+ * that key data is incomplete by adding "+" sign at the end, like
+ * this: * "k1,2,3,45,+,".
+ *
+ * Note that we shortest key info (if present) is "k+," so we
+ * can only try to trim if key data is longer than that.
+ */
+ if (full_len && size < full_len + 1 && klen > 3) {
+ remainder = full_len - len;
+ /*
+ * We can only trim if we have space for the remainder
+ * and also for at least "k+," which is 3 more characters.
+ */
+ if (remainder <= space - 3) {
+ /*
+ * We are guaranteed to have 'k' in the buffer, so
+ * we need at least 3 additional bytes for storing
+ * "+," in addition to the remainder.
+ */
+ for (int i = size - 1 - remainder - 3; i >= 0; i--) {
+ if (buf[i] == 'k' || buf[i] == ',') {
+ strcpy(buf + i + 1, "+,");
+ len = i + 3; /* Not counting '\0' */
+ break;
+ }
+ }
+ }
+ }
+
len += input_print_modalias_bits(buf + len, size - len,
'r', id->relbit, 0, REL_MAX);
len += input_print_modalias_bits(buf + len, size - len,
@@ -1376,22 +1416,37 @@ static int input_print_modalias(char *buf, int size, const struct input_dev *id,
len += input_print_modalias_bits(buf + len, size - len,
'w', id->swbit, 0, SW_MAX);
- if (add_cr)
- len += snprintf(buf + len, max(size - len, 0), "\n");
-
return len;
}
+static int input_print_modalias(char *buf, int size, const struct input_dev *id)
+{
+ int full_len;
+
+ /*
+ * Printing is done in 2 passes: first one figures out total length
+ * needed for the modalias string, second one will try to trim key
+ * data in case when buffer is too small for the entire modalias.
+ * If the buffer is too small regardless, it will fill as much as it
+ * can (without trimming key data) into the buffer and leave it to
+ * the caller to figure out what to do with the result.
+ */
+ full_len = input_print_modalias_parts(NULL, 0, 0, id);
+ return input_print_modalias_parts(buf, size, full_len, id);
+}
+
static ssize_t input_dev_show_modalias(struct device *dev,
struct device_attribute *attr,
char *buf)
{
struct input_dev *id = to_input_dev(dev);
- ssize_t len;
+ size_t len;
- len = input_print_modalias(buf, PAGE_SIZE, id, 1);
+ len = input_print_modalias(buf, PAGE_SIZE, id);
+ if (len < PAGE_SIZE - 2)
+ len += snprintf(buf + len, PAGE_SIZE - len, "\n");
- return min_t(int, len, PAGE_SIZE);
+ return min(len, PAGE_SIZE);
}
static DEVICE_ATTR(modalias, S_IRUGO, input_dev_show_modalias, NULL);
@@ -1601,6 +1656,23 @@ static int input_add_uevent_bm_var(struct kobj_uevent_env *env,
return 0;
}
+/*
+ * This is a pretty gross hack. When building uevent data the driver core
+ * may try adding more environment variables to kobj_uevent_env without
+ * telling us, so we have no idea how much of the buffer we can use to
+ * avoid overflows/-ENOMEM elsewhere. To work around this let's artificially
+ * reduce amount of memory we will use for the modalias environment variable.
+ *
+ * The potential additions are:
+ *
+ * SEQNUM=18446744073709551615 - (%llu - 28 bytes)
+ * HOME=/ (6 bytes)
+ * PATH=/sbin:/bin:/usr/sbin:/usr/bin (34 bytes)
+ *
+ * 68 bytes total. Allow extra buffer - 96 bytes
+ */
+#define UEVENT_ENV_EXTRA_LEN 96
+
static int input_add_uevent_modalias_var(struct kobj_uevent_env *env,
const struct input_dev *dev)
{
@@ -1610,9 +1682,11 @@ static int input_add_uevent_modalias_var(struct kobj_uevent_env *env,
return -ENOMEM;
len = input_print_modalias(&env->buf[env->buflen - 1],
- sizeof(env->buf) - env->buflen,
- dev, 0);
- if (len >= (sizeof(env->buf) - env->buflen))
+ (int)sizeof(env->buf) - env->buflen -
+ UEVENT_ENV_EXTRA_LEN,
+ dev);
+ if (len >= ((int)sizeof(env->buf) - env->buflen -
+ UEVENT_ENV_EXTRA_LEN))
return -ENOMEM;
env->buflen += len;
--
2.44.0.769.g3c40516874-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] Input: try trimming too long modalias strings
From: Dmitry Torokhov @ 2024-04-29 22:01 UTC (permalink / raw)
To: Jason Andryuk
Cc: linux-input, Hans de Goede, Benjamin Tissoires, linux-kernel,
Peter Hutterer
In-Reply-To: <CAKf6xpv76BO_n2VSAcbRfWowceXjiBSKHjx1nGakXzFHUiS6+Q@mail.gmail.com>
On Sun, Apr 28, 2024 at 07:43:52PM -0400, Jason Andryuk wrote:
> Hi Dmitry,
>
> On Sat, Apr 27, 2024 at 10:06 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > If an input device declares too many capability bits then modalias
> > string for such device may become too long and not fit into uevent
> > buffer, resulting in failure of sending said uevent. This, in turn,
> > may prevent userspace from recognizing existence of such devices.
> >
> > This is typically not a concern for real hardware devices as they have
> > limited number of keys, but happen with synthetic devices such as
> > ones created by xen-kbdfront driver, which creates devices as being
> > capable of delivering all possible keys, since it doesn't know what
> > keys the backend may produce.
> >
> > To deal with such devices input core will attempt to trim key data,
> > in the hope that the rest of modalias string will fit in the given
> > buffer. When trimming key data it will indicate that it is not
> > complete by placing "+," sign, resulting in conversions like this:
> >
> > old: k71,72,73,74,78,7A,7B,7C,7D,8E,9E,A4,AD,E0,E1,E4,F8,174,
> > new: k71,72,73,74,78,7A,7B,7C,+,
> >
> > This should allow existing udev rules continue to work with existing
> > devices, and will also allow writing more complex rules that would
> > recognize trimmed modalias and check input device characteristics by
> > other means (for example by parsing KEY= data in uevent or parsing
> > input device sysfs attributes).
>
> I think adding these links may be useful for cross referencing:
> [1] https://github.com/systemd/systemd/issues/22944
> [2] https://lore.kernel.org/xen-devel/87o8dw52jc.fsf@vps.thesusis.net/T/
>
> > Reported-by: Jason Andryuk <jandryuk@gmail.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Thank you for looking at this. I think this idea of truncating the
> modalias is better than just dropping keys.
>
> cat-ing the individual sysfs files works, but there is still an issue:
>
> # sudo udevadm trigger --action=change
> [ 601.379977] ------------[ cut here ]------------
> [ 601.395959] add_uevent_var: buffer size too small
> [ 601.412009] WARNING: CPU: 0 PID: 630 at lib/kobject_uevent.c:671
> add_uevent_var+0x11c/0x130
...
> Another path needs to truncate the buffer? Or the problem is that the
> total uevent buffer size is what matters - not just the keys modalias?
Yes, exactly right - the driver core may add more environment variables,
such as SEQNUM=, HOME=, PATH=.
I created v2 of the patch that tries to leave some space at the end for
these additional variables.
>
> My other thought is wondering whether the presence of '+' will cause
> parsing errors? Has '+' been used before - or will it be an
> unexpected character?
I hope not. It is after "," and not a hex digit, so old code parsing
keys hopefully considers it as a separate part of modalias. I.e. it was
coping with:
...-e0,1,2,4,k110,111,112,113,114,115,r0,1,8,...
so I believe it should cope with
...-e0,1,2,4,k110,111,112,113,114,115,+,r0,1,8,...
But let's see Peter and Benjamin will say...
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH 0/2] HID: i2c-hid: Unify device wake-up logic
From: Kenny Levinsen @ 2024-04-29 23:33 UTC (permalink / raw)
To: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki,
Lukasz Majczak
Cc: Kenny Levinsen
Third time was not the charm[0]. After Dmitry's comment[1], and after
looking some more at the I2C drivers, I have given up on removing the
smbus probe for now. We can always revisit this later if the situation
improves enough, but there are more important things to fix.
Instead, go all in on the address probe with retry and use it for both
initial probe and resume, replacing the previous retry on power on
commands. This gives us consistency and a single place to update and
document.
[0]: https://lore.kernel.org/all/20240426225739.2166-1-kl@kl.wtf/
[1]: https://lore.kernel.org/all/ZixvUNooESC02cJK@google.com/
^ permalink raw reply
* [PATCH 1/2] HID: i2c-hid: Retry address probe after delay
From: Kenny Levinsen @ 2024-04-29 23:33 UTC (permalink / raw)
To: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki,
Lukasz Majczak
Cc: Kenny Levinsen
In-Reply-To: <20240429233924.6453-1-kl@kl.wtf>
Some STM microcontrollers need 400µs after rising clock edge in order to
come out of their deep sleep state. This in turn means that our address
probe will fail as the device is not ready to service it.
Retry the probe once after a delay to see if the device came alive,
otherwise treat the device as missing.
Link: https://lore.kernel.org/all/20240405102436.3479210-1-lma@chromium.org/#t
Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
Co-developed-by: Lukasz Majczak <lma@chromium.org>
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index d965382196c6..a40489bb7643 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -163,6 +163,24 @@ static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
return quirks;
}
+static int i2c_hid_probe_address(struct i2c_hid *ihid)
+{
+ int ret;
+
+ /*
+ * Some STM-based devices need 400µs after a rising clock edge to wake
+ * from deep sleep, in which case the first read will fail. Try after a
+ * short sleep to see if the device came alive on the bus. Certain
+ * Weida Tech devices also need this.
+ */
+ ret = i2c_smbus_read_byte(ihid->client);
+ if (ret < 0) {
+ usleep_range(400, 500);
+ ret = i2c_smbus_read_byte(ihid->client);
+ }
+ return ret < 0 ? ret : 0;
+}
+
static int i2c_hid_xfer(struct i2c_hid *ihid,
u8 *send_buf, int send_len, u8 *recv_buf, int recv_len)
{
@@ -992,8 +1010,7 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
struct hid_device *hid = ihid->hid;
int ret;
- /* Make sure there is something at this address */
- ret = i2c_smbus_read_byte(client);
+ ret = i2c_hid_probe_address(ihid);
if (ret < 0) {
i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
return -ENXIO;
--
2.44.0
^ permalink raw reply related
* [PATCH 2/2] HID: i2c-hid: Use address probe to wake on resume
From: Kenny Levinsen @ 2024-04-29 23:33 UTC (permalink / raw)
To: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki,
Lukasz Majczak
Cc: Kenny Levinsen
In-Reply-To: <20240429233924.6453-1-kl@kl.wtf>
Certain devices, both from STM and Weida Tech, need to be woken up after
having entered a deeper sleep state. The relevant places to wake up such
device is during our initial HID probe, and after resuming.
A retry for power commands was previously added to i2c_hid_set_power to
wake up Weida Tech devices, but lacked sufficient sleep for STM devices.
Replace the power command retry with the same address probe we using
during our initial HID probe.
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index a40489bb7643..3e3885ae6ce2 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -402,19 +402,6 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
i2c_hid_dbg(ihid, "%s\n", __func__);
- /*
- * Some devices require to send a command to wakeup before power on.
- * The call will get a return value (EREMOTEIO) but device will be
- * triggered and activated. After that, it goes like a normal device.
- */
- if (power_state == I2C_HID_PWR_ON) {
- ret = i2c_hid_set_power_command(ihid, I2C_HID_PWR_ON);
-
- /* Device was already activated */
- if (!ret)
- goto set_pwr_exit;
- }
-
ret = i2c_hid_set_power_command(ihid, power_state);
if (ret)
dev_err(&ihid->client->dev,
@@ -977,6 +964,14 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid)
enable_irq(client->irq);
+ /* Make sure the device is awake on the bus */
+ ret = i2c_hid_probe_address(ihid);
+ if (ret < 0) {
+ dev_err(&client->dev, "nothing at address after resume: %d\n",
+ ret);
+ return -ENXIO;
+ }
+
/* Instead of resetting device, simply powers the device on. This
* solves "incomplete reports" on Raydium devices 2386:3118 and
* 2386:4B33 and fixes various SIS touchscreens no longer sending
--
2.44.0
^ permalink raw reply related
* [PATCH v2 0/7] regulator: new API for voltage reference supplies
From: David Lechner @ 2024-04-29 23:40 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Jean Delvare, Guenter Roeck,
Jonathan Cameron, Dmitry Torokhov
Cc: David Lechner, Jonathan Corbet, Support Opensource,
Cosmin Tanislav, Lars-Peter Clausen, Michael Hennerich,
Antoniu Miclaus, Greg Kroah-Hartman, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input,
Jonathan Cameron
In the IIO subsystem, we noticed a pattern in many drivers where we need
to get, enable and get the voltage of a supply that provides a reference
voltage. In these cases, we only need the voltage and not a handle to
the regulator. Another common pattern is for chips to have an internal
reference voltage that is used when an external reference is not
available. There are also a few drivers outside of IIO that do the same.
So we would like to propose a new regulator consumer API to handle these
specific cases to avoid repeating the same boilerplate code in multiple
drivers.
As an example of how these functions are used, I have included a few
patches to consumer drivers. But to avoid a giant patch bomb, I have
omitted the iio/adc and iio/dac patches I have prepared from this
series. I will send those separately but these will add 36 more users
of devm_regulator_get_enable_read_voltage() in addition to the 6 here.
In total, this will eliminate nearly 1000 lines of similar code and will
simplify writing and reviewing new drivers in the future.
---
Changes in v2:
- Reworked regulator patch to remove dev_err_probe() and properly unwind
- Combined two proposed functions into a single function
- Renamed function to devm_regulator_get_enable_read_voltage() everywhere
- Fixed other feedback on consumer patches (see individual patches)
- Picked up Jonathan's Reviewed-bys (hopefully changes were trivial enough)
- Link to v1: https://lore.kernel.org/r/20240327-regulator-get-enable-get-votlage-v1-0-5f4517faa059@baylibre.com
---
David Lechner (7):
regulator: devres: add API for reference voltage supplies
hwmon: (adc128d818) Use devm_regulator_get_enable_read_voltage()
hwmon: (da9052) Use devm_regulator_get_enable_read_voltage()
iio: addac: ad74115: Use devm_regulator_get_enable_read_voltage()
iio: frequency: admv1013: Use devm_regulator_get_enable_read_voltage()
staging: iio: impedance-analyzer: ad5933: Use devm_regulator_get_enable_read_voltage()
Input: mpr121: Use devm_regulator_get_enable_read_voltage()
Documentation/driver-api/driver-model/devres.rst | 1 +
drivers/hwmon/adc128d818.c | 57 +++++++----------------
drivers/hwmon/da9052-hwmon.c | 38 ++++-----------
drivers/iio/addac/ad74115.c | 40 ++++++----------
drivers/iio/frequency/admv1013.c | 40 ++++------------
drivers/input/keyboard/mpr121_touchkey.c | 45 ++----------------
drivers/regulator/devres.c | 59 ++++++++++++++++++++++++
drivers/staging/iio/impedance-analyzer/ad5933.c | 26 +----------
include/linux/regulator/consumer.h | 7 +++
9 files changed, 124 insertions(+), 189 deletions(-)
---
base-commit: 84c1815e46bdcb8bb0259db3d6cdd934cb70a0e9
change-id: 20240326-regulator-get-enable-get-votlage-5dedf40ff338
^ permalink raw reply
* [PATCH v2 1/7] regulator: devres: add API for reference voltage supplies
From: David Lechner @ 2024-04-29 23:40 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Jean Delvare, Guenter Roeck,
Jonathan Cameron, Dmitry Torokhov
Cc: David Lechner, Jonathan Corbet, Support Opensource,
Cosmin Tanislav, Lars-Peter Clausen, Michael Hennerich,
Antoniu Miclaus, Greg Kroah-Hartman, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <20240429-regulator-get-enable-get-votlage-v2-0-b1f11ab766c1@baylibre.com>
A common use case for regulators is to supply a reference voltage to an
analog input or output device. This adds a new devres API to get,
enable, and get the voltage in a single call. This allows eliminating
boilerplate code in drivers that use reference supplies in this way.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v2 changes:
* removed dev_err_probe() on error return
* dropped optional version of the function since there is no more
difference after dev_err_probe() is removed
* renamed function to devm_regulator_get_enable_read_voltage()
* added unwinding on error paths
---
Documentation/driver-api/driver-model/devres.rst | 1 +
drivers/regulator/devres.c | 59 ++++++++++++++++++++++++
include/linux/regulator/consumer.h | 7 +++
3 files changed, 67 insertions(+)
diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 7be8b8dd5f00..18caebad7376 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -433,6 +433,7 @@ REGULATOR
devm_regulator_bulk_put()
devm_regulator_get()
devm_regulator_get_enable()
+ devm_regulator_get_enable_read_voltage()
devm_regulator_get_enable_optional()
devm_regulator_get_exclusive()
devm_regulator_get_optional()
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 90bb0d178885..4f290b9b559b 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -145,6 +145,65 @@ struct regulator *devm_regulator_get_optional(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_regulator_get_optional);
+/**
+ * devm_regulator_get_enable_read_voltage - Resource managed regulator get and
+ * enable that returns the voltage
+ * @dev: device to supply
+ * @id: supply name or regulator ID.
+ *
+ * Get and enable regulator for duration of the device life-time.
+ * regulator_disable() and regulator_put() are automatically called on driver
+ * detach. See regulator_get_optional(), regulator_enable(), and
+ * regulator_get_voltage() for more information.
+ *
+ * This is a convenience function for supplies that provide a reference voltage
+ * where the consumer driver just needs to know the voltage and keep the
+ * regulator enabled.
+ *
+ * In cases where the supply is not strictly required, callers can check for
+ * -ENODEV error and handle it accordingly.
+ *
+ * Returns: voltage in microvolts on success, or an error code on failure.
+ */
+int devm_regulator_get_enable_read_voltage(struct device *dev, const char *id)
+{
+ struct regulator *r;
+ int ret;
+
+ /*
+ * Since we need a real voltage, we use devm_regulator_get_optional()
+ * rather than getting a dummy regulator with devm_regulator_get() and
+ * then letting regulator_get_voltage() fail with -EINVAL. This way, the
+ * caller can handle the -ENODEV error code if needed instead of the
+ * ambiguous -EINVAL.
+ */
+ r = devm_regulator_get_optional(dev, id);
+ if (IS_ERR(r))
+ return PTR_ERR(r);
+
+ ret = regulator_enable(r);
+ if (ret)
+ goto err_regulator_put;
+
+ ret = devm_add_action_or_reset(dev, regulator_action_disable, r);
+ if (ret)
+ goto err_regulator_put;
+
+ ret = regulator_get_voltage(r);
+ if (ret < 0)
+ goto err_release_action;
+
+ return 0;
+
+err_release_action:
+ devm_release_action(dev, regulator_action_disable, r);
+err_regulator_put:
+ devm_regulator_put(r);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_regulator_get_enable_read_voltage);
+
static int devm_regulator_match(struct device *dev, void *res, void *data)
{
struct regulator **r = res;
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index ed180ca419da..59d0b9a79e6e 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -164,6 +164,7 @@ struct regulator *__must_check devm_regulator_get_optional(struct device *dev,
const char *id);
int devm_regulator_get_enable(struct device *dev, const char *id);
int devm_regulator_get_enable_optional(struct device *dev, const char *id);
+int devm_regulator_get_enable_read_voltage(struct device *dev, const char *id);
void regulator_put(struct regulator *regulator);
void devm_regulator_put(struct regulator *regulator);
@@ -329,6 +330,12 @@ static inline int devm_regulator_get_enable_optional(struct device *dev,
return 0;
}
+static inline int devm_regulator_get_enable_read_voltage(struct device *dev,
+ const char *id)
+{
+ return -ENODEV;
+}
+
static inline struct regulator *__must_check
regulator_get_optional(struct device *dev, const char *id)
{
--
2.43.2
^ permalink raw reply related
* [PATCH v2 2/7] hwmon: (adc128d818) Use devm_regulator_get_enable_read_voltage()
From: David Lechner @ 2024-04-29 23:40 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Jean Delvare, Guenter Roeck,
Jonathan Cameron, Dmitry Torokhov
Cc: David Lechner, Jonathan Corbet, Support Opensource,
Cosmin Tanislav, Lars-Peter Clausen, Michael Hennerich,
Antoniu Miclaus, Greg Kroah-Hartman, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input,
Jonathan Cameron
In-Reply-To: <20240429-regulator-get-enable-get-votlage-v2-0-b1f11ab766c1@baylibre.com>
We can reduce boilerplate code and eliminate the driver remove()
function by using devm_regulator_get_enable_read_voltage().
A new external_vref flag is added since we no longer have the handle
to the regulator to check if it is present.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v2 changes:
* rename to devm_regulator_get_enable_read_voltage()
* use vref instead of err for return value
* simplify last error check to return PTR_ERR directly
---
drivers/hwmon/adc128d818.c | 57 ++++++++++++++--------------------------------
1 file changed, 17 insertions(+), 40 deletions(-)
diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index 46e3c8c50765..2a35acb011eb 100644
--- a/drivers/hwmon/adc128d818.c
+++ b/drivers/hwmon/adc128d818.c
@@ -58,7 +58,6 @@ static const u8 num_inputs[] = { 7, 8, 4, 6 };
struct adc128_data {
struct i2c_client *client;
- struct regulator *regulator;
int vref; /* Reference voltage in mV */
struct mutex update_lock;
u8 mode; /* Operation mode */
@@ -389,7 +388,7 @@ static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info)
return 0;
}
-static int adc128_init_client(struct adc128_data *data)
+static int adc128_init_client(struct adc128_data *data, bool external_vref)
{
struct i2c_client *client = data->client;
int err;
@@ -408,7 +407,7 @@ static int adc128_init_client(struct adc128_data *data)
regval |= data->mode << 1;
/* If external vref is selected, configure the chip to use it */
- if (data->regulator)
+ if (external_vref)
regval |= 0x01;
/* Write advanced configuration register */
@@ -430,9 +429,9 @@ static int adc128_init_client(struct adc128_data *data)
static int adc128_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
- struct regulator *regulator;
struct device *hwmon_dev;
struct adc128_data *data;
+ bool external_vref;
int err, vref;
data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL);
@@ -440,20 +439,15 @@ static int adc128_probe(struct i2c_client *client)
return -ENOMEM;
/* vref is optional. If specified, is used as chip reference voltage */
- regulator = devm_regulator_get_optional(dev, "vref");
- if (!IS_ERR(regulator)) {
- data->regulator = regulator;
- err = regulator_enable(regulator);
- if (err < 0)
- return err;
- vref = regulator_get_voltage(regulator);
- if (vref < 0) {
- err = vref;
- goto error;
- }
- data->vref = DIV_ROUND_CLOSEST(vref, 1000);
- } else {
+ vref = devm_regulator_get_enable_read_voltage(dev, "vref");
+ if (vref == -ENODEV) {
+ external_vref = false;
data->vref = 2560; /* 2.56V, in mV */
+ } else if (vref < 0) {
+ return vref;
+ } else {
+ external_vref = true;
+ data->vref = DIV_ROUND_CLOSEST(vref, 1000);
}
/* Operation mode is optional. If unspecified, keep current mode */
@@ -461,13 +455,12 @@ static int adc128_probe(struct i2c_client *client)
if (data->mode > 3) {
dev_err(dev, "invalid operation mode %d\n",
data->mode);
- err = -EINVAL;
- goto error;
+ return -EINVAL;
}
} else {
err = i2c_smbus_read_byte_data(client, ADC128_REG_CONFIG_ADV);
if (err < 0)
- goto error;
+ return err;
data->mode = (err >> 1) & ADC128_REG_MASK;
}
@@ -476,31 +469,16 @@ static int adc128_probe(struct i2c_client *client)
mutex_init(&data->update_lock);
/* Initialize the chip */
- err = adc128_init_client(data);
+ err = adc128_init_client(data, external_vref);
if (err < 0)
- goto error;
+ return err;
hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
data, adc128_groups);
- if (IS_ERR(hwmon_dev)) {
- err = PTR_ERR(hwmon_dev);
- goto error;
- }
+ if (IS_ERR(hwmon_dev))
+ return PTR_ERR(hwmon_dev);
return 0;
-
-error:
- if (data->regulator)
- regulator_disable(data->regulator);
- return err;
-}
-
-static void adc128_remove(struct i2c_client *client)
-{
- struct adc128_data *data = i2c_get_clientdata(client);
-
- if (data->regulator)
- regulator_disable(data->regulator);
}
static const struct i2c_device_id adc128_id[] = {
@@ -522,7 +500,6 @@ static struct i2c_driver adc128_driver = {
.of_match_table = of_match_ptr(adc128_of_match),
},
.probe = adc128_probe,
- .remove = adc128_remove,
.id_table = adc128_id,
.detect = adc128_detect,
.address_list = normal_i2c,
--
2.43.2
^ permalink raw reply related
* [PATCH v2 3/7] hwmon: (da9052) Use devm_regulator_get_enable_read_voltage()
From: David Lechner @ 2024-04-29 23:40 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Jean Delvare, Guenter Roeck,
Jonathan Cameron, Dmitry Torokhov
Cc: David Lechner, Jonathan Corbet, Support Opensource,
Cosmin Tanislav, Lars-Peter Clausen, Michael Hennerich,
Antoniu Miclaus, Greg Kroah-Hartman, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input,
Jonathan Cameron
In-Reply-To: <20240429-regulator-get-enable-get-votlage-v2-0-b1f11ab766c1@baylibre.com>
We can reduce boilerplate code by using
devm_regulator_get_enable_read_voltage().
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v2 changes:
* rename to devm_regulator_get_enable_read_voltage()
* add local variable tsiref_uv instead of using err
* restored error message via dev_err_probe()
* shortened pdev->dev to dev in lines we are touching anyway
---
drivers/hwmon/da9052-hwmon.c | 38 ++++++++++----------------------------
1 file changed, 10 insertions(+), 28 deletions(-)
diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
index 2bd7ae8100d7..7fb0c57dfef5 100644
--- a/drivers/hwmon/da9052-hwmon.c
+++ b/drivers/hwmon/da9052-hwmon.c
@@ -26,7 +26,6 @@ struct da9052_hwmon {
struct mutex hwmon_lock;
bool tsi_as_adc;
int tsiref_mv;
- struct regulator *tsiref;
struct completion tsidone;
};
@@ -397,7 +396,7 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct da9052_hwmon *hwmon;
struct device *hwmon_dev;
- int err;
+ int err, tsiref_uv;
hwmon = devm_kzalloc(dev, sizeof(struct da9052_hwmon), GFP_KERNEL);
if (!hwmon)
@@ -414,32 +413,20 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
device_property_read_bool(pdev->dev.parent, "dlg,tsi-as-adc");
if (hwmon->tsi_as_adc) {
- hwmon->tsiref = devm_regulator_get(pdev->dev.parent, "tsiref");
- if (IS_ERR(hwmon->tsiref)) {
- err = PTR_ERR(hwmon->tsiref);
- dev_err(&pdev->dev, "failed to get tsiref: %d", err);
- return err;
- }
-
- err = regulator_enable(hwmon->tsiref);
- if (err)
- return err;
-
- hwmon->tsiref_mv = regulator_get_voltage(hwmon->tsiref);
- if (hwmon->tsiref_mv < 0) {
- err = hwmon->tsiref_mv;
- goto exit_regulator;
- }
+ tsiref_uv = devm_regulator_get_enable_read_voltage(dev->parent,
+ "tsiref");
+ if (tsiref_uv < 0)
+ return dev_err_probe(dev, tsiref_uv,
+ "failed to get tsiref voltage\n");
/* convert from microvolt (DT) to millivolt (hwmon) */
- hwmon->tsiref_mv /= 1000;
+ hwmon->tsiref_mv = tsiref_uv / 1000;
/* TSIREF limits from datasheet */
if (hwmon->tsiref_mv < 1800 || hwmon->tsiref_mv > 2600) {
dev_err(hwmon->da9052->dev, "invalid TSIREF voltage: %d",
hwmon->tsiref_mv);
- err = -ENXIO;
- goto exit_regulator;
+ return -ENXIO;
}
/* disable touchscreen features */
@@ -456,7 +443,7 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
if (err) {
dev_err(&pdev->dev, "Failed to register TSIRDY IRQ: %d",
err);
- goto exit_regulator;
+ return err;
}
}
@@ -472,9 +459,6 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
exit_irq:
if (hwmon->tsi_as_adc)
da9052_free_irq(hwmon->da9052, DA9052_IRQ_TSIREADY, hwmon);
-exit_regulator:
- if (hwmon->tsiref)
- regulator_disable(hwmon->tsiref);
return err;
}
@@ -483,10 +467,8 @@ static void da9052_hwmon_remove(struct platform_device *pdev)
{
struct da9052_hwmon *hwmon = platform_get_drvdata(pdev);
- if (hwmon->tsi_as_adc) {
+ if (hwmon->tsi_as_adc)
da9052_free_irq(hwmon->da9052, DA9052_IRQ_TSIREADY, hwmon);
- regulator_disable(hwmon->tsiref);
- }
}
static struct platform_driver da9052_hwmon_driver = {
--
2.43.2
^ permalink raw reply related
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