* Re: [PATCH] HID: usbhid: Use seq_buf_putc() in three functions
From: sashiko-bot @ 2026-06-25 9:32 UTC (permalink / raw)
To: Markus Elfring; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <89e9c672-efee-4ed2-9ef5-637ec9707f0b@web.de>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Replacing `seq_buf_puts()` with `seq_buf_putc()` introduces a NUL-termination bug when the manufacturer string is exactly 127 bytes long, leading to an out-of-bounds read.
--
commit c41669030bdd8635988fc175a1caf4183a28344a
Author: Markus Elfring <elfring@users.sourceforge.net>
HID: usbhid: Use seq_buf_putc() in three functions
This patch updates three probe functions to use seq_buf_putc() instead
of seq_buf_puts() when inserting a single space character into sequence
buffers.
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1412,7 +1412,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>
> if (dev->product) {
> if (dev->manufacturer)
> - seq_buf_puts(&hid_name, " ");
> + seq_buf_putc(&hid_name, ' ');
[Severity: Medium]
Does this change introduce a missing null termination bug in usbhid_probe()
if the manufacturer string is exactly 127 bytes long?
The previous code using seq_buf_puts() requires two bytes of available
capacity (one for the space, one for the trailing null byte) and safely fails
if there isn't enough room.
Looking at seq_buf_putc(), it only checks if there is room for one byte.
If the sequence buffer is exactly 128 bytes long and already contains 127
characters from dev->manufacturer plus the null terminator, seq_buf_putc()
will overwrite the null terminator at index 127 with a space character.
This leaves hid->name completely unterminated. When exposed to userspace via
sysfs or input ioctls, this could lead to an out-of-bounds read into the
adjacent hid->phys memory field.
> seq_buf_puts(&hid_name, dev->product);
> }
>
> diff --git a/drivers/hid/usbhid/usbkbd.c b/drivers/hid/usbhid/usbkbd.c
> --- a/drivers/hid/usbhid/usbkbd.c
> +++ b/drivers/hid/usbhid/usbkbd.c
[ ... ]
> @@ -302,7 +302,7 @@ static int usb_kbd_probe(struct usb_interface *iface,
>
> if (dev->product) {
> if (dev->manufacturer)
> - seq_buf_puts(&kbd_name, " ");
> + seq_buf_putc(&kbd_name, ' ');
[Severity: Medium]
Does this have the same unterminated string issue for kbd->name in
usb_kbd_probe()?
If an untrusted USB device presents a manufacturer string of exactly 127
bytes, this overwrite could allow an out-of-bounds read into the adjacent
kbd->phys array.
> seq_buf_puts(&kbd_name, dev->product);
> }
>
> diff --git a/drivers/hid/usbhid/usbmouse.c b/drivers/hid/usbhid/usbmouse.c
> --- a/drivers/hid/usbhid/usbmouse.c
> +++ b/drivers/hid/usbhid/usbmouse.c
[ ... ]
> @@ -150,7 +150,7 @@ static int usb_mouse_probe(struct usb_interface *intf, const struct usb_device_i
>
> if (dev->product) {
> if (dev->manufacturer)
> - seq_buf_puts(&mouse_name, " ");
> + seq_buf_putc(&mouse_name, ' ');
[Severity: Medium]
Is mouse->name vulnerable to the same null terminator overwrite here in
usb_mouse_probe()?
Similarly, this could leak the contents of mouse->phys to userspace.
> seq_buf_puts(&mouse_name, dev->product);
> }
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/89e9c672-efee-4ed2-9ef5-637ec9707f0b@web.de?part=1
^ permalink raw reply
* Re: [PATCH] HID: usbhid: Use seq_buf_putc() in three functions
From: Markus Elfring @ 2026-06-25 9:55 UTC (permalink / raw)
To: sashiko-reviews, linux-input, linux-usb, Benjamin Tissoires,
Jiri Kosina, Mahad Ibrahim
Cc: Dmitry Torokhov, Woradorn Laodhanadhaworn, LKML, kernel-janitors
In-Reply-To: <20260625093209.34B6B1F000E9@smtp.kernel.org>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Replacing `seq_buf_puts()` with `seq_buf_putc()` introduces a NUL-termination bug when the manufacturer string is exactly 127 bytes long, leading to an out-of-bounds read.
These functions were used without error detection so far.
> --
>
> commit c41669030bdd8635988fc175a1caf4183a28344a
> Author: Markus Elfring <elfring@users.sourceforge.net>
>
> HID: usbhid: Use seq_buf_putc() in three functions
>
> This patch updates three probe functions to use seq_buf_putc() instead
> of seq_buf_puts() when inserting a single space character into sequence
> buffers.
>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1412,7 +1412,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>>
>> if (dev->product) {
>> if (dev->manufacturer)
>> - seq_buf_puts(&hid_name, " ");
>> + seq_buf_putc(&hid_name, ' ');
>
> [Severity: Medium]
> Does this change introduce a missing null termination bug in usbhid_probe()
> if the manufacturer string is exactly 127 bytes long?
>
> The previous code using seq_buf_puts() requires two bytes of available
> capacity (one for the space, one for the trailing null byte) and safely fails
> if there isn't enough room.
Can descriptions be extended for possible failure conditions?
https://elixir.bootlin.com/linux/v7.1.1/source/lib/seq_buf.c#L203-L221
> Looking at seq_buf_putc(), it only checks if there is room for one byte.
> If the sequence buffer is exactly 128 bytes long and already contains 127
> characters from dev->manufacturer plus the null terminator, seq_buf_putc()
> will overwrite the null terminator at index 127 with a space character.
>
> This leaves hid->name completely unterminated. When exposed to userspace via
> sysfs or input ioctls, this could lead to an out-of-bounds read into the
> adjacent hid->phys memory field.
>
>> seq_buf_puts(&hid_name, dev->product);
How likely is it that such product information will be appended as expected
at mentioned source code places?
Regards,
Markus
^ permalink raw reply
* Re: [PATCH v6 3/6 RESEND] dt-bindings: mfd: motorola-cpcap: convert to DT schema
From: sashiko-bot @ 2026-06-25 10:34 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: dmitry.torokhov, linux-input, lee, linux-leds, robh, conor+dt,
devicetree
In-Reply-To: <20260625081812.33474-4-clamor95@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The YAML schema introduces a `ports` wrapper node for the `audio-codec`, which conflicts with the original TXT binding and existing device trees that define `port@X` directly as children.
- [Medium] The schema explicitly expects the power button subnode to be named exactly `key-power`, but the in-tree device tree uses `button`.
- [Medium] The `spi-cs-high` property, which is physically required by the hardware and was explicitly listed as required in the legacy text binding, has been omitted from the `required` list in the YAML schema.
--
commit 30a40e2d4eb937fd9bbca306ed8246e061a54ab4
Author: Svyatoslav Ryhel <clamor95@gmail.com>
dt-bindings: mfd: motorola-cpcap: convert to DT schema
Convert devicetree bindings for the Motorola CPCAP MFD from TXT to YAML.
> diff --git a/Documentation/devicetree/bindings/mfd/motorola,cpcap.yaml b/Documentation/devicetree/bindings/mfd/motorola,cpcap.yaml
> new file mode 100644
> index 0000000000000..76705ea56805a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/motorola,cpcap.yaml
[ ... ]
> + audio-codec:
> + type: object
> + additionalProperties: false
> +
> + properties:
[ ... ]
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
[Severity: Medium]
Will this break validation for existing device trees?
The legacy text binding example shows port@0 and port@1 directly under the
audio-codec node, but this schema requires them to be wrapped in a ports
node. Because additionalProperties is set to false on audio-codec, direct
port@X nodes in existing device trees will be rejected.
[ ... ]
> + charger:
> + $ref: /schemas/power/supply/cpcap-charger.yaml#
> +
> + key-power:
> + $ref: /schemas/input/motorola,cpcap-pwrbutton.yaml#
[Severity: Medium]
Is it intentional to restrict the subnode name strictly to key-power?
Existing in-tree device trees use button for this node. By strictly matching
key-power, existing configurations will be rejected as an undocumented
property during schema validation.
[ ... ]
> +patternProperties:
> + "^led(-[a-z]+)?$":
> + $ref: /schemas/leds/motorola,cpcap-leds.yaml#
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - interrupt-controller
> + - "#interrupt-cells"
> + - spi-max-frequency
[Severity: Medium]
Does this schema incorrectly omit spi-cs-high from the required list?
The legacy text binding explicitly listed spi-cs-high as a required property.
Without it, incomplete device trees lacking this hardware-critical property
will incorrectly pass validation.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260625081812.33474-1-clamor95@gmail.com?part=3
^ permalink raw reply
* [PATCH v2] iio: temperature: hid-sensor-temperature: switch to non-devm iio_device_register()
From: Sanjay Chitroda @ 2026-06-25 12:16 UTC (permalink / raw)
To: jikos, jic23, srinivas.pandruvada
Cc: dlechner, nuno.sa, andy, hongyan.song, linux-input, linux-iio,
linux-kernel
From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
Avoid using devm_iio_device_register(), as this driver requires explicit
error handling and teardown ordering.
With devm_iio_device_register(), IIO device remains registered until the
devres cleanup phase. However, driver's remove() callback removes the
sensor hub callback and trigger support. This can create a race window
where IIO device is still visible and read_raw() requests are issued.
These requests might call sensor_hub_input_attr_get_raw_value(), which
waits up to 5 seconds for a response from the sensor hub callback that
has already been removed.
Add an explicit iio_device_unregister() call in the teardown path to
ensure deterministic cleanup, so that userspace can no longer access the
device once backend resources begin to be dismantled.
Fixes: 59d0f2da3569 ("iio: hid: Add temperature sensor support")
Cc: stable@vger.kernel.org
Reviewed-by: Maxwell Doose <m32285159@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
Changes in v2:
- Added review tags of Maxwell and Andy along with stable Cc
- Based on input from Srinivas and investigation use-after-free
explanation was not sufficiently justified so updated the commit
message with appropriate information and kept Fixes tag
- Link to v1: https://lore.kernel.org/all/20260622052135.1804135-1-sanjayembedded@gmail.com
---
drivers/iio/temperature/hid-sensor-temperature.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/temperature/hid-sensor-temperature.c b/drivers/iio/temperature/hid-sensor-temperature.c
index a8d3a15f9c53..ab6ec8f659b8 100644
--- a/drivers/iio/temperature/hid-sensor-temperature.c
+++ b/drivers/iio/temperature/hid-sensor-temperature.c
@@ -244,7 +244,7 @@ static int hid_temperature_probe(struct platform_device *pdev)
if (ret)
goto error_remove_trigger;
- ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
+ ret = iio_device_register(indio_dev);
if (ret)
goto error_remove_callback;
@@ -264,6 +264,7 @@ static void hid_temperature_remove(struct platform_device *pdev)
struct iio_dev *indio_dev = platform_get_drvdata(pdev);
struct temperature_state *temp_st = iio_priv(indio_dev);
+ iio_device_unregister(indio_dev);
sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TEMPERATURE);
hid_sensor_remove_trigger(indio_dev, &temp_st->common_attributes);
}
--
--
^ permalink raw reply related
* Re: [PATCH v2] iio: temperature: hid-sensor-temperature: switch to non-devm iio_device_register()
From: sashiko-bot @ 2026-06-25 12:39 UTC (permalink / raw)
To: Sanjay Chitroda; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260625121611.2003780-1-sanjayembedded@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The driver incorrectly uses a static global structure `temperature_callbacks` to hold per-device state, causing cross-device data corruption and potential Use-After-Free.
- [High] A race condition during device unbind can lead to a Use-After-Free or NULL pointer dereference because `sensor_hub_remove_callback` does not wait for concurrently executing events.
--
commit 0a06e7ba97cafc3e2c79740421bf8186cb0fc540
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>
iio: temperature: hid-sensor-temperature: switch to non-devm iio_device_register()
The patch switches from devm_iio_device_register() to iio_device_register()
to fix a teardown ordering issue. It adds an explicit iio_device_unregister()
in the remove function to ensure the device is not accessed after its backend
resources begin to be dismantled.
> diff --git a/drivers/iio/temperature/hid-sensor-temperature.c b/drivers/iio/temperature/hid-sensor-temperature.c
> index 9f628a8e5cfbe..34bff7e9f3a33 100644
> --- a/drivers/iio/temperature/hid-sensor-temperature.c
> +++ b/drivers/iio/temperature/hid-sensor-temperature.c
> @@ -244,7 +244,7 @@ static int hid_temperature_probe(struct platform_device *pdev)
> if (ret)
> goto error_remove_trigger;
>
> - ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> + ret = iio_device_register(indio_dev);
[Severity: High]
This is a pre-existing issue, but does this driver incorrectly use a static
global structure for per-device state?
Looking at hid_temperature_probe(), we overwrite the shared global pdev
pointer in the statically allocated temperature_callbacks structure:
drivers/iio/temperature/hid-sensor-temperature.c:hid_temperature_probe() {
...
temperature_callbacks.pdev = pdev;
ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_TEMPERATURE,
&temperature_callbacks);
...
}
If multiple temperature sensors are probed, the most recently probed device
will overwrite this shared pointer. If the last probed device is later
unbound, its driver data is cleared.
Subsequent events for other active devices will dereference the cleared pdev
in temperature_proc_event(), leading to a null pointer dereference or
use-after-free.
> if (ret)
> goto error_remove_callback;
>
> @@ -264,6 +264,7 @@ static void hid_temperature_remove(struct platform_device *pdev)
> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> struct temperature_state *temp_st = iio_priv(indio_dev);
>
> + iio_device_unregister(indio_dev);
> sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TEMPERATURE);
[Severity: High]
This isn't a bug introduced by this patch, but can a race condition during
device unbind lead to a use-after-free or null pointer dereference?
The sensor_hub_remove_callback() removes the callback from the sensor hub's
list, but does not synchronize with concurrently executing callback handlers
like sensor_hub_raw_event().
If an event from the sensor hub arrives exactly as the driver is unbinding,
the event handler might fetch the callback just before it is removed. When
hid_temperature_remove() completes, devres frees indio_dev, and the driver
core sets drvdata to NULL.
The concurrent event handler then executes temperature_capture_sample(),
calling platform_get_drvdata(pdev) which either returns NULL or a dangling
pointer to the freed indio_dev, causing a crash.
> hid_sensor_remove_trigger(indio_dev, &temp_st->common_attributes);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260625121611.2003780-1-sanjayembedded@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH] HID: usbhid: Use seq_buf_putc() in three functions
From: Markus Elfring @ 2026-06-25 16:55 UTC (permalink / raw)
To: sashiko-reviews, linux-input, linux-usb, Benjamin Tissoires,
Jiri Kosina, Mahad Ibrahim
Cc: Dmitry Torokhov, Steven Rostedt, Woradorn Laodhanadhaworn, LKML,
kernel-janitors
In-Reply-To: <20260625093209.34B6B1F000E9@smtp.kernel.org>
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1412,7 +1412,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>>
>> if (dev->product) {
>> if (dev->manufacturer)
>> - seq_buf_puts(&hid_name, " ");
>> + seq_buf_putc(&hid_name, ' ');
>
> [Severity: Medium]
> Does this change introduce a missing null termination bug in usbhid_probe()
> if the manufacturer string is exactly 127 bytes long?
>
> The previous code using seq_buf_puts() requires two bytes of available
> capacity (one for the space, one for the trailing null byte) and safely fails
> if there isn't enough room.
I see a need to reconsider such information in more detail.
> Looking at seq_buf_putc(), it only checks if there is room for one byte.
> If the sequence buffer is exactly 128 bytes long
This is the case as such a technical detail can be determined from the member “name”
of the data structure “hid_device”.
https://elixir.bootlin.com/linux/v7.1.1/source/include/linux/hid.h#L721
> and already contains 127
> characters from dev->manufacturer plus the null terminator, seq_buf_putc()
> will overwrite the null terminator at index 127 with a space character.
Other contributors can add further experiences for corresponding data length characteristics.
> This leaves hid->name completely unterminated.
I would like to point the comment “Don't count the trailing null byte against the capacity”
once more from the implementation of the function “seq_buf_puts”.
https://elixir.bootlin.com/linux/v7.1.1/source/lib/seq_buf.c#L193-L194
> When exposed to userspace via
> sysfs or input ioctls, this could lead to an out-of-bounds read into the
> adjacent hid->phys memory field.
If such a data reuse would be attempted, API requirements for sequence buffers
should probably be taken better into account at a concrete place.
Regards,
Markus
^ permalink raw reply
* Re: [PATCH v2 2/2] arm64: dts: qcom: sdm845-oneplus: Update compatible to include model
From: Dmitry Torokhov @ 2026-06-25 16:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David Heidelberg, Krzysztof Kozlowski, Konrad Dybcio, Rob Herring,
Conor Dooley, Jason A. Donenfeld, Matthias Schiffer,
Vincent Huang, Bjorn Andersson, Konrad Dybcio, linux-input,
devicetree, linux-kernel, linux-arm-msm, phone-devel
In-Reply-To: <f81e4d83-90d9-47c8-aee9-319df3f8b0fb@kernel.org>
Hi Krzysztof,
On Thu, Jun 25, 2026 at 10:23:54AM +0200, Krzysztof Kozlowski wrote:
> On 25/06/2026 06:53, Dmitry Torokhov wrote:
> > On Wed, Jun 24, 2026 at 04:37:25PM +0200, David Heidelberg wrote:
> >> On 24/06/2026 06:28, Dmitry Torokhov wrote:
> >>> Hi David,
> >>>
> >>> On Sun, Jun 21, 2026 at 07:11:45PM +0200, David Heidelberg wrote:
> >>>> On 28/05/2026 00:13, David Heidelberg wrote:
> >>>>> On 27/05/2026 23:56, Dmitry Torokhov wrote:
> >>>>>> Hi David,
> >>>>>>
> >>>>>> On Sat, May 23, 2026 at 11:45:35AM +0200, David Heidelberg via B4 Relay wrote:
> >>>>>>> From: David Heidelberg <david@ixit.cz>
> >>>>>>>
> >>>>>>> We know the driver is reporting s3706b, introduce the compatible so we
> >>>>>>> can more easily introduce quirks for weird touchscreen replacements in
> >>>>>>> followup series.
> >>>>>>>
> >>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>> Signed-off-by: David Heidelberg <david@ixit.cz>
> >>>>>>> ---
> >>>>>>> arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi | 2 +-
> >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> >>>>>>> b/arch/ arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> >>>>>>> index 6b7378cf4d493..148164d456a5a 100644
> >>>>>>> --- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> >>>>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> >>>>>>> @@ -475,17 +475,17 @@ bq27441_fg: bq27441-battery@55 {
> >>>>>>> };
> >>>>>>> };
> >>>>>>> &i2c12 {
> >>>>>>> status = "okay";
> >>>>>>> clock-frequency = <400000>;
> >>>>>>> synaptics-rmi4-i2c@20 {
> >>>>>>> - compatible = "syna,rmi4-i2c";
> >>>>>>> + compatible = "syna,rmi4-s3706b", "syna,rmi4-i2c";
> >>>>>>
> >>>>>> So I believe we established that this device (s3706b) does not in fact
> >>>>>> implement rmi4 protocol properly. Why do we have "syna,rmi4-i2c" as a
> >>>>>> fallback? Shouldn't it be just "syna,rmi4-s3706b"?
> >>>>>
> >>>>> The vendor supplies s3706b which does implement the RMI4 properly.
> >>>>>
> >>>>> The 3rd party replacement impersonating original parts may not implement
> >>>>> it properly, but I don't address this issue in this initial submission.
> >>>>>
> >>>>> With this compatible we know which original part is used by the vendor
> >>>>> and installed in the phones, so later we can deduct specific sequences
> >>>>> for the replacement aftermarket parts to keep phone touchscreen working
> >>>>> same as they do on Android without affecting other devices.
> >>>>
> >>>> Hello Dmitry.
> >>>>
> >>>> May I ask what is currently preventing this series from moving forward?
> >>>>
> >>>> The first version was posted in 2023 [1]. I picked it up again in 2025 [2]
> >>>> and am now on the 9th iteration (this patchset). At this point, the series
> >>>> has been under discussion for well over a year, with relatively little
> >>>> feedback and increasingly long gaps between review rounds.
> >>>>
> >>>> The current approach is based on the guidance I have received so far,
> >>>> including suggestions from the device-tree maintainers. When concerns were
> >>>> raised, I tried to address them and rework the series accordingly.
> >>>>
> >>>> What I am struggling with is understanding what specific issue still needs
> >>>> to be resolved before these patches can be accepted. If there are remaining
> >>>> requirements, objections to the approach, or technical concerns that I have
> >>>> not addressed, I would appreciate having them stated explicitly so I can
> >>>> work on them.
> >>>>
> >>>> I also split out the straightforward, self-contained changes in the hope
> >>>> that at least those could progress independently while I continued working
> >>>> on any follow-up requirements. However, even those patches do not appear to
> >>>> be moving forward.
> >>>>
> >>>> Could you please clarify what outcome you would like to see from this
> >>>> series, and what concrete changes would be required to get it accepted?
> >>>
> >>> I am still confused about how you want to differentiate between the full
> >>> RMI4 support vs the OnePlus flavor. The "syna,rmi4-s3706b", as you
> >>> mentioned, implements RMI4 protocol properly, so we do not need to
> >>> actually have it documented neither in binding nor in DTS.
> >>
> >> --- part 1 ---
> >>
> >> This series addresses identification within device-tree. It's normal
> >> recommended practice.
> >>
> >> If we know, the device ships specific, but **compliant** variant, we just
> >> put it as compatible = "more-specific", "less-specific"; in this case
> >> "syna,rmi4-s3706b", "syna,rmi4-i2c"
> >>
> >> This approach is used everywhere. This has nothing to do with after-market parts.
> >
> > We do this in many cases, sometimes when a part has different timings or
> > maybe additional functionality compared to the base model.
>
> Generic expectation is to have always dedicated front compatible for
> every device. rmi4-i2c is not really specific enough, more like a
> family, thus a specific device compatible is essential by the DT rules.
Essential in what way? What will break if such compatible is not there?
We have lived without it for many years and will continue live happily
without it for years to come.
We keep having this conversation each time there is self-describing
protocol that does not require knowledge of a specific part number:
i2c-hid, rmi4, spi-hid coming over soon.
We might need a device-specific compatible if we need to implement
particular power on sequence/adjust timings, and that's when it starts
making sense to introduce one.
>
> It does not matter if that specific compatible is ever used.
>
> >
> > How does this new compatible for controller that fully implements RMI4
> > protocol help here?
>
> It does not matter. This is a different device, thus it needs
> front-specific compatible.
Different from what?
$ git grep syna,rmi4 -- arch/ | wc -l
43
Do you have plans to list each and every chip currently covered by
syna,rmi4* ?
>
> Also, the commit msg actually did mention how this helps: allowing
> further quirks (I did not verify that in practice, but explanation is
> plausible).
Well, the devil is in the details. And that is what I am trying to
understand.
>
> >
> >>
> >> --- part 2 (irrelevant for this series) ---
> >>
> >>>
> >>> The issue you have with after-market parts that are not compliant and we
> >>> need to figure out how to deal with them. Inside the driver I
> >>
> >> As was suggested by device-tree folks, this is the first step, there isn't
> >> better one available. If there is, please suggest one, and I'll apply it.
> >
> > Was it clearly communicated to DT folks that the compatible you are
> > adding is fully compatible with the base "syna,rmi4-i2c" but other ones
> > will not be compatible?
>
> That was not communicated but also did not have to. You can install in
> your board whatever you wish, e.g. replacing foo device with bar being
> something completely different and incompatible. Does not matter really
> if this is after-market or a person just swapped things.
>
> DT does not solve that problem simply, because we describe static
> hardware configuration.
But the core issue that David is trying to solve is the fact that these
headsets do not work well with aftermarket parts with the upstream
kernels. It is not a theoretical problem for him, it is something that
he's been trying to solve for a while.
However from my POV I need to make sure the changes to the driver do not
affect or limit well-behaved devices implementing RMI4 protocol
properly.
>
> >
> >>
> >>> essentially need a"incomplete protocol" flag that we can use to
> >>> implement additional checks or skip known to be not implemented
> >>> functions/queries. In DT we could introduce something like
> >>> "oneplus,rmi4-i2c" that is decidedly not compatible with "syna,rmi4-i2c"
> >>> and neither one should be a fallback for the other.
> >>>
> >>> This of course needs buy-in from DT maintainers.
> >>
> >> As you can see, this still holds Acked-by and Reviewed-by from the relevant
> >> people - Krzysztof and Konrad.
> >
> > I see that but the commit does not explain how exactly you are planning
> > to deal with knockoffs.
>
> I think it does not have to. David does not need to solve all possible
> problems. He is solving his problems and not preventing the other
> problems to be solved in the future. Unless you expect that the second
> part - future problems - will be blocked by this code.
Well, as I mentioned above my understanding is that David really wants
to solve the issue with after market parts not working properly.
>
> But then I think no one ever solved in DT a problem of replacement of
> valid parts with after-market incompatible parts. Therefore I do not
> even know what solution I could suggest to David.
>
> >
> >>
> >>>
> >>> Does this make sense?
> >>
> >> For the scope we're discussing it doesn't seems so.
> >>
> >> This discussion should be associated with the last revision of the full
> >> series I sent 3 months ago. We're in very unflattering state, where:
> >>
> >> 2018 - these aftermarket touchscreen worked on Android well enough for
> >> people to have working touch (let's say with slightly worse experience then
> >> the original).
> >>
> >> 2026 in the mainline, we cannot even more forward and report to user-space
> >> there is aftermarket non-compliant piece of hardware installed.
> >>
> >> Actionable steps I suggest after this series lands:
> >>
> >> 1. don't do any changes, but since we know what 3rd party touchscreen do
> >> incorrectly deviating from the standard, REPORT it to the userspace, so USER
> >> know, their device (phone/tablet) doesn't have original part.
> >>
> >> 2. then figure out, IF we can reasonably well workaround it and HOW to do it
> >>
> >> These two steps present some progress which could be discussed and could
> >> lead us somewhere, what do you think?
> >
> > So since we know that these devices can come with controllers that do
> > not implement RMI4 fully, can we:
> >
> > 1. Establish a new compatible that is separate from syna,rmi4-i2c? As I
> > mentioned, it could be oneplus,rmi4-i2c or event a concrete controller
> > vendor,id combo. The point that it should be completely separate from
> > the current compatible and not use the current compatible as a fallback.
>
> Board/machine level compatible already gives you that.
> after-market-incompatible-parts is not a problem of this s3706b, if I
> understood correctly. It feels to me more of a problem of the board:
> people install into this board some incompatible replacement parts.
So is the suggestion to check if the board matches "oneplus,enchilada",
"oneplus,fajitas" with of_machine_compatible_match()?
>
> I would understand that this is not a board/machine level problem, if
> multiple different boards with s3706b were affected. Is this the case?
David?
Thanks.
--
Dmitry
^ permalink raw reply
page: | 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