Re: [PATCH 00/11] Face lift for bu21013_ts driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Wed, Aug 21, 2019 at 02:39:41PM +0200, Linus Walleij wrote:
> On Sat, Aug 10, 2019 at 2:20 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > 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 them
> 
> I 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.c
> b/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