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

[Lucas Stach <l.stach@pengutronix.de>]

On Mo, 2020-01-20 at 11:54 -0800, Dmitry Torokhov wrote:
> 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.

The cached information here is only used for the sysfs FW info
attributes. In v2 I split those out into a separate group, which gets
removed and registered again when refreshing this information after the
FW update. This keeps the devm ordering the same and fixes a possible
use-after-free through sysfs reads at the same time.

Regards,
Lucas