Re: [PATCH v2 1/1] elantech: Add support for trackpoint found on some v3 models

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

On Fri, Jun 13, 2014 at 9:24 AM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>> You rely on external data here, so please check for truncation. If
>> anyone changes psmouse->ps2dev.serio->phys, they would not notice that
>> you rely on it here. So I'd do sth like:
>>
>> size_t n = snprintf(etd->tp_phys, sizeof(etd->tp_phys), "%s/input1",
>> psmouse->ps2dev.serio->phys);
>> if (n >= sizeof(etd->tp_phys))
>>         goto init_fail_tp_reg;
>
> Ugh no, we've this snprintf pattern for foo->phys everywhere in the kernel
> and we don't do a truncation check anywhere. No-one in its right mind will
> make phys smaller, and even if phys gets truncated things will likely
> still work fine.
>
>> Or at least force a terminating-zero on phys by decreasing the length
>> by 1 (phys is initialized to zero):
>
> snprintf already forces a terminating zero, no need for weird tricks.
>
>>
>> snprintf(etd->tp_phys, sizeof(etd->tp_phys) - 1, "%s/input1",
>> psmouse->ps2dev.serio->phys);
>
> So this is wrong, no need for the - 1.

Oh, right, snprintf() always writes null. Fair enough.

>>
>>> +               tp_dev->phys = etd->tp_phys;
>>> +               tp_dev->name = "Elantech PS/2 TrackPoint";
>>> +               tp_dev->id.bustype = BUS_I8042;
>>> +               tp_dev->id.vendor  = 0x0002;
>>> +               tp_dev->id.product = PSMOUSE_ELANTECH;
>>> +               tp_dev->id.version = 0x0000;
>>> +               tp_dev->dev.parent = &psmouse->ps2dev.serio->dev;
>>> +               tp_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
>>> +               tp_dev->relbit[BIT_WORD(REL_X)] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
>>> +               tp_dev->keybit[BIT_WORD(BTN_LEFT)] =
>>> +                       BIT_MASK(BTN_LEFT) | BIT_MASK(BTN_MIDDLE) | BIT_MASK(BTN_RIGHT);
>>
>> Ugh, why not use __set_bit()? But ok, seems to be normal to use these
>> shortcuts for mice.
>>
>>> +               if (input_register_device(etd->tp_dev))
>>> +                       goto init_fail_tp_reg;
>>
>> Please forward the error code:
>>
>> int error = -EINVAL;
>>
>> ...
>>
>> error = input_register_device(tp_dev);
>> if (error < 0)
>>         goto init_fail_tp_reg;
>>
>> ...
>>
>> init_fail:
>> kfree(etd);
>> return error;
>>
>>> +       }
>>>
>>>         psmouse->protocol_handler = elantech_process_byte;
>>>         psmouse->disconnect = elantech_disconnect;
>>> @@ -1504,8 +1597,13 @@ int elantech_init(struct psmouse *psmouse)
>>>         psmouse->pktsize = etd->hw_version > 1 ? 6 : 4;
>>>
>>>         return 0;
>>> -
>>> + init_fail_tp_reg:
>>> +       input_free_device(tp_dev);
>>> + init_fail_tp_alloc:
>>> +       sysfs_remove_group(&psmouse->ps2dev.serio->dev.kobj,
>>> +                          &elantech_attr_group);
>>>   init_fail:
>>> +        psmouse_reset(psmouse);
>>
>> elantech_disconnect() does not call psmouse_reset, so why add it here?
>> What am I missing?
>
> AFAIK elantech disconnect will only get called if the ps2 mouse driver gets
> unloaded / the mouse gets hot-unplugged. Where as this gets called on
> probe failure, after which we want to turn the ps/2 mouse emulation
> back on so that it will work as a regular mouse.

But this sounds like a bugfix to me, which is unrelated to this
comment? If it is, please consider sending it separately.

Thanks
David
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]