Re: [PATCH v9] ASoC: tas2781: Add tas2781 driver

On 3/28/2023 11:49 AM, Shenghao Ding wrote:

Create tas2781 driver.

Signed-off-by: Shenghao Ding 13916275206@139.com

---

Changes in v9:

• rewording some commets
• Add comments on fimware parsing error handling -- all the memory resources will be released in the end of tasdevice_rca_ready (fw_parse_data, fw_parse_program_data & fw_parse_configuration_data).
• Add comments on fw_parse_calibration_data -- DSP can still work with default calibrated data, memory resource related to calibrated data will be released in the tasdevice_codec_remove.

modified: sound/soc/codecs/Kconfig modified: sound/soc/codecs/Makefile new file: sound/soc/codecs/tas2781-dsp.c new file: sound/soc/codecs/tas2781-dsp.h new file: sound/soc/codecs/tas2781-i2c.c new file: sound/soc/codecs/tas2781.h

---

...

+static int fw_parse_block_data_kernel(struct tasdevice_fw *tas_fmw,

• struct tasdev_blk *block, const struct firmware *fmw, int offset)

+{

• const unsigned char *data = fmw->data;
• if (offset + 4 > fmw->size) {

• dev_err(tas_fmw->dev, "%s: File Size error\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• block->type = SMS_HTONL(data[offset], data[offset + 1],

• data[offset + 2], data[offset + 3]);
  

• offset += 4;
• if (offset + 1 > fmw->size) {

• dev_err(tas_fmw->dev, "%s: PChkSumPresent error\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• block->is_pchksum_present = data[offset];
• offset++;
• if (offset + 1 > fmw->size) {

• dev_err(tas_fmw->dev, "%s: mnPChkSum error\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• block->pchksum = data[offset];
• offset++;
• if (offset + 1 > fmw->size) {

• dev_err(tas_fmw->dev, "%s: YChkSumPresent error\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• block->is_ychksum_present = data[offset];
• offset++;
• if (offset + 1 > fmw->size) {

• dev_err(tas_fmw->dev, "%s: mnYChkSum error\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• block->ychksum = data[offset];
• offset++;
• if (offset + 4 > fmw->size) {

• dev_err(tas_fmw->dev, "%s: blk_size error\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• block->blk_size = SMS_HTONL(data[offset], data[offset + 1],

• data[offset + 2], data[offset + 3]);
  

• offset += 4;
• if (offset + 4 > fmw->size) {

• dev_err(tas_fmw->dev, "%s: nSublocks error\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• block->nr_subblocks = SMS_HTONL(data[offset], data[offset + 1],

• data[offset + 2], data[offset + 3]);
  

• offset += 4;
• if (offset + block->blk_size > fmw->size) {

• dev_err(tas_fmw->dev, "%s: nSublocks error\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• block->data = kmemdup(&data[offset], block->blk_size, GFP_KERNEL);
• if (!block->data) {

• offset = -ENOMEM;
  

• goto out;
  

• }
• offset += block->blk_size;

+out:

• return offset;

+}

I have a question regarding those if (offset + x > fmw->size) { do error } offset += x;

Can you instead of doing check before every assignment, just check once if you have enough data to parse, before parsing whole piece of data instead?

For above function it would be something like:

static int fw_parse_block_data_kernel(struct tasdevice_fw *tas_fmw, struct tasdev_blk *block, const struct firmware *fmw, int offset) { const unsigned char *data = fmw->data;

if (offset + 16 + block->blk_size > fmw->size) { dev_err(tas_fmw->dev, "%s: File Size error\n", __func__); offset = -EINVAL; goto out; } block->type = SMS_HTONL(data[offset], data[offset + 1], data[offset + 2], data[offset + 3]); block->is_pchksum_present = data[offset + 4]; block->pchksum = data[offset + 5]; block->is_ychksum_present = data[offset + 6]; block->ychksum = data[offset + 7]; block->blk_size = SMS_HTONL(data[offset + 8], data[offset + 9], data[offset + 10], data[offset + 11]); block->nr_subblocks = SMS_HTONL(data[offset + 12], data[offset + 13], data[offset + 14], data[offset + 15]); offset += 16;

block->data = kmemdup(&data[offset], block->blk_size, GFP_KERNEL); if (!block->data) { offset = -ENOMEM; goto out; } offset += block->blk_size;

out: return offset; }

Additionally if you defined 'struct tasdev_blk' to reflect data which you copy here it can probably be simplified further to simple memcpy, instead of doing field by field assignments, which would reduce amount of code significantly and make it easier to read.

Above applies to all similar parsing in the patch.

+static int fw_parse_data_kernel(struct tasdevice_fw *tas_fmw,

• struct tasdevice_data *img_data, const struct firmware *fmw,
• int offset)

+{

• const unsigned char *data = fmw->data;
• struct tasdev_blk *blk;
• unsigned int i;

...

+#define SMS_HTONS(a, b) ((((a)&0x00FF)<<8) | ((b)&0x00FF)) +#define SMS_HTONL(a, b, c, d) ((((a)&0x000000FF)<<24) | \

• (((b)&0x000000FF)<<16) | (((c)&0x000000FF)<<8) | \
• ((d)&0x000000FF))

Kernel has htons and htonl (in source/include/linux/byteorder/generic.h), but I assume that this means that your FW may use different endianness than host cpu (big endian?), so I would suggest using be16_to_cpu and be32_to_cpu macros instead, as this should be more understandable in this case.