linux-kernel - Re: [PATCH 1/1] Revert "media: imx290: Convert to new CCI register access helpers"

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] [day] [month] [year] [list]

Message-ID: <ZUIuNDTJAN_fz3q6@kekkonen.localdomain>
Date:   Wed, 1 Nov 2023 10:53:40 +0000
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Alexander Stein <alexander.stein@...tq-group.com>,
        Alain Volmat <alain.volmat@...s.st.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] Revert "media: imx290: Convert to new CCI register
 access helpers"

Hi Hans,

On Wed, Nov 01, 2023 at 11:41:36AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/1/23 11:31, Sakari Ailus wrote:
> > Hi Alexander,
> > 
> > On Wed, Nov 01, 2023 at 11:09:00AM +0100, Alexander Stein wrote:
> >> cci_write() unconditionally writes multi-byte data in big-endian format.
> >> But IMX290 and IMX327 use little-endian format for multi-byte registers.
> >> Revert the CCI usage until little-endian is supported by CCI register
> >> access helpers.
> >> This reverts commit af73323b97702e53b0a336972aaf23e7dd92c850.
> >>
> >> Signed-off-by: Alexander Stein <alexander.stein@...tq-group.com>
> >> ---
> >> The difference is subtile, but imx290_write() uses put_unaligned_le24(),
> >> while cci_write() uses put_unaligned_be24().
> >>
> >> I assume this should go into stable as well. How to deal with that?
> >> Shall a revert get the fixes tag as well?
> > 
> > I'd very much prefer addressing this in v4l2-cci instead. It should also be
> > a much smaller patch (or patches).
> 
> I was under the impression the v4l2-cci conversion was already
> reverted because this breakage has been known for a while now.
> 
> Anyways, if someone wants to fix this directly this has been
> discussed in this thread:
> 
> https://lore.kernel.org/linux-media/20231030173637.GA2977515@gnbcxd0016.gnb.st.com/
> 
> and the consensus is that the best way to fix this is to
> add CCI_REG16_LE(x) CCI_REG24_LE(x), etc. macros to
> mirror the existing CCI_REG16(x), etc. macros.
> 
> The _LE macros would then look something like this:
> 
> #define CCI_REG_LE		BIT(20)
> 
> #define CCI_REG16_LE(x)                (CCI_REG_LE | (2 << CCI_REG_WIDTH_SHIFT) | (x))
> 
> etc.
> 
> And then the get_unaligned_beXX() and put_unaligned_beXX()
> calls in drivers/media/v4l2-core/v4l2-cci.c would need
> to be made conditional on a check for the CCI_REG_LE flag
> and if that flag is set use the _le_ versions of those
> functions instead.
> 
> The reason to go this way instead of a global LE flag
> somewhere is that some sensors have mixed endianness
> for different registers, so encoding this in the
> register-address high bits is the best solution.

Seems good to me.

> 
> Alexander, perhaps you can prepare 2 patches:
> 
> 1. Adding the discussed CCI_REGXX_LE(x) macros to v4l2-cci
> described above.
> 
> 2. Patch the imx290 code to use the _LE versions for the
> registers which are 2 or more bytes wide.
> 
> ?
> 
> Note I know that Alain (added to the Cc) from the thread
> linked above is also looking into implementing 1. 

-- 
Regards,

Sakari Ailus