Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 28 Apr 2023 09:47:45 +0200
From:   Maxime Chevallier <maxime.chevallier@bootlin.com>
To:     Mark Brown <broonie@kernel.org>
Cc:     Colin Foster <colin.foster@in-advantage.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Lee Jones <lee@kernel.org>, linux-kernel@vger.kernel.org,
        thomas.petazzoni@bootlin.com, alexis.lothore@bootlin.com
Subject: Re: [PATCH] regmap: don't check for alignment when using reg_shift
Message-ID: <20230428094745.63677228@pc-7.home>
In-Reply-To: <20230428093010.07e61080@pc-7.home>
References: <20230420150617.381922-1-maxime.chevallier@bootlin.com>
        <ZEKwxhJJNkuX7VTr@colin-ia-desktop>
        <bb836be3-456c-48fd-9b19-62279fee6b8d@sirena.org.uk>
        <20230428093010.07e61080@pc-7.home>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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    
>