Re: [PATCH v2 3/9] HID: multitouch: add support for Nexio 42" panel

[Benjamin Tissoires]

On Mon, Feb 4, 2013 at 12:42 PM, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote:
> Hi Benjamin,
>
>> > Why not an index here?
>>
>> Just because an index is not sufficient. You need two things: an index
>> within the field, and the actual field (a pointer to a struct
>> hid_field). Each .value is local to a field, and even if in most of
>> the case, the contact count is alone in its field, it would mean to
>> take the risk that a new device does not follow this logic.
>
> The field value is passed to process_mt_event() in a fairly
> straight-forward fashion, I was imagining that behavior could be
> copied somehow.
>
>> So the actual pointer to the contact count value seemed to be the
>> shortest way to do it. But it can be easily changed.
>
> Keeping a pointer into the core structure creates unwanted
> dependencies to the scope of that value, making an eventual core
> refactoring even harder, not to mention trickier to debug. So even
> though it looks neat in the code, it pushes the problem forward.
>
>> >> @@ -251,6 +257,9 @@ static ssize_t mt_set_quirks(struct device *dev,
>> >>
>> >>       td->mtclass.quirks = val;
>> >>
>> >> +     if (!td->contactcount)
>> >> +             td->mtclass.quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
>> >> +
>> >
>> > Why override the overrider here?
>>
>> This callback is called from the user-space through the sysfs
>> attribute. So, it is not called in the same time that the
>> mt_post_parse function. This is just to avoid a user setting this
>> quirk once the device is up and running leading to a potential oops.
>
> Yes, but the quirk _is_ user modifiable. Hence, the problem lies in
> equating the user-modifiable quirk with the branch control of the
> program.

I'm not sure I understood what you meant there.
The quirk is indeed user modifiable, but through the callback
mt_set_quirks() only. If the HID field Contact Count is not present,
this quirk should not be allowed to be present, thus the two removals
of the quirk in met_set_quirks() and mt_post_parse(). As there are no
other entry points, I'm quite confuse to understand where the pitfall
is.

>
>> > An index into the the struct actually passed in mt_report() feels safer.
>>
>> again, you need to store "field" and "usage->usage_index". Agree, it
>> would be safer but it will take more space... :)
>
> If you think the code change is not only correct, but also moves the
> whole code base in a good direction, by all means.

Ok, will do. After a deeper look at it, I can even have two int
indexes (and no pointers): one for the field and one other for the
value within the field.

Cheers,
Benjamin

>
>> >> @@ -750,11 +765,15 @@ static void mt_post_parse_default_settings(struct mt_device *td)
>> >>  static void mt_post_parse(struct mt_device *td)
>> >>  {
>> >>       struct mt_fields *f = td->fields;
>> >> +     struct mt_class *cls = &td->mtclass;
>> >>
>> >>       if (td->touches_by_report > 0) {
>> >>               int field_count_per_touch = f->length / td->touches_by_report;
>> >>               td->last_slot_field = f->usages[field_count_per_touch - 1];
>> >>       }
>> >> +
>> >> +     if (!td->contactcount)
>> >> +             cls->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
>> >
>> > Since MT_QUIRK_CONTACT_CNT_ACCURATE is a quirk, modifiable by the
>> > user, it should probably not validate num_expected in the code. Better
>> > use the contact count index or something equivalent for that.
>>
>> As when the user changes the quirk, we validate it, this is not required.
>
> True, barring the comments above.
>
> Thanks,
> Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/