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

The code looks much better, and now that you've replaced all the -1 error codes with -EINVAL or -ENOMEM we can see clearly cases with memory leaks in the error handling path.

• /* Skip the unused prog_size */
• offset += (4 * (TASDEVICE_MAXPROGRAM_NUM_KERNEL -

• tas_fmw->nr_programs));
  

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

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

• offset = -EINVAL;
  

• goto out;
  

• }
• tas_fmw->nr_configurations = SMS_HTONL(buf[offset], buf[offset + 1],

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

• offset += 4;
• /* The max of conf for more than equal to 4 pcs of tas2781s is

• * different from the one less than 4 pcs of tas2781s.
  

• */
  

Consider rewording, this comment is just confusing. Not sure what 'pcs' means here, and "more than equal' and 'one less than' should be replaced by 'greater than' and 'lower than'.

+static int fw_parse_data(struct tasdevice_fw *tas_fmw,

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

+{

• const unsigned char *data = (unsigned char *)fmw->data;
• struct tasdev_blk *blk;
• unsigned int i;
• int n;
• if (offset + 64 > fmw->size) {

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

• offset = -EINVAL;
  

• goto out;
  

• }
• memcpy(img_data->name, &data[offset], 64);
• offset += 64;
• n = strlen((char *)&data[offset]);
• n++;
• if (offset + n > fmw->size) {

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

• offset = -EINVAL;
  

• goto out;
  

• }
• offset += n;
• if (offset + 2 > fmw->size) {

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

• offset = -EINVAL;
  

• goto out;
  

• }
• img_data->nr_blk = SMS_HTONS(data[offset], data[offset + 1]);
• offset += 2;
• 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(tas_fmw, blk, fmw, offset);
  

• if (offset < 0) {
  

• 	offset = -EINVAL;
  

• 	goto out;
  

• }
  

• }

+out:

• return offset;

memory leak on img_data->dev_blks, you need to free memory in the error handling path.

+}

+static int fw_parse_calibration_data(struct tasdevice_priv *tas_dev,

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

+{

• struct tasdevice_calibration *calibration;
• unsigned char *data = (unsigned char *)fmw->data;
• unsigned int i, n;
• if (offset + 2 > fmw->size) {

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

• offset = -EINVAL;
  

• goto out;
  

• }
• tas_fmw->nr_calibrations = SMS_HTONS(data[offset], data[offset + 1]);
• offset += 2;
• if (tas_fmw->nr_calibrations != 1) {

• dev_err(tas_dev->dev,
  

• 	"%s: only support one calibraiton(%d)!\n",
  

• 	__func__, tas_fmw->nr_calibrations);
  

• goto out;
  

• }
• tas_fmw->calibrations = kcalloc(tas_fmw->nr_calibrations,

• sizeof(struct tasdevice_calibration), GFP_KERNEL);
  

• if (!tas_fmw->calibrations) {

• offset = -ENOMEM;
  

• goto out;
  

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

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

• 	dev_err(tas_dev->dev, "Calibrations error\n");
  

• 	offset = -EINVAL;
  

memory leak on tas_fmw->calibrations, you need to change the error handling path.

• 	goto out;
  

• }
  

• calibration = &(tas_fmw->calibrations[i]);
  

• offset += 64;
  

• n = strlen((char *)&data[offset]);
  

• n++;
  

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

• 	dev_err(tas_dev->dev, "Description err\n");
  

• 	offset = -EINVAL;
  

• 	goto out;
  

• }
  

• offset += n;
  

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

• 	dev_err(tas_dev->dev, "%s: Prog err, offset = %d\n",
  

• 		__func__, offset);
  

• 	offset = -EINVAL;
  

• 	goto out;
  

• }
  

• offset++;
  

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

• 	dev_err(tas_dev->dev, "%s: Conf err, offset = %d\n",
  

• 		__func__, offset);
  

• 	offset = -EINVAL;
  

• 	goto out;
  

• }
  

• offset++;
  

• offset = fw_parse_data(tas_fmw, &(calibration->dev_data), fmw,
  

• 	offset);
  

• if (offset < 0)
  

• 	goto out;
  

• }

+out:

• return offset;

+}

+static int fw_parse_program_data(struct tasdevice_priv *tas_dev,

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

+{

• unsigned char *buf = (unsigned char *)fmw->data;
• struct tasdevice_prog *program;
• int i;
• if (offset + 2 > fmw->size) {

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

• offset = -EINVAL;
  

• goto out;
  

• }
• tas_fmw->nr_programs = SMS_HTONS(buf[offset], buf[offset + 1]);
• offset += 2;
• if (tas_fmw->nr_programs == 0) {

• /*Not error in calibration Data file, return directly*/
  

• dev_info(tas_dev->dev, "%s: No Programs data, maybe calbin\n",
  

• 	__func__);
  

• 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++) {

• int n = 0;
  

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

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

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

• 	offset = -EINVAL;
  

• 	goto out;
  

and memory leak here as well.

Stopping the review here, please revisit the error handling. you probably need two labels when memory is allocated, and a kfree() for one of the two.