[PATCH 1/3] usb: misc: ljca: move usb_autopm_put_interface() after wait for response

[PATCH 1/3] usb: misc: ljca: move usb_autopm_put_interface() after wait for response

[Stanislaw Gruszka]

Do not mark interface as ready to suspend when we are still waiting
for response messages from the device.

Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
Cc: stable@vger.kernel.org
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/usb/misc/usb-ljca.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
index 01ceafc4ab78..dcb3c5d248ac 100644
--- a/drivers/usb/misc/usb-ljca.c
+++ b/drivers/usb/misc/usb-ljca.c
@@ -332,9 +332,6 @@ static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
 
 	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
 			   msg_len, &transferred, LJCA_WRITE_TIMEOUT_MS);
-
-	usb_autopm_put_interface(adap->intf);
-
 	if (ret < 0)
 		goto out;
 	if (transferred != msg_len) {
@@ -353,6 +350,8 @@ static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
 	ret = adap->actual_length;
 
 out:
+	usb_autopm_put_interface(adap->intf);
+
 	spin_lock_irqsave(&adap->lock, flags);
 	adap->ex_buf = NULL;
 	adap->ex_buf_len = 0;
-- 
2.34.1

[PATCH 2/3] usb: misc: ljca: set small runtime autosuspend delay

[Stanislaw Gruszka]

On some Lenovo platforms, the patch workarounds problems with ov2740
sensor initialization, which manifest themself like below:

[    4.540476] ov2740 i2c-INT3474:01: error -EIO: failed to find sensor
[    4.542066] ov2740 i2c-INT3474:01: probe with driver ov2740 failed with error -5

or

[    7.742633] ov2740 i2c-INT3474:01: chip id mismatch: 2740 != 0
[    7.742638] ov2740 i2c-INT3474:01: error -ENXIO: failed to find sensor

and also by random failures of video stream start.

Issue can be reproduced by this script:

n=0
k=0
while [ $n -lt 50 ] ; do
	sudo modprobe -r ov2740
	sleep `expr $RANDOM % 5`
	sudo modprobe ov2740
	if media-ctl -p  | grep -q ov2740 ; then
		let k++
	fi
	let n++
done
echo Success rate $k/$n

Without the patch, success rate is approximately 15 or 50 tries.
With the patch it does not fail.

This problem is some hardware or firmware malfunction, that can not be
easy debug and fix. While setting small autosuspend delay is not perfect
workaround as user can configure it to any value, it will prevent
the failures by default.

Additionally setting small autosuspend delay should have positive effect
on power consumption as for most ljca workloads device is used for just
a few milliseconds flowed by long periods of at least 100ms of inactivity
(usually more).

Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
Cc: stable@vger.kernel.org
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/usb/misc/usb-ljca.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
index dcb3c5d248ac..062b7fb47114 100644
--- a/drivers/usb/misc/usb-ljca.c
+++ b/drivers/usb/misc/usb-ljca.c
@@ -810,6 +810,7 @@ static int ljca_probe(struct usb_interface *interface,
 	if (ret)
 		goto err_free;
 
+	pm_runtime_set_autosuspend_delay(&usb_dev->dev, 10);
 	usb_enable_autosuspend(usb_dev);
 
 	return 0;
-- 
2.34.1

Re: [PATCH 2/3] usb: misc: ljca: set small runtime autosuspend delay

[Hans de Goede]

Hi,

On 4-Nov-24 9:50 AM, Stanislaw Gruszka wrote:
> On some Lenovo platforms, the patch workarounds problems with ov2740
> sensor initialization, which manifest themself like below:
> 
> [    4.540476] ov2740 i2c-INT3474:01: error -EIO: failed to find sensor
> [    4.542066] ov2740 i2c-INT3474:01: probe with driver ov2740 failed with error -5
> 
> or
> 
> [    7.742633] ov2740 i2c-INT3474:01: chip id mismatch: 2740 != 0
> [    7.742638] ov2740 i2c-INT3474:01: error -ENXIO: failed to find sensor
> 
> and also by random failures of video stream start.
> 
> Issue can be reproduced by this script:
> 
> n=0
> k=0
> while [ $n -lt 50 ] ; do
> 	sudo modprobe -r ov2740
> 	sleep `expr $RANDOM % 5`
> 	sudo modprobe ov2740
> 	if media-ctl -p  | grep -q ov2740 ; then
> 		let k++
> 	fi
> 	let n++
> done
> echo Success rate $k/$n
> 
> Without the patch, success rate is approximately 15 or 50 tries.
> With the patch it does not fail.
> 
> This problem is some hardware or firmware malfunction, that can not be
> easy debug and fix. While setting small autosuspend delay is not perfect
> workaround as user can configure it to any value, it will prevent
> the failures by default.
> 
> Additionally setting small autosuspend delay should have positive effect
> on power consumption as for most ljca workloads device is used for just
> a few milliseconds flowed by long periods of at least 100ms of inactivity
> (usually more).
> 
> Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

Thank you so much for looking into this and fixing it!

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

I have also given this a test run on a "ThinkPad X1 Yoga Gen 8" and
everything there works at least as well as before:

Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740

Regards,

Hans

p.s.

I take it from the commit message that you have no clear idea what exactly is
happening in the failure case ?







> ---
>  drivers/usb/misc/usb-ljca.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> index dcb3c5d248ac..062b7fb47114 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -810,6 +810,7 @@ static int ljca_probe(struct usb_interface *interface,
>  	if (ret)
>  		goto err_free;
>  
> +	pm_runtime_set_autosuspend_delay(&usb_dev->dev, 10);
>  	usb_enable_autosuspend(usb_dev);
>  
>  	return 0;

Re: [PATCH 2/3] usb: misc: ljca: set small runtime autosuspend delay

[Stanislaw Gruszka]

On Mon, Nov 04, 2024 at 02:50:31PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 4-Nov-24 9:50 AM, Stanislaw Gruszka wrote:
> > On some Lenovo platforms, the patch workarounds problems with ov2740
> > sensor initialization, which manifest themself like below:
> > 
> > [    4.540476] ov2740 i2c-INT3474:01: error -EIO: failed to find sensor
> > [    4.542066] ov2740 i2c-INT3474:01: probe with driver ov2740 failed with error -5
> > 
> > or
> > 
> > [    7.742633] ov2740 i2c-INT3474:01: chip id mismatch: 2740 != 0
> > [    7.742638] ov2740 i2c-INT3474:01: error -ENXIO: failed to find sensor
> > 
> > and also by random failures of video stream start.
> > 
> > Issue can be reproduced by this script:
> > 
> > n=0
> > k=0
> > while [ $n -lt 50 ] ; do
> > 	sudo modprobe -r ov2740
> > 	sleep `expr $RANDOM % 5`
> > 	sudo modprobe ov2740
> > 	if media-ctl -p  | grep -q ov2740 ; then
> > 		let k++
> > 	fi
> > 	let n++
> > done
> > echo Success rate $k/$n
> > 
> > Without the patch, success rate is approximately 15 or 50 tries.
> > With the patch it does not fail.
> > 
> > This problem is some hardware or firmware malfunction, that can not be
> > easy debug and fix. While setting small autosuspend delay is not perfect
> > workaround as user can configure it to any value, it will prevent
> > the failures by default.
> > 
> > Additionally setting small autosuspend delay should have positive effect
> > on power consumption as for most ljca workloads device is used for just
> > a few milliseconds flowed by long periods of at least 100ms of inactivity
> > (usually more).
> > 
> > Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> 
> Thank you so much for looking into this and fixing it!
> 
> Patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> I have also given this a test run on a "ThinkPad X1 Yoga Gen 8" and
> everything there works at least as well as before:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> I take it from the commit message that you have no clear idea what exactly is
> happening in the failure case ?

Yes, that's correct. We have only some suspicions, but none of them
was confirmed.

Regards
Stanislaw

Re: [PATCH 2/3] usb: misc: ljca: set small runtime autosuspend delay

[Sakari Ailus]

Hi Stanislaw,

On Mon, Nov 04, 2024 at 09:50:55AM +0100, Stanislaw Gruszka wrote:
> On some Lenovo platforms, the patch workarounds problems with ov2740
> sensor initialization, which manifest themself like below:
> 
> [    4.540476] ov2740 i2c-INT3474:01: error -EIO: failed to find sensor
> [    4.542066] ov2740 i2c-INT3474:01: probe with driver ov2740 failed with error -5
> 
> or
> 
> [    7.742633] ov2740 i2c-INT3474:01: chip id mismatch: 2740 != 0
> [    7.742638] ov2740 i2c-INT3474:01: error -ENXIO: failed to find sensor
> 
> and also by random failures of video stream start.
> 
> Issue can be reproduced by this script:
> 
> n=0
> k=0
> while [ $n -lt 50 ] ; do
> 	sudo modprobe -r ov2740
> 	sleep `expr $RANDOM % 5`
> 	sudo modprobe ov2740
> 	if media-ctl -p  | grep -q ov2740 ; then
> 		let k++
> 	fi
> 	let n++
> done
> echo Success rate $k/$n
> 
> Without the patch, success rate is approximately 15 or 50 tries.
> With the patch it does not fail.
> 
> This problem is some hardware or firmware malfunction, that can not be
> easy debug and fix. While setting small autosuspend delay is not perfect
> workaround as user can configure it to any value, it will prevent
> the failures by default.
> 
> Additionally setting small autosuspend delay should have positive effect
> on power consumption as for most ljca workloads device is used for just
> a few milliseconds flowed by long periods of at least 100ms of inactivity
> (usually more).

I'm not very happy about this patch. While it makes the problem go away,
apparently, the result seems to be for a reason that should have nothing to
do with the underlying issue.

I'm still not saying no to the patch as it hides the problem or at least,
but we should properly describe the problem in the driver. It may well be
that after an unrelated update elsewhere in the kernel the problem
reappears again.

> 
> Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  drivers/usb/misc/usb-ljca.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> index dcb3c5d248ac..062b7fb47114 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -810,6 +810,7 @@ static int ljca_probe(struct usb_interface *interface,
>  	if (ret)
>  		goto err_free;
>  
> +	pm_runtime_set_autosuspend_delay(&usb_dev->dev, 10);
>  	usb_enable_autosuspend(usb_dev);
>  
>  	return 0;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/3] usb: misc: ljca: set small runtime autosuspend delay

[Stanislaw Gruszka]

Hello Sakari,

On Mon, Nov 04, 2024 at 03:13:53PM +0000, Sakari Ailus wrote:
> Hi Stanislaw,
> 
> On Mon, Nov 04, 2024 at 09:50:55AM +0100, Stanislaw Gruszka wrote:
> > On some Lenovo platforms, the patch workarounds problems with ov2740
> > sensor initialization, which manifest themself like below:
> > 
> > [    4.540476] ov2740 i2c-INT3474:01: error -EIO: failed to find sensor
> > [    4.542066] ov2740 i2c-INT3474:01: probe with driver ov2740 failed with error -5
> > 
> > or
> > 
> > [    7.742633] ov2740 i2c-INT3474:01: chip id mismatch: 2740 != 0
> > [    7.742638] ov2740 i2c-INT3474:01: error -ENXIO: failed to find sensor
> > 
> > and also by random failures of video stream start.
> > 
> > Issue can be reproduced by this script:
> > 
> > n=0
> > k=0
> > while [ $n -lt 50 ] ; do
> > 	sudo modprobe -r ov2740
> > 	sleep `expr $RANDOM % 5`
> > 	sudo modprobe ov2740
> > 	if media-ctl -p  | grep -q ov2740 ; then
> > 		let k++
> > 	fi
> > 	let n++
> > done
> > echo Success rate $k/$n
> > 
> > Without the patch, success rate is approximately 15 or 50 tries.
> > With the patch it does not fail.
> > 
> > This problem is some hardware or firmware malfunction, that can not be
> > easy debug and fix. While setting small autosuspend delay is not perfect
> > workaround as user can configure it to any value, it will prevent
> > the failures by default.
> > 
> > Additionally setting small autosuspend delay should have positive effect
> > on power consumption as for most ljca workloads device is used for just
> > a few milliseconds flowed by long periods of at least 100ms of inactivity
> > (usually more).
> 
> I'm not very happy about this patch. While it makes the problem go away,
I'm not very happy having unreliable camera on my laptop ;-)

> apparently, the result seems to be for a reason that should have nothing to
> do with the underlying issue.
I can not be certain here, but I think when we do suspend ljca device
it either resets part of HW internally or do some latch of output gpio pins.
I think that is related to the root of the problem.

> I'm still not saying no to the patch as it hides the problem or at least,
> but we should properly describe the problem in the driver.
Ok, I can add comment before setting the delay.

Regards
Stanislaw

> It may well be
> that after an unrelated update elsewhere in the kernel the problem
> reappears again.
> 
> > 
> > Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > ---
> >  drivers/usb/misc/usb-ljca.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> > index dcb3c5d248ac..062b7fb47114 100644
> > --- a/drivers/usb/misc/usb-ljca.c
> > +++ b/drivers/usb/misc/usb-ljca.c
> > @@ -810,6 +810,7 @@ static int ljca_probe(struct usb_interface *interface,
> >  	if (ret)
> >  		goto err_free;
> >  
> > +	pm_runtime_set_autosuspend_delay(&usb_dev->dev, 10);
> >  	usb_enable_autosuspend(usb_dev);
> >  
> >  	return 0;
> 
> -- 
> Kind regards,
> 
> Sakari Ailus

[PATCH 3/3] usb: misc: ljca: print firmware version

[Stanislaw Gruszka]

For diagnostics purposes read firmware version from device
and print it to dmesg during initialization.

Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/usb/misc/usb-ljca.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
index 062b7fb47114..0f8751c51bf6 100644
--- a/drivers/usb/misc/usb-ljca.c
+++ b/drivers/usb/misc/usb-ljca.c
@@ -43,6 +43,7 @@ enum ljca_client_type {
 
 /* MNG client commands */
 enum ljca_mng_cmd {
+	LJCA_MNG_GET_VERSION = 1,
 	LJCA_MNG_RESET = 2,
 	LJCA_MNG_ENUM_GPIO = 4,
 	LJCA_MNG_ENUM_I2C = 5,
@@ -68,6 +69,13 @@ struct ljca_msg {
 	u8 data[] __counted_by(len);
 } __packed;
 
+struct ljca_fw_version {
+	u8 major;
+	u8 minor;
+	__le16 patch;
+	__le16 build;
+} __packed;
+
 struct ljca_i2c_ctr_info {
 	u8 id;
 	u8 capacity;
@@ -694,6 +702,24 @@ static int ljca_reset_handshake(struct ljca_adapter *adap)
 	return 0;
 }
 
+static void ljca_print_fw_version(struct ljca_adapter *adap)
+{
+	struct ljca_fw_version version = {};
+	int ret;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_GET_VERSION, NULL, 0,
+			(u8 *)&version, sizeof(version), true, LJCA_WRITE_ACK_TIMEOUT_MS);
+
+	if (ret != sizeof(version)) {
+		dev_err(adap->dev, "Get version failed, ret: %d\n", ret);
+		return;
+	}
+
+	dev_info(adap->dev, "Firmware version: %d.%d.%d.%d\n",
+		 version.major, version.minor,
+		 le16_to_cpu(version.patch), le16_to_cpu(version.build));
+}
+
 static int ljca_enumerate_clients(struct ljca_adapter *adap)
 {
 	struct ljca_client *client, *next;
@@ -810,6 +836,8 @@ static int ljca_probe(struct usb_interface *interface,
 	if (ret)
 		goto err_free;
 
+	ljca_print_fw_version(adap);
+
 	pm_runtime_set_autosuspend_delay(&usb_dev->dev, 10);
 	usb_enable_autosuspend(usb_dev);
 
-- 
2.34.1

Re: [PATCH 3/3] usb: misc: ljca: print firmware version

[Hans de Goede]

Hi,

On 4-Nov-24 9:50 AM, Stanislaw Gruszka wrote:
> For diagnostics purposes read firmware version from device
> and print it to dmesg during initialization.
> 
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

I have also given this a test run on a "ThinkPad X1 Yoga Gen 8" and
everything there works at least as well as before:

Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740

On this laptop the reported LJCA firmware version is:

[   25.704000] ljca 3-8:1.0: Firmware version: 1.0.0.544

Regards,

Hans



> ---
>  drivers/usb/misc/usb-ljca.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> index 062b7fb47114..0f8751c51bf6 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -43,6 +43,7 @@ enum ljca_client_type {
>  
>  /* MNG client commands */
>  enum ljca_mng_cmd {
> +	LJCA_MNG_GET_VERSION = 1,
>  	LJCA_MNG_RESET = 2,
>  	LJCA_MNG_ENUM_GPIO = 4,
>  	LJCA_MNG_ENUM_I2C = 5,
> @@ -68,6 +69,13 @@ struct ljca_msg {
>  	u8 data[] __counted_by(len);
>  } __packed;
>  
> +struct ljca_fw_version {
> +	u8 major;
> +	u8 minor;
> +	__le16 patch;
> +	__le16 build;
> +} __packed;
> +
>  struct ljca_i2c_ctr_info {
>  	u8 id;
>  	u8 capacity;
> @@ -694,6 +702,24 @@ static int ljca_reset_handshake(struct ljca_adapter *adap)
>  	return 0;
>  }
>  
> +static void ljca_print_fw_version(struct ljca_adapter *adap)
> +{
> +	struct ljca_fw_version version = {};
> +	int ret;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_GET_VERSION, NULL, 0,
> +			(u8 *)&version, sizeof(version), true, LJCA_WRITE_ACK_TIMEOUT_MS);
> +
> +	if (ret != sizeof(version)) {
> +		dev_err(adap->dev, "Get version failed, ret: %d\n", ret);
> +		return;
> +	}
> +
> +	dev_info(adap->dev, "Firmware version: %d.%d.%d.%d\n",
> +		 version.major, version.minor,
> +		 le16_to_cpu(version.patch), le16_to_cpu(version.build));
> +}
> +
>  static int ljca_enumerate_clients(struct ljca_adapter *adap)
>  {
>  	struct ljca_client *client, *next;
> @@ -810,6 +836,8 @@ static int ljca_probe(struct usb_interface *interface,
>  	if (ret)
>  		goto err_free;
>  
> +	ljca_print_fw_version(adap);
> +
>  	pm_runtime_set_autosuspend_delay(&usb_dev->dev, 10);
>  	usb_enable_autosuspend(usb_dev);
>  

Re: [PATCH 3/3] usb: misc: ljca: print firmware version

[Sakari Ailus]

Hi Stanislaw,

Thanks for the patch.

On Mon, Nov 04, 2024 at 09:50:56AM +0100, Stanislaw Gruszka wrote:
> For diagnostics purposes read firmware version from device
> and print it to dmesg during initialization.
> 
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  drivers/usb/misc/usb-ljca.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> index 062b7fb47114..0f8751c51bf6 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -43,6 +43,7 @@ enum ljca_client_type {
>  
>  /* MNG client commands */
>  enum ljca_mng_cmd {
> +	LJCA_MNG_GET_VERSION = 1,
>  	LJCA_MNG_RESET = 2,
>  	LJCA_MNG_ENUM_GPIO = 4,
>  	LJCA_MNG_ENUM_I2C = 5,
> @@ -68,6 +69,13 @@ struct ljca_msg {
>  	u8 data[] __counted_by(len);
>  } __packed;
>  
> +struct ljca_fw_version {
> +	u8 major;
> +	u8 minor;
> +	__le16 patch;
> +	__le16 build;
> +} __packed;
> +
>  struct ljca_i2c_ctr_info {
>  	u8 id;
>  	u8 capacity;
> @@ -694,6 +702,24 @@ static int ljca_reset_handshake(struct ljca_adapter *adap)
>  	return 0;
>  }
>  
> +static void ljca_print_fw_version(struct ljca_adapter *adap)
> +{
> +	struct ljca_fw_version version = {};
> +	int ret;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_GET_VERSION, NULL, 0,
> +			(u8 *)&version, sizeof(version), true, LJCA_WRITE_ACK_TIMEOUT_MS);

This would be nicer wrapped to up to 80 chars per line.

> +
> +	if (ret != sizeof(version)) {
> +		dev_err(adap->dev, "Get version failed, ret: %d\n", ret);
> +		return;
> +	}
> +
> +	dev_info(adap->dev, "Firmware version: %d.%d.%d.%d\n",
> +		 version.major, version.minor,
> +		 le16_to_cpu(version.patch), le16_to_cpu(version.build));
> +}
> +
>  static int ljca_enumerate_clients(struct ljca_adapter *adap)
>  {
>  	struct ljca_client *client, *next;
> @@ -810,6 +836,8 @@ static int ljca_probe(struct usb_interface *interface,
>  	if (ret)
>  		goto err_free;
>  
> +	ljca_print_fw_version(adap);
> +
>  	pm_runtime_set_autosuspend_delay(&usb_dev->dev, 10);
>  	usb_enable_autosuspend(usb_dev);
>  

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/3] usb: misc: ljca: move usb_autopm_put_interface() after wait for response

[Hans de Goede]

Hi,

On 4-Nov-24 9:50 AM, Stanislaw Gruszka wrote:
> Do not mark interface as ready to suspend when we are still waiting
> for response messages from the device.
> 
> Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  drivers/usb/misc/usb-ljca.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> index 01ceafc4ab78..dcb3c5d248ac 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -332,9 +332,6 @@ static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
>  
>  	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
>  			   msg_len, &transferred, LJCA_WRITE_TIMEOUT_MS);
> -
> -	usb_autopm_put_interface(adap->intf);
> -
>  	if (ret < 0)
>  		goto out;
>  	if (transferred != msg_len) {
> @@ -353,6 +350,8 @@ static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
>  	ret = adap->actual_length;
>  
>  out:
> +	usb_autopm_put_interface(adap->intf);
> +

I'm afraid that this now does a double pm_runtime_put() on
usb_autopm_get_interface() failures. usb_autopm_get_interface() uses
pm_runtime_resume_and_get() which already does a pm_runtime_put()
on failure.

So you need to add a second out label which skips the new
location of the usb_autopm_put_interface(adap->intf); call
and jump to this second label on usb_autopm_get_interface()
failures.

With that fixed this is:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

I have also given this a test run on a "ThinkPad X1 Yoga Gen 8" and
everything there works at least as well as before:

Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740

Regards,

Hans




>  	spin_lock_irqsave(&adap->lock, flags);
>  	adap->ex_buf = NULL;
>  	adap->ex_buf_len = 0;

Re: [PATCH 1/3] usb: misc: ljca: move usb_autopm_put_interface() after wait for response

[Stanislaw Gruszka]

On Mon, Nov 04, 2024 at 02:39:57PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 4-Nov-24 9:50 AM, Stanislaw Gruszka wrote:
> > Do not mark interface as ready to suspend when we are still waiting
> > for response messages from the device.
> > 
> > Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > ---
> >  drivers/usb/misc/usb-ljca.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> > index 01ceafc4ab78..dcb3c5d248ac 100644
> > --- a/drivers/usb/misc/usb-ljca.c
> > +++ b/drivers/usb/misc/usb-ljca.c
> > @@ -332,9 +332,6 @@ static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> >  
> >  	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> >  			   msg_len, &transferred, LJCA_WRITE_TIMEOUT_MS);
> > -
> > -	usb_autopm_put_interface(adap->intf);
> > -
> >  	if (ret < 0)
> >  		goto out;
> >  	if (transferred != msg_len) {
> > @@ -353,6 +350,8 @@ static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> >  	ret = adap->actual_length;
> >  
> >  out:
> > +	usb_autopm_put_interface(adap->intf);
> > +
> 
> I'm afraid that this now does a double pm_runtime_put() on
> usb_autopm_get_interface() failures. usb_autopm_get_interface() uses
> pm_runtime_resume_and_get() which already does a pm_runtime_put()
> on failure.
> 
> So you need to add a second out label which skips the new
> location of the usb_autopm_put_interface(adap->intf); call
> and jump to this second label on usb_autopm_get_interface()
> failures.

Good point, thanks for catching this, I will fix and post v2.

Regards
Stanislaw