Re: [RFC PATCH 2/2] ASoc: tas2563: DSP Firmware loading support

[Mark Brown <broonie@kernel.org>]

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

On Fri, Jun 12, 2020 at 12:30:29PM -0500, Dan Murphy wrote:
> On 6/9/20 12:50 PM, Mark Brown wrote:
> > On Tue, Jun 09, 2020 at 12:28:41PM -0500, Dan Murphy wrote:

> > > +		/* Reset Page to 0 */
> > > +		ret = regmap_write(tas2562->regmap, TAS2562_PAGE_CTRL, 0);
> > > +		if (ret)
> > > +			return ret;

> > Why?

> Well the reason to set this back to page 0 is that is where the book
> register is.

> So setting this back to page 0 set the Book register appropriately.

Oh dear, usually the paging register is always visible :/

> > This manual paging doesn't fill me with with joy especially with regard
> > to caching and doing the books behind the back of regmap.  I didn't spot
> > anything disabling cache or anything in the code.  I think you should
> > either bypass the cache while doing this or teach regmap about the
> > books (which may require core updates, I can't remember if the range
> > code copes with nested levels of paging - I remember thinking about it).

> Yeah. After reading this and thinking about this for a couple days.  This
> actually has contention issues with the ALSA controls.

> There needs to also be some locks put into place.

That's not too surprising for something like this.

> I prefer to disable the cache.  Not sure how many other devices use Books
> and pages for register maps besides TI.

> Adding that to regmap might be to specific to our devices.

Single level pages are in there already, TI is a big user but I've seen
this from other vendors too.  I do remember some discussion of multi
level paging before, I think with a TI part, and I *think* it already
happens to work without needing to do anything but I'd need to go back
and check and it's 7pm on a Friday night.  IIRC if one paging register
is within another paged region the code manages to recurse and sort
things out, but I could be wrong.  I agree that it's a bit specialist if
it needs anything non-trivial and something driver local would be
reasonable.

> > > +	tas2562->fw_data->fw_hdr = devm_kzalloc(tas2562->dev, hdr_size,
> > > +						GFP_KERNEL);
> > > +	if (!tas2562->fw_data->fw_hdr)
> > > +		return -ENOMEM;
> > > +
> > > +	memcpy(tas2562->fw_data->fw_hdr, &fw->data[0], hdr_size);

> > Should validate that the firmware is actually at least hdr_size big, and
> > similarly for all the other lengths we get from the header we should
> > check that there's actually enough data in the file.  ATM we just
> > blindly copy.

> I will have to look into doing this.  I blindly copy this data because there
> is really not a viable and reliable way to check sizes against the
> structures.

Yeah, that's reasonable - I was just thinking about checking it against
the size of the file to make sure it doesn't actually overflow the
buffer and corrupt things or crash.  Obviously sanity checking is good
but there's limits on what's sensible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]