Re: [PATCH 4/4] Input: exc3000: add firmware update support

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Jan 07, 2020 at 06:17:40PM +0100, Lucas Stach wrote:
> This change allows the device firmware to be updated by putting a firmware
> file in /lib/firmware and providing the name of the file via the update_fw
> sysfs property. The driver will then flash the firmware image into the
> controller internal storage and restart the controller to activate the new
> firmware.
> 
> The implementation was done by looking at the the messages passed between
> the controller and proprietary vendor update tool. Not every detail of the
> protocol is totally well understood, so the implementation still has some
> "monkey see, monkey do" parts, as far as they have been found to be required
> for the update to succeed.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/input/touchscreen/exc3000.c | 248 +++++++++++++++++++++++++++-
>  1 file changed, 246 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index ce83914d65ff..f9a9820dc232 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -3,8 +3,8 @@
>   * Driver for I2C connected EETI EXC3000 multiple touch controller
>   *
>   * Copyright (C) 2017 Ahmet Inan <inan@distec.de>
> - *
> - * minimal implementation based on egalax_ts.c and egalax_i2c.c
> + * Copyright (C) 2019 Pengutronix <kernel@pengutronix.de>
> + * Copyright (C) 2019 Zodiac Inflight Innovations
>   */
>  
>  #include <linux/bitops.h>
> @@ -18,6 +18,8 @@
>  #include <linux/of.h>
>  #include <linux/timer.h>
>  #include <asm/unaligned.h>
> +#include <linux/firmware.h>
> +#include <linux/delay.h>
>  
>  #define EXC3000_NUM_SLOTS		10
>  #define EXC3000_SLOTS_PER_FRAME		5
> @@ -37,6 +39,7 @@ struct exc3000_data {
>  	struct mutex vendor_data_lock;
>  	struct completion vendor_data_done;
>  	char *type, *model, *fw_rev;
> +	int update_status;
>  };
>  
>  static void exc3000_report_slots(struct input_dev *input,
> @@ -215,6 +218,8 @@ static int exc3000_populate_device_info(struct exc3000_data *data)
>  	if (ret < 0)
>  		return -ENODEV;
>  
> +	if (data->type)
> +		devm_kfree(dev, data->type);
>  	data->type = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);

This makes me uncomfortable, as this changes the release order of the
resources (newly re-allocated memory will be freed first). Please
reassure me that it will work fine WRT device removal (or,
alternatively, we will need devm_realloc() that would preserve devm
release ordering.

Thanks.

-- 
Dmitry