Re: [PATCH] atmel: fix an error handle in mxt_probe

[Pan Xinhui]

HI, Dmitry
	thanks for your reply :)

On 2015å05æ14æ 01:41, Dmitry Torokhov wrote:
> Hi,
>
> On Wed, Apr 22, 2015 at 06:46:58PM +0800, Pan Xinhui wrote:
> > mxt_probe() may fail at last step, and the queue_work scheduled by request_firmware_nowait
> > may run later and then access some data which is freed.
> > To handle this error, add one mutex_lock to cover such case. It may cause module load delay only when the probe fails.
> >
> > here is the detail.
> >
> > module load:                                                             worker_thread:
> > mxt_probe -> mxt_initialize -> request_firmware_nowait (schedule_work)
> >                     |
> >              sysfs_create_group (fails)                                    mxt_config_cb -> mxt_configure_objects (may access data freed)
> >                     |
> >              err_free_object: some cleanup work, like free(data).
> >
> > Signed-off-by: xinhuix.pan <xinhuix.pan@xxxxxxxxx>
> > ---
> >   drivers/input/touchscreen/atmel_mxt_ts.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 2875ddf..af057c0 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -1978,10 +1978,19 @@ err_free_mem:
> >   static int mxt_configure_objects(struct mxt_data *data,
> >   				 const struct firmware *cfg);
> > +static DEFINE_MUTEX(err_probe_lock);
> > +static int err_probe;
>
> While you are right that bad things will happen if we let
> request_firmware_nowait() run after driver fails to bind to the device
> using statics to indicate success or failure is not good idea since you
> may have several such devices in your unit. Also it still doe snot help
> if you decide to unbind the device quickly or unlock the module.
>
> I guess the best way is to signal a completion from callback and wait
> for it in error path and in remove().
>
> Thanks.

yes, statics is not good.
It's also a good point that do some work both in err patch and in_remove().

thanks for your nice advices. I will work out patch V2.

thanks

xinhui

> > +
> >   static void mxt_config_cb(const struct firmware *cfg, void *ctx)
> >   {
> > +	mutex_lock(&err_probe_lock);
> > +	if (err_probe) {
> > +		mutex_unlock(&err_probe_lock);
> > +		return;
> > +	}
> >   	mxt_configure_objects(ctx, cfg);
> >   	release_firmware(cfg);
> > +	mutex_unlock(&err_probe_lock);
> >   }
> >   static int mxt_initialize(struct mxt_data *data)
> > @@ -2423,6 +2432,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >   	const struct mxt_platform_data *pdata;
> >   	int error;
> > +	err_probe = 0;
> > +
> >   	pdata = dev_get_platdata(&client->dev);
> >   	if (!pdata) {
> >   		pdata = mxt_parse_dt(client);
> > @@ -2472,6 +2483,9 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >   	return 0;
> >   err_free_object:
> > +	mutex_lock(&err_probe_lock);
> > +	err_probe = -1;
> > +	mutex_unlock(&err_probe_lock);
> >   	mxt_free_input_device(data);
> >   	mxt_free_object_table(data);
> >   err_free_irq:
> > --
> > 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/