linux-kernel - Re: [PATCH 2/2] media: ov5675: Elongate reset to first transaction minimum gap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aea131bf-416e-4e58-a64b-0353b63340ea@linaro.org>
Date: Thu, 11 Jul 2024 12:24:32 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>,
 Quentin Schulz <quentin.schulz@...rry.de>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Quentin Schulz <quentin.schulz@...obroma-systems.com>,
 Jacopo Mondi <jacopo@...ndi.org>, Johan Hovold <johan@...nel.org>,
 Kieran Bingham <kieran.bingham@...asonboard.com>,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH 2/2] media: ov5675: Elongate reset to first transaction
 minimum gap

On 11/07/2024 12:17, Dave Stevenson wrote:
> Hi Quentin and Bryan
> 
> On Thu, 11 Jul 2024 at 11:40, Quentin Schulz <quentin.schulz@...rry.de> wrote:
>>
>> Hi Bryan,
>>
>> On 7/11/24 12:20 PM, Bryan O'Donoghue wrote:
>>> The ov5675 specification says that the gap between XSHUTDN deassert and the
>>> first I2C transaction should be a minimum of 8192 XVCLK cycles.
>>>
>>> Right now we use a usleep_rage() that gives a sleep time of between about
>>> 430 and 860 microseconds.
>>>
>>> On the Lenovo X13s we have observed that in about 1/20 cases the current
>>> timing is too tight and we start transacting before the ov5675's reset
>>> cycle completes, leading to I2C bus transaction failures.
>>>
>>> The reset racing is sometimes triggered at initial chip probe but, more
>>> usually on a subsequent power-off/power-on cycle e.g.
>>>
>>> [   71.451662] ov5675 24-0010: failed to write reg 0x0103. error = -5
>>> [   71.451686] ov5675 24-0010: failed to set plls
>>>
>>> The current quiescence period we have is too tight, doubling the minimum
>>> appears to fix the issue observed on X13s.
>>>
>>> Fixes: 49d9ad719e89 ("media: ov5675: add device-tree support and support runtime PM")
>>> Cc: stable@...r.kernel.org
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
>>> ---
>>>    drivers/media/i2c/ov5675.c | 9 +++++++--
>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>>> index 92bd35133a5d..0498f8f3064d 100644
>>> --- a/drivers/media/i2c/ov5675.c
>>> +++ b/drivers/media/i2c/ov5675.c
>>> @@ -1018,8 +1018,13 @@ static int ov5675_power_on(struct device *dev)
>>>
>>>        gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
>>>
>>> -     /* 8192 xvclk cycles prior to the first SCCB transation */
>>> -     usleep_range(delay_us, delay_us * 2);
>>> +     /* The spec calls for a minimum delay of 8192 XVCLK cycles prior to
>>> +      * transacting on the I2C bus, which translates to about 430
>>> +      * microseconds at 19.2 MHz.
>>> +      * Testing shows the range 8192 - 16384 cycles to be unreliable.
>>> +      * Grant a more liberal 2x -3x clock cycle grace time.
>>> +      */
>>> +     usleep_range(delay_us * 2, delay_us * 3);
>>>
>>
>> Would it make sense to have power_off have the same logic? We do a
>> usleep_range of those same values currently, so keeping them in sync
>> seems to make sense to me.
>>
>> Also, I'm wondering if it isn't an issue with the gpio not being high
>> right after gpoiod_set_value_cansleep() returns, i.e. the time it
>> actually takes for the HW to reach the IO level that means "high" for
>> the camera. And that this increased sleep is just a way to mitigate that?
>>
>> With this patch we essentially postpone the power_on by another 430ms
>> making it almost a full second before we can start using the camera.
>> That's quite a lot I think? We don't have a usecase right now that
>> requires this to be blazing fast (and we anyway would need at the very
>> least 430ms), so take this remark as what it is, a remark.
> 
> I think you've misread 430 usec as 430 msec.
> 
> I was looking at the series and trying to decide whether it's worth
> going to the effort of computing the time at all when even on the
> slowest 6MHz XVCLK we're sub 1.5ms for the required delay.
> At the max XVLCK of 24MHz you could save 1ms. I know of very few use
> cases that would suffer for a 1ms delay.
> 
> I know we all like to be precise, but it sounds like the precision
> actually causes grief in this situation.

Yeah the first draft of the patch just had a post-reset delay of I 
forget - I think I just used usleep_range(2000, 2200); again but I kind 
respected the attempt to hit the specification and wanted to fix the 
original logic, which is close but no cigar ATM.

---
bod