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

[PATCH v3 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
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
v2: fix handing error of usb_autopm_get_interface(),
    add R-b, T-b tags from Hans
v3: add A-b tag from Sakari

 drivers/usb/misc/usb-ljca.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
index 01ceafc4ab78..8056c65e4548 100644
--- a/drivers/usb/misc/usb-ljca.c
+++ b/drivers/usb/misc/usb-ljca.c
@@ -332,14 +332,11 @@ 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;
+		goto out_put;
 	if (transferred != msg_len) {
 		ret = -EIO;
-		goto out;
+		goto out_put;
 	}
 
 	if (ack) {
@@ -347,11 +344,14 @@ static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
 						  timeout);
 		if (!ret) {
 			ret = -ETIMEDOUT;
-			goto out;
+			goto out_put;
 		}
 	}
 	ret = adap->actual_length;
 
+out_put:
+	usb_autopm_put_interface(adap->intf);
+
 out:
 	spin_lock_irqsave(&adap->lock, flags);
 	adap->ex_buf = NULL;
-- 
2.34.1

[PATCH v3 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
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
v2: add R-b, T-b tags from Hans
v3: add A-b tag from Sakari

 drivers/usb/misc/usb-ljca.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
index 8056c65e4548..d9c21f783055 100644
--- a/drivers/usb/misc/usb-ljca.c
+++ b/drivers/usb/misc/usb-ljca.c
@@ -811,6 +811,14 @@ static int ljca_probe(struct usb_interface *interface,
 	if (ret)
 		goto err_free;
 
+	/*
+	 * This works around problems with ov2740 initialization on some
+	 * Lenovo platforms. The autosuspend delay, has to be smaller than
+	 * the delay after setting the reset_gpio line in ov2740_resume().
+	 * Otherwise the sensor randomly fails to initialize.
+	 */
+	pm_runtime_set_autosuspend_delay(&usb_dev->dev, 10);
+
 	usb_enable_autosuspend(usb_dev);
 
 	return 0;
-- 
2.34.1

[PATCH v3 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.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
v2: 80-chars line wrap
v3: use dev_dbg for printing the firmware version

 drivers/usb/misc/usb-ljca.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
index d9c21f783055..9a44acc48898 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;
@@ -695,6 +703,25 @@ 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_dbg(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;
@@ -811,6 +838,8 @@ static int ljca_probe(struct usb_interface *interface,
 	if (ret)
 		goto err_free;
 
+	ljca_print_fw_version(adap);
+
 	/*
 	 * This works around problems with ov2740 initialization on some
 	 * Lenovo platforms. The autosuspend delay, has to be smaller than
-- 
2.34.1

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

[Greg KH]

On Thu, Nov 07, 2024 at 08:37:18AM +0100, Stanislaw Gruszka wrote:
> For diagnostics purposes read firmware version from device
> and print it to dmesg during initialization.

This changelog text isn't correct anymore :(

thanks,

greg k-h

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

[Stanislaw Gruszka]

On Thu, Nov 07, 2024 at 09:37:34AM +0100, Greg KH wrote:
> On Thu, Nov 07, 2024 at 08:37:18AM +0100, Stanislaw Gruszka wrote:
> > For diagnostics purposes read firmware version from device
> > and print it to dmesg during initialization.
> 
> This changelog text isn't correct anymore :(

I'll drop this one from the set.
 
Regards
Stanislaw

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

[Stanislaw Gruszka]

On Thu, Nov 07, 2024 at 10:27:29AM +0100, Stanislaw Gruszka wrote:
> On Thu, Nov 07, 2024 at 09:37:34AM +0100, Greg KH wrote:
> > On Thu, Nov 07, 2024 at 08:37:18AM +0100, Stanislaw Gruszka wrote:
> > > For diagnostics purposes read firmware version from device
> > > and print it to dmesg during initialization.
> > 
> > This changelog text isn't correct anymore :(
> 
> I'll drop this one from the set.

It was decided infernally to rewrite this patch to use
sysfs attribute for providing FW version. I'll do this and
will keep the patch in the next posting of the patchset.

Regards
Stanislaw

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

[Greg KH]

On Thu, Nov 07, 2024 at 08:37:16AM +0100, 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
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
> v2: fix handing error of usb_autopm_get_interface(),
>     add R-b, T-b tags from Hans
> v3: add A-b tag from Sakari
> 
>  drivers/usb/misc/usb-ljca.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

For some reason you aren't sending this to the usb maintainer to be
applied, oh well...

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

[Stanislaw Gruszka]

On Thu, Nov 07, 2024 at 09:16:01AM +0100, Greg KH wrote:
> On Thu, Nov 07, 2024 at 08:37:16AM +0100, 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
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > ---
> > v2: fix handing error of usb_autopm_get_interface(),
> >     add R-b, T-b tags from Hans
> > v3: add A-b tag from Sakari
> > 
> >  drivers/usb/misc/usb-ljca.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> For some reason you aren't sending this to the usb maintainer to be
> applied, oh well...

Sorry, I thought sending patches to the mailing list is sufficient
to get them applied. 

Regards
Stanislaw

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

[Greg KH]

On Thu, Nov 07, 2024 at 09:55:01AM +0100, Stanislaw Gruszka wrote:
> On Thu, Nov 07, 2024 at 09:16:01AM +0100, Greg KH wrote:
> > On Thu, Nov 07, 2024 at 08:37:16AM +0100, 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
> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
> > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > > ---
> > > v2: fix handing error of usb_autopm_get_interface(),
> > >     add R-b, T-b tags from Hans
> > > v3: add A-b tag from Sakari
> > > 
> > >  drivers/usb/misc/usb-ljca.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > For some reason you aren't sending this to the usb maintainer to be
> > applied, oh well...
> 
> Sorry, I thought sending patches to the mailing list is sufficient
> to get them applied. 

Please work with the Intel internal group to get their approval and
review before sending out your next round of patches for this, as they
know how to catch trivial stuff like this before forcing others to point
it out :)

thanks,

greg k-h

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

[Sakari Ailus]

Hi Greg,

On Thu, Nov 07, 2024 at 10:03:56AM +0100, Greg KH wrote:
> On Thu, Nov 07, 2024 at 09:55:01AM +0100, Stanislaw Gruszka wrote:
> > On Thu, Nov 07, 2024 at 09:16:01AM +0100, Greg KH wrote:
> > > On Thu, Nov 07, 2024 at 08:37:16AM +0100, 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
> > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > > Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
> > > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > > > ---
> > > > v2: fix handing error of usb_autopm_get_interface(),
> > > >     add R-b, T-b tags from Hans
> > > > v3: add A-b tag from Sakari
> > > > 
> > > >  drivers/usb/misc/usb-ljca.c | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > For some reason you aren't sending this to the usb maintainer to be
> > > applied, oh well...
> > 
> > Sorry, I thought sending patches to the mailing list is sufficient
> > to get them applied. 
> 
> Please work with the Intel internal group to get their approval and
> review before sending out your next round of patches for this, as they
> know how to catch trivial stuff like this before forcing others to point
> it out :)

Stanislaw's choses recipients are aligned with get_maintainer.pl output,
sauf some who posted occasional cleanups. If that list wasn't correct, then
there's something to fix in MAINTAINERS.

-- 
Kind regards,

Sakari Ailus

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

[Greg KH]

On Thu, Nov 07, 2024 at 09:32:38AM +0000, Sakari Ailus wrote:
> Hi Greg,
> 
> On Thu, Nov 07, 2024 at 10:03:56AM +0100, Greg KH wrote:
> > On Thu, Nov 07, 2024 at 09:55:01AM +0100, Stanislaw Gruszka wrote:
> > > On Thu, Nov 07, 2024 at 09:16:01AM +0100, Greg KH wrote:
> > > > On Thu, Nov 07, 2024 at 08:37:16AM +0100, 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
> > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > > > Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
> > > > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > > > > ---
> > > > > v2: fix handing error of usb_autopm_get_interface(),
> > > > >     add R-b, T-b tags from Hans
> > > > > v3: add A-b tag from Sakari
> > > > > 
> > > > >  drivers/usb/misc/usb-ljca.c | 12 ++++++------
> > > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > 
> > > > For some reason you aren't sending this to the usb maintainer to be
> > > > applied, oh well...
> > > 
> > > Sorry, I thought sending patches to the mailing list is sufficient
> > > to get them applied. 
> > 
> > Please work with the Intel internal group to get their approval and
> > review before sending out your next round of patches for this, as they
> > know how to catch trivial stuff like this before forcing others to point
> > it out :)
> 
> Stanislaw's choses recipients are aligned with get_maintainer.pl output,
> sauf some who posted occasional cleanups. If that list wasn't correct, then
> there's something to fix in MAINTAINERS.

Nothing is wrong with MAINTAINERS:

	$ ./scripts/get_maintainer.pl --file drivers/usb/misc/usb-ljca.c
	Wentong Wu <wentong.wu@intel.com> (maintainer:INTEL LA JOLLA COVE ADAPTER (LJCA) USB I/O EXPA...)
	Sakari Ailus <sakari.ailus@linux.intel.com> (maintainer:INTEL LA JOLLA COVE ADAPTER (LJCA) USB I/O EXPA...)
	Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
	linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
	linux-kernel@vger.kernel.org (open list)