Re: [PATCH 00/11] Face lift for bu21013_ts driver
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2019-08-21 17:43:03
Also in:
lkml
On Wed, Aug 21, 2019 at 02:39:41PM +0200, Linus Walleij wrote:
quoted hunk ↗ jump to hunk
On Sat, Aug 10, 2019 at 2:20 AM Dmitry Torokhov [off-list ref] wrote:quoted
So your patch has prompted me to take a look at the driver and try to clean it up. I am sure I screwed up somewhere, but you said you have the device, so please take a look at the series and see if you can salvage themI will funnel patch 1/11 in the ARM SoC tree. The rest work fine except on the resource release in the error path. I had to do this:diff --git a/drivers/input/touchscreen/bu21013_ts.cb/drivers/input/touchscreen/bu21013_ts.c index c89a00a6e67c..bdae4cd4243a 100644--- a/drivers/input/touchscreen/bu21013_ts.c +++ b/drivers/input/touchscreen/bu21013_ts.c@@ -390,18 +390,18 @@ static int bu21013_init_chip(struct bu21013_ts *ts) return 0; } -static void bu21013_power_off(void *_ts) +static void bu21013_power_off(void *data) { - struct bu21013_ts *ts = ts; + struct regulator *regulator = data; - regulator_disable(ts->regulator); + regulator_disable(regulator); } -static void bu21013_disable_chip(void *_ts) +static void bu21013_disable_chip(void *data) { - struct bu21013_ts *ts = ts; + struct gpio_desc *gpiod = data; - gpiod_set_value(ts->cs_gpiod, 0); + gpiod_set_value(gpiod, 0); } static int bu21013_probe(struct i2c_client *client,@@ -488,7 +488,8 @@ static int bu21013_probe(struct i2c_client *client, return error; } - error = devm_add_action_or_reset(&client->dev, bu21013_power_off, ts); + error = devm_add_action_or_reset(&client->dev, bu21013_power_off, + ts->regulator); if (error) { dev_err(&client->dev, "failed to install power off handler\n"); return error;@@ -505,7 +506,7 @@ static int bu21013_probe(struct i2c_client *client, gpiod_set_consumer_name(ts->cs_gpiod, "BU21013 CS"); error = devm_add_action_or_reset(&client->dev, - bu21013_disable_chip, ts); + bu21013_disable_chip, ts->cs_gpiod); if (error) { dev_err(&client->dev, "failed to install chip disable handler\n");I think this is because when probe() fails it first free:s the devm_kzalloc() allocations, so the ts->foo will result in NULL dereference.
No, the release is done in opposite order of acquiring resources,
anything else would be madness and would not work.
The issue is this:
static void bu21013_disable_chip(void *_ts)
{
struct bu21013_ts *ts = ts;
which shuts up gcc about the fact that 'ts' is uninitialized, it should
have said "ts = _ts". I guess it is a lesson for me to not call the voi
d pointer argument almost the same name as the structure, as it is easy
to miss in the review. The compiler would not care in either case, but a
human might have noticed.
Can you please try making this change (and the same in power off
handler)?
Thanks.
--
Dmitry