Linux Input/HID development
 help / color / mirror / Atom feed
* Re: [PATCH v2] dt-bindings: input: touchscreen: goodix: clarify irq-gpios misleading text
From: Rob Herring @ 2023-09-25 16:25 UTC (permalink / raw)
  To: Luca Ceresoli
  Cc: linux-input, linux-kernel, Dmitry Torokhov, Rob Herring,
	Krzysztof Kozlowski, Thomas Petazzoni, Jeff LaBundy, devicetree,
	Conor Dooley
In-Reply-To: <20230925032208.11698-1-luca.ceresoli@bootlin.com>


On Mon, 25 Sep 2023 05:22:08 +0200, Luca Ceresoli wrote:
> The irq-gpios description misleading, apparently saying that driving the
> IRQ GPIO resets the device, which is even more puzzling as there is a reset
> GPIO as well.
> 
> In reality the IRQ pin can be driven during the reset sequence to configure
> the client address, as it becomes clear after checking both the datasheet
> and the driver code. Improve the text to clarify that.
> 
> Also rephrase to remove reference to the driver, which is not appropriate
> in the bindings.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changed in v2:
>  - reworded to clarify even further
> ---
>  .../devicetree/bindings/input/touchscreen/goodix.yaml        | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>


^ permalink raw reply

* [PATCH] Input: xpad - add PXN V900 support
From: Matthias Berndt @ 2023-09-25 16:05 UTC (permalink / raw)
  To: linux-input, Ismael Ferreras Morezuelas

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

Hi Ismael, Hi linux-input,

I recently got my hands on a used PXN V900 steering wheel which didn't work 
out of the box on Linux. I've attached a patch that makes it work for me, 
please consider merging it.

All the best,
Matthias

[-- Attachment #2: 0001-Input-xpad-add-PXN-V900-support.patch --]
[-- Type: text/x-patch, Size: 1435 bytes --]

From 9b0af40bc3c064be1c7c5ba36d7fb4b8d6535fc7 Mon Sep 17 00:00:00 2001
From: Matthias Berndt <matthias_berndt@gmx.de>
Date: Mon, 25 Sep 2023 17:54:13 +0200
Subject: [PATCH] Input: xpad - add PXN V900 support

Add VID and PID to the xpad_device table to allow driver
to use the PXN V900 steering wheel, which is
XTYPE_XBOX360 compatible in xinput mode.
---
 drivers/input/joystick/xpad.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index ede380551e55..478bf657efc2 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -272,6 +272,7 @@ static const struct xpad_device {
 	{ 0x1038, 0x1430, "SteelSeries Stratus Duo", 0, XTYPE_XBOX360 },
 	{ 0x1038, 0x1431, "SteelSeries Stratus Duo", 0, XTYPE_XBOX360 },
 	{ 0x11c9, 0x55f0, "Nacon GC-100XF", 0, XTYPE_XBOX360 },
+	{ 0x11ff, 0x0511, "PXN V900", 0, XTYPE_XBOX360 },
 	{ 0x1209, 0x2882, "Ardwiino Controller", 0, XTYPE_XBOX360 },
 	{ 0x12ab, 0x0004, "Honey Bee Xbox360 dancepad", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360 },
 	{ 0x12ab, 0x0301, "PDP AFTERGLOW AX.1", 0, XTYPE_XBOX360 },
@@ -503,6 +504,7 @@ static const struct usb_device_id xpad_table[] = {
 	XPAD_XBOX360_VENDOR(0x3285),		/* Nacon GC-100 */
 	XPAD_XBOX360_VENDOR(0x3537),		/* GameSir Controllers */
 	XPAD_XBOXONE_VENDOR(0x3537),		/* GameSir Controllers */
+	XPAD_XBOX360_VENDOR(0x11ff),		/* PXN V900 */
 	{ }
 };
 
-- 
2.41.0


^ permalink raw reply related

* [PATCH] Input: xpad - add PXN V900 support
From: Matthias Berndt @ 2023-09-25 16:09 UTC (permalink / raw)
  To: linux-input, Ismael Ferreras Morezuelas

[-- Attachment #1: Type: text/plain, Size: 238 bytes --]

Hi Ismael, Hi linux-input,

I recently got my hands on a used PXN V900 steering wheel which didn't work
out of the box on Linux. I've attached a patch that makes it work for me,
please consider merging it.

All the best,
Matthias

[-- Attachment #2: 0001-Input-xpad-add-PXN-V900-support.patch --]
[-- Type: text/x-patch, Size: 1468 bytes --]

From 9b0af40bc3c064be1c7c5ba36d7fb4b8d6535fc7 Mon Sep 17 00:00:00 2001
From: Matthias Berndt <matthias_berndt@gmx.de>
Date: Mon, 25 Sep 2023 17:54:13 +0200
Subject: [PATCH] Input: xpad - add PXN V900 support

Add VID and PID to the xpad_device table to allow driver
to use the PXN V900 steering wheel, which is
XTYPE_XBOX360 compatible in xinput mode.
---
 drivers/input/joystick/xpad.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index ede380551e55..478bf657efc2 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -272,6 +272,7 @@ static const struct xpad_device {
 	{ 0x1038, 0x1430, "SteelSeries Stratus Duo", 0, XTYPE_XBOX360 },
 	{ 0x1038, 0x1431, "SteelSeries Stratus Duo", 0, XTYPE_XBOX360 },
 	{ 0x11c9, 0x55f0, "Nacon GC-100XF", 0, XTYPE_XBOX360 },
+	{ 0x11ff, 0x0511, "PXN V900", 0, XTYPE_XBOX360 },
 	{ 0x1209, 0x2882, "Ardwiino Controller", 0, XTYPE_XBOX360 },
 	{ 0x12ab, 0x0004, "Honey Bee Xbox360 dancepad", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360 },
 	{ 0x12ab, 0x0301, "PDP AFTERGLOW AX.1", 0, XTYPE_XBOX360 },
@@ -503,6 +504,7 @@ static const struct usb_device_id xpad_table[] = {
 	XPAD_XBOX360_VENDOR(0x3285),		/* Nacon GC-100 */
 	XPAD_XBOX360_VENDOR(0x3537),		/* GameSir Controllers */
 	XPAD_XBOXONE_VENDOR(0x3537),		/* GameSir Controllers */
+	XPAD_XBOX360_VENDOR(0x11ff),		/* PXN V900 */
 	{ }
 };

--
2.41.0


^ permalink raw reply related

* Re: [PATCH v10 1/4] Input: Add driver for Cypress Generation 5 touchscreen
From: James Hilliard @ 2023-09-25 11:05 UTC (permalink / raw)
  To: Alistair Francis
  Cc: devicetree, linux-kernel, linux-arm-kernel, linux-input,
	krzysztof.kozlowski+dt, linus.walleij, robh+dt, dmitry.torokhov,
	shawnguo, rydberg, alistair23, s.hauer, andreas, Maxime Ripard,
	Mylène Josserand, Peter Geis
In-Reply-To: <20221026114908.191472-2-alistair@alistair23.me>

On Wed, Oct 26, 2022 at 6:05 AM Alistair Francis <alistair@alistair23.me> wrote:
>
> This is the basic driver for the Cypress TrueTouch Gen5 touchscreen
> controllers. This driver supports only the I2C bus but it uses regmap
> so SPI support could be added later.
> The touchscreen can retrieve some defined zone that are handled as
> buttons (according to the hardware). That is why it handles
> button and multitouch events.
>
> Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com>
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> Tested-by: Andreas Kemnade <andreas@kemnade.info> # Kobo Clara HD
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> ---
>  drivers/input/touchscreen/Kconfig   |  16 +
>  drivers/input/touchscreen/Makefile  |   1 +
>  drivers/input/touchscreen/cyttsp5.c | 902 ++++++++++++++++++++++++++++
>  3 files changed, 919 insertions(+)
>  create mode 100644 drivers/input/touchscreen/cyttsp5.c
>
> +
> +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
> +{
> +       int rc;
> +       u8 cmd[HID_OUTPUT_BL_LAUNCH_APP];
> +       u16 crc;
> +
> +       put_unaligned_le16(HID_OUTPUT_BL_LAUNCH_APP_SIZE, cmd);
> +       cmd[2] = HID_BL_OUTPUT_REPORT_ID;
> +       cmd[3] = 0x0; /* Reserved */
> +       cmd[4] = HID_OUTPUT_BL_SOP;
> +       cmd[5] = HID_OUTPUT_BL_LAUNCH_APP;
> +       put_unaligned_le16(0x00, &cmd[6]);
> +       crc = crc_itu_t(0xFFFF, &cmd[4], 4);
> +       put_unaligned_le16(crc, &cmd[8]);
> +       cmd[10] = HID_OUTPUT_BL_EOP;
> +
> +       rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
> +                          HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> +       if (rc) {
> +               dev_err(ts->dev, "Failed to write command %d", rc);
> +               return rc;
> +       }
> +
> +       rc = wait_for_completion_interruptible_timeout(&ts->cmd_done,
> +                                               msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT_MS));
> +       if (rc <= 0) {
> +               dev_err(ts->dev, "HID output cmd execution timed out\n");
> +               rc = -ETIMEDOUT;
> +               return rc;
> +       }

I've been seeing this timeout error somewhat randomly on a Variscite i.MX6
QUAD/DUAL VAR-SOM-MX6 Custom Board based device at startup:

[    2.234089] cyttsp5 2-0024: HID output cmd execution timed out
[    2.239957] cyttsp5 2-0024: Error on launch app r=-110
[    2.245150] cyttsp5 2-0024: Fail initial startup r=-110
[    2.257502] cyttsp5: probe of 2-0024 failed with error -110

When it doesn't error I just see this:

[    2.061176] input: cyttsp5 as
/devices/platform/soc/2100000.bus/21a8000.i2c/i2c-2/2-0024/input/input0

I'm not sure if this is a driver issue or potentially a device tree issue, the
upstream kernel device tree node looks like this:

touchscreen@24 {
    compatible = "cypress,tt21000";
    reg = <0x24>;
    interrupt-parent = <&gpio3>;
    interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
    reset-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
    vdd-supply = <&reg_3p3v>;
    touchscreen-size-x = <880>;
    touchscreen-size-y = <1280>;
};

Full device tree here:
https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/tree/arch/arm/boot/dts/nxp/imx/imx6q-var-mx6customboard.dts?h=for-next

The original vendor device tree node for the downstream kernel looked like this:
tsc@0x24 {
    compatible = "cy,cyttsp5_i2c_adapter";
    reg = <0x24>;
    interrupts = <0x07 0x02>;
    interrupt-parent = <0x07>;
    cy,adapter_id = "cyttsp5_i2c_adapter";

    cy,core {
        cy,name = "cyttsp5_core";

        cy,irq_gpio = <71>;
        cy,rst_gpio = <141>;
        cy,hid_desc_register = <0x01>;
        /* CY_CORE_FLAG_RESTORE_PARAMETERS */
        cy,flags = <4>;
        /* CY_CORE_EWG_NONE */
        cy,easy_wakeup_gesture = <0>;
        cy,btn_keys-tag = <0>;

        cy,mt {
            cy,name = "cyttsp5_mt";

            cy,inp_dev_name = "cyttsp5_mt";
            /* CY_MT_FLAG_NONE */
            cy,flags = <0x00>;
            cy,abs =
                /* ABS_MT_POSITION_X, CY_ABS_MIN_X, CY_ABS_MAX_X, 0, 0 */
                <0x35 0 880 0 0
                /* ABS_MT_POSITION_Y, CY_ABS_MIN_Y, CY_ABS_MAX_Y, 0, 0 */
                0x36 0 1280 0 0
                /* ABS_MT_PRESSURE, CY_ABS_MIN_P, CY_ABS_MAX_P, 0, 0 */
                0x3a 0 255 0 0
                /* CY_IGNORE_VALUE, CY_ABS_MIN_W, CY_ABS_MAX_W, 0, 0 */
                0xffff 0 255 0 0
                /* ABS_MT_TRACKING_ID, CY_ABS_MIN_T, CY_ABS_MAX_T, 0, 0 */
                0x39 0 15 0 0
                /* ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0 */
                0x30 0 255 0 0
                /* ABS_MT_TOUCH_MINOR, 0, 255, 0, 0 */
                0x31 0 255 0 0
                /* ABS_MT_ORIENTATION, -127, 127, 0, 0 */
                0x34 0xffffff81 127 0 0
                /* ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0 */
                0x37 0 1 0 0
                /* ABS_MT_DISTANCE, 0, 255, 0, 0 */
                0x3b 0 255 0 0>;
        };
    };
};

^ permalink raw reply

* [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd()
From: Martin Kepplinger @ 2023-09-25 10:23 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, jm, linux-kernel
  Cc: linux-input, Martin Kepplinger
In-Reply-To: <20230925102302.13094-1-martink@posteo.de>

These custom commands will be sent to both the USB keyboard & mouse
devices but only the mouse will respond. Avoid sending known-useless
messages by always prepending the filter before sending them.

Suggested-by: Jamie Lentin <jm@lentin.co.uk>
Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/hid/hid-lenovo.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 29aa6d372bad..922f3e5462f4 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -521,6 +521,14 @@ static void lenovo_features_set_cptkbd(struct hid_device *hdev)
 	int ret;
 	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
 
+	/* All the custom action happens on the USBMOUSE device for USB */
+	if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
+	    (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
+	    hdev->type != HID_TYPE_USBMOUSE) {
+		hid_dbg(hdev, "Ignoring keyboard half of device\n");
+		return;
+	}
+
 	/*
 	 * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
 	 * regular keys
@@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct hid_device *hdev)
 	int ret;
 	struct lenovo_drvdata *cptkbd_data;
 
-	/* All the custom action happens on the USBMOUSE device for USB */
-	if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
-	    (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
-	    hdev->type != HID_TYPE_USBMOUSE) {
-		hid_dbg(hdev, "Ignoring keyboard half of device\n");
-		return 0;
-	}
-
 	cptkbd_data = devm_kzalloc(&hdev->dev,
 					sizeof(*cptkbd_data),
 					GFP_KERNEL);
@@ -1264,16 +1264,7 @@ static int lenovo_probe(struct hid_device *hdev,
 #ifdef CONFIG_PM
 static int lenovo_reset_resume(struct hid_device *hdev)
 {
-	switch (hdev->product) {
-	case USB_DEVICE_ID_LENOVO_CUSBKBD:
-		if (hdev->type == HID_TYPE_USBMOUSE) {
-			lenovo_features_set_cptkbd(hdev);
-		}
-
-		break;
-	default:
-		break;
-	}
+	lenovo_features_set_cptkbd(hdev);
 
 	return 0;
 }
-- 
2.39.2


^ permalink raw reply related

* [RFC PATCH 1/2] hid: lenovo: Resend all settings on reset_resume for compact keyboards
From: Martin Kepplinger @ 2023-09-25 10:23 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, jm, linux-kernel
  Cc: linux-input, Martin Kepplinger
In-Reply-To: <140b721bc345a846863a37ebf17c3174@lentin.co.uk>

From: Jamie Lentin <jm@lentin.co.uk>

The USB Compact Keyboard variant requires a reset_resume function to
restore keyboard configuration after a suspend in some situations. Move
configuration normally done on probe to lenovo_features_set_cptkbd(), then
recycle this for use on reset_resume.

Without, the keyboard and driver would end up in an inconsistent state,
breaking middle-button scrolling amongst other problems, and twiddling
sysfs values wouldn't help as the middle-button mode won't be set until
the driver is reloaded.

Tested on a USB and Bluetooth Thinkpad Compact Keyboard.

Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for compact keyboards")
Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
Signed-off-by: Martin Kepplinger <martink@posteo.de>
---

hi Jamie,

thanks for sharing your patch! This works equally well for me. Is the
2nd patch what you had in mind? If so, let's create a new squashed non-RFC
patch.

thanks again,

                         martin


 drivers/hid/hid-lenovo.c | 50 +++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..29aa6d372bad 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -521,6 +521,19 @@ static void lenovo_features_set_cptkbd(struct hid_device *hdev)
 	int ret;
 	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
 
+	/*
+	 * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
+	 * regular keys
+	 */
+	ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
+	if (ret)
+		hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n", ret);
+
+	/* Switch middle button to native mode */
+	ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
+	if (ret)
+		hid_warn(hdev, "Failed to switch middle button: %d\n", ret);
+
 	ret = lenovo_send_cmd_cptkbd(hdev, 0x05, cptkbd_data->fn_lock);
 	if (ret)
 		hid_err(hdev, "Fn-lock setting failed: %d\n", ret);
@@ -1126,22 +1139,6 @@ static int lenovo_probe_cptkbd(struct hid_device *hdev)
 	}
 	hid_set_drvdata(hdev, cptkbd_data);
 
-	/*
-	 * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
-	 * regular keys (Compact only)
-	 */
-	if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD ||
-	    hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD) {
-		ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
-		if (ret)
-			hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n", ret);
-	}
-
-	/* Switch middle button to native mode */
-	ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
-	if (ret)
-		hid_warn(hdev, "Failed to switch middle button: %d\n", ret);
-
 	/* Set keyboard settings to known state */
 	cptkbd_data->middlebutton_state = 0;
 	cptkbd_data->fn_lock = true;
@@ -1264,6 +1261,24 @@ static int lenovo_probe(struct hid_device *hdev,
 	return ret;
 }
 
+#ifdef CONFIG_PM
+static int lenovo_reset_resume(struct hid_device *hdev)
+{
+	switch (hdev->product) {
+	case USB_DEVICE_ID_LENOVO_CUSBKBD:
+		if (hdev->type == HID_TYPE_USBMOUSE) {
+			lenovo_features_set_cptkbd(hdev);
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+#endif
+
 static void lenovo_remove_tpkbd(struct hid_device *hdev)
 {
 	struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
@@ -1380,6 +1395,9 @@ static struct hid_driver lenovo_driver = {
 	.raw_event = lenovo_raw_event,
 	.event = lenovo_event,
 	.report_fixup = lenovo_report_fixup,
+#ifdef CONFIG_PM
+	.reset_resume = lenovo_reset_resume,
+#endif
 };
 module_hid_driver(lenovo_driver);
 
-- 
2.39.2


^ permalink raw reply related

* Re: [PATCH V2 1/2] dt-bindings: input: Introduce Himax HID-over-SPI device
From: yang tylor @ 2023-09-25 10:16 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Conor Dooley, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
	linux-kernel, poyuan_chang, hbarnor, jingyliang@chromium.org,
	wuxy23, luolm1, hung poyu
In-Reply-To: <20230925-cod-vacancy-08dc8d88f90e@wendy>

On Mon, Sep 25, 2023 at 4:41 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Mon, Sep 25, 2023 at 09:44:21AM +0800, yang tylor wrote:
> > On Fri, Sep 22, 2023 at 11:31 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Fri, Sep 22, 2023 at 05:43:54PM +0800, yang tylor wrote:
> > > > On Fri, Sep 22, 2023 at 5:22 PM Conor Dooley <conor@kernel.org> wrote:
> > > > >
> > > > > On Fri, Sep 22, 2023 at 03:56:25PM +0800, yang tylor wrote:
> > > > > > On Tue, Sep 19, 2023 at 7:09 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > > On Tue, Sep 19, 2023 at 05:31:29PM +0800, yang tylor wrote:
> > > > >
> > > > > > > > The behavior of "himax,boot_time_fw_upgrade" seems not stable and
> > > > > > > > should be removed. "himax,fw_in_flash", I use the kernel config for
> > > > > > > > user to select.
> > > > > > >
> > > > > > > That seems like a bad idea, we want to be able to build one kernel that
> > > > > > > works for all hardware at the same time.
> > > > > > >
> > > > > > I see, so I should take that back?
> > > > > > I'll explain more about it.
> > > > >
> > > > > Are there particular ICs where the firmware would always be in flash and
> > > > > others where it would never be? Or is this a choice made by the board or
> > > > > system designer?
> > > > >
> > > > Most cases it's about the system designer's decision. But some ICs may be forced
> > > > to use flash because of its architecture(multiple IC inside, need to
> > > > load firmware to
> > > > multiple IC's sram by master IC). But if there is no limitation on
> > > > this part, most system
> > > > designers will prefer flashless.
> > >
> > > Forgive me if I am not understanding correctly, there are some ICs that
> > > will need to load the firmware from flash and there are some where it
> > > will be a decision made by the designer of the board. Is the flash part
> > > of the IC or is it an external flash chip?
> > >
> >
> > Both are possible, it depends on the IC type. For TDDI, the IC is long
> > and thin, placed on panel PCB, flash will be located at the external
> > flash chip. For the OLED TP, IC is usually placed at FPC and its flash
> > is embedded, thus the IC size is large compared to TDDI. But from the
> > driver's perspective either external flash or embedded flash, the IC
> > itself will load firmware from flash automatically when reset pin is
> > released. Only if firmware is loading from the host storage system,
> > the driver needs to operate the IC in detail.
>
>
> Since there are ICs that can use the external flash or have it loaded
> from the host, it sounds like you do need a property to differentiate
> between those cases.
Yep.

> Is it sufficient to just set the firmware-name property for these cases?
> If the property exists, then you know you need to load firmware & what
> its name is. If it doesn't, then the firmware either isn't needed or
> will be automatically loaded from the external flash.
We have a default prefix firmware name(like himax_xxxx.bin) in the driver code.
So we'll look for it when no-flash-flag is specified. In our experience,
forcing a prefix firmware name helps the user to aware what firmware
they are dealing with.

Thanks,
Tylor

^ permalink raw reply

* Re: [PATCH V2 1/2] dt-bindings: input: Introduce Himax HID-over-SPI device
From: Conor Dooley @ 2023-09-25  8:41 UTC (permalink / raw)
  To: yang tylor
  Cc: Conor Dooley, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
	linux-kernel, poyuan_chang, hbarnor, jingyliang@chromium.org
In-Reply-To: <CAGD2q_Y467jJJnwCVH+3F-hh6a-1-OYRugcy0DdjPnTCC77Z8A@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

On Mon, Sep 25, 2023 at 09:44:21AM +0800, yang tylor wrote:
> On Fri, Sep 22, 2023 at 11:31 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Fri, Sep 22, 2023 at 05:43:54PM +0800, yang tylor wrote:
> > > On Fri, Sep 22, 2023 at 5:22 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Fri, Sep 22, 2023 at 03:56:25PM +0800, yang tylor wrote:
> > > > > On Tue, Sep 19, 2023 at 7:09 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > On Tue, Sep 19, 2023 at 05:31:29PM +0800, yang tylor wrote:
> > > >
> > > > > > > The behavior of "himax,boot_time_fw_upgrade" seems not stable and
> > > > > > > should be removed. "himax,fw_in_flash", I use the kernel config for
> > > > > > > user to select.
> > > > > >
> > > > > > That seems like a bad idea, we want to be able to build one kernel that
> > > > > > works for all hardware at the same time.
> > > > > >
> > > > > I see, so I should take that back?
> > > > > I'll explain more about it.
> > > >
> > > > Are there particular ICs where the firmware would always be in flash and
> > > > others where it would never be? Or is this a choice made by the board or
> > > > system designer?
> > > >
> > > Most cases it's about the system designer's decision. But some ICs may be forced
> > > to use flash because of its architecture(multiple IC inside, need to
> > > load firmware to
> > > multiple IC's sram by master IC). But if there is no limitation on
> > > this part, most system
> > > designers will prefer flashless.
> >
> > Forgive me if I am not understanding correctly, there are some ICs that
> > will need to load the firmware from flash and there are some where it
> > will be a decision made by the designer of the board. Is the flash part
> > of the IC or is it an external flash chip?
> >
> 
> Both are possible, it depends on the IC type. For TDDI, the IC is long
> and thin, placed on panel PCB, flash will be located at the external
> flash chip. For the OLED TP, IC is usually placed at FPC and its flash
> is embedded, thus the IC size is large compared to TDDI. But from the
> driver's perspective either external flash or embedded flash, the IC
> itself will load firmware from flash automatically when reset pin is
> released. Only if firmware is loading from the host storage system,
> the driver needs to operate the IC in detail.


Since there are ICs that can use the external flash or have it loaded
from the host, it sounds like you do need a property to differentiate
between those cases.
Is it sufficient to just set the firmware-name property for these cases?
If the property exists, then you know you need to load firmware & what
its name is. If it doesn't, then the firmware either isn't needed or
will be automatically loaded from the external flash.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply

* [PATCH v2] Input: axp20x-pek - avoid needless newline removal
From: Justin Stitt @ 2023-09-25  4:31 UTC (permalink / raw)
  To: Dmitry Torokhov, Chen-Yu Tsai
  Cc: linux-input, linux-kernel, linux-hardening, Kees Cook,
	Justin Stitt

This code is doing more work than it needs to.

Before handing off `val_str` to `kstrtouint()` we are eagerly removing
any trailing newline which requires copying `buf`, validating it's
length and checking/replacing any potential newlines.

kstrtouint() handles this implicitly:
kstrtouint ->
  kstrotoull -> (documentation)
|   /**
|    * kstrtoull - convert a string to an unsigned long long
|    * @s: The start of the string. The string must be null-terminated, and may also
|    *  include a single newline before its terminating null. The first character
|    ...

Let's remove the redundant functionality and let kstrtouint handle it.

Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Changes in v2:
- remove eager newline removal (thanks Kees)
- use better subject line (thanks Kees)
- rebase on 6465e260f4879080
- Link to v1: https://lore.kernel.org/r/20230921-strncpy-drivers-input-misc-axp20x-pek-c-v1-1-f7f6f4a5cf81@google.com
---
Note: build-tested only.
---
 drivers/input/misc/axp20x-pek.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
index 4581606a28d6..24f9e9d893de 100644
--- a/drivers/input/misc/axp20x-pek.c
+++ b/drivers/input/misc/axp20x-pek.c
@@ -133,20 +133,11 @@ static ssize_t axp20x_store_attr(struct device *dev,
 				 size_t count)
 {
 	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
-	char val_str[20];
-	size_t len;
 	int ret, i;
 	unsigned int val, idx = 0;
 	unsigned int best_err = UINT_MAX;
 
-	val_str[sizeof(val_str) - 1] = '\0';
-	strncpy(val_str, buf, sizeof(val_str) - 1);
-	len = strlen(val_str);
-
-	if (len && val_str[len - 1] == '\n')
-		val_str[len - 1] = '\0';
-
-	ret = kstrtouint(val_str, 10, &val);
+	ret = kstrtouint(buf, 10, &val);
 	if (ret)
 		return ret;
 

---
base-commit: 6465e260f48790807eef06b583b38ca9789b6072
change-id: 20230921-strncpy-drivers-input-misc-axp20x-pek-c-3c4708924bbd

Best regards,
--
Justin Stitt <justinstitt@google.com>


^ permalink raw reply related

* [PATCH v2] dt-bindings: input: touchscreen: goodix: clarify irq-gpios misleading text
From: Luca Ceresoli @ 2023-09-25  3:22 UTC (permalink / raw)
  To: devicetree
  Cc: Luca Ceresoli, Dmitry Torokhov, Rob Herring, Jeff LaBundy,
	Krzysztof Kozlowski, Conor Dooley, linux-input, linux-kernel,
	Thomas Petazzoni

The irq-gpios description misleading, apparently saying that driving the
IRQ GPIO resets the device, which is even more puzzling as there is a reset
GPIO as well.

In reality the IRQ pin can be driven during the reset sequence to configure
the client address, as it becomes clear after checking both the datasheet
and the driver code. Improve the text to clarify that.

Also rephrase to remove reference to the driver, which is not appropriate
in the bindings.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v2:
 - reworded to clarify even further
---
 .../devicetree/bindings/input/touchscreen/goodix.yaml        | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
index 3d016b87c8df..2a2d86cfd104 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
@@ -37,8 +37,9 @@ properties:
     maxItems: 1
 
   irq-gpios:
-    description: GPIO pin used for IRQ. The driver uses the interrupt gpio pin
-      as output to reset the device.
+    description: GPIO pin used for IRQ input. Additionally, this line is
+      sampled by the device on reset deassertion to select the I2C client
+      address, thus it can be driven by the host during the reset sequence.
     maxItems: 1
 
   reset-gpios:
-- 
2.34.1


^ permalink raw reply related

* Re: [RESEND PATCH v6 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2023-09-25  2:54 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: linux-arm-msm, linux-kernel, krzysztof.kozlowski+dt, robh+dt,
	agross, andersson, Konrad Dybcio, Dmitry Torokhov, linux-input,
	quic_collinsd, quic_subbaram, quic_kamalw, jestar, Luca Weiss
In-Reply-To: <CAA8EJpoW8DJOTVHBu9_+BQs5DtxyJu3xrCfDNyYHn2MeHZHV4w@mail.gmail.com>



On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
>> +
>> +       switch (vib->data->hw_type) {
>> +       case SSBI_VIB:
>>                  mask = SSBI_VIB_DRV_LEVEL_MASK;
>>                  shift = SSBI_VIB_DRV_SHIFT;
>> +               break;
>> +       case SPMI_VIB:
>> +               mask = SPMI_VIB_DRV_LEVEL_MASK;
>> +               shift = SPMI_VIB_DRV_SHIFT;
>> +               break;
>> +       case SPMI_VIB_GEN2:
>> +               mask = SPMI_VIB_GEN2_DRV_MASK;
>> +               shift = SPMI_VIB_GEN2_DRV_SHIFT;
>> +               break;
>> +       default:
>> +               return -EINVAL;
> Could you please move the switch to the previous patch? Then it would
> be more obvious that you are just adding the SPMI_VIB_GEN2 here.
> 
> Other than that LGTM.

Sure, I can move the switch to the previous refactoring patch.

> 
>>          }

^ permalink raw reply

* Re: [RESEND PATCH v6 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator
From: Fenglin Wu @ 2023-09-25  2:52 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: linux-arm-msm, linux-kernel, krzysztof.kozlowski+dt, robh+dt,
	agross, andersson, Konrad Dybcio, Dmitry Torokhov, linux-input,
	quic_collinsd, quic_subbaram, quic_kamalw, jestar
In-Reply-To: <CAA8EJpo7puWxNte5YHiy6=3GdQSeTYCZMe024-b4N0vnxCV0dQ@mail.gmail.com>



On 9/24/2023 3:05 AM, Dmitry Baryshkov wrote:
>> +#define SSBL_VIB_DRV_REG               0x4A
> SSBI_VIB....
> 

Thanks for catching the typo, I will fix it in next patch.

>> +#define SSBI_VIB_DRV_EN_MANUAL_MASK    GENMASK(7, 2)
>> -       /* operate in manual mode */
>> -       error = regmap_read(vib->regmap, regs->drv_addr, &val);
>> -       if (error < 0)
>> -               return error;
>> +       if (data->hw_type != SSBI_VIB) {
> You can drop this condition, if ssbi_vib_data.drv_addr is 0.

I am not sure if I understood this comment: 1st, ssbi_vib_data.drv_addr 
is defined as a constant value 0x4A, so it would never be 0. 2nd, The 
condition check here is to ignore reading the register base address for 
SSBI_VIB HW, so we should do the check based on the HW type.

> 
>> +               error = fwnode_property_read_u32(pdev->dev.fwnode, "reg", &reg_base);
>> +               if (error < 0) {
>> +                       dev_err(&pdev->dev, "Failed to read reg address, rc=%d\n", error);
>> +                       return error;
>> +               }
>> +
>> +               vib->reg_base += reg_base;

^ permalink raw reply

* [dtor-input:next] BUILD SUCCESS c50fdc48643a25f5218f6b8bba4f28e1b4a94708
From: kernel test robot @ 2023-09-25  2:27 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input

tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: c50fdc48643a25f5218f6b8bba4f28e1b4a94708  Input: wm97xx-core - convert to platform remove callback returning void

elapsed time: 1375m

configs tested: 199
configs skipped: 2

The following configs have been built successfully.
More configs may be tested in the coming days.

tested configs:
alpha                             allnoconfig   gcc  
alpha                            allyesconfig   gcc  
alpha                               defconfig   gcc  
arc                              allmodconfig   gcc  
arc                               allnoconfig   gcc  
arc                              allyesconfig   gcc  
arc                                 defconfig   gcc  
arc                     nsimosci_hs_defconfig   gcc  
arc                   randconfig-001-20230924   gcc  
arc                        vdk_hs38_defconfig   gcc  
arc                    vdk_hs38_smp_defconfig   gcc  
arm                              allmodconfig   gcc  
arm                               allnoconfig   gcc  
arm                              allyesconfig   gcc  
arm                       aspeed_g4_defconfig   clang
arm                                 defconfig   gcc  
arm                   randconfig-001-20230924   gcc  
arm                   randconfig-001-20230925   gcc  
arm                           u8500_defconfig   gcc  
arm                         vf610m4_defconfig   gcc  
arm64                            allmodconfig   gcc  
arm64                             allnoconfig   gcc  
arm64                            allyesconfig   gcc  
arm64                               defconfig   gcc  
csky                             allmodconfig   gcc  
csky                              allnoconfig   gcc  
csky                             allyesconfig   gcc  
csky                                defconfig   gcc  
i386                             allmodconfig   gcc  
i386                              allnoconfig   gcc  
i386                             allyesconfig   gcc  
i386         buildonly-randconfig-001-20230924   gcc  
i386         buildonly-randconfig-002-20230924   gcc  
i386         buildonly-randconfig-003-20230924   gcc  
i386         buildonly-randconfig-004-20230924   gcc  
i386         buildonly-randconfig-005-20230924   gcc  
i386         buildonly-randconfig-006-20230924   gcc  
i386                              debian-10.3   gcc  
i386                                defconfig   gcc  
i386                  randconfig-001-20230924   gcc  
i386                  randconfig-001-20230925   gcc  
i386                  randconfig-002-20230924   gcc  
i386                  randconfig-002-20230925   gcc  
i386                  randconfig-003-20230924   gcc  
i386                  randconfig-003-20230925   gcc  
i386                  randconfig-004-20230924   gcc  
i386                  randconfig-004-20230925   gcc  
i386                  randconfig-005-20230924   gcc  
i386                  randconfig-005-20230925   gcc  
i386                  randconfig-006-20230924   gcc  
i386                  randconfig-006-20230925   gcc  
i386                  randconfig-011-20230924   gcc  
i386                  randconfig-011-20230925   gcc  
i386                  randconfig-012-20230924   gcc  
i386                  randconfig-012-20230925   gcc  
i386                  randconfig-013-20230924   gcc  
i386                  randconfig-013-20230925   gcc  
i386                  randconfig-014-20230924   gcc  
i386                  randconfig-014-20230925   gcc  
i386                  randconfig-015-20230924   gcc  
i386                  randconfig-015-20230925   gcc  
i386                  randconfig-016-20230924   gcc  
i386                  randconfig-016-20230925   gcc  
loongarch                        allmodconfig   gcc  
loongarch                         allnoconfig   gcc  
loongarch                        allyesconfig   gcc  
loongarch                           defconfig   gcc  
loongarch             randconfig-001-20230924   gcc  
m68k                             allmodconfig   gcc  
m68k                              allnoconfig   gcc  
m68k                             allyesconfig   gcc  
m68k                                defconfig   gcc  
microblaze                       allmodconfig   gcc  
microblaze                        allnoconfig   gcc  
microblaze                       allyesconfig   gcc  
microblaze                          defconfig   gcc  
mips                             allmodconfig   gcc  
mips                              allnoconfig   gcc  
mips                             allyesconfig   gcc  
mips                       bmips_be_defconfig   gcc  
mips                           ip32_defconfig   gcc  
nios2                            allmodconfig   gcc  
nios2                             allnoconfig   gcc  
nios2                            allyesconfig   gcc  
nios2                               defconfig   gcc  
openrisc                         allmodconfig   gcc  
openrisc                          allnoconfig   gcc  
openrisc                         allyesconfig   gcc  
openrisc                            defconfig   gcc  
parisc                           allmodconfig   gcc  
parisc                            allnoconfig   gcc  
parisc                           allyesconfig   gcc  
parisc                              defconfig   gcc  
parisc64                            defconfig   gcc  
powerpc                          allmodconfig   clang
powerpc                          allmodconfig   gcc  
powerpc                           allnoconfig   gcc  
powerpc                          allyesconfig   gcc  
powerpc                       maple_defconfig   gcc  
powerpc                     mpc83xx_defconfig   gcc  
powerpc                      pmac32_defconfig   clang
powerpc                     stx_gp3_defconfig   gcc  
powerpc                     tqm8548_defconfig   gcc  
riscv                            allmodconfig   gcc  
riscv                             allnoconfig   gcc  
riscv                            allyesconfig   gcc  
riscv                               defconfig   gcc  
riscv                    nommu_k210_defconfig   gcc  
riscv                 randconfig-001-20230924   gcc  
riscv                          rv32_defconfig   gcc  
s390                             allmodconfig   gcc  
s390                              allnoconfig   gcc  
s390                             allyesconfig   gcc  
s390                                defconfig   gcc  
s390                  randconfig-001-20230924   gcc  
sh                               allmodconfig   gcc  
sh                                allnoconfig   gcc  
sh                               allyesconfig   gcc  
sh                         apsh4a3a_defconfig   gcc  
sh                                  defconfig   gcc  
sh                      rts7751r2d1_defconfig   gcc  
sh                           se7206_defconfig   gcc  
sparc                            allmodconfig   gcc  
sparc                             allnoconfig   gcc  
sparc                            allyesconfig   gcc  
sparc                               defconfig   gcc  
sparc                 randconfig-001-20230924   gcc  
sparc                 randconfig-001-20230925   gcc  
sparc64                          allmodconfig   gcc  
sparc64                          allyesconfig   gcc  
sparc64                             defconfig   gcc  
um                               allmodconfig   clang
um                                allnoconfig   clang
um                               allyesconfig   clang
um                                  defconfig   gcc  
um                             i386_defconfig   gcc  
um                           x86_64_defconfig   gcc  
x86_64                            allnoconfig   gcc  
x86_64                           allyesconfig   gcc  
x86_64       buildonly-randconfig-001-20230924   gcc  
x86_64       buildonly-randconfig-001-20230925   gcc  
x86_64       buildonly-randconfig-002-20230924   gcc  
x86_64       buildonly-randconfig-002-20230925   gcc  
x86_64       buildonly-randconfig-003-20230924   gcc  
x86_64       buildonly-randconfig-003-20230925   gcc  
x86_64       buildonly-randconfig-004-20230924   gcc  
x86_64       buildonly-randconfig-004-20230925   gcc  
x86_64       buildonly-randconfig-005-20230924   gcc  
x86_64       buildonly-randconfig-005-20230925   gcc  
x86_64       buildonly-randconfig-006-20230924   gcc  
x86_64       buildonly-randconfig-006-20230925   gcc  
x86_64                              defconfig   gcc  
x86_64                                  kexec   gcc  
x86_64                randconfig-001-20230924   gcc  
x86_64                randconfig-001-20230925   gcc  
x86_64                randconfig-002-20230924   gcc  
x86_64                randconfig-002-20230925   gcc  
x86_64                randconfig-003-20230924   gcc  
x86_64                randconfig-003-20230925   gcc  
x86_64                randconfig-004-20230924   gcc  
x86_64                randconfig-004-20230925   gcc  
x86_64                randconfig-005-20230924   gcc  
x86_64                randconfig-005-20230925   gcc  
x86_64                randconfig-006-20230924   gcc  
x86_64                randconfig-006-20230925   gcc  
x86_64                randconfig-011-20230924   gcc  
x86_64                randconfig-011-20230925   gcc  
x86_64                randconfig-012-20230924   gcc  
x86_64                randconfig-012-20230925   gcc  
x86_64                randconfig-013-20230924   gcc  
x86_64                randconfig-013-20230925   gcc  
x86_64                randconfig-014-20230924   gcc  
x86_64                randconfig-014-20230925   gcc  
x86_64                randconfig-015-20230924   gcc  
x86_64                randconfig-015-20230925   gcc  
x86_64                randconfig-016-20230924   gcc  
x86_64                randconfig-016-20230925   gcc  
x86_64                randconfig-071-20230924   gcc  
x86_64                randconfig-071-20230925   gcc  
x86_64                randconfig-072-20230924   gcc  
x86_64                randconfig-072-20230925   gcc  
x86_64                randconfig-073-20230924   gcc  
x86_64                randconfig-073-20230925   gcc  
x86_64                randconfig-074-20230924   gcc  
x86_64                randconfig-074-20230925   gcc  
x86_64                randconfig-075-20230924   gcc  
x86_64                randconfig-075-20230925   gcc  
x86_64                randconfig-076-20230924   gcc  
x86_64                randconfig-076-20230925   gcc  
x86_64                           rhel-8.3-bpf   gcc  
x86_64                          rhel-8.3-func   gcc  
x86_64                    rhel-8.3-kselftests   gcc  
x86_64                         rhel-8.3-kunit   gcc  
x86_64                           rhel-8.3-ltp   gcc  
x86_64                          rhel-8.3-rust   clang
x86_64                               rhel-8.3   gcc  
xtensa                            allnoconfig   gcc  
xtensa                           allyesconfig   gcc  
xtensa                  nommu_kc705_defconfig   gcc  
xtensa                    smp_lx200_defconfig   gcc  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply

* Re: [PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload of policy binary
From: kernel test robot @ 2023-09-25  2:14 UTC (permalink / raw)
  To: Shyam Sundar S K, hdegoede, markgross, basavaraj.natikar, jikos,
	benjamin.tissoires, alexander.deucher, christian.koenig,
	Xinhui.Pan, airlied, daniel
  Cc: oe-kbuild-all, Shyam Sundar S K, amd-gfx, platform-driver-x86,
	dri-devel, Patil.Reddy, linux-input, mario.limonciello
In-Reply-To: <20230922175056.244940-11-Shyam-sundar.S-k@amd.com>

Hi Shyam,

kernel test robot noticed the following build warnings:

[auto build test WARNING on hid/for-next]
[also build test WARNING on linus/master v6.6-rc3 next-20230921]
[cannot apply to drm-misc/drm-misc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shyam-Sundar-S-K/platform-x86-amd-pmf-Add-PMF-TEE-interface/20230923-015418
base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link:    https://lore.kernel.org/r/20230922175056.244940-11-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload of policy binary
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230925/202309251031.awDDkRgS-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230925/202309251031.awDDkRgS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309251031.awDDkRgS-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/amd/pmf/tee-if.c:305:5: warning: no previous prototype for 'amd_pmf_open_pb' [-Wmissing-prototypes]
     305 | int amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry *debugfs_root)
         |     ^~~~~~~~~~~~~~~


vim +/amd_pmf_open_pb +305 drivers/platform/x86/amd/pmf/tee-if.c

   304	
 > 305	int amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry *debugfs_root)
   306	{
   307		struct dentry *file = NULL;
   308	
   309		dev->esbin = debugfs_create_dir("pb", debugfs_root);
   310		if (IS_ERR(dev->esbin))
   311			return -EINVAL;
   312	
   313		file = debugfs_create_file("update_policy", 0644, dev->esbin, dev, &pb_fops);
   314		if (!file)
   315			return -EINVAL;
   316	
   317		return 0;
   318	}
   319	#endif
   320	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply

* Re: [PATCH V2 1/2] dt-bindings: input: Introduce Himax HID-over-SPI device
From: yang tylor @ 2023-09-25  1:44 UTC (permalink / raw)
  To: Conor Dooley
  Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, jikos,
	benjamin.tissoires, linux-input, devicetree, linux-kernel,
	poyuan_chang, hbarnor, jingyliang@chromium.org
In-Reply-To: <20230922-removable-footwork-f1d4d96d38dd@spud>

On Fri, Sep 22, 2023 at 11:31 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Sep 22, 2023 at 05:43:54PM +0800, yang tylor wrote:
> > On Fri, Sep 22, 2023 at 5:22 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Fri, Sep 22, 2023 at 03:56:25PM +0800, yang tylor wrote:
> > > > On Tue, Sep 19, 2023 at 7:09 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > On Tue, Sep 19, 2023 at 05:31:29PM +0800, yang tylor wrote:
> > >
> > > > > > The behavior of "himax,boot_time_fw_upgrade" seems not stable and
> > > > > > should be removed. "himax,fw_in_flash", I use the kernel config for
> > > > > > user to select.
> > > > >
> > > > > That seems like a bad idea, we want to be able to build one kernel that
> > > > > works for all hardware at the same time.
> > > > >
> > > > I see, so I should take that back?
> > > > I'll explain more about it.
> > >
> > > Are there particular ICs where the firmware would always be in flash and
> > > others where it would never be? Or is this a choice made by the board or
> > > system designer?
> > >
> > Most cases it's about the system designer's decision. But some ICs may be forced
> > to use flash because of its architecture(multiple IC inside, need to
> > load firmware to
> > multiple IC's sram by master IC). But if there is no limitation on
> > this part, most system
> > designers will prefer flashless.
>
> Forgive me if I am not understanding correctly, there are some ICs that
> will need to load the firmware from flash and there are some where it
> will be a decision made by the designer of the board. Is the flash part
> of the IC or is it an external flash chip?
>

Both are possible, it depends on the IC type. For TDDI, the IC is long
and thin, placed on panel PCB, flash will be located at the external
flash chip. For the OLED TP, IC is usually placed at FPC and its flash
is embedded, thus the IC size is large compared to TDDI. But from the
driver's perspective either external flash or embedded flash, the IC
itself will load firmware from flash automatically when reset pin is
released. Only if firmware is loading from the host storage system,
the driver needs to operate the IC in detail.

> Cheers,
> Conor.


Thanks,
Tylor

^ permalink raw reply

* Re: [PATCH 00/52] input: Convert to platform remove callback returning void
From: Uwe Kleine-König @ 2023-09-24 15:50 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Lee Jones, Linus Walleij, Alexandre Torgue, Pavel Machek,
	Guenter Roeck, Liang He, linux-stm32, Daniel Lezcano,
	chrome-platform, Arnd Bergmann, Samuel Holland, Andrey Moiseev,
	Michal Simek, Ruan Jinjie, Yangtao Li, Jernej Skrabec,
	joewu (吳仲振), Miloslav Trmac, Robert Jarzmik,
	Chen-Yu Tsai, Andy Gross, linux-input, Jeff LaBundy, linux-sunxi,
	linux-arm-msm, Maxime Coquelin, Michael Hennerich, Rob Herring,
	ye xingchen, Kalle Valo, Steven Rostedt (Google), Hans de Goede,
	Siarhei Volkau, Christophe JAILLET, Jonathan Cameron,
	Andy Shevchenko, Benson Leung, linux-arm-kernel, Paolo Abeni,
	Support Opensource, Chen Jun, Greg Kroah-Hartman,
	Mattijs Korpershoek, Rafael J. Wysocki, Konrad Dybcio,
	Krzysztof Kozlowski, kernel, patches, Dmitry Baryshkov,
	Jonathan Corbet, Bjorn Andersson
In-Reply-To: <ZQ+jddG+UbuSD7pP@google.com>

[-- Attachment #1: Type: text/plain, Size: 1511 bytes --]

Hello Dmitry,

On Sat, Sep 23, 2023 at 07:48:21PM -0700, Dmitry Torokhov wrote:
> On Wed, Sep 20, 2023 at 02:57:37PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > this series converts all platform drivers below drivers/input to use
> > remove_new. The motivation is to get rid of an integer return code
> > that is (mostly) ignored by the platform driver core and error prone on
> > the driver side.
> > 
> > See commit 5c5a7680e67b ("platform: Provide a remove callback that
> > returns no value") for an extended explanation and the eventual goal.
> > 
> > There are no interdependencies between the patches. As there are still
> > quite a few drivers to convert, I'm happy about every patch that makes
> > it in. So even if there is a merge conflict with one patch until you
> > apply or a subject prefix is suboptimal, please apply the remainder of
> > this series anyhow.
> 
> Applied the lot (fixing the i8042-sparcio patch subject), thank you!

Thanks. In the meantime I found out why my process failed here: I only
fixed *.c, but this driver struct is defined in a header file
i8042-sparcio.h.

This file is only included by drivers/input/serio/i8042.h which in turn
is only included by drivers/input/serio/i8042.c. So there is only one
instance created, but I'd call that unusual anyhow.

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

* Re: [PATCH v2 0/9] Support light color temperature and chromaticity
From: Basavaraj Natikar @ 2023-09-24 14:20 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Jiri Kosina, Basavaraj Natikar, benjamin.tissoires, lars,
	srinivas.pandruvada, linux-input, linux-iio
In-Reply-To: <20230924134224.64e54daa@jic23-huawei>


On 9/24/2023 6:12 PM, Jonathan Cameron wrote:
> On Wed, 20 Sep 2023 20:53:33 +0530
> Basavaraj Natikar <bnatikar@amd.com> wrote:
>
>> On 9/20/2023 7:43 PM, Jiri Kosina wrote:
>>> On Tue, 19 Sep 2023, Basavaraj Natikar wrote:
>>>  
>>>> This series adds support for light color temperature and chromaticity.
>>>>
>>>> v1->v2:
>>>> *Rename the series.
>>>> *Rename als_illum to als channel as it supports other channels.
>>>> *Update patch description to include same reading for the two existing
>>>>  channels to use channel index to support more hub attributes.
>>>> *Keep line length under 80chars in hid-sensor-als.
>>>> *Add new channel type IIO_COLORTEMP.
>>>> *Update patch description and its subject to add channel type for 
>>>>  chromaticity. 
>>>>
>>>> Basavaraj Natikar (9):
>>>>   iio: hid-sensor-als: Use channel index to support more hub attributes
>>>>   iio: Add channel type light color temperature
>>>>   iio: hid-sensor-als: Add light color temperature support
>>>>   HID: amd_sfh: Add support for light color temperature
>>>>   HID: amd_sfh: Add support for SFH1.1 light color temperature
>>>>   iio: Add channel type for chromaticity
>>>>   iio: hid-sensor-als: Add light chromaticity support
>>>>   HID: amd_sfh: Add light chromaticity support
>>>>   HID: amd_sfh: Add light chromaticity for SFH1.1
>>>>
>>>>  Documentation/ABI/testing/sysfs-bus-iio       |  15 ++
>>>>  .../hid_descriptor/amd_sfh_hid_desc.c         |   7 +
>>>>  .../hid_descriptor/amd_sfh_hid_desc.h         |   3 +
>>>>  .../hid_descriptor/amd_sfh_hid_report_desc.h  |  21 +++
>>>>  drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c |   9 ++
>>>>  .../amd-sfh-hid/sfh1_1/amd_sfh_interface.h    |  15 ++
>>>>  drivers/iio/industrialio-core.c               |   2 +
>>>>  drivers/iio/light/hid-sensor-als.c            | 130 +++++++++++++++---
>>>>  include/linux/hid-sensor-ids.h                |   4 +
>>>>  include/uapi/linux/iio/types.h                |   2 +
>>>>  tools/iio/iio_event_monitor.c                 |   3 +
>>>>  11 files changed, 195 insertions(+), 16 deletions(-)  
>>> I believe this should go through Jonathan's tree as a whole, right?  
>> Yes, this should go through Jonathan's tree as a whole.
>> If you don't have concerns, can you please ack HID amd_sfh changes?
> I'll do an immutable branch in case this needs pulling into the hid tree
> later in the cycle.
>
> In short that means I'll create a branch with just this series on top of v6.6-rc1
> and push that out as ib-iio-hid-sensors-v6.6-rc1.
> I'll then merge that into the IIO tree before I do a pull request.
> The advantage of this being that it can be pulled into other trees as necessary
> and keep the same git IDs etc so that git can cleanly unwind the splitting and
> merging of the history to cover the different paths.
>
> However, note this will be messy as the merge into IIO isn't clean. I'll fix it
> up but please take a quick look at the testing branch of iio.git on kernel.org
> where the results of that merge will be.  Some other channel types were added
> recently. So the fix was obvious.
>
> So applied to the branch ib-iio-hid-sensors-6.6-rc1.  I'll merge that into the
> IIO tree. That will get pushed out as testing for the build bots to see if they can
> find anything we missed before I push this out as togreg which is what
> linux-next picks up.
>
> Note the IB branch might be rebased if any test issues show up.

Sure I will check testing branch, Thank you Jonathan.

Thanks,
--
Basavaraj



^ permalink raw reply

* [PATCH v5 RESEND] HID: nintendo: cleanup LED code
From: Martino Fontana @ 2023-09-24 14:13 UTC (permalink / raw)
  To: djogorchock, jikos, benjamin.tissoires, linux-input, linux-kernel
  Cc: Martino Fontana

- Support player LED patterns up to 8 players.
  (Note that the behavior still consinsts in increasing the player number
  every time a controller is connected, never decreasing it. It should be
  as is described in https://bugzilla.kernel.org/show_bug.cgi?id=216225.
  However, any implementation here would stop making sense as soon as a
  non-Nintendo controller is connected, which is why I'm not bothering.)

- Split part of `joycon_home_led_brightness_set` (which is called by hid)
  into `joycon_set_home_led` (which is what actually sets the LEDs), for
  consistency with player LEDs.

- `joycon_player_led_brightness_set` won't try it to "determine which
  player led this is" anymore: it's already looking at every LED
  brightness value.

- Instead of first registering the `led_classdev`, then attempting to set
  the LED and unregistering the `led_classdev` if it fails, first attempt
  to set the LED, then register the `led_classdev` only if it succeeds
  (the class is still filled up in either case).

- If setting the player LEDs fails, still attempt setting the home LED.
  (I don't know there's a third party controller where this may actually
  happen, but who knows...)

- Use `JC_NUM_LEDS` where appropriate instead of 4.

- Print return codes in more places.

- Use spinlock instead of mutex for `input_num`. Copy its value to a local
  variable, so that it can be unlocked immediately.

- `input_num` starts counting from 0

- Less holding of mutexes in general.

Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
---
Changes for v2:

Applied suggestions, commit message explains more stuff, restored `return ret`
when `devm_led_classdev_register` fails (since all other hid drivers do this).
If setting LED fails, `hid_warn` now explicitly says "skipping registration".

Changes for v3 and v4:

Fixed errors and warnings from test robot.

Changes for v5:

I thought that when connecting the controller on an actual Nintendo Switch,
only the nth player LED would turn on, which is true... on Wii and Wii U.
So I reverted that, and to compensate, now this supports the LED patterns
up to 8 players.

 drivers/hid/hid-nintendo.c | 133 +++++++++++++++++++++----------------
 1 file changed, 76 insertions(+), 57 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 10468f727..138f154fe 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -410,6 +410,18 @@ static const char * const joycon_player_led_names[] = {
 	LED_FUNCTION_PLAYER4,
 };
 #define JC_NUM_LEDS		ARRAY_SIZE(joycon_player_led_names)
+#define JC_NUM_LED_PATTERNS 8
+/* Taken from https://www.nintendo.com/my/support/qa/detail/33822 */
+static const enum led_brightness joycon_player_led_patterns[JC_NUM_LED_PATTERNS][JC_NUM_LEDS] = {
+	{ 1, 0, 0, 0 },
+	{ 1, 1, 0, 0 },
+	{ 1, 1, 1, 0 },
+	{ 1, 1, 1, 1 },
+	{ 1, 0, 0, 1 },
+	{ 1, 0, 1, 0 },
+	{ 1, 0, 1, 1 },
+	{ 0, 1, 1, 0 },
+};
 
 /* Each physical controller is associated with a joycon_ctlr struct */
 struct joycon_ctlr {
@@ -699,6 +711,25 @@ static int joycon_set_player_leds(struct joycon_ctlr *ctlr, u8 flash, u8 on)
 	return joycon_send_subcmd(ctlr, req, 1, HZ/4);
 }
 
+static int joycon_set_home_led(struct joycon_ctlr *ctlr, enum led_brightness brightness)
+{
+	struct joycon_subcmd_request *req;
+	u8 buffer[sizeof(*req) + 5] = { 0 };
+	u8 *data;
+
+	req = (struct joycon_subcmd_request *)buffer;
+	req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
+	data = req->data;
+	data[0] = 0x01;
+	data[1] = brightness << 4;
+	data[2] = brightness | (brightness << 4);
+	data[3] = 0x11;
+	data[4] = 0x11;
+
+	hid_dbg(ctlr->hdev, "setting home led brightness\n");
+	return joycon_send_subcmd(ctlr, req, 5, HZ/4);
+}
+
 static int joycon_request_spi_flash_read(struct joycon_ctlr *ctlr,
 					 u32 start_addr, u8 size, u8 **reply)
 {
@@ -1840,6 +1871,7 @@ static int joycon_input_create(struct joycon_ctlr *ctlr)
 	return 0;
 }
 
+/* Because the subcommand sets all the leds at once, the brightness argument is ignored */
 static int joycon_player_led_brightness_set(struct led_classdev *led,
 					    enum led_brightness brightness)
 {
@@ -1849,7 +1881,6 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
 	int val = 0;
 	int i;
 	int ret;
-	int num;
 
 	ctlr = hid_get_drvdata(hdev);
 	if (!ctlr) {
@@ -1857,21 +1888,10 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
 		return -ENODEV;
 	}
 
-	/* determine which player led this is */
-	for (num = 0; num < JC_NUM_LEDS; num++) {
-		if (&ctlr->leds[num] == led)
-			break;
-	}
-	if (num >= JC_NUM_LEDS)
-		return -EINVAL;
+	for (i = 0; i < JC_NUM_LEDS; i++)
+		val |= ctlr->leds[i].brightness << i;
 
 	mutex_lock(&ctlr->output_mutex);
-	for (i = 0; i < JC_NUM_LEDS; i++) {
-		if (i == num)
-			val |= brightness << i;
-		else
-			val |= ctlr->leds[i].brightness << i;
-	}
 	ret = joycon_set_player_leds(ctlr, 0, val);
 	mutex_unlock(&ctlr->output_mutex);
 
@@ -1884,9 +1904,6 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
 	struct device *dev = led->dev->parent;
 	struct hid_device *hdev = to_hid_device(dev);
 	struct joycon_ctlr *ctlr;
-	struct joycon_subcmd_request *req;
-	u8 buffer[sizeof(*req) + 5] = { 0 };
-	u8 *data;
 	int ret;
 
 	ctlr = hid_get_drvdata(hdev);
@@ -1894,43 +1911,35 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
 		hid_err(hdev, "No controller data\n");
 		return -ENODEV;
 	}
-
-	req = (struct joycon_subcmd_request *)buffer;
-	req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
-	data = req->data;
-	data[0] = 0x01;
-	data[1] = brightness << 4;
-	data[2] = brightness | (brightness << 4);
-	data[3] = 0x11;
-	data[4] = 0x11;
-
-	hid_dbg(hdev, "setting home led brightness\n");
 	mutex_lock(&ctlr->output_mutex);
-	ret = joycon_send_subcmd(ctlr, req, 5, HZ/4);
+	ret = joycon_set_home_led(ctlr, brightness);
 	mutex_unlock(&ctlr->output_mutex);
-
 	return ret;
 }
 
-static DEFINE_MUTEX(joycon_input_num_mutex);
+static DEFINE_SPINLOCK(joycon_input_num_spinlock);
 static int joycon_leds_create(struct joycon_ctlr *ctlr)
 {
 	struct hid_device *hdev = ctlr->hdev;
 	struct device *dev = &hdev->dev;
 	const char *d_name = dev_name(dev);
 	struct led_classdev *led;
+	int led_val = 0;
 	char *name;
-	int ret = 0;
+	int ret;
 	int i;
-	static int input_num = 1;
+	unsigned long flags;
+	int player_led_pattern;
+	static int input_num;
 
-	/* Set the default controller player leds based on controller number */
-	mutex_lock(&joycon_input_num_mutex);
-	mutex_lock(&ctlr->output_mutex);
-	ret = joycon_set_player_leds(ctlr, 0, 0xF >> (4 - input_num));
-	if (ret)
-		hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
-	mutex_unlock(&ctlr->output_mutex);
+	/*
+	 * Set the player leds based on controller number
+	 * Because there is no standard concept of "player number", the pattern
+	 * number will simply increase by 1 every time a controller is connected.
+	 */
+	spin_lock_irqsave(&joycon_input_num_spinlock, flags);
+	player_led_pattern = input_num++ % JC_NUM_LED_PATTERNS;
+	spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
 
 	/* configure the player LEDs */
 	for (i = 0; i < JC_NUM_LEDS; i++) {
@@ -1938,31 +1947,37 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
 				      d_name,
 				      "green",
 				      joycon_player_led_names[i]);
-		if (!name) {
-			mutex_unlock(&joycon_input_num_mutex);
+		if (!name)
 			return -ENOMEM;
-		}
 
 		led = &ctlr->leds[i];
 		led->name = name;
-		led->brightness = ((i + 1) <= input_num) ? 1 : 0;
+		led->brightness = joycon_player_led_patterns[player_led_pattern][i];
 		led->max_brightness = 1;
 		led->brightness_set_blocking =
 					joycon_player_led_brightness_set;
 		led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
 
+		led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
+	}
+	mutex_lock(&ctlr->output_mutex);
+	ret = joycon_set_player_leds(ctlr, 0, led_val);
+	mutex_unlock(&ctlr->output_mutex);
+	if (ret) {
+		hid_warn(hdev, "Failed to set players LEDs, skipping registration; ret=%d\n", ret);
+		goto home_led;
+	}
+
+	for (i = 0; i < JC_NUM_LEDS; i++) {
+		led = &ctlr->leds[i];
 		ret = devm_led_classdev_register(&hdev->dev, led);
 		if (ret) {
-			hid_err(hdev, "Failed registering %s LED\n", led->name);
-			mutex_unlock(&joycon_input_num_mutex);
+			hid_err(hdev, "Failed to register player %d LED; ret=%d\n", i + 1, ret);
 			return ret;
 		}
 	}
 
-	if (++input_num > 4)
-		input_num = 1;
-	mutex_unlock(&joycon_input_num_mutex);
-
+home_led:
 	/* configure the home LED */
 	if (jc_type_has_right(ctlr)) {
 		name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
@@ -1978,16 +1993,20 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
 		led->max_brightness = 0xF;
 		led->brightness_set_blocking = joycon_home_led_brightness_set;
 		led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
-		ret = devm_led_classdev_register(&hdev->dev, led);
+
+		/* Set the home LED to 0 as default state */
+		mutex_lock(&ctlr->output_mutex);
+		ret = joycon_set_home_led(ctlr, 0);
+		mutex_unlock(&ctlr->output_mutex);
 		if (ret) {
-			hid_err(hdev, "Failed registering home led\n");
-			return ret;
+			hid_warn(hdev, "Failed to set home LED, skipping registration; ret=%d\n", ret);
+			return 0;
 		}
-		/* Set the home LED to 0 as default state */
-		ret = joycon_home_led_brightness_set(led, 0);
+
+		ret = devm_led_classdev_register(&hdev->dev, led);
 		if (ret) {
-			hid_warn(hdev, "Failed to set home LED default, unregistering home LED");
-			devm_led_classdev_unregister(&hdev->dev, led);
+			hid_err(hdev, "Failed to register home LED; ret=%d\n", ret);
+			return ret;
 		}
 	}
 
-- 
2.42.0


^ permalink raw reply related

* [PATCH v3] HID: nintendo: reinitialize USB Pro Controller after resuming from suspend
From: Martino Fontana @ 2023-09-24 14:06 UTC (permalink / raw)
  To: djogorchock, jikos, benjamin.tissoires, linux-input, linux-kernel
  Cc: Martino Fontana

When suspending the computer, a Switch Pro Controller connected via USB will
lose its internal status. However, because the USB connection was technically
never lost, when resuming the computer, the driver will attempt to communicate
with the controller as if nothing happened (and fail).
Because of this, the user was forced to manually disconnect the controller
(or to press the sync button on the controller to power it off), so that it
can be re-initialized.

With this patch, the controller will be automatically re-initialized after
resuming from suspend.

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=216233

Signed-off-by: Martino Fontana <tinozzo123@gmail.com>

---
Changes for v2 and v3: Applied suggestions

 drivers/hid/hid-nintendo.c | 175 ++++++++++++++++++++++---------------
 1 file changed, 103 insertions(+), 72 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 250f5d2f8..10468f727 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -2088,7 +2088,9 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
 	struct joycon_input_report *report;
 
 	req.subcmd_id = JC_SUBCMD_REQ_DEV_INFO;
+	mutex_lock(&ctlr->output_mutex);
 	ret = joycon_send_subcmd(ctlr, &req, 0, HZ);
+	mutex_unlock(&ctlr->output_mutex);
 	if (ret) {
 		hid_err(ctlr->hdev, "Failed to get joycon info; ret=%d\n", ret);
 		return ret;
@@ -2117,6 +2119,85 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
 	return 0;
 }
 
+static int joycon_init(struct hid_device *hdev)
+{
+	struct joycon_ctlr *ctlr = hid_get_drvdata(hdev);
+	int ret = 0;
+
+	mutex_lock(&ctlr->output_mutex);
+	/* if handshake command fails, assume ble pro controller */
+	if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
+	    !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
+		hid_dbg(hdev, "detected USB controller\n");
+		/* set baudrate for improved latency */
+		ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
+		if (ret) {
+			hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
+			goto out_unlock;
+		}
+		/* handshake */
+		ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
+		if (ret) {
+			hid_err(hdev, "Failed handshake; ret=%d\n", ret);
+			goto out_unlock;
+		}
+		/*
+		 * Set no timeout (to keep controller in USB mode).
+		 * This doesn't send a response, so ignore the timeout.
+		 */
+		joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
+	} else if (jc_type_is_chrggrip(ctlr)) {
+		hid_err(hdev, "Failed charging grip handshake\n");
+		ret = -ETIMEDOUT;
+		goto out_unlock;
+	}
+
+	/* get controller calibration data, and parse it */
+	ret = joycon_request_calibration(ctlr);
+	if (ret) {
+		/*
+		 * We can function with default calibration, but it may be
+		 * inaccurate. Provide a warning, and continue on.
+		 */
+		hid_warn(hdev, "Analog stick positions may be inaccurate\n");
+	}
+
+	/* get IMU calibration data, and parse it */
+	ret = joycon_request_imu_calibration(ctlr);
+	if (ret) {
+		/*
+		 * We can function with default calibration, but it may be
+		 * inaccurate. Provide a warning, and continue on.
+		 */
+		hid_warn(hdev, "Unable to read IMU calibration data\n");
+	}
+
+	/* Set the reporting mode to 0x30, which is the full report mode */
+	ret = joycon_set_report_mode(ctlr);
+	if (ret) {
+		hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
+		goto out_unlock;
+	}
+
+	/* Enable rumble */
+	ret = joycon_enable_rumble(ctlr);
+	if (ret) {
+		hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
+		goto out_unlock;
+	}
+
+	/* Enable the IMU */
+	ret = joycon_enable_imu(ctlr);
+	if (ret) {
+		hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
+		goto out_unlock;
+	}
+
+out_unlock:
+	mutex_unlock(&ctlr->output_mutex);
+	return ret;
+}
+
 /* Common handler for parsing inputs */
 static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
 							      int size)
@@ -2248,85 +2329,19 @@ static int nintendo_hid_probe(struct hid_device *hdev,
 
 	hid_device_io_start(hdev);
 
-	/* Initialize the controller */
-	mutex_lock(&ctlr->output_mutex);
-	/* if handshake command fails, assume ble pro controller */
-	if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
-	    !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
-		hid_dbg(hdev, "detected USB controller\n");
-		/* set baudrate for improved latency */
-		ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
-		if (ret) {
-			hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
-			goto err_mutex;
-		}
-		/* handshake */
-		ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
-		if (ret) {
-			hid_err(hdev, "Failed handshake; ret=%d\n", ret);
-			goto err_mutex;
-		}
-		/*
-		 * Set no timeout (to keep controller in USB mode).
-		 * This doesn't send a response, so ignore the timeout.
-		 */
-		joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
-	} else if (jc_type_is_chrggrip(ctlr)) {
-		hid_err(hdev, "Failed charging grip handshake\n");
-		ret = -ETIMEDOUT;
-		goto err_mutex;
-	}
-
-	/* get controller calibration data, and parse it */
-	ret = joycon_request_calibration(ctlr);
+	ret = joycon_init(hdev);
 	if (ret) {
-		/*
-		 * We can function with default calibration, but it may be
-		 * inaccurate. Provide a warning, and continue on.
-		 */
-		hid_warn(hdev, "Analog stick positions may be inaccurate\n");
-	}
-
-	/* get IMU calibration data, and parse it */
-	ret = joycon_request_imu_calibration(ctlr);
-	if (ret) {
-		/*
-		 * We can function with default calibration, but it may be
-		 * inaccurate. Provide a warning, and continue on.
-		 */
-		hid_warn(hdev, "Unable to read IMU calibration data\n");
-	}
-
-	/* Set the reporting mode to 0x30, which is the full report mode */
-	ret = joycon_set_report_mode(ctlr);
-	if (ret) {
-		hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
-		goto err_mutex;
-	}
-
-	/* Enable rumble */
-	ret = joycon_enable_rumble(ctlr);
-	if (ret) {
-		hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
-		goto err_mutex;
-	}
-
-	/* Enable the IMU */
-	ret = joycon_enable_imu(ctlr);
-	if (ret) {
-		hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
-		goto err_mutex;
+		hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret);
+		goto err_close;
 	}
 
 	ret = joycon_read_info(ctlr);
 	if (ret) {
 		hid_err(hdev, "Failed to retrieve controller info; ret=%d\n",
 			ret);
-		goto err_mutex;
+		goto err_close;
 	}
 
-	mutex_unlock(&ctlr->output_mutex);
-
 	/* Initialize the leds */
 	ret = joycon_leds_create(ctlr);
 	if (ret) {
@@ -2352,8 +2367,6 @@ static int nintendo_hid_probe(struct hid_device *hdev,
 	hid_dbg(hdev, "probe - success\n");
 	return 0;
 
-err_mutex:
-	mutex_unlock(&ctlr->output_mutex);
 err_close:
 	hid_hw_close(hdev);
 err_stop:
@@ -2383,6 +2396,20 @@ static void nintendo_hid_remove(struct hid_device *hdev)
 	hid_hw_stop(hdev);
 }
 
+#ifdef CONFIG_PM
+
+static int nintendo_hid_resume(struct hid_device *hdev)
+{
+	int ret = joycon_init(hdev);
+
+	if (ret)
+		hid_err(hdev, "Failed to restore controller after resume");
+
+	return ret;
+}
+
+#endif
+
 static const struct hid_device_id nintendo_hid_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO,
 			 USB_DEVICE_ID_NINTENDO_PROCON) },
@@ -2404,6 +2431,10 @@ static struct hid_driver nintendo_hid_driver = {
 	.probe		= nintendo_hid_probe,
 	.remove		= nintendo_hid_remove,
 	.raw_event	= nintendo_hid_event,
+
+#ifdef CONFIG_PM
+	.resume		= nintendo_hid_resume,
+#endif
 };
 module_hid_driver(nintendo_hid_driver);
 
-- 
2.42.0


^ permalink raw reply related

* Re: [PATCH v2 0/9] Support light color temperature and chromaticity
From: Jonathan Cameron @ 2023-09-24 12:42 UTC (permalink / raw)
  To: Basavaraj Natikar
  Cc: Jiri Kosina, Basavaraj Natikar, benjamin.tissoires, lars,
	srinivas.pandruvada, linux-input, linux-iio
In-Reply-To: <520df871-a6d1-0db3-fba1-cffd35d9cc81@amd.com>

On Wed, 20 Sep 2023 20:53:33 +0530
Basavaraj Natikar <bnatikar@amd.com> wrote:

> On 9/20/2023 7:43 PM, Jiri Kosina wrote:
> > On Tue, 19 Sep 2023, Basavaraj Natikar wrote:
> >  
> >> This series adds support for light color temperature and chromaticity.
> >>
> >> v1->v2:
> >> *Rename the series.
> >> *Rename als_illum to als channel as it supports other channels.
> >> *Update patch description to include same reading for the two existing
> >>  channels to use channel index to support more hub attributes.
> >> *Keep line length under 80chars in hid-sensor-als.
> >> *Add new channel type IIO_COLORTEMP.
> >> *Update patch description and its subject to add channel type for 
> >>  chromaticity. 
> >>
> >> Basavaraj Natikar (9):
> >>   iio: hid-sensor-als: Use channel index to support more hub attributes
> >>   iio: Add channel type light color temperature
> >>   iio: hid-sensor-als: Add light color temperature support
> >>   HID: amd_sfh: Add support for light color temperature
> >>   HID: amd_sfh: Add support for SFH1.1 light color temperature
> >>   iio: Add channel type for chromaticity
> >>   iio: hid-sensor-als: Add light chromaticity support
> >>   HID: amd_sfh: Add light chromaticity support
> >>   HID: amd_sfh: Add light chromaticity for SFH1.1
> >>
> >>  Documentation/ABI/testing/sysfs-bus-iio       |  15 ++
> >>  .../hid_descriptor/amd_sfh_hid_desc.c         |   7 +
> >>  .../hid_descriptor/amd_sfh_hid_desc.h         |   3 +
> >>  .../hid_descriptor/amd_sfh_hid_report_desc.h  |  21 +++
> >>  drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c |   9 ++
> >>  .../amd-sfh-hid/sfh1_1/amd_sfh_interface.h    |  15 ++
> >>  drivers/iio/industrialio-core.c               |   2 +
> >>  drivers/iio/light/hid-sensor-als.c            | 130 +++++++++++++++---
> >>  include/linux/hid-sensor-ids.h                |   4 +
> >>  include/uapi/linux/iio/types.h                |   2 +
> >>  tools/iio/iio_event_monitor.c                 |   3 +
> >>  11 files changed, 195 insertions(+), 16 deletions(-)  
> > I believe this should go through Jonathan's tree as a whole, right?  
> 
> Yes, this should go through Jonathan's tree as a whole.
> If you don't have concerns, can you please ack HID amd_sfh changes?

I'll do an immutable branch in case this needs pulling into the hid tree
later in the cycle.

In short that means I'll create a branch with just this series on top of v6.6-rc1
and push that out as ib-iio-hid-sensors-v6.6-rc1.
I'll then merge that into the IIO tree before I do a pull request.
The advantage of this being that it can be pulled into other trees as necessary
and keep the same git IDs etc so that git can cleanly unwind the splitting and
merging of the history to cover the different paths.

However, note this will be messy as the merge into IIO isn't clean. I'll fix it
up but please take a quick look at the testing branch of iio.git on kernel.org
where the results of that merge will be.  Some other channel types were added
recently. So the fix was obvious.

So applied to the branch ib-iio-hid-sensors-6.6-rc1.  I'll merge that into the
IIO tree. That will get pushed out as testing for the build bots to see if they can
find anything we missed before I push this out as togreg which is what
linux-next picks up.

Note the IB branch might be rebased if any test issues show up.

Thanks,

Jonathan

> 
> Thanks,
> --
> Basavaraj
> 


^ permalink raw reply

* Re: [PATCH v2 6/9] iio: Add channel type for chromaticity
From: Jonathan Cameron @ 2023-09-24 12:31 UTC (permalink / raw)
  To: Basavaraj Natikar
  Cc: jikos, benjamin.tissoires, lars, srinivas.pandruvada, linux-input,
	linux-iio
In-Reply-To: <20230919081054.2050714-7-Basavaraj.Natikar@amd.com>

On Tue, 19 Sep 2023 13:40:51 +0530
Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote:

> In most cases, ambient color sensors also support the x and y light
> colors, which represent the coordinates on the CIE 1931 chromaticity
> diagram. Thus, add channel type for chromaticity.
> 
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
One small thing I fixed up whilst applying.

Jonathan

> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 8 ++++++++
>  drivers/iio/industrialio-core.c         | 1 +
>  include/uapi/linux/iio/types.h          | 1 +
>  tools/iio/iio_event_monitor.c           | 1 +
>  4 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 4cf7ed9ca57b..0c9389ad3709 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -2186,3 +2186,11 @@ Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Represents light color temperature, which measures light color
>  		temperature in Kelvin.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_chromaticity_x_raw
> +What:		/sys/bus/iio/devices/iio:deviceX/in_chromaticity_y_raw
> +KernelVersion:	6.7
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The x and y light color coordinate on the CIE 1931 chromaticity
> +		diagram.
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index cba942cadf97..6dc4d2b296bb 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -91,6 +91,7 @@ static const char * const iio_chan_type_name_spec[] = {
>  	[IIO_PHASE] = "phase",
>  	[IIO_MASSCONCENTRATION] = "massconcentration",
>  	[IIO_COLORTEMP] = "colortemp",
> +	[IIO_CHROMATICITY] = "chromaticity",
>  };
>  
>  static const char * const iio_modifier_names[] = {
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index 08c20e540c13..4832c611c027 100644
> --- a/include/uapi/linux/iio/types.h
> +++ b/include/uapi/linux/iio/types.h
> @@ -48,6 +48,7 @@ enum iio_chan_type {
>  	IIO_PHASE,
>  	IIO_MASSCONCENTRATION,
>  	IIO_COLORTEMP,
> +	IIO_CHROMATICITY,
>  };
>  
>  enum iio_modifier {
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index a63741e43ddf..5edacc358c5d 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c
> @@ -175,6 +175,7 @@ static bool event_is_known(struct iio_event_data *event)
>  	case IIO_PHASE:
>  	case IIO_MASSCONCENTRATION:
>  	case IIO_COLORTEMP:
> +	case IIO_CHROMATICITY:
This is missing updating the strings in the same file.
I only notice whilst dealing with a merge conflict where the colortemp one was
there and this wasn't.
>  		break;
>  	default:
>  		return false;


^ permalink raw reply

* Re: [PATCH] HID: lenovo: Fix middle-button behaviour for system suspend
From: Jamie Lentin @ 2023-09-24  8:11 UTC (permalink / raw)
  To: Martin Kepplinger; +Cc: jikos, benjamin.tissoires, linux-kernel, linux-input
In-Reply-To: <20230921092140.120533-1-martink@posteo.de>

On 2023-09-21 10:21, Martin Kepplinger wrote:
> After system suspend the middle-button mode is being reset to
> compatibility mode which simply breaks functionality for the devices
> where native mode is configured during probe().
> 
> Fix this by setting native mode in reset_resume() for the appropriate
> devices.
> 
> Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for
> compact keyboards")
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---
>  drivers/hid/hid-lenovo.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 44763c0da444..d20562b9eca6 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -1344,6 +1344,28 @@ static int lenovo_input_configured(struct
> hid_device *hdev,
>  	return 0;
>  }
> 
> +static int __maybe_unused lenovo_resume(struct hid_device *hdev)
> +{
> +	int ret;
> +
> +	switch (hdev->product) {
> +	case USB_DEVICE_ID_LENOVO_CUSBKBD:
> +	case USB_DEVICE_ID_LENOVO_CBTKBD:
> +	case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
> +	case USB_DEVICE_ID_LENOVO_TPIIBTKBD:
> +		/* Switch middle button to native mode again */
> +		ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);

This will only reset the middle button state and not the Function key 
state, which I believe has similar problems. This was my attempt at 
solving this:

   
https://github.com/lentinj/linux/commit/f1c4e2de780abf8526bcdc9496c463f1ff4fe53b

...which should ensure everything is in a consistent state with what the 
kernel expects.

I never submitted this since sending commands was sporadically resulting 
in timeouts, although I'm fairly sure it was unrelated to this patch, 
and quite possibly a hardware problem with my keyboard. I'd be 
interested to know how you get on.

Also, the above will send the command to both the USB keyboard & mouse 
devices, only the mouse will respond. So worth prepending something 
like:

	if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
	    (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
	    hdev->type != HID_TYPE_USBMOUSE) {
		hid_dbg(hdev, "Ignoring keyboard half of device\n");
		return 0;
	}

...to avoid sending known-useless messages.

> +		if (ret)
> +			hid_warn(hdev, "Failed to switch middle button: %d\n",
> +				 ret);
> +		break;
> +	default:
> +		ret = 0;
> +		break;
> +	}
> +
> +	return ret;
> +}
> 
>  static const struct hid_device_id lenovo_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
> @@ -1380,6 +1402,9 @@ static struct hid_driver lenovo_driver = {
>  	.raw_event = lenovo_raw_event,
>  	.event = lenovo_event,
>  	.report_fixup = lenovo_report_fixup,
> +#ifdef CONFIG_PM
> +	.reset_resume = lenovo_resume,
> +#endif
>  };
>  module_hid_driver(lenovo_driver);

^ permalink raw reply

* Re: [PATCH] Input: synaptics-rmi4 - replace deprecated strncpy
From: Kees Cook @ 2023-09-24  3:32 UTC (permalink / raw)
  To: Justin Stitt; +Cc: Dmitry Torokhov, linux-input, linux-kernel, linux-hardening
In-Reply-To: <20230921-strncpy-drivers-input-rmi4-rmi_f34-c-v1-1-4aef2e84b8d2@google.com>

On Thu, Sep 21, 2023 at 09:58:11AM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1]
> 
> Let's use memcpy() as the bounds have already been checked and this
> decays into a simple byte copy from one buffer to another removing any
> ambiguity that strncpy has.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Note: build-tested only.
> 
> Similar to Kees' suggestion here [2]
> 
> [2]: https://lore.kernel.org/all/202309142045.7CBAE940E@keescook/

Agreed. This is best as memcpy.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  drivers/input/rmi4/rmi_f34.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f34.c b/drivers/input/rmi4/rmi_f34.c
> index 0d9a5756e3f5..3b3ac71e53dc 100644
> --- a/drivers/input/rmi4/rmi_f34.c
> +++ b/drivers/input/rmi4/rmi_f34.c
> @@ -471,7 +471,7 @@ static ssize_t rmi_driver_update_fw_store(struct device *dev,
>  	if (buf[count - 1] == '\0' || buf[count - 1] == '\n')
>  		copy_count -= 1;
>  
> -	strncpy(fw_name, buf, copy_count);
> +	memcpy(fw_name, buf, copy_count);
>  	fw_name[copy_count] = '\0';
>  
>  	ret = request_firmware(&fw, fw_name, dev);
> 
> ---
> base-commit: 2cf0f715623872823a72e451243bbf555d10d032
> change-id: 20230921-strncpy-drivers-input-rmi4-rmi_f34-c-4a6945316cea
> 
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
> 

-- 
Kees Cook

^ permalink raw reply

* Re: [PATCH] Input: axp20x-pek - refactor deprecated strncpy
From: Kees Cook @ 2023-09-24  3:29 UTC (permalink / raw)
  To: Justin Stitt
  Cc: Dmitry Torokhov, Chen-Yu Tsai, linux-input, linux-kernel,
	linux-hardening
In-Reply-To: <20230921-strncpy-drivers-input-misc-axp20x-pek-c-v1-1-f7f6f4a5cf81@google.com>

On Thu, Sep 21, 2023 at 09:17:25AM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
> 
> Ensuring we have a trailing NUL-byte and checking the length of bytes
> copied are both intrinsic behavior of strscpy.
> 
> Therefore, a suitable replacement is `strscpy` [2] due to the fact that
> it guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
> 
> It should be noted that the original code can silently truncate and so
> can this refactoring. However, a check could be added if truncation
> is an issue:
> | len = strscpy(val_str, buf, sizeof(val_str));
> | if (len < 0) { // add this
> |   return -E2BIG; // or -EINVAL
> | }
> 
> Also, now check for `len > 0` instead of just a truthy `len` because
> `len` is now a signed type and we could run into problems if strscpy
> returned -E2BIG which would pass the truthy test.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Note: build-tested only.
> ---
>  drivers/input/misc/axp20x-pek.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
> index 4581606a28d6..abcf78785b45 100644
> --- a/drivers/input/misc/axp20x-pek.c
> +++ b/drivers/input/misc/axp20x-pek.c
> @@ -134,16 +134,14 @@ static ssize_t axp20x_store_attr(struct device *dev,
>  {
>  	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
>  	char val_str[20];
> -	size_t len;
> +	ssize_t len;
>  	int ret, i;
>  	unsigned int val, idx = 0;
>  	unsigned int best_err = UINT_MAX;
>  
> -	val_str[sizeof(val_str) - 1] = '\0';
> -	strncpy(val_str, buf, sizeof(val_str) - 1);
> -	len = strlen(val_str);
> +	len = strscpy(val_str, buf, sizeof(val_str));
>  
> -	if (len && val_str[len - 1] == '\n')
> +	if (len > 0 && val_str[len - 1] == '\n')
>  		val_str[len - 1] = '\0';
>  
>  	ret = kstrtouint(val_str, 10, &val);

This code is doing a LOT of work before handing it off to kstrtouint(),
and none of it is needed. val_str is never used again, and the work is
to make sure the newline is dropped -- but kstrtouint() does this
already. I think this can just be:


diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
index 4581606a28d6..b1389a4c7702 100644
--- a/drivers/input/misc/axp20x-pek.c
+++ b/drivers/input/misc/axp20x-pek.c
@@ -134,19 +134,11 @@ static ssize_t axp20x_store_attr(struct device *dev,
 {
 	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
 	char val_str[20];
-	size_t len;
 	int ret, i;
 	unsigned int val, idx = 0;
 	unsigned int best_err = UINT_MAX;
 
-	val_str[sizeof(val_str) - 1] = '\0';
-	strncpy(val_str, buf, sizeof(val_str) - 1);
-	len = strlen(val_str);
-
-	if (len && val_str[len - 1] == '\n')
-		val_str[len - 1] = '\0';
-
-	ret = kstrtouint(val_str, 10, &val);
+	ret = kstrtouint(buf, 10, &val);
 	if (ret)
 		return ret;
 
And, [broken record] for v2, please update the Subject to better describe
the resulting change. :)

-Kees

-- 
Kees Cook

^ permalink raw reply related

* Re: [PATCH 00/52] input: Convert to platform remove callback returning void
From: Dmitry Torokhov @ 2023-09-24  2:48 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Michael Hennerich, linux-input, kernel, Benson Leung,
	Guenter Roeck, Greg Kroah-Hartman, Jonathan Cameron,
	joewu (吳仲振), chrome-platform,
	Andy Shevchenko, Mattijs Korpershoek, Jeff LaBundy, Rob Herring,
	Siarhei Volkau, Pavel Machek, Steven Rostedt (Google),
	Paolo Abeni, Kalle Valo, Yangtao Li, ye xingchen, Maxime Coquelin,
	Alexandre Torgue, linux-stm32, linux-arm-kernel,
	Support Opensource, Andrey Moiseev, Lee Jones, Linus Walleij,
	Andy Gross, Bjorn Andersson, Konrad Dybcio, linux-arm-msm,
	Hans de Goede, Miloslav Trmac, patches, Christophe JAILLET,
	Liang He, Chen Jun, Ruan Jinjie, Chen-Yu Tsai, Jernej Skrabec,
	Samuel Holland, linux-sunxi, Michal Simek, Robert Jarzmik,
	Dmitry Baryshkov, Arnd Bergmann, Rafael J. Wysocki,
	Krzysztof Kozlowski, Daniel Lezcano, Jonathan Corbet
In-Reply-To: <20230920125829.1478827-1-u.kleine-koenig@pengutronix.de>

On Wed, Sep 20, 2023 at 02:57:37PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> this series converts all platform drivers below drivers/input to use
> remove_new. The motivation is to get rid of an integer return code
> that is (mostly) ignored by the platform driver core and error prone on
> the driver side.
> 
> See commit 5c5a7680e67b ("platform: Provide a remove callback that
> returns no value") for an extended explanation and the eventual goal.
> 
> There are no interdependencies between the patches. As there are still
> quite a few drivers to convert, I'm happy about every patch that makes
> it in. So even if there is a merge conflict with one patch until you
> apply or a subject prefix is suboptimal, please apply the remainder of
> this series anyhow.

Applied the lot (fixing the i8042-sparcio patch subject), thank you!

-- 
Dmitry

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox