linux-kernel - Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

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: <20230113110252.GK36097@ediswmail.ad.cirrus.com>
Date:   Fri, 13 Jan 2023 11:02:52 +0000
From:   Charles Keepax <ckeepax@...nsource.cirrus.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
CC:     Mark Brown <broonie@...nel.org>, <vkoul@...nel.org>,
        <yung-chuan.liao@...ux.intel.com>, <sanyog.r.kale@...el.com>,
        <linux-kernel@...r.kernel.org>, <patches@...nsource.cirrus.com>
Subject: Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

On Thu, Jan 12, 2023 at 02:19:29PM -0600, Pierre-Louis Bossart wrote:
> On 1/12/23 13:50, Mark Brown wrote:
> > On Thu, Jan 12, 2023 at 12:43:46PM -0600, Pierre-Louis Bossart wrote:
> >> On 1/12/23 12:14, Mark Brown wrote:
> > 
> >>> The regmap gather_write() operation allows the bus to take two buffers,
> >>> one for the register and one for the value, rather than requiring the
> >>> core combine everything into a single buffer (mainly useful for large
> >>> transfers like firmware downloads).
> > 
> >> Right, but that's not supported in SoundWire. sdw_nwrite() will only
> >> work with consecutive addresses - and the auto-increment is handled in
> >> software, not hardware.
> > 
> > No, that's exactly what this is for.  It's for the *register address*
> > being in a separate buffer, the data is then a sequence of consecutive
> > register values.>
> >> What's suggested here is to use the first element of reg_buf, which begs
> >> the question how different this is from a regular write. If there was a
> >> discontinuity in reg_buf then this wouldn't work at all.
> > 
> > reg_buf contains the address of exactly one register.
> 
> So what's the difference with a plain write() of N data?

There are two back end interfaces in regmap, the reg_write/read
and the plain write/read. Both have currently have some
limitations when dealing with SoundWire.

The reg_write/reg_read can only deal with a single register
at a time, which is really far from ideal, since it means
all transactions will be broken up into individual registers
at the regmap level, mostly depriving the SoundWire side
of the opportunity to do things like a BRA transfer if it
deems that suitable. And denying users the ability to use the
regmap_raw_read/write API at all.

The write/read interface allows us to pass the full transaction
through, but does have the downside it copies the address around
a bit more and does some pointless endian swaps on big endian
systems. This interface is generally used by buses like I2C/SPI
where there is no actual concept of a register address only a
buffer of bytes to be sent/read, thus prefers to pass a single
working buffer if it sensibly can. I went with this solution
because it enables all the functionality and the downside is
fairly minimal, apart from looking a little clunky as you note.

I guess ideally from SoundWire's point of view expanding the
first interface to allow multi-register transactions would
make the most sense, but that a fairly invasive change to
regmap and one I am a little hesitant to get into right now.

The half-way house would be the current CODEC i am working on
doesn't currently need any BRA/raw transfers so I could stick
to the reg_write/reg_read functions, but still allow larger
than 8-bit registers. It would get me what I need for now,
look a little cleaner, and we can deal with the other issues
when they come up.

Thanks,
Charles