linux-kernel - Re: [PATCH v5 1/3] debugfs_create_regset32() support 8/16 bit width registers

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]

Date:   Mon, 2 Oct 2023 15:07:06 -0400
From:   Frank Li <Frank.li@....com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Vinod Koul <vkoul@...nel.org>, Baoquan He <bhe@...hat.com>,
        dmaengine@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        imx@...ts.linux.dev, linux-kernel@...r.kernel.org,
        kernel test robot <lkp@...el.com>,
        oe-kbuild-all@...ts.linux.dev,
        "Rafael J . Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v5 1/3] debugfs_create_regset32() support 8/16 bit width
 registers

On Mon, Oct 02, 2023 at 08:55:23PM +0200, Arnd Bergmann wrote:
> On Mon, Oct 2, 2023, at 20:37, Frank Li wrote:
> > Enhance the flexibility of `debugfs_create_regset32()` to support registers
> > of various bit widths. The key changes are as follows:
> >
> > 1. Renamed '*reg32' and '*regset32' to '*reg' and '*regset' in relevant
> >    code to reflect that the register width is not limited to 32 bits.
> >
> > 2. Added 'size' and 'bigendian' fields to the `struct debugfs_reg` to allow
> >    for specifying the size and endianness of registers. These additions
> >    enable `debugfs_create_regset()` to support a wider range of register
> >    types.
> >
> > 3. When 'size' is set to 0, it signifies a 32-bit register. This change
> >    maintains compatibility with existing code that assumes 32-bit
> >    registers.
> >
> > Improve the versatility of `debugfs_create_regset()` and enable it to
> > handle registers of different sizes and endianness, offering greater
> > flexibility for debugging and monitoring.
> >
> > Signed-off-by: Frank Li <Frank.Li@....com>
> 
> This version looks correct to me, I see no fundamental problems with it.
> In fact, you could list "support for ioport_map() output" to the features
> above.
> 
> A few other thoughts from my side, all of which could be ignored:
> 
> - if the ioport access is not an important feature, we can instead
>   support 64-bit readl() as I commented in a previous email. We just
>   can't easily have both.

We will get 64bit dma edma soon. So I can test and upstream it when I get
it.

> 
> - instead of treating every value of "regs->size" other than 1 and 2
>   as meaning '32-bit read', I would explicitly check for 0 and 4
>   here

Yes, I will update next version.

> 
> - Another more complicated but also more featureful variant would
>   be to use the 'regmap' infrastructure as the abstraction, this would
>   also provide access to big-endian, variable register width
>   (including 64-bit), and pio, along with additional features and
>   other bus types. Not sure it's worth it, but could be interesting
>   to try out.

Yes, debugfs_create_regset may need big change or create new version for
regmap. It is out of scope this patches.  I will try later.

> 
>      Arnd