Hi Andy,
On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> mt_free_input_name() was never called during .remove(): hid_hw_stop()
> removes the hid_input items in hdev->inputs, and so the list is
> therefore empty after the call. In the end, we never free the special
> names that has been allocated during .probe().
>
> We switch to devm_kzalloc that manages resources when driver is removed.
>
> Signed-off-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Reported-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> ---
> drivers/hid/hid-multitouch.c | 37 +++++++++++++------------------------
> 1 files changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index d99b959..1f5876e 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -261,14 +261,6 @@ static struct mt_class mt_classes[] = {
> { }
> };
>
> -static void mt_free_input_name(struct hid_input *hi)
> -{
> - struct hid_device *hdev = hi->report->device;
> -
> - if (hi->input->name != hdev->name)
> - kfree(hi->input->name);
> -}
> -
> static ssize_t mt_show_quirks(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -412,10 +404,12 @@ static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
> static void mt_pen_input_configured(struct hid_device *hdev,
> struct hid_input *hi)
> {
> - char *name = kzalloc(strlen(hi->input->name) + 5, GFP_KERNEL);
> - if (name) {
> - sprintf(name, "%s Pen", hi->input->name);
> - mt_free_input_name(hi);
> + char *name;
> +
> + if (hdev->name) {
hdev->name is always not null, so no need to check this (hint: it
contains hid->name when allocated).
> + name = devm_kzalloc(&hdev->dev, strlen(hdev->name) + 5,
> + GFP_KERNEL);
Does devm_kzalloc always return a valid pointer? If not, you should
just use devm_kzalloc instead of kzalloc and keep the old ordering of
allocation, test, and snprintf.
> + sprintf(name, "%s Pen", hdev->name);
> hi->input->name = name;
> }
>
> @@ -925,16 +919,18 @@ static void mt_post_parse(struct mt_device *td)
> static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
> {
> struct mt_device *td = hid_get_drvdata(hdev);
> - char *name = kstrdup(hdev->name, GFP_KERNEL);
> -
> - if (name)
> - hi->input->name = name;
>
> if (hi->report->id == td->mt_report_id)
> mt_touch_input_configured(hdev, hi);
>
> if (hi->report->id == td->pen_report_id)
> mt_pen_input_configured(hdev, hi);
> +
> + if (!hi->input->name) {
will never happen, so can be dropped.
> + hi->input->name = devm_kzalloc(&hdev->dev,
> + strlen(hdev->name) + 1, GFP_KERNEL);
> + strcpy(hi->input->name, hdev->name);
> + }
> }
>
> static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> @@ -993,7 +989,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>
> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> if (ret)
> - goto hid_fail;
> + goto fail;
>
> ret = sysfs_create_group(&hdev->dev.kobj, &mt_attribute_group);
>
> @@ -1005,9 +1001,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>
> return 0;
>
> -hid_fail:
> - list_for_each_entry(hi, &hdev->inputs, list)
> - mt_free_input_name(hi);
> fail:
> kfree(td->fields);
> kfree(td);
> @@ -1037,14 +1030,10 @@ static int mt_resume(struct hid_device *hdev)
> static void mt_remove(struct hid_device *hdev)
> {
> struct mt_device *td = hid_get_drvdata(hdev);
> - struct hid_input *hi;
>
> sysfs_remove_group(&hdev->dev.kobj, &mt_attribute_group);
> hid_hw_stop(hdev);
>
> - list_for_each_entry(hi, &hdev->inputs, list)
> - mt_free_input_name(hi);
> -
> kfree(td);
> hid_set_drvdata(hdev, NULL);
> }
> --
> 1.7.7.6
>
Cheers,
Benjamin
--
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
