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.