[PATCH] regmap: fix writes to non incrementing registers

[PATCH] regmap: fix writes to non incrementing registers

[Ben Whitten]

When checking if a register is writable we must first check if the
register is a non incrementing writable register.
Non incrementing register are deep and do not move to the next
register when writing, for example a FIFO.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 drivers/base/regmap/regmap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index f1025452bb39..70645a28897c 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1489,10 +1489,11 @@ static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
 	WARN_ON(!map->bus);
 
 	/* Check for unwritable registers before we start */
-	for (i = 0; i < val_len / map->format.val_bytes; i++)
-		if (!regmap_writeable(map,
-				     reg + regmap_get_offset(map, i)))
-			return -EINVAL;
+	if (!regmap_writeable_noinc(map, reg))
+		for (i = 0; i < val_len / map->format.val_bytes; i++)
+			if (!regmap_writeable(map,
+					     reg + regmap_get_offset(map, i)))
+				return -EINVAL;
 
 	if (!map->cache_bypass && map->format.parse_val) {
 		unsigned int ival;
-- 
2.17.1

Re: [PATCH] regmap: fix writes to non incrementing registers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On Tue, Aug 13, 2019 at 10:22:51PM +0100, Ben Whitten wrote:

> @@ -1489,10 +1489,11 @@ static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
>  	WARN_ON(!map->bus);
>  
>  	/* Check for unwritable registers before we start */
> -	for (i = 0; i < val_len / map->format.val_bytes; i++)
> -		if (!regmap_writeable(map,
> -				     reg + regmap_get_offset(map, i)))
> -			return -EINVAL;
> +	if (!regmap_writeable_noinc(map, reg))
> +		for (i = 0; i < val_len / map->format.val_bytes; i++)
> +			if (!regmap_writeable(map,
> +					     reg + regmap_get_offset(map, i)))
> +				return -EINVAL;

This feels like we're getting ourselves confused about nonincrementing
registers and probably have other breakage somewhere else - we're
already checking for nonincrementability in regmap_write_noinc(), and
here we're only checking if the first register in the block has that
property which might blow up on us if there's a register in the middle
of the block that is nonincrementable.  If we're going to check this
here I think we should check on every register, but this is
_raw_write_impl() which is part of the call path for implementing
regmap_noinc_write() so checking here will break the API purpose
designed for nonincrementing writes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: fix writes to non incrementing registers

[Ben Whitten]

On Wed, 14 Aug 2019 at 11:01, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Aug 13, 2019 at 10:22:51PM +0100, Ben Whitten wrote:
>
> > @@ -1489,10 +1489,11 @@ static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
> >       WARN_ON(!map->bus);
> >
> >       /* Check for unwritable registers before we start */
> > -     for (i = 0; i < val_len / map->format.val_bytes; i++)
> > -             if (!regmap_writeable(map,
> > -                                  reg + regmap_get_offset(map, i)))
> > -                     return -EINVAL;
> > +     if (!regmap_writeable_noinc(map, reg))
> > +             for (i = 0; i < val_len / map->format.val_bytes; i++)
> > +                     if (!regmap_writeable(map,
> > +                                          reg + regmap_get_offset(map, i)))
> > +                             return -EINVAL;
>
> This feels like we're getting ourselves confused about nonincrementing
> registers and probably have other breakage somewhere else - we're
> already checking for nonincrementability in regmap_write_noinc(), and
> here we're only checking if the first register in the block has that
> property which might blow up on us if there's a register in the middle
> of the block that is nonincrementable.  If we're going to check this
> here I think we should check on every register, but this is
> _raw_write_impl() which is part of the call path for implementing
> regmap_noinc_write() so checking here will break the API purpose
> designed for nonincrementing writes.

So it appeared that the last patch in this area for validating a register
block [1] broke the regmap_noinc_write use case.
Because regmap_noinc_write calls _regmap_raw_write and in
turn hits the _regmap_raw_write_impl, the val_len is the depth of the
one register to write to and not a block of registers which is assumed
by the previous check. By inserting a check that the first (and only)
register is a noinc one allows me to start writing to my FIFO again.

I'm all for an alternative solution though if there is a cleaner approach.

Kind regards,
Ben

[1] https://lore.kernel.org/patchwork/patch/1057184/

Re: [PATCH] regmap: fix writes to non incrementing registers

[Andreas Färber]

Am 14.08.19 um 15:09 schrieb Ben Whitten:
> On Wed, 14 Aug 2019 at 11:01, Mark Brown <broonie@kernel.org> wrote:
>>
>> On Tue, Aug 13, 2019 at 10:22:51PM +0100, Ben Whitten wrote:
>>
>>> @@ -1489,10 +1489,11 @@ static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
>>>       WARN_ON(!map->bus);
>>>
>>>       /* Check for unwritable registers before we start */
>>> -     for (i = 0; i < val_len / map->format.val_bytes; i++)
>>> -             if (!regmap_writeable(map,
>>> -                                  reg + regmap_get_offset(map, i)))
>>> -                     return -EINVAL;
>>> +     if (!regmap_writeable_noinc(map, reg))
>>> +             for (i = 0; i < val_len / map->format.val_bytes; i++)
>>> +                     if (!regmap_writeable(map,
>>> +                                          reg + regmap_get_offset(map, i)))
>>> +                             return -EINVAL;
>>
>> This feels like we're getting ourselves confused about nonincrementing
>> registers and probably have other breakage somewhere else - we're
>> already checking for nonincrementability in regmap_write_noinc(), and
>> here we're only checking if the first register in the block has that
>> property which might blow up on us if there's a register in the middle
>> of the block that is nonincrementable.  If we're going to check this
>> here I think we should check on every register, but this is
>> _raw_write_impl() which is part of the call path for implementing
>> regmap_noinc_write() so checking here will break the API purpose
>> designed for nonincrementing writes.
> 
> So it appeared that the last patch in this area for validating a register
> block [1] broke the regmap_noinc_write use case.

Then please add a Fixes: header to your commit message, so that it gets
backported to all affected upstream and downstream trees.

Thanks,
Andreas

> Because regmap_noinc_write calls _regmap_raw_write and in
> turn hits the _regmap_raw_write_impl, the val_len is the depth of the
> one register to write to and not a block of registers which is assumed
> by the previous check. By inserting a check that the first (and only)
> register is a noinc one allows me to start writing to my FIFO again.
> 
> I'm all for an alternative solution though if there is a cleaner approach.
> 
> Kind regards,
> Ben
> 
> [1] https://lore.kernel.org/patchwork/patch/1057184/
> 


-- 
SUSE Linux GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH] regmap: fix writes to non incrementing registers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

On Wed, Aug 14, 2019 at 03:32:37PM +0200, Andreas Färber wrote:

> Then please add a Fixes: header to your commit message, so that it gets
> backported to all affected upstream and downstream trees.

This still isn't a sensible fix for the reasons I outlined.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: fix writes to non incrementing registers

[Andreas Färber]

Am Mittwoch, den 14.08.2019, 17:05 +0100 schrieb Mark Brown:
> On Wed, Aug 14, 2019 at 03:32:37PM +0200, Andreas Färber wrote:
> 
> > Then please add a Fixes: header to your commit message, so that it
> > gets
> > backported to all affected upstream and downstream trees.
> 
> This still isn't a sensible fix for the reasons I outlined.

No contradiction here.

Cheers,
Andreas

-- 
SUSE Linux GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH] regmap: fix writes to non incrementing registers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Wed, Aug 14, 2019 at 02:09:11PM +0100, Ben Whitten wrote:

> So it appeared that the last patch in this area for validating a register
> block [1] broke the regmap_noinc_write use case.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

> Because regmap_noinc_write calls _regmap_raw_write and in
> turn hits the _regmap_raw_write_impl, the val_len is the depth of the
> one register to write to and not a block of registers which is assumed
> by the previous check. By inserting a check that the first (and only)
> register is a noinc one allows me to start writing to my FIFO again.

> I'm all for an alternative solution though if there is a cleaner approach.

Like I said if we're checking for nonincrementing registers it shouldn't
just be on the first register, it should be for every address in the
range.  Probably accept it if the nonincrementing register is the first
and error otherwise, with some documentation explaining what's going on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: fix writes to non incrementing registers

[Ben Whitten]

On Wed, 14 Aug 2019 at 17:19, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Aug 14, 2019 at 02:09:11PM +0100, Ben Whitten wrote:
>
> > So it appeared that the last patch in this area for validating a register
> > block [1] broke the regmap_noinc_write use case.
>
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

Sure thing, the patch adds in a call which checks if range of registers
is writeable within _regmap_raw_write_impl. This check uses the length
of the data given to _regmap_raw_write_impl to determine the range
of registers to check from the given starting register.

> > Because regmap_noinc_write calls _regmap_raw_write and in
> > turn hits the _regmap_raw_write_impl, the val_len is the depth of the
> > one register to write to and not a block of registers which is assumed
> > by the previous check. By inserting a check that the first (and only)
> > register is a noinc one allows me to start writing to my FIFO again.
>
> > I'm all for an alternative solution though if there is a cleaner approach.
>
> Like I said if we're checking for nonincrementing registers it shouldn't
> just be on the first register, it should be for every address in the
> range.  Probably accept it if the nonincrementing register is the first
> and error otherwise, with some documentation explaining what's going on.

I see yes, we don't want to stumble into a noinc register by mistake when
writing a register range.

You mentioned that we likely have breakage elsewhere, I believe that
regmap_noinc_write probably shouldn't ever have been calling
_regmap_raw_write.

Whilst regmap_noinc_read calls _regmap_raw_read, this function doesn't
do any massage and just calls map->bus->read after selecting a page.
regmap_raw_write on the other hand is meant for writing blocks to
register ranges as these added checks show, and does split work based
on page length etc.
This splitting up is something that shouldn't apply to the FIFO as it's a
deep register.

I modified regmap_noinc_write to be much more like the raw_read
internals, just select page then attempt a map->bus->gather_write,
falling back to linearising if that's not supported.
This approach worked at getting writing working into the FIFO.

So I would propose that there are two 'Fixes:' patches here, one
enhancing the checking when writing a register range to ensure you
don't stumble into a noinc register.
And one to fix noinc_writes to send data directly to the bus once the
page is selected with no splitting up as you would a range.

Kind regards,
Ben

Re: [PATCH] regmap: fix writes to non incrementing registers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]

On Tue, Sep 03, 2019 at 10:42:59AM +0100, Ben Whitten wrote:

> You mentioned that we likely have breakage elsewhere, I believe that
> regmap_noinc_write probably shouldn't ever have been calling
> _regmap_raw_write.

> Whilst regmap_noinc_read calls _regmap_raw_read, this function doesn't
> do any massage and just calls map->bus->read after selecting a page.
> regmap_raw_write on the other hand is meant for writing blocks to
> register ranges as these added checks show, and does split work based
> on page length etc.
> This splitting up is something that shouldn't apply to the FIFO as it's a
> deep register.

This just means that we need to take care of nonincrementing registers
in there, we don't want to end up with too many different places where
we might have to handle formatting since that leads to duplication.
It's about marshalling for physical I/O, it's a bit misnamed at this
point but nonincrementing registers definitely fit in there for me.

> I modified regmap_noinc_write to be much more like the raw_read
> internals, just select page then attempt a map->bus->gather_write,
> falling back to linearising if that's not supported.
> This approach worked at getting writing working into the FIFO.

Modify raw_write() to handle this instead, it is a mirror of the read
side it's just that writes are more complicated since there's more
things that could happen as you're discovering here.

> So I would propose that there are two 'Fixes:' patches here, one
> enhancing the checking when writing a register range to ensure you
> don't stumble into a noinc register.

Yes.

> And one to fix noinc_writes to send data directly to the bus once the
> page is selected with no splitting up as you would a range.

Push that into raw_write(), it just needs to special case when the first
register in a block is non-incrementing in the page selection logic (add
a !nonincrmenting in the while loop I think).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]