On 02/12/2014 01:48 PM, 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.
Hi,
This arrived a few seconds after I sent my reply. Looks like mail is
slow today.
I am OK to either split or lump the patch - Dmitry can make that call.
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.
We print messages like that in a lot of places. Based on your prior
comments, I figured to do a blanket up date that removes all of those at
once across the driver. Would that be an OK solution?
+ 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?
Unfortunately, no. I've filed a bug on that. In the meantime, I've
found the following:
* It looks like there's a control register F01_RMI_Ctrl4 which is
present if the has_lts bit is set, but is not used in any shipped LTS
products.
* Both F01_RMI_Ctrl2 and F01_RMI_Ctrl3 (doze_interval and
wakeup_threshold) are controlled by the has_adjustable_doze bit.
The patch I sent a bit ago includes fixes based on this info.
Thanks,
Chris
--
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
