Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] regmap: move readable check before accessing regcache.
To:     Mark Brown <broonie@kernel.org>
Cc:     srivasam@codeaurora.org, rafael@kernel.org,
        dp@opensource.wolfsonmicro.com, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org, Marek Szyprowski <m.szyprowski@samsung.com>
References: <20210618113558.10046-1-srinivas.kandagatla@linaro.org>
 <20210618115104.GB4920@sirena.org.uk>
 <666da41f-173e-152d-84e5-e9b32baa60da@linaro.org>
 <20210618154836.GC4920@sirena.org.uk>
 <6eca27bf-5696-3ffc-24a5-5a58407f6e93@linaro.org>
 <20210621112747.GC4094@sirena.org.uk>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <01e3c429-62b8-378e-9977-7d3a11df6bc4@linaro.org>
Date:   Mon, 21 Jun 2021 13:43:10 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20210621112747.GC4094@sirena.org.uk>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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

>