Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix

[Courtney Cavin <courtney.cavin@sonymobile.com>]

On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> Hi Chris,
> 
> On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
> > Correctly free driver related data when initialization fails.
> >
> > Trivial: Clarify a diagnostic message.
> >
> > Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Cc: Linux Walleij <linus.walleij@linaro.org>
> > Cc: David Herrmann <dh.herrmann@gmail.com>
> > Cc: Jiri Kosina <jkosina@suse.cz>
> >
> > ---
> >
> >  drivers/input/rmi4/rmi_f01.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> > index 381ad60..e4a6df9 100644
> > --- a/drivers/input/rmi4/rmi_f01.c
> > +++ b/drivers/input/rmi4/rmi_f01.c
> > @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> >
> >       f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
> >       if (!f01) {
> > -             dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
> > +             dev_err(&fn->dev, "Failed to allocate f01_data.\n");
> >               return -ENOMEM;
> >       }
> >
> > @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> >                       GFP_KERNEL);
> >       if (!f01->device_control.interrupt_enable) {
> >               dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
> > +             devm_kfree(&fn->dev, f01);
> 
> As Courtney mentioned if you are calling devm_kfree() you are most
> likely doing something wrong.
> 
> How about the patch below? Please check the XXX comment, I have some
> concerns about lts vs doze_holdoff check mismatch in probe() and
> config().
> 
> Thanks.
> 
> --
> Dmitry
> 
> Input: synaptics-rmi4 - F01 initialization cleanup
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> - rename data to f01 where appropriate;
> - switch to using rmi_read()/rmi_write() for single-byte data;
> - allocate interrupt mask together with the main structure;
> - do not kfree() memory allocated with devm;
> - do not write config data in probe(), we have config() for that;
> - drop unneeded rmi_f01_remove().

These seem like unrelated changes and make this patch hard to read, I
would prefer if we could separate these out.  Perhaps like so?
	[1] bug-fix
		- do not kfree() memory allocated with devm
	[2] simplify probe/remove logic
		- allocate interrupt mask together with the main structure
		- do not write config data in probe(), we have config() for that
		- drop unneeded rmi_f01_remove()
	[3] non-behavioral changes/cleanup
		- switch to using rmi_read()/rmi_write() for single-byte data
		- rename data to f01 where appropriate

Disregarding that, and the nitpick below, it looks good to me.

> 
> Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/rmi4/rmi_f01.c |  397 ++++++++++++++++++------------------------
>  1 file changed, 172 insertions(+), 225 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 381ad60..8f7840e 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
[...] 
> -static int rmi_f01_initialize(struct rmi_function *fn)
> +static int rmi_f01_probe(struct rmi_function *fn)
>  {
> -       u8 temp;
> -       int error;
> -       u16 ctrl_base_addr;
>         struct rmi_device *rmi_dev = fn->rmi_dev;
>         struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> -       struct f01_data *data = fn->data;
> -       struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> +       const struct rmi_device_platform_data *pdata =
> +                               to_rmi_platform_data(rmi_dev);
> +       struct f01_data *f01;
> +       size_t f01_size;
> +       int error;
> +       u16 ctrl_base_addr;
> +       u8 device_status;
> +       u8 temp;
> +
> +       f01_size = sizeof(struct f01_data) +
> +                               sizeof(u8) * driver_data->num_of_irq_regs;
> +       f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> +       if (!f01) {
> +               dev_err(&fn->dev, "Failed to allocate fn01_data.\n");

Nitpick: Can we drop this printout in the process?  It's much less
useful than the error and backtrace coming from kmalloc on failure anyway.

> +               return -ENOMEM;
> +       }
[...]

> +       /* XXX: why we check has_lts here but has_adjustable_doze in probe? */

Hrm.  This register is poorly documented in the spec.  All of these bits
are reserved.  Chris, is there a newer version of the spec which
documents these bits?

-Courtney