Message-ID: <01e3c429-62b8-378e-9977-7d3a11df6bc4@linaro.org>
Date:   Mon, 21 Jun 2021 13:43:10 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     srivasam@...eaurora.org, rafael@...nel.org,
        dp@...nsource.wolfsonmicro.com, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH] regmap: move readable check before accessing regcache.



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

> 

Powered by blists - more mailing lists