linux-kernel - Re: [PATCH] media: i2c: Add note to prevent buggy code re-use

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: <462e5ec1-cc26-4003-ac5c-adde2c243959@oss.qualcomm.com>
Date: Tue, 30 Dec 2025 10:35:28 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@....qualcomm.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Hans Verkuil <hverkuil@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Pavel Machek <pavel@...nel.org>, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: i2c: Add note to prevent buggy code re-use

On 30/12/2025 10:05, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> Thanks for the patch.
> 
> On Tue, Dec 30, 2025 at 09:34:36AM +0100, Krzysztof Kozlowski wrote:
>> adv7604 and et8ek8 sensor drivers have mixed up logical and line level
>> for reset/powerdown signal.  They call it a reset signal (it indeed
>> behaves like that), but drivers assert the reset to operate which is
>> clearly incorrect and relies on wrong ACTIVE_HIGH flag in the DTS.
>>
>> People in discussions copy existing poor code and claim they can repeat
>> same mistake, so add a note to prevent that.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@....qualcomm.com>
>>
>> ---
>>
>> Similar to my commit 9d108d226224 ("media: i2c: imx: Add note to prevent
>> buggy code re-use"). I went through rest of i2c drivers and found only
>> these two doing it incorrectly.
>> ---
>>  drivers/media/i2c/adv7604.c              | 8 +++++++-
>>  drivers/media/i2c/et8ek8/et8ek8_driver.c | 4 ++++
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
>> index 516553fb17e9..67116a4ef134 100644
>> --- a/drivers/media/i2c/adv7604.c
>> +++ b/drivers/media/i2c/adv7604.c
>> @@ -3453,7 +3453,13 @@ static int configure_regmaps(struct adv76xx_state *state)
>>  static void adv76xx_reset(struct adv76xx_state *state)
>>  {
>>  	if (state->reset_gpio) {
>> -		/* ADV76XX can be reset by a low reset pulse of minimum 5 ms. */
>> +		/*
>> +		 * Note: Misinterpretation of reset assertion - do not re-use
>> +		 * this code.  The reset pin is using incorrect (for a reset
>> +		 * signal) logical level.
>> +		 *
>> +		 * ADV76XX can be reset by a low reset pulse of minimum 5 ms.
>> +		 */
>>  		gpiod_set_value_cansleep(state->reset_gpio, 0);
>>  		usleep_range(5000, 10000);
>>  		gpiod_set_value_cansleep(state->reset_gpio, 1);
>> diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
>> index 2cb7b718782b..50121c3e5b48 100644
>> --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
>> +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
>> @@ -835,6 +835,10 @@ static int et8ek8_power_on(struct et8ek8_sensor *sensor)
>>  
>>  	udelay(10); /* I wish this is a good value */
>>  
>> +	/*
>> +	 * Note: Misinterpretation of reset assertion - do not re-use this code.
>> +	 * The reset pin is using incorrect (for a reset signal) logical level.
>> +	 */
>>  	gpiod_set_value(sensor->reset, 1);
>>  
>>  	msleep(5000 * 1000 / sensor->xclk_freq + 1); /* Wait 5000 cycles */
> 
> Related to the topic, would you be able to comment on the discussion
> related to <20251114133822.434171-2-loic.poulain@....qualcomm.com>? I
> believe you're cc'd, with your @kernel.org address.

I don't have it in my inbox (it's limited to ~10k of messages, so the
roll out of inbox after 1 month usually).

Best regards,
Krzysztof