On Thu, Feb 13, 2014 at 12:28:00AM +0100, Dmitry Torokhov wrote:
> On Wed, Feb 12, 2014 at 01:48:19PM -0800, Courtney Cavin wrote:
> > 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@xxxxxxxxxxxxx>
> > > > Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > > > Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > > > Cc: Linux Walleij <linus.walleij@xxxxxxxxxx>
> > > > Cc: David Herrmann <dh.herrmann@xxxxxxxxx>
> > > > Cc: Jiri Kosina <jkosina@xxxxxxx>
> > > >
> > > > ---
> > > >
> > > > 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@xxxxxxxxx>
> > >
> > > - 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.
>
> OK, I can do that...
>
> >
> > >
> > > Reported-by: Courtney Cavin <courtney.cavin@xxxxxxxxxxxxxx>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > > ---
> > > 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.
>
> Actually I like messages: who knows when we decided to change kmalloc
> behavior and it also helps when there are several allocations in the
> same function to know which one failed.
Of course it's your choice, but I find addr2line does a pretty good job
here with the backtrace. Maybe we'll get #define GFP_KERNEL __GFP_NOWARN,
but I think someone will go through with some cocci scripts at that point.
-Courtney
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
