| Message ID | 20200609172841.22541-1-dmurphy@ti.com |
|---|---|
| Headers | show |
| Series | TAS2563 DSP Firmware Loader | expand |
On Tue, Jun 09, 2020 at 12:28:41PM -0500, Dan Murphy wrote: > .val_bits = 8, > > - .max_register = 5 * 128, > + .max_register = 255 * 128, > .cache_type = REGCACHE_RBTREE, > .reg_defaults = tas2562_reg_defaults, > .num_reg_defaults = ARRAY_SIZE(tas2562_reg_defaults), Should some or all of the DSP memory be marked as volatile? I guess if we only write program to it then on reload after power off it should be fine to just blast everything in again and ignore the fact that some will have changed, but it might be helpful for debugging to be able to read the live values back and do something more clever for restore. > #define TAS2562_PAGE_CTRL 0x00 > +#define TAS2562_BOOK_CTRL 0x7f *sigh* Of course the two levels of paging register are not located anywhere near each other so we can't easily pretend they're one double width page address. :/ > +static int tas25xx_process_fw_single(struct tas2562_data *tas2562, > + struct tas25xx_cmd_data *cmd_data, > + u8 *fw_out) > +{ > + int num_writes = cpu_to_be16(cmd_data->length); > + int i; > + int ret; > + int offset = 4; > + int reg_data, write_reg; > + > + for (i = 0; i < num_writes; i++) { > + /* Reset Page to 0 */ > + ret = regmap_write(tas2562->regmap, TAS2562_PAGE_CTRL, 0); > + if (ret) > + return ret; Why? > + > + cmd_data->book = fw_out[offset]; > + cmd_data->page = fw_out[offset + 1]; > + cmd_data->offset = fw_out[offset + 2]; > + reg_data = fw_out[offset + 3]; > + offset += 4; > + > + ret = regmap_write(tas2562->regmap, TAS2562_BOOK_CTRL, > + cmd_data->book); > + if (ret) > + return ret; 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). > +static ssize_t write_config_store(struct device *dev, > + struct device_attribute *tas25xx_attr, > + const char *buf, size_t size) > +{ This looks like it could just be an enum (it looks like there's names we could use) or just a simple numbered control? Same for all the other controls, they're just small integers so don't look hard to handle. But perhaps I'm missing something? > + 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. It'd also be good to double check that the number of configs and programs is within bounds.
On Tue, Jun 09, 2020 at 12:28:39PM -0500, Dan Murphy wrote: > These programs and configurations are selectable via files under the I2C dev > node. There may be a better way to select this through ALSA controls but I was > unable to find a good example of this. This is why this is an RFC patchset. I think you can just use enums for most of this - what you want to do I think is parse the firmware, build templates for the controls and then add them with snd_soc_add_component_controls(). Userspace *should* cope with controls being hotplugged.
Mark On 6/9/20 12:52 PM, Mark Brown wrote: > On Tue, Jun 09, 2020 at 12:28:39PM -0500, Dan Murphy wrote: > >> These programs and configurations are selectable via files under the I2C dev >> node. There may be a better way to select this through ALSA controls but I was >> unable to find a good example of this. This is why this is an RFC patchset. > I think you can just use enums for most of this - what you want to do I > think is parse the firmware, build templates for the controls and then > add them with snd_soc_add_component_controls(). Userspace *should* cope > with controls being hotplugged. Yes this was my concern if userspace could cope with dynamic controls. Dan
On Tue, Jun 09, 2020 at 01:07:50PM -0500, Dan Murphy wrote: > On 6/9/20 12:52 PM, Mark Brown wrote: > > I think you can just use enums for most of this - what you want to do I > > think is parse the firmware, build templates for the controls and then > > add them with snd_soc_add_component_controls(). Userspace *should* cope > > with controls being hotplugged. > Yes this was my concern if userspace could cope with dynamic controls. Things like alsactl definitely do, and obviously anything that starts after the firmware loads will be fine too.
Mark On 6/9/20 12:50 PM, Mark Brown wrote: > On Tue, Jun 09, 2020 at 12:28:41PM -0500, Dan Murphy wrote: > >> .val_bits = 8, >> >> - .max_register = 5 * 128, >> + .max_register = 255 * 128, >> .cache_type = REGCACHE_RBTREE, >> .reg_defaults = tas2562_reg_defaults, >> .num_reg_defaults = ARRAY_SIZE(tas2562_reg_defaults), > Should some or all of the DSP memory be marked as volatile? I guess if > we only write program to it then on reload after power off it should be > fine to just blast everything in again and ignore the fact that some > will have changed, but it might be helpful for debugging to be able to > read the live values back and do something more clever for restore. Well the only values I see that change that regmap should care about are in first page of the register map. After reverse engineering a binary I found that its contents modify page 0 registers of the device. Not a fan of this as it causes un-wanted changes that may have been setup. > >> #define TAS2562_PAGE_CTRL 0x00 >> +#define TAS2562_BOOK_CTRL 0x7f > *sigh* Of course the two levels of paging register are not located > anywhere near each other so we can't easily pretend they're one double > width page address. :/ Yes I agree > >> +static int tas25xx_process_fw_single(struct tas2562_data *tas2562, >> + struct tas25xx_cmd_data *cmd_data, >> + u8 *fw_out) >> +{ >> + int num_writes = cpu_to_be16(cmd_data->length); >> + int i; >> + int ret; >> + int offset = 4; >> + int reg_data, write_reg; >> + >> + for (i = 0; i < num_writes; i++) { >> + /* 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. > >> + >> + cmd_data->book = fw_out[offset]; >> + cmd_data->page = fw_out[offset + 1]; >> + cmd_data->offset = fw_out[offset + 2]; >> + reg_data = fw_out[offset + 3]; >> + offset += 4; >> + >> + ret = regmap_write(tas2562->regmap, TAS2562_BOOK_CTRL, >> + cmd_data->book); >> + if (ret) >> + return ret; > 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. 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. > >> +static ssize_t write_config_store(struct device *dev, >> + struct device_attribute *tas25xx_attr, >> + const char *buf, size_t size) >> +{ > This looks like it could just be an enum (it looks like there's names we > could use) or just a simple numbered control? Same for all the other > controls, they're just small integers so don't look hard to handle. But > perhaps I'm missing something? No you are right. The issue with using enums is that the binary is not parsed until after codec_probe and the device is registered. So the controls would appear later which could be a race condition for the user space. > >> + 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. > It'd also be good to double check that the number of configs and > programs is within bounds. This I can check once the data is copied. Dan
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.