Re: [PATCH v1 7/8] ALSA: hda/tas2781: Add tas2781 SPI-based driver

+static const char deviceNumber[TASDEVICE_DSP_TAS_MAX_DEVICE] = {

• 1, 2, 1, 2, 1, 1, 0, 2, 4, 3, 1, 2, 3, 4

+};

A comment on those values wouldn't hurt...

+static struct tasdevice_config_info *tasdevice_add_config(

• struct tasdevice_priv *tas_priv, unsigned char *config_data,
• unsigned int config_size, int *status)

+{

• struct tasdevice_config_info *cfg_info;
• struct tasdev_blk_data **bk_da;
• unsigned int config_offset = 0;
• unsigned int i;
• /* In most projects are many audio cases, such as music, handfree,

• * receiver, games, audio-to-haptics, PMIC record, bypass mode,
  

• * portrait, landscape, etc. Even in multiple audios, one or
  

• * two of the chips will work for the special case, such as
  

• * ultrasonic application. In order to support these variable-numbers
  

• * of audio cases, flexible configs have been introduced in the
  

• * dsp firmware.
  

• */
  

• cfg_info = kzalloc(sizeof(struct tasdevice_config_info), GFP_KERNEL);
• if (!cfg_info) {

• *status = -ENOMEM;
  

• goto out;
  

• }
• if (tas_priv->rcabin.fw_hdr.binary_version_num >= 0x105) {

• if (config_offset + 64 > (int)config_size) {
  

• 	*status = -EINVAL;
  

• 	dev_err(tas_priv->dev, "add conf: Out of boundary\n");
  

• 	goto out;
  

• }
  

• config_offset += 64;
  

• }
• if (config_offset + 4 > (int)config_size) {

• *status = -EINVAL;
  

• dev_err(tas_priv->dev, "add config: Out of boundary\n");
  

• goto out;
  

memory leaks for each of those goto out;

You need to use different labels and free-up what was allocated before.

• }
• /* convert data[offset], data[offset + 1], data[offset + 2] and

• * data[offset + 3] into host
  

• */
  

• cfg_info->nblocks =

• be32_to_cpup((__be32 *)&config_data[config_offset]);
  

• config_offset += 4;
• /* Several kinds of dsp/algorithm firmwares can run on tas2781,

• * the number and size of blk are not fixed and different among
  

• * these firmwares.
  

• */
  

• bk_da = cfg_info->blk_data = kcalloc(cfg_info->nblocks,

• sizeof(struct tasdev_blk_data *), GFP_KERNEL);
  

• if (!bk_da) {

• *status = -ENOMEM;
  

• goto out;
  

• }
• cfg_info->real_nblocks = 0;
• for (i = 0; i < cfg_info->nblocks; i++) {

• if (config_offset + 12 > config_size) {
  

• 	*status = -EINVAL;
  

• 	dev_err(tas_priv->dev,
  

• 		"%s: Out of boundary: i = %d nblocks = %u!\n",
  

• 		__func__, i, cfg_info->nblocks);
  

• 	break;
  

• }
  

• bk_da[i] = kzalloc(sizeof(struct tasdev_blk_data), GFP_KERNEL);
  

• if (!bk_da[i]) {
  

• 	*status = -ENOMEM;
  

• 	break;
  

• }
  

• bk_da[i]->dev_idx = config_data[config_offset];
  

• config_offset++;
  

• bk_da[i]->block_type = config_data[config_offset];
  

• config_offset++;
  

• if (bk_da[i]->block_type == TASDEVICE_BIN_BLK_PRE_POWER_UP) {
  

• 	if (bk_da[i]->dev_idx == 0)
  

• 		cfg_info->active_dev =
  

• 			(1 << tas_priv->ndev) - 1;
  

• 	else
  

• 		cfg_info->active_dev |= 1 <<
  

• 			(bk_da[i]->dev_idx - 1);
  

• }
  

• bk_da[i]->yram_checksum =
  

• 	be16_to_cpup((__be16 *)&config_data[config_offset]);
  

• config_offset += 2;
  

• bk_da[i]->block_size =
  

• 	be32_to_cpup((__be32 *)&config_data[config_offset]);
  

• config_offset += 4;
  

• bk_da[i]->n_subblks =
  

• 	be32_to_cpup((__be32 *)&config_data[config_offset]);
  

• config_offset += 4;
  

• if (config_offset + bk_da[i]->block_size > config_size) {
  

• 	*status = -EINVAL;
  

• 	dev_err(tas_priv->dev,
  

• 		"%s: Out of boundary: i = %d blks = %u!\n",
  

• 		__func__, i, cfg_info->nblocks);
  

• 	break;
  

• }
  

• /* instead of kzalloc+memcpy */
  

• bk_da[i]->regdata = kmemdup(&config_data[config_offset],
  

• 	bk_da[i]->block_size, GFP_KERNEL);
  

• if (!bk_da[i]->regdata) {
  

• 	*status = -ENOMEM;
  

• 	goto out;
  

• }
  

• config_offset += bk_da[i]->block_size;
  

• cfg_info->real_nblocks += 1;
  

• }

+out:

• return cfg_info;

error handling needs to be revisited/redone.

+}

+int tasdevice_spi_rca_parser(void *context, const struct firmware *fmw) +{

• struct tasdevice_priv *tas_priv = context;
• struct tasdevice_config_info **cfg_info;
• struct tasdevice_rca_hdr *fw_hdr;
• struct tasdevice_rca *rca;
• unsigned int total_config_sz = 0;
• unsigned char *buf;
• int offset = 0;
• int ret = 0;
• int i;
• rca = &(tas_priv->rcabin);
• fw_hdr = &(rca->fw_hdr);
• if (!fmw || !fmw->data) {

• dev_err(tas_priv->dev, "Failed to read %s\n",
  

• 	tas_priv->rca_binaryname);
  

• tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
  

• ret = -EINVAL;
  

• goto out;
  

• }
• buf = (unsigned char *)fmw->data;
• fw_hdr->img_sz = be32_to_cpup((__be32 *)&buf[offset]);
• offset += 4;
• if (fw_hdr->img_sz != fmw->size) {

• dev_err(tas_priv->dev,
  

• 	"File size not match, %d %u", (int)fmw->size,
  

• 	fw_hdr->img_sz);
  

• tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
  

• ret = -EINVAL;
  

• goto out;
  

• }
• fw_hdr->checksum = be32_to_cpup((__be32 *)&buf[offset]);
• offset += 4;
• fw_hdr->binary_version_num = be32_to_cpup((__be32 *)&buf[offset]);
• if (fw_hdr->binary_version_num < 0x103) {

• dev_err(tas_priv->dev, "File version 0x%04x is too low",
  

• 	fw_hdr->binary_version_num);
  

• tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
  

• ret = -EINVAL;
  

• goto out;
  

• }
• offset += 4;
• fw_hdr->drv_fw_version = be32_to_cpup((__be32 *)&buf[offset]);
• offset += 8;
• fw_hdr->plat_type = buf[offset];
• offset += 1;
• fw_hdr->dev_family = buf[offset];
• offset += 1;
• fw_hdr->reserve = buf[offset];
• offset += 1;
• fw_hdr->ndev = buf[offset];
• offset += 1;

buf[offset++] would read better?

• if (fw_hdr->ndev != tas_priv->ndev) {

• dev_err(tas_priv->dev,
  

• 	"ndev(%u) in rcabin mismatch ndev(%u) in DTS\n",
  

• 	fw_hdr->ndev, tas_priv->ndev);
  

• tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
  

• ret = -EINVAL;
  

• goto out;
  

• }
• if (offset + TASDEVICE_DEVICE_SUM > fw_hdr->img_sz) {

• dev_err(tas_priv->dev, "rca_ready: Out of boundary!\n");
  

• ret = -EINVAL;
  

• tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
  

• goto out;
  

• }
• for (i = 0; i < TASDEVICE_DEVICE_SUM; i++, offset++)

• fw_hdr->devs[i] = buf[offset];
  

• fw_hdr->nconfig = be32_to_cpup((__be32 *)&buf[offset]);
• offset += 4;
• for (i = 0; i < TASDEVICE_CONFIG_SUM; i++) {

• fw_hdr->config_size[i] = be32_to_cpup((__be32 *)&buf[offset]);
  

• offset += 4;
  

• total_config_sz += fw_hdr->config_size[i];
  

• }
• if (fw_hdr->img_sz - total_config_sz != (unsigned int)offset) {

• dev_err(tas_priv->dev, "Bin file error!\n");
  

• ret = -EINVAL;
  

• tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
  

• goto out;
  

• }
• cfg_info = kcalloc(fw_hdr->nconfig, sizeof(*cfg_info), GFP_KERNEL);
• if (!cfg_info) {

• ret = -ENOMEM;
  

• tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
  

• goto out;
  

• }
• rca->cfg_info = cfg_info;
• rca->ncfgs = 0;
• for (i = 0; i < (int)fw_hdr->nconfig; i++) {

• rca->ncfgs += 1;
  

• cfg_info[i] = tasdevice_add_config(tas_priv, &buf[offset],
  

• 	fw_hdr->config_size[i], &ret);
  

• if (ret) {
  

• 	tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
  

• 	goto out;
  

• }
  

• offset += (int)fw_hdr->config_size[i];
  

• }

+out:

• return ret;

+} +EXPORT_SYMBOL_NS_GPL(tasdevice_spi_rca_parser, SND_SOC_TAS2781_FMWLIB);

+/* fixed m68k compiling issue: mapping table can save code field */ +static unsigned char map_dev_idx(struct tasdevice_fw *tas_fmw,

• struct tasdev_blk *block)

+{

• struct blktyp_devidx_map *p =

• (struct blktyp_devidx_map *)non_ppc3_mapping_table;
  

• struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr);
• struct tasdevice_fw_fixed_hdr *fw_fixed_hdr = &(fw_hdr->fixed_hdr);
• int i, n = ARRAY_SIZE(non_ppc3_mapping_table);

useless init for n

• unsigned char dev_idx = 0;
• if (fw_fixed_hdr->ppcver >= PPC3_VERSION_TAS2781) {

• p = (struct blktyp_devidx_map *)ppc3_tas2781_mapping_table;
  

• n = ARRAY_SIZE(ppc3_tas2781_mapping_table);
  

• } else if (fw_fixed_hdr->ppcver >= PPC3_VERSION) {

• p = (struct blktyp_devidx_map *)ppc3_mapping_table;
  

• n = ARRAY_SIZE(ppc3_mapping_table);
  

• }
• for (i = 0; i < n; i++) {

• if (block->type == p[i].blktyp) {
  

• 	dev_idx = p[i].dev_idx;
  

• 	break;
  

• }
  

• }
• return dev_idx;

+}

+static int fw_parse_variable_header_kernel(

• struct tasdevice_priv *tas_priv, const struct firmware *fmw,
• int offset)

+{

• struct tasdevice_fw *tas_fmw = tas_priv->fmw;
• struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr);
• struct tasdevice_prog *program;
• struct tasdevice_config *config;
• const unsigned char *buf = fmw->data;
• unsigned short max_confs;
• unsigned int i;
• if (offset + 12 + 4 * TASDEVICE_MAXPROGRAM_NUM_KERNEL > fmw->size) {

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

• offset = -EINVAL;
  

• goto out;
  

• }
• fw_hdr->device_family = be16_to_cpup((__be16 *)&buf[offset]);
• if (fw_hdr->device_family != 0) {

• dev_err(tas_priv->dev, "%s:not TAS device\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• offset += 2;
• fw_hdr->device = be16_to_cpup((__be16 *)&buf[offset]);
• if (fw_hdr->device >= TASDEVICE_DSP_TAS_MAX_DEVICE ||> +

+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 > fmw->size) {

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

• offset = -EINVAL;
  

• goto out;
  

• }
• /* convert data[offset], data[offset + 1], data[offset + 2] and

• * data[offset + 3] into host
  

• */
  

• block->type = be32_to_cpup((__be32 *)&data[offset]);
• offset += 4;
• block->is_pchksum_present = data[offset];
• offset++;
• block->pchksum = data[offset];
• offset++;
• block->is_ychksum_present = data[offset];
• offset++;
• block->ychksum = data[offset];
• offset++;
• block->blk_size = be32_to_cpup((__be32 *)&data[offset]);
• offset += 4;
• block->nr_subblocks = be32_to_cpup((__be32 *)&data[offset]);
• offset += 4;
• /* fixed m68k compiling issue:

• * 1. mapping table can save code field.
  

• * 2. storing the dev_idx as a member of block can reduce unnecessary
  

• *    time and system resource comsumption of dev_idx mapping every
  

• *    time the block data writing to the dsp.
  

• */
  

• block->dev_idx = map_dev_idx(tas_fmw, block);
• if (offset + block->blk_size > fmw->size) {

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

• offset = -EINVAL;
  

• goto out;
  

• }
• /* instead of kzalloc+memcpy */
• block->data = kmemdup(&data[offset], block->blk_size, GFP_KERNEL);
• if (!block->data) {

• offset = -ENOMEM;
  

• goto out;
  

• }
• offset += block->blk_size;

+out:

• return offset;

+}

+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;
• if (offset + 4 > fmw->size) {

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

• offset = -EINVAL;
  

• goto out;
  

• }
• img_data->nr_blk = be32_to_cpup((__be32 *)&data[offset]);
• offset += 4;
• img_data->dev_blks = kcalloc(img_data->nr_blk,

• sizeof(struct tasdev_blk), GFP_KERNEL);
  

• if (!img_data->dev_blks) {

• offset = -ENOMEM;
  

• goto out;
  

• }
• for (i = 0; i < img_data->nr_blk; i++) {

• blk = &(img_data->dev_blks[i]);
  

• offset = fw_parse_block_data_kernel(tas_fmw, blk, fmw, offset);
  

• if (offset < 0) {
  

• 	offset = -EINVAL;
  

• 	break;
  

• }
  

• }

+out:

• return offset;

+}

+static int fw_parse_program_data_kernel(

• struct tasdevice_priv *tas_priv, struct tasdevice_fw *tas_fmw,
• const struct firmware *fmw, int offset)

+{

• struct tasdevice_prog *program;
• unsigned int i;
• for (i = 0; i < tas_fmw->nr_programs; i++) {

• program = &(tas_fmw->programs[i]);
  

• if (offset + 72 > fmw->size) {
  

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

• 	offset = -EINVAL;
  

• 	goto out;
  

• }
  

• /*skip 72 unused byts*/
  

• offset += 72;
  

• offset = fw_parse_data_kernel(tas_fmw, &(program->dev_data),
  

• 	fmw, offset);
  

• if (offset < 0)
  

• 	goto out;
  

• }

+out:

• return offset;

+}

+static int fw_parse_configuration_data_kernel(

• struct tasdevice_priv *tas_priv,
• struct tasdevice_fw *tas_fmw, const struct firmware *fmw, int offset)

+{

• const unsigned char *data = fmw->data;
• struct tasdevice_config *config;
• unsigned int i;
• for (i = 0; i < tas_fmw->nr_configurations; i++) {

• config = &(tas_fmw->configs[i]);
  

• if (offset + 80 > fmw->size) {
  

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

• 	offset = -EINVAL;
  

• 	goto out;
  

• }
  

• memcpy(config->name, &data[offset], 64);
  

• /*skip extra 16 bytes*/
  

• offset += 80;
  

• offset = fw_parse_data_kernel(tas_fmw, &(config->dev_data),
  

• 	fmw, offset);
  

• if (offset < 0)
  

• 	goto out;
  

• }

+out:

• return offset;

+}

• fw_hdr->device == 6) {
  

• dev_err(tas_priv->dev, "Unsupported dev %d\n", fw_hdr->device);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• offset += 2;
• fw_hdr->ndev = deviceNumber[fw_hdr->device];
• if (fw_hdr->ndev != tas_priv->ndev) {

• dev_err(tas_priv->dev,
  

• 	"%s: ndev(%u) in dspbin mismatch ndev(%u) in DTS\n",
  

• 	__func__, fw_hdr->ndev, tas_priv->ndev);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• tas_fmw->nr_programs = be32_to_cpup((__be32 *)&buf[offset]);
• offset += 4;
• if (tas_fmw->nr_programs == 0 || tas_fmw->nr_programs >

• TASDEVICE_MAXPROGRAM_NUM_KERNEL) {
  

• dev_err(tas_priv->dev, "mnPrograms is invalid\n");
  

• offset = -EINVAL;
  

• goto out;
  

• }
• tas_fmw->programs = kcalloc(tas_fmw->nr_programs,

• sizeof(struct tasdevice_prog), GFP_KERNEL);
  

• if (!tas_fmw->programs) {

• offset = -ENOMEM;
  

• goto out;
  

• }
• for (i = 0; i < tas_fmw->nr_programs; i++) {

• program = &(tas_fmw->programs[i]);
  

• program->prog_size = be32_to_cpup((__be32 *)&buf[offset]);
  

• offset += 4;
  

• }
• /* Skip the unused prog_size */
• offset += 4 * (TASDEVICE_MAXPROGRAM_NUM_KERNEL - tas_fmw->nr_programs);
• tas_fmw->nr_configurations = be32_to_cpup((__be32 *)&buf[offset]);
• offset += 4;
• /* The max number of config in firmware greater than 4 pieces of

• * tas2781s is different from the one lower than 4 pieces of
  

• * tas2781s.
  

• */
  

• max_confs = (fw_hdr->ndev >= 4) ?

• TASDEVICE_MAXCONFIG_NUM_KERNEL_MULTIPLE_AMPS :
  

• TASDEVICE_MAXCONFIG_NUM_KERNEL;
  

• if (tas_fmw->nr_configurations == 0 ||

• tas_fmw->nr_configurations > max_confs) {
  

• dev_err(tas_priv->dev, "%s: Conf is invalid\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• if (offset + 4 * max_confs > fmw->size) {

• dev_err(tas_priv->dev, "%s: mpConfigurations err\n", __func__);
  

• offset = -EINVAL;
  

• goto out;
  

• }
• tas_fmw->configs = kcalloc(tas_fmw->nr_configurations,

• sizeof(struct tasdevice_config), GFP_KERNEL);
  

• if (!tas_fmw->configs) {

• offset = -ENOMEM;
  

• goto out;
  

• }
• for (i = 0; i < tas_fmw->nr_programs; i++) {

• config = &(tas_fmw->configs[i]);
  

• config->cfg_size = be32_to_cpup((__be32 *)&buf[offset]);
  

• offset += 4;
  

• }
• /* Skip the unused configs */
• offset += 4 * (max_confs - tas_fmw->nr_programs);

+out:

• return offset;

same here, error handling is not quite right

I'll stop the review here.