On Mon, Feb 4, 2013 at 12:42 PM, Henrik Rydberg [off-list ref] wrote:

Hi Benjamin,

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

@@ -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.

quoted

quoted

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

quoted

quoted

quoted

@@ -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