Hi Yongzhi Liu,
On 3/7/24 12:57, Yongzhi Liu wrote:
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), callback function ljca_auxdev_release
> calls kfree(auxdev->dev.platform_data) to free the parameter data
> of the function ljca_new_client_device. The callers of
> ljca_new_client_device shouldn't call kfree() again
> in the error handling path to free the platform data.
>
> Fix this by cleaning up the redundant kfree() in all callers and
> adding kfree() the passed in platform_data on errors which happen
> before auxiliary_device_init() succeeds .
>
> Fixes: acd6199f195d ("usb: Add support for Intel LJCA device")
> Signed-off-by: Yongzhi Liu <hyperlyzcs@xxxxxxxxx>
> ---
> drivers/usb/misc/usb-ljca.c | 26 +++++++++-----------------
> 1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> index 35770e608c64..bd9ccbea6e72 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -518,8 +518,10 @@ static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id,
> int ret;
>
> client = kzalloc(sizeof *client, GFP_KERNEL);
> - if (!client)
> + if (!client) {
> + kfree(data);
> return -ENOMEM;
> + }
>
> client->type = type;
> client->id = id;
> @@ -535,8 +537,10 @@ static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id,
> auxdev->dev.release = ljca_auxdev_release;
>
> ret = auxiliary_device_init(auxdev);
> - if (ret)
> + if (ret) {
> + kfree(data);
> goto err_free;
> + }
>
> ljca_auxdev_acpi_bind(adap, auxdev, adr, id);
>
> @@ -590,12 +594,8 @@ static int ljca_enumerate_gpio(struct ljca_adapter *adap)
> valid_pin[i] = get_unaligned_le32(&desc->bank_desc[i].valid_pins);
> bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num);
>
> - ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
> + return ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
> gpio_info, LJCA_GPIO_ACPI_ADR);
> - if (ret)
> - kfree(gpio_info);
> -
> - return ret;
> }
>
> static int ljca_enumerate_i2c(struct ljca_adapter *adap)
> @@ -626,13 +626,9 @@ static int ljca_enumerate_i2c(struct ljca_adapter *adap)
> i2c_info->capacity = desc->info[i].capacity;
> i2c_info->intr_pin = desc->info[i].intr_pin;
>
> - ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
> + return ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
> "ljca-i2c", i2c_info,
> LJCA_I2C1_ACPI_ADR + i);
> - if (ret) {
> - kfree(i2c_info);
> - return ret;
> - }
This is in a for loop, by using:
return ljca_new_client_device(...);
you are now only creating the first ljca-i2c controller, while
there may be more instead just drop the kfree() from the if (ret) {}
block:
ret = ljca_new_client_device(...);
if (ret)
return ret;
Sorry for not noticing this with the v1 posting.
> }
>
> return 0;
> @@ -666,13 +662,9 @@ static int ljca_enumerate_spi(struct ljca_adapter *adap)
> spi_info->id = desc->info[i].id;
> spi_info->capacity = desc->info[i].capacity;
>
> - ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
> + return ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
> "ljca-spi", spi_info,
> LJCA_SPI1_ACPI_ADR + i);
> - if (ret) {
> - kfree(spi_info);
> - return ret;
> - }
> }
Same remark as with the creation of the i2c controllers, please use:
ret = ljca_new_client_device(...);
if (ret)
return ret;
Regards,
Hans
