Re: [PATCH] regmap: move readable check before accessing regcache.

[Srinivas Kandagatla]

On 21/06/2021 12:27, Mark Brown wrote:
> On Mon, Jun 21, 2021 at 11:30:00AM +0100, Srinivas Kandagatla wrote:
> > On 18/06/2021 16:48, Mark Brown wrote:
> > > On Fri, Jun 18, 2021 at 01:29:50PM +0100, Srinivas Kandagatla wrote:
>
> > > > _regmap_update_bits() checks _regmap_read() return value before bailing out.
> > > > In non cache path we have this regmap_readable() check however in cached
> > > > patch we do not have this check, so _regmap_read() will return success in
> > > > this case so regmap_update_bits() never reports any error.
> > >
> > > > driver in question does check the return value.
>
> > > OK, so everything is working fine then - what's the problem?  The value
>
> > How can this be working fine?
>
> > In this particular setup the register is marked as write only and is not
> > readable. Should it really store value in cache at the first instance?
>
> Yes, we know exactly what the value in the register is since we wrote it
> so there's no problem with us remembering and using that.
>
> > Also on the other note, if we mark the same regmap as uncached this usecase
> > will fail straightaway with -EIO, so why is the behavior different in
> > regcache path?
>
> If the register is marked as uncachable then obviously the cache
> behaviour is going to be different to that for a register which we can
> cache for whatever reason the register was marked volatile.
>
> > Shouldn't the regcache path check if the register is readable before trying
> > to cache the value?
>
> Why?  If we know what the value is we can cache it and then use it,
> meaning things like restoring the value in a cache sync and update_bits()
> work, this is useful especially on devices which have no read support at

Thanks for the insight,
Yes that makes more sense to have cache for write-only too.

> all.  What would the benefit it not caching it be? >
> >  From "APQ8016E Technical Reference Manual" https://developer.qualcomm.com/qfile/28813/lm80-p0436-7_f_410e_proc_apq8016e_device_spec.pdf
>
> > Section: 4.5.9.6.19
> > this register LPASS_LPAIF_IRQ_CLEARa is clearly marked with Type: W
>
> > with this description:
> > "Writing a 1 to a bit in this register clears the latched interrupt event
>
> > So am not 100% sure if we read this we will get anything real from the
> > register. I always get zeros if I do that.
>
> > Should this behavior treated as volatile?
>
> Yes.  This is indistingusihable from a register that is volatile because
> it doesn't latch written values, given that you're saying readback
> actually works there's an argument here that the documentation isn't
> accurate here.  My guess is that this device doesn't have any write only
> registers as far as anything outside the device is concerned since the
> I/O hardware won't fault or anything on reads, it just has addresses
> where the read side isn't wired up to anything.
>
> > If we mark this register as volatile and make it readable then it would work
> > but that just sounds like a hack to avoid cache.
>
> > Am sure other hardware platforms have similar write-only registers, how do
> > they handle regmap_update_bits case if they have regcache enabled?
>
> They either mark the registers as volatile or just don't do any
> operations that involve reading the value so it's a non-issue.
I agree,
qcom lpass driver is already doing the second one here, so we should 
mark the register as volatile and readable to avoid the reported issue.

--srini

>