atmel: fix a race between fw_load and data free
Subsystems (2)
PATCH v2 1/1 atmel: fix a race between fw_load and data free
mxt_probe() may fail at last step, or we jsut unload mxt module soon. the queue_work scheduled by request_firmware_nowait may run later, and then access some data which is freed.
To handle this issue, add fw_load_completion field in mxt_data. then we wait for it complete both in probe error path and mxt_remove().
here is the detail in probe, similar in remove.
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).---
change in V2:
use fw_load_completion instead of statics.
fix a race both in mxt_remove and mxt_probe.
drivers/input/touchscreen/atmel_mxt_ts.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/input/touchscreen/atmel_mxt_ts.c +10 -0
@@ -313,6 +313,9 @@ struct mxt_data {/* for config update handling */ struct completion crc_completion; + +/* for fw load handling */ +struct completion fw_load_completion; };static size_t mxt_obj_size (const struct mxt_object *obj)
@@ -1982,8 +1985,10 @@ static int mxt_configure_objects(struct mxt_data *data,static void mxt_config_cb (const struct firmware *cfg,void *ctx) { +struct mxt_data *data = ctx;mxt_configure_objects (ctx, cfg);release_firmware (cfg); +complete (&data->fw_load_completion); }static int mxt_initialize (struct mxt_data *data)
@@ -2556,6 +2561,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)init_completion (&data->bl_completion);init_completion (&data->reset_completion);init_completion (&data->crc_completion); +init_completion (&data->fw_load_completion); error = request_threaded_irq(client->irq, NULL, mxt_interrupt, pdata->irqflags | IRQF_ONESHOT,
@@ -2581,6 +2587,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)return 0 ; err_free_object: +mxt_wait_for_completion (data, &data->fw_load_completion, +MXT_FW_RESET_TIME );mxt_free_input_device (data);mxt_free_object_table (data); err_free_irq:
@@ -2594,6 +2602,8 @@ static int mxt_remove(struct i2c_client *client) {struct mxt_data *data =i2c_get_clientdata (client); +mxt_wait_for_completion (data, &data->fw_load_completion, +MXT_FW_RESET_TIME );sysfs_remove_group (&client->dev.kobj, &mxt_attr_group);free_irq (data->irq, data);mxt_free_input_device (data);
Signature · 1 line
1.9.1
Hi-
This looks like a good tidy up, one minor comment:
Review
Quoted 2 lines · Pan Xinhui
(quoted diff) +2
-0+ mxt_wait_for_completion(data, &data->fw_load_completion, + MXT_FW_RESET_TIME);
this should probably just be
wait_for_completion(&data->fw_load_completion);
rather than timing out (the MXT_FW_RESET_TIME may not be an appropriate length anyway)
cheers
Nick
hi,
yes, we can't guarantee MXT_FW_RESET_TIME is enough. patch v3 is sent out :)
thanks
xinhui
Quoted 19 lines · Nick Dyer
Hi-
This looks like a good tidy up, one minor comment:
Review
Quoted 2 lines · Pan Xinhui
(quoted diff) +2
-0+ mxt_wait_for_completion(data, &data->fw_load_completion, + MXT_FW_RESET_TIME);this should probably just be
wait_for_completion(&data->fw_load_completion);rather than timing out (the
MXT_FW_RESET_TIMEmay not be an appropriate length anyway)cheers
Nick