regcache_sync() errors for read-only registers cache

regcache_sync() errors for read-only registers cache

[Takashi Iwai]

Hi Mark,

the current regcache_sync() has a problem when there are read-only
registers that are cached in regmap: it tries to write all cached
registers no matter whether it's writable or not, resulting in
kernel errors like "Unable to sync register 0x1234. -5".

A quick fix is the patch like below, but obviously it doesn't cover
the all cases but only addresses the signle rw.

Also, _regmap_write() itself calls again regmap_writeable(), so it's
superfluous.  Alternatively, we may check -EIO from _regmap_write()
and treat as a special case not to show the error.  Or, add a
parameter to skip regmap_writeable() call.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] regmap: Skip read-only registers in regcache_sync_block_single()

regcache_sync() spews warnings when a value was cached for a read-only
register as it tries to write all registers no matter whether they are
writable or not.  This patch fixes (a part of) the problem by adding
regmap_writeable() check in regcache_sync_block_single().

Note that the patch covers only the code path using single rw.  When a
raw block write is used, the problem may still exist.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---

 drivers/base/regmap/regcache.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index f373c35f9e1d..30534864735c 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -617,6 +617,8 @@ static int regcache_sync_block_single(struct regmap *map, void *block,
 		ret = regcache_lookup_reg(map, regtmp);
 		if (ret >= 0 && val == map->reg_defaults[ret].def)
 			continue;
+		if (!regmap_writeable(map, regtmp))
+			continue;
 
 		map->cache_bypass = 1;
 
-- 
2.3.0

Re: regcache_sync() errors for read-only registers cache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

On Fri, Feb 27, 2015 at 01:59:30PM +0100, Takashi Iwai wrote:

> A quick fix is the patch like below, but obviously it doesn't cover
> the all cases but only addresses the signle rw.

Please don't bury patches in the middle of mails, that just means that
if the patch is useful it's painful to apply.  Your patch seems fine but
can you please resend in a directly applyable format unless something in
the below indicates against that...

> Also, _regmap_write() itself calls again regmap_writeable(), so it's
> superfluous.  Alternatively, we may check -EIO from _regmap_write()
> and treat as a special case not to show the error.  Or, add a
> parameter to skip regmap_writeable() call.

I'm sorry but I can't parse the above - what is "it" in this context?
Silently ignoring -EIO from the physical register write sounds like a
very bad idea though, that seems likely to discard actual errors.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: regcache_sync() errors for read-only registers cache

[Takashi Iwai]

At Mon, 2 Mar 2015 18:24:18 +0000,
Mark Brown wrote:
> 
> On Fri, Feb 27, 2015 at 01:59:30PM +0100, Takashi Iwai wrote:
> 
> > A quick fix is the patch like below, but obviously it doesn't cover
> > the all cases but only addresses the signle rw.
> 
> Please don't bury patches in the middle of mails, that just means that
> if the patch is useful it's painful to apply.

The --scissors option of git am is your friend.

> Your patch seems fine but
> can you please resend in a directly applyable format unless something in
> the below indicates against that...

Hm, so do you think that my patch is the best way to fix?  I wasn't
sure about it, that's why I wrote in that style.

> > Also, _regmap_write() itself calls again regmap_writeable(), so it's
> > superfluous.  Alternatively, we may check -EIO from _regmap_write()
> > and treat as a special case not to show the error.  Or, add a
> > parameter to skip regmap_writeable() call.
> 
> I'm sorry but I can't parse the above - what is "it" in this context?

regmap_wrietable() call in _regmap_write().

> Silently ignoring -EIO from the physical register write sounds like a
> very bad idea though, that seems likely to discard actual errors.

Right, in that case, a special error code might be used.  But this
sounds like an overkill, too.


thanks,

Takashi

Re: regcache_sync() errors for read-only registers cache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Mon, Mar 02, 2015 at 08:15:23PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > Please don't bury patches in the middle of mails, that just means that
> > if the patch is useful it's painful to apply.

> The --scissors option of git am is your friend.

That's still pain.

> > Your patch seems fine but
> > can you please resend in a directly applyable format unless something in
> > the below indicates against that...

> Hm, so do you think that my patch is the best way to fix?  I wasn't
> sure about it, that's why I wrote in that style.

Well, it's either that or adding the values read back from the chip to
the defaults.

> > > Also, _regmap_write() itself calls again regmap_writeable(), so it's
> > > superfluous.  Alternatively, we may check -EIO from _regmap_write()
> > > and treat as a special case not to show the error.  Or, add a
> > > parameter to skip regmap_writeable() call.

> > I'm sorry but I can't parse the above - what is "it" in this context?

> regmap_wrietable() call in _regmap_write().

It's superfluous with respect to what?  Still a bit confused, sorry.

> > Silently ignoring -EIO from the physical register write sounds like a
> > very bad idea though, that seems likely to discard actual errors.

> Right, in that case, a special error code might be used.  But this
> sounds like an overkill, too.

It also sounds like it's heading towards the complex and fragile.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: regcache_sync() errors for read-only registers cache

[Takashi Iwai]

At Tue, 3 Mar 2015 09:09:29 +0000,
Mark Brown wrote:
> 
> On Mon, Mar 02, 2015 at 08:15:23PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > Please don't bury patches in the middle of mails, that just means that
> > > if the patch is useful it's painful to apply.
> 
> > The --scissors option of git am is your friend.
> 
> That's still pain.

But it's still better than sending two mails even if you don't know
whether it's the right patch.  It's even not tag as an RFC.  The patch
was there just as a reference.

> > > Your patch seems fine but
> > > can you please resend in a directly applyable format unless something in
> > > the below indicates against that...
> 
> > Hm, so do you think that my patch is the best way to fix?  I wasn't
> > sure about it, that's why I wrote in that style.
> 
> Well, it's either that or adding the values read back from the chip to
> the defaults.

For fixing the single rw, it's easy in either way (although the latter
sounds bad from the performance POV).  But what about the block rw?

> > > > Also, _regmap_write() itself calls again regmap_writeable(), so it's
> > > > superfluous.  Alternatively, we may check -EIO from _regmap_write()
> > > > and treat as a special case not to show the error.  Or, add a
> > > > parameter to skip regmap_writeable() call.
> 
> > > I'm sorry but I can't parse the above - what is "it" in this context?
> 
> > regmap_wrietable() call in _regmap_write().
> 
> It's superfluous with respect to what?  Still a bit confused, sorry.

regmap_writeable() is called twice in that code path with my patch.
First before calling _regmap_write() and again in _regmap_write().
The second call is superfluous in this code path although it's needed
for other paths.

regmap_writeable() isn't usually that heavy, but it's still
suboptimal.


Takashi

Re: regcache_sync() errors for read-only registers cache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

On Tue, Mar 03, 2015 at 10:22:59AM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > > The --scissors option of git am is your friend.

> > That's still pain.

> But it's still better than sending two mails even if you don't know
> whether it's the right patch.  It's even not tag as an RFC.  The patch
> was there just as a reference.

I actually find it harder to work with TBH - it breaks up the mail to
have the extra stuff around the code in there and it's harder to apply
if it's OK.  During a discussion it feels more natural to just have the
diff hunk with the mail text around it instead.

> > Well, it's either that or adding the values read back from the chip to
> > the defaults.

> For fixing the single rw, it's easy in either way (although the latter
> sounds bad from the performance POV).  But what about the block rw?

Why should adding something to the defaults hurt performance (it should
just be a one time cost to insert the default which we've got a
reasonable chance of making back later)?  I guess if there's a lot of
these registers it'll add up but they're pretty rare, usually it's a few
ID and revision registers and anything else is volatile so wouldn't get
cached at all.

Block I/O can just get the same fix I think, the logic is basically the
same it's just what we do with differences that changes.

> > > regmap_wrietable() call in _regmap_write().

> > It's superfluous with respect to what?  Still a bit confused, sorry.

> regmap_writeable() is called twice in that code path with my patch.
> First before calling _regmap_write() and again in _regmap_write().
> The second call is superfluous in this code path although it's needed
> for other paths.

> regmap_writeable() isn't usually that heavy, but it's still
> suboptimal.

Oh, right.  The two checks are logically distinct to me - the check in
_write() is more of an assert than something that's expected to go off,
anything relying on it is in trouble, while the one in the cache sync is
there as part of normal operation.  If anyone cared about performance to
that extent it probably ought to be a build option to even check though
since the I/O is generally so slow and it's rare to implement writeable
at all it doesn't normally matter.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: regcache_sync() errors for read-only registers cache

[Takashi Iwai]

At Tue, 3 Mar 2015 14:54:38 +0000,
Mark Brown wrote:
> 
> On Tue, Mar 03, 2015 at 10:22:59AM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > > The --scissors option of git am is your friend.
> 
> > > That's still pain.
> 
> > But it's still better than sending two mails even if you don't know
> > whether it's the right patch.  It's even not tag as an RFC.  The patch
> > was there just as a reference.
> 
> I actually find it harder to work with TBH - it breaks up the mail to
> have the extra stuff around the code in there and it's harder to apply
> if it's OK.  During a discussion it feels more natural to just have the
> diff hunk with the mail text around it instead.

Well, this is just a matter of taste, IMO :)
I find a full patch is always better, if any.

> > > Well, it's either that or adding the values read back from the chip to
> > > the defaults.
> 
> > For fixing the single rw, it's easy in either way (although the latter
> > sounds bad from the performance POV).  But what about the block rw?
> 
> Why should adding something to the defaults hurt performance (it should
> just be a one time cost to insert the default which we've got a
> reasonable chance of making back later)?  I guess if there's a lot of
> these registers it'll add up but they're pretty rare, usually it's a few
> ID and revision registers and anything else is volatile so wouldn't get
> cached at all.

I caught this bug because of the currently developed HD-audio regmap
support that has lots of read-only stuff (mostly for parameters, dozen
of such per each widget node).  Registers range of 32bit wide and
there are no static default values that can be stored in a flat
table.  Nasty, eh?  I'll be glad if any better workaround is present
in regmap.

> Block I/O can just get the same fix I think, the logic is basically the
> same it's just what we do with differences that changes.

Yeah, looks so.

> > > > regmap_wrietable() call in _regmap_write().
> 
> > > It's superfluous with respect to what?  Still a bit confused, sorry.
> 
> > regmap_writeable() is called twice in that code path with my patch.
> > First before calling _regmap_write() and again in _regmap_write().
> > The second call is superfluous in this code path although it's needed
> > for other paths.
> 
> > regmap_writeable() isn't usually that heavy, but it's still
> > suboptimal.
> 
> Oh, right.  The two checks are logically distinct to me - the check in
> _write() is more of an assert than something that's expected to go off,
> anything relying on it is in trouble, while the one in the cache sync is
> there as part of normal operation.  If anyone cared about performance to
> that extent it probably ought to be a build option to even check though
> since the I/O is generally so slow and it's rare to implement writeable
> at all it doesn't normally matter.

Right, that's my assumption.  But I wasn't sure whether it was
acceptable.

I can resend the patch if you prefer, of course, if the original patch
is OK.  Just let me know.


thanks,

Takashi

Re: regcache_sync() errors for read-only registers cache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

On Tue, Mar 03, 2015 at 04:33:21PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > Why should adding something to the defaults hurt performance (it should
> > just be a one time cost to insert the default which we've got a
> > reasonable chance of making back later)?  I guess if there's a lot of
> > these registers it'll add up but they're pretty rare, usually it's a few
> > ID and revision registers and anything else is volatile so wouldn't get
> > cached at all.

> I caught this bug because of the currently developed HD-audio regmap
> support that has lots of read-only stuff (mostly for parameters, dozen
> of such per each widget node).  Registers range of 32bit wide and
> there are no static default values that can be stored in a flat
> table.  Nasty, eh?  I'll be glad if any better workaround is present
> in regmap.

Well, if they're quick to read marking them as volatile to avoid the
cache would be the obvious thing, or not caring about writeability.

You can also specify num_reg_defaults_raw with no defaults table and
it'll read the hardware in one fell swoop, though that would need some
improvement for sparse registers and probably won't do the right thing
for the other registers either (it's a bit specialist for PMICs).

> I can resend the patch if you prefer, of course, if the original patch
> is OK.  Just let me know.

Yes, please resend - I'd guess you want to fix the block case too?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: regcache_sync() errors for read-only registers cache

[Takashi Iwai]

At Tue, 3 Mar 2015 20:04:26 +0000,
Mark Brown wrote:
> 
> On Tue, Mar 03, 2015 at 04:33:21PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > Why should adding something to the defaults hurt performance (it should
> > > just be a one time cost to insert the default which we've got a
> > > reasonable chance of making back later)?  I guess if there's a lot of
> > > these registers it'll add up but they're pretty rare, usually it's a few
> > > ID and revision registers and anything else is volatile so wouldn't get
> > > cached at all.
> 
> > I caught this bug because of the currently developed HD-audio regmap
> > support that has lots of read-only stuff (mostly for parameters, dozen
> > of such per each widget node).  Registers range of 32bit wide and
> > there are no static default values that can be stored in a flat
> > table.  Nasty, eh?  I'll be glad if any better workaround is present
> > in regmap.
> 
> Well, if they're quick to read marking them as volatile to avoid the
> cache would be the obvious thing, or not caring about writeability.

Reading is much slower than writing due to sync.

> You can also specify num_reg_defaults_raw with no defaults table and
> it'll read the hardware in one fell swoop, though that would need some
> improvement for sparse registers and probably won't do the right thing
> for the other registers either (it's a bit specialist for PMICs).

For HD-audio, we can't read the whole verbs beforehand because we
don't know exactly which verbs will be used until the whole parsing is
done and the state changes.  So, the best option is just caching the
read-only parameters.

> > I can resend the patch if you prefer, of course, if the original patch
> > is OK.  Just let me know.
> 
> Yes, please resend - I'd guess you want to fix the block case too?

OK, will resend later together with the block raw fix.


Takashi