| Message ID | 1369858343-681-1-git-send-email-andy.shevchenko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi Andy, On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko <andy.shevchenko@gmail.com> 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@gmail.com> > Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> 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. Thanks for review. See my answers below. >> --- a/drivers/hid/hid-multitouch.c >> +++ b/drivers/hid/hid-multitouch.c >> @@ -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). Okay, I'll fix it. >> + 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. Good point, will fix. >> @@ -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. Why not? As far as I understood this logic the input->name is assigned accordingly to what device is configured. Like input->name = hdev->name, except for pen it equals to hdev->name + " Pen". The last one is assigned in the mt_pen_input_configure. Otherwise input->name is NULL. Am I correct? -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jun 1, 2013 at 1:33 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires > <benjamin.tissoires@gmail.com> wrote: >> On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko >> <andy.shevchenko@gmail.com> 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. > > Thanks for review. See my answers below. > >>> --- a/drivers/hid/hid-multitouch.c >>> +++ b/drivers/hid/hid-multitouch.c > >>> @@ -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). > > Okay, I'll fix it. thanks. And sorry, I thought the test was against hi->input->name, thus my "hint" which was exactly the same as what you tested. Anyway, hdev->name is still never null, so the remark still applies. > >>> + 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. > > Good point, will fix. > >>> @@ -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. > > Why not? As far as I understood this logic the input->name is assigned > accordingly to what device is configured. Like input->name = > hdev->name, except for pen it equals to hdev->name + " Pen". The last > one is assigned in the mt_pen_input_configure. Otherwise input->name > is NULL. Am I correct? No. When the hid_input struct is allocated in hid-input.c, the field hi->input->name is set to hdev->name (my previous "hint" should be here). The whole dirty part of the patch when I included the "Pen" in the name was because of that: hi->input->name is never null, and it was difficult to change this in hid-core.c without any side effects in the other hid drivers. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) { + name = devm_kzalloc(&hdev->dev, strlen(hdev->name) + 5, + GFP_KERNEL); + 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) { + 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); }
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@gmail.com> Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- drivers/hid/hid-multitouch.c | 37 +++++++++++++------------------------ 1 files changed, 13 insertions(+), 24 deletions(-)