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

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

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