linux-kernel - Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence

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 for Android: free password hash cracker in your pocket

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

Date: Fri, 7 Jun 2024 18:57:04 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Conor Dooley <conor@...nel.org>
Cc: linux-media@...r.kernel.org, Conor Dooley <conor.dooley@...rochip.com>,
	Dave Stevenson <dave.stevenson@...pberrypi.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Adam Ford <aford173@...il.com>,
	Andrey Konovalov <andrey.konovalov@...aro.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] media: i2c: imx219: fix msr access command sequence

Hi Conor,

Thank you for the patch.

On Fri, Jun 07, 2024 at 04:50:23PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@...rochip.com>
> 
> It was reported to me that the imx219 didn't work on one of our
> development kits partly because the access sequence is incorrect.
> The datasheet I could find [1] for this camera has the access sequence:
> Seq. No. Address (Hex) data
> 1        30EB          05
> 2        30EB          0C
> 3        300A          FF
> 4        300B          FF
> 5        30EB          05
> 6        30EB          09
> 
> but the driver swaps the first two elements. Laurent pointed out on IRC
> that the original code used the correct sequence for 1920x1080 but the
> current sequence for 3280x2464 and 1640x1232. During refactoring of the
> init sequence the current order was used for all formats.
> 
> Switch to using the documented sequence.
> 
> Link: https://www.opensourceinstruments.com/Electronics/Data/IMX219PQ.pdf [1]
> Fixes: 8508455961d5 ("media: i2c: imx219: Split common registers from mode tables")
> Fixes: 1283b3b8f82b ("media: i2c: Add driver for Sony IMX219 sensor")
> Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>

This looks reasonable, based on the above link.

Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>

Dave, could you check the impact on the Raspberry Pi kernel ? It seems
to be shipping the incorrect sequence unconditionally.

Any information about what the 12 undocumented MSRs that are programmed
by the driver do would be appreciated too ;-)

> ---
> I got the report of this third hand, I don't have a device and can't
> test this. I do wonder why the RPis get away with the sequence that
> seemingly doesn't work for the guy that reported this to me. My theory
> is either that they noticed the sequence was wrong while adding some
> other MSR access that is needed on this board while either cross
> checking the values written or because the other MSR accesses didn't
> take effect.
> 
> CC: Dave Stevenson <dave.stevenson@...pberrypi.com>
> CC: Sakari Ailus <sakari.ailus@...ux.intel.com>
> CC: Mauro Carvalho Chehab <mchehab@...nel.org>
> CC: Adam Ford <aford173@...il.com>
> CC: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> CC: Andrey Konovalov <andrey.konovalov@...aro.org>
> CC: linux-media@...r.kernel.org
> CC: linux-kernel@...r.kernel.org
> ---
>  drivers/media/i2c/imx219.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 51ebf5453fce..e78a80b2bb2e 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -162,8 +162,8 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
>  	{ IMX219_REG_MODE_SELECT, 0x00 },	/* Mode Select */
>  
>  	/* To Access Addresses 3000-5fff, send the following commands */
> -	{ CCI_REG8(0x30eb), 0x0c },
>  	{ CCI_REG8(0x30eb), 0x05 },
> +	{ CCI_REG8(0x30eb), 0x0c },
>  	{ CCI_REG8(0x300a), 0xff },
>  	{ CCI_REG8(0x300b), 0xff },
>  	{ CCI_REG8(0x30eb), 0x05 },

-- 
Regards,

Laurent Pinchart