Date:	Tue, 03 Mar 2015 16:33:21 +0100
From:	Takashi Iwai <tiwai@...e.de>
To:	Mark Brown <broonie@...nel.org>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: regcache_sync() errors for read-only registers cache

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists