Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934354AbbEMRmF (ORCPT ); Wed, 13 May 2015 13:42:05 -0400 Received: from mail-ig0-f182.google.com ([209.85.213.182]:37576 "EHLO mail-ig0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932909AbbEMRmB (ORCPT ); Wed, 13 May 2015 13:42:01 -0400 Date: Wed, 13 May 2015 10:41:56 -0700 From: Dmitry Torokhov To: Pan Xinhui Cc: linux-kernel@vger.kernel.org, nick.dyer@itdev.co.uk, miletus@chromium.org, bleung@chromium.org, djkurtz@chromium.org, mnipxh@gmail.com, yanmin_zhang@linux.intel.com Subject: Re: [PATCH] atmel: fix an error handle in mxt_probe Message-ID: <20150513174156.GC11891@dtor-ws> References: <55377C22.9040309@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55377C22.9040309@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3037 Lines: 82 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 > --- > 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. > + > 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 -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/