linux-kernel - Re: [PATCH] Revert "mfd: syscon: Remove repetition of the regmap_get_val

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: <Y0Udqwp9j1HNEwn8@google.com>
Date:   Tue, 11 Oct 2022 08:39:23 +0100
From:   Lee Jones <lee@...nel.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        "Jason A. Donenfeld" <Jason@...c4.com>, arnd@...db.de,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Revert "mfd: syscon: Remove repetition of the
 regmap_get_val_endian()"

On Sat, 08 Oct 2022, Andy Shevchenko wrote:

> On Sat, Oct 08, 2022 at 09:45:16AM -0700, Linus Torvalds wrote:
> > On Sat, Oct 8, 2022 at 8:47 AM Jason A. Donenfeld <Jason@...c4.com> wrote:
> > >
> > > This reverts commit 72a95859728a7866522e6633818bebc1c2519b17. It broke
> > > reboots on big-endian MIPS and MIPS64 malta QEMU instances, which use
> > > the syscon driver. Little-endian is not effected, which means likely
> > > it's important to handle regmap_get_val_endian() in this function after
> > > all.
> > 
> > Hmm. The revert may indeed be the right thing to do, but we're still
> > early in the release process, so let's go through the channels.
> > 
> > I do note that commit 72a95859728a points to commit 0dbdb76c0ca8
> > ("regmap: mmio: Parse endianness definitions from DT") as the reason
> > why it's not necessary any more, but that commit
> > 
> >  (a) doesn't seem to set config->val_format_endian (which somebody may
> > care about). It does set the operation pointers etc, but doesn't set
> > that field.
> 
> It should.
> 
> of_syscon_register() calls to regmap_init_mmio() with syscon_config data
> structure as a parameter.
> 
> Before 72a95859728a the of_syscon_register() fills the val_format_endian with
> something it parses from DT. After that commit the default value (0) is
> REGMAP_ENDIAN_DEFAULT. Now when __regmap_init_mmio_clk() is called it
> creates a context base on DT since the field is 0.
> 
> >  (b) it uses regmap_get_val_endian(), which doesn't actually even look
> > at the OF properties unless config->val_format_endian is
> > REGMAP_ENDIAN_DEFAULT
> 
> Which is 0!
> 
> > so the code that commit 72a95859728a removed was actually quite a bit
> > different from the code in commit 0dbdb76c0ca8.
> > 
> > Maybe the problem is related to those semantic differences, and is
> > easy to fix for somebody who knows what the heck that stuff is doing.
> 
> But while looking into this, I think I know what is going on,
> of_syscon_register() calls regmap API with dev == NULL, hence
> fwnode == NULL, hence nothing to read from DT.
> 
> But default (via regmap bus configuration) is LE and LE works fine.
> 
> > And if not, please just send me the revert through the normal channels. Ok?
> 
> Yeah, revert is a good move here.

Could you review and provide a tag for the revert patch please?

-- 
Lee Jones [李琼斯]