On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxx> wrote:
> 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.
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
