regmap: don't check for alignment when using reg_shift

[PATCH] regmap: don't check for alignment when using reg_shift

Posted by Maxime Chevallier 2 years, 8 months ago

On regmap consumers that require address translation through
up/downshifting, the alignment check in the regmap core doesn't take the
translation into account. This doesn't matter when downshifting the
register address, as any address that fits a given alignment requirement
will still meet it when downshifted (a 4-byte aligned address will
always also be 2-bytes aligned for example).

However, when upshifting, this check causes spurious errors, as it
occurs before the upshifting.

Therefore, we can just skip the alignment check when using
up/downshifting, as it's not relevant.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/base/regmap/regmap.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 167b3c73c13f..8eb26ac24356 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2022,7 +2022,7 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
 {
 	int ret;
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 		return -EINVAL;
 
 	map->lock(map->lock_arg);
@@ -2049,7 +2049,7 @@ int regmap_write_async(struct regmap *map, unsigned int reg, unsigned int val)
 {
 	int ret;
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 		return -EINVAL;
 
 	map->lock(map->lock_arg);
@@ -2264,7 +2264,7 @@ int regmap_noinc_write(struct regmap *map, unsigned int reg,
 		return -EINVAL;
 	if (val_len % map->format.val_bytes)
 		return -EINVAL;
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 		return -EINVAL;
 	if (val_len == 0)
 		return -EINVAL;
@@ -2405,7 +2405,7 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 	int ret = 0, i;
 	size_t val_bytes = map->format.val_bytes;
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 		return -EINVAL;
 
 	/*
@@ -2644,7 +2644,7 @@ static int _regmap_multi_reg_write(struct regmap *map,
 			int reg = regs[i].reg;
 			if (!map->writeable_reg(map->dev, reg))
 				return -EINVAL;
-			if (!IS_ALIGNED(reg, map->reg_stride))
+			if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 				return -EINVAL;
 		}
 
@@ -2795,7 +2795,7 @@ int regmap_raw_write_async(struct regmap *map, unsigned int reg,
 
 	if (val_len % map->format.val_bytes)
 		return -EINVAL;
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 		return -EINVAL;
 
 	map->lock(map->lock_arg);
@@ -2917,7 +2917,7 @@ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
 {
 	int ret;
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 		return -EINVAL;
 
 	map->lock(map->lock_arg);
@@ -2951,7 +2951,7 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 
 	if (val_len % map->format.val_bytes)
 		return -EINVAL;
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 		return -EINVAL;
 	if (val_count == 0)
 		return -EINVAL;
@@ -3046,7 +3046,7 @@ int regmap_noinc_read(struct regmap *map, unsigned int reg,
 
 	if (val_len % map->format.val_bytes)
 		return -EINVAL;
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 		return -EINVAL;
 	if (val_len == 0)
 		return -EINVAL;
@@ -3168,7 +3168,7 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 	size_t val_bytes = map->format.val_bytes;
 	bool vol = regmap_volatile_range(map, reg, val_count);
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
 		return -EINVAL;
 	if (val_count == 0)
 		return -EINVAL;
-- 
2.39.2

Re: [PATCH] regmap: don't check for alignment when using reg_shift

Posted by Colin Foster 2 years, 7 months ago

Hi Maxime,

On Thu, Apr 20, 2023 at 05:06:17PM +0200, Maxime Chevallier wrote:
> On regmap consumers that require address translation through
> up/downshifting, the alignment check in the regmap core doesn't take the
> translation into account. This doesn't matter when downshifting the
> register address, as any address that fits a given alignment requirement
> will still meet it when downshifted (a 4-byte aligned address will
> always also be 2-bytes aligned for example).
> 
> However, when upshifting, this check causes spurious errors, as it
> occurs before the upshifting.

I don't follow why upshifting should make a difference to alignment.
Assuming it does though, would it make sense to test

map->format.reg_shift > 0

instead of just !map->format.reg_shift?

> -	if (!IS_ALIGNED(reg, map->reg_stride))
> +	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
>  		return -EINVAL;

In the case of ocelot_spi, we'd want to flag an invalid access to a
register like 0x71070003... Before this patch it would return -EINVAL,
after this patch it would access 0x71070000.

Colin Foster

Re: [PATCH] regmap: don't check for alignment when using reg_shift

Posted by Mark Brown 2 years, 7 months ago

On Fri, Apr 21, 2023 at 08:50:30AM -0700, Colin Foster wrote:
> On Thu, Apr 20, 2023 at 05:06:17PM +0200, Maxime Chevallier wrote:

> > On regmap consumers that require address translation through
> > up/downshifting, the alignment check in the regmap core doesn't take the
> > translation into account. This doesn't matter when downshifting the
> > register address, as any address that fits a given alignment requirement
> > will still meet it when downshifted (a 4-byte aligned address will
> > always also be 2-bytes aligned for example).

> > However, when upshifting, this check causes spurious errors, as it
> > occurs before the upshifting.

> I don't follow why upshifting should make a difference to alignment.
> Assuming it does though, would it make sense to test

> map->format.reg_shift > 0

> instead of just !map->format.reg_shift?

Yeah, I think the question is more when we should run the alignment
check than if we should have one.  I think running the check after any
shifting makes sense, we'd be better off reorganising the checks if
needed than removing them.

> 
> > -	if (!IS_ALIGNED(reg, map->reg_stride))
> > +	if (!map->format.reg_shift && !IS_ALIGNED(reg, map->reg_stride))
> >  		return -EINVAL;
> 
> In the case of ocelot_spi, we'd want to flag an invalid access to a
> register like 0x71070003... Before this patch it would return -EINVAL,
> after this patch it would access 0x71070000.
> 
> Colin Foster

Re: [PATCH] regmap: don't check for alignment when using reg_shift

Posted by Maxime Chevallier 2 years, 7 months ago

Hello Mark, Colin,

On Tue, 25 Apr 2023 13:56:23 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Apr 21, 2023 at 08:50:30AM -0700, Colin Foster wrote:
> > On Thu, Apr 20, 2023 at 05:06:17PM +0200, Maxime Chevallier wrote:  
> 
> > > On regmap consumers that require address translation through
> > > up/downshifting, the alignment check in the regmap core doesn't
> > > take the translation into account. This doesn't matter when
> > > downshifting the register address, as any address that fits a
> > > given alignment requirement will still meet it when downshifted
> > > (a 4-byte aligned address will always also be 2-bytes aligned for
> > > example).  
> 
> > > However, when upshifting, this check causes spurious errors, as it
> > > occurs before the upshifting.  
> 
> > I don't follow why upshifting should make a difference to alignment.
> > Assuming it does though, would it make sense to test  
> 
> > map->format.reg_shift > 0  
> 
> > instead of just !map->format.reg_shift?  
> 
> Yeah, I think the question is more when we should run the alignment
> check than if we should have one.  I think running the check after any
> shifting makes sense, we'd be better off reorganising the checks if
> needed than removing them.

In the initial RFC I suggested this [1] approach, which checked for
alignment after shifting, that way we are sure that the alignment check
is done according to the underlying regmap provider's constraints. Maybe
this could be sufficient ?

Thanks,

Maxime

> >   
> > > -	if (!IS_ALIGNED(reg, map->reg_stride))
> > > +	if (!map->format.reg_shift && !IS_ALIGNED(reg,
> > > map->reg_stride)) return -EINVAL;  
> > 
> > In the case of ocelot_spi, we'd want to flag an invalid access to a
> > register like 0x71070003... Before this patch it would return
> > -EINVAL, after this patch it would access 0x71070000.
> > 
> > Colin Foster

Re: [PATCH] regmap: don't check for alignment when using reg_shift

Posted by Maxime Chevallier 2 years, 7 months ago

On Fri, 28 Apr 2023 09:30:10 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hello Mark, Colin,
> 
> On Tue, 25 Apr 2023 13:56:23 +0100
> Mark Brown <broonie@kernel.org> wrote:
> 
> > On Fri, Apr 21, 2023 at 08:50:30AM -0700, Colin Foster wrote:  
> > > On Thu, Apr 20, 2023 at 05:06:17PM +0200, Maxime Chevallier
> > > wrote:    
> >   
> > > > On regmap consumers that require address translation through
> > > > up/downshifting, the alignment check in the regmap core doesn't
> > > > take the translation into account. This doesn't matter when
> > > > downshifting the register address, as any address that fits a
> > > > given alignment requirement will still meet it when downshifted
> > > > (a 4-byte aligned address will always also be 2-bytes aligned
> > > > for example).    
> >   
> > > > However, when upshifting, this check causes spurious errors, as
> > > > it occurs before the upshifting.    
> >   
> > > I don't follow why upshifting should make a difference to
> > > alignment. Assuming it does though, would it make sense to test
> > >  
> >   
> > > map->format.reg_shift > 0    
> >   
> > > instead of just !map->format.reg_shift?    
> > 
> > Yeah, I think the question is more when we should run the alignment
> > check than if we should have one.  I think running the check after
> > any shifting makes sense, we'd be better off reorganising the
> > checks if needed than removing them.  
> 
> In the initial RFC I suggested this [1] approach, which checked for
> alignment after shifting, that way we are sure that the alignment
> check is done according to the underlying regmap provider's
> constraints. Maybe this could be sufficient ?

Oops I'm missing the actual link, sorry about that :(

[1] :
https://lore.kernel.org/all/20230324093644.464704-3-maxime.chevallier@bootlin.com/

> Thanks,
> 
> Maxime
> 
> > >     
> > > > -	if (!IS_ALIGNED(reg, map->reg_stride))
> > > > +	if (!map->format.reg_shift && !IS_ALIGNED(reg,
> > > > map->reg_stride)) return -EINVAL;    
> > > 
> > > In the case of ocelot_spi, we'd want to flag an invalid access to
> > > a register like 0x71070003... Before this patch it would return
> > > -EINVAL, after this patch it would access 0x71070000.
> > > 
> > > Colin Foster    
>

Re: [PATCH] regmap: don't check for alignment when using reg_shift

Posted by Colin Foster 2 years, 7 months ago

Hi Maxime,

On Fri, Apr 28, 2023 at 09:47:45AM +0200, Maxime Chevallier wrote:
> On Fri, 28 Apr 2023 09:30:10 +0200
> Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> 
> > Hello Mark, Colin,
> > 
> > On Tue, 25 Apr 2023 13:56:23 +0100
> > Mark Brown <broonie@kernel.org> wrote:
> > 
> > > On Fri, Apr 21, 2023 at 08:50:30AM -0700, Colin Foster wrote:  
> > > > On Thu, Apr 20, 2023 at 05:06:17PM +0200, Maxime Chevallier
> > > > wrote:    
> > >   
> > > > > On regmap consumers that require address translation through
> > > > > up/downshifting, the alignment check in the regmap core doesn't
> > > > > take the translation into account. This doesn't matter when
> > > > > downshifting the register address, as any address that fits a
> > > > > given alignment requirement will still meet it when downshifted
> > > > > (a 4-byte aligned address will always also be 2-bytes aligned
> > > > > for example).    
> > >   
> > > > > However, when upshifting, this check causes spurious errors, as
> > > > > it occurs before the upshifting.    
> > >   

Sorry for a really delayed response, but I just got back around to
thinking about this. Crazy busy times for me.

What about an explicit parameter in regmap_config that will disable
alignment checks? That seems like it might be a welcome feature
addition.


Colin Foster

Re: [PATCH] regmap: don't check for alignment when using reg_shift

Posted by Mark Brown 2 years, 7 months ago

On Fri, May 05, 2023 at 10:19:29AM -0700, Colin Foster wrote:

> Sorry for a really delayed response, but I just got back around to
> thinking about this. Crazy busy times for me.

> What about an explicit parameter in regmap_config that will disable
> alignment checks? That seems like it might be a welcome feature
> addition.

You can already just not specify an alignment requirement - if you can
configure the regmap to specify some new flag you could also just not
specify the alignment requirement in the first place.

Re: [PATCH] regmap: don't check for alignment when using reg_shift

Posted by Maxime Chevallier 2 years, 7 months ago

Hello Mark, Colin,

On Sat, 6 May 2023 09:18:19 +0900
Mark Brown <broonie@kernel.org> wrote:

> On Fri, May 05, 2023 at 10:19:29AM -0700, Colin Foster wrote:
> 
> > Sorry for a really delayed response, but I just got back around to
> > thinking about this. Crazy busy times for me.  
> 
> > What about an explicit parameter in regmap_config that will disable
> > alignment checks? That seems like it might be a welcome feature
> > addition.  
> 
> You can already just not specify an alignment requirement - if you can
> configure the regmap to specify some new flag you could also just not
> specify the alignment requirement in the first place.

Ok thanks, I will try that approach then. Thanks !

Maxime