From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754889AbbENCoe (ORCPT ); Wed, 13 May 2015 22:44:34 -0400 Received: from mga03.intel.com ([134.134.136.65]:53631 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753170AbbENCoc (ORCPT ); Wed, 13 May 2015 22:44:32 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,425,1427785200"; d="scan'208";a="694630443" Message-ID: <55555D2B.9000905@intel.com> Date: Fri, 15 May 2015 10:42:51 +0800 From: Pan Xinhui User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Dmitry Torokhov 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 References: <55377C22.9040309@intel.com> <20150513174156.GC11891@dtor-ws> In-Reply-To: <20150513174156.GC11891@dtor-ws> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 >