| Message ID | 20190606161322.47192-1-jeffrey.l.hugo@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | Basic DT support for Lenovo Miix 630 | expand |
On Thu, Jun 06, 2019 at 09:13:22AM -0700, Jeffrey Hugo wrote: > There needs to be coordination between hid-quirks and the elan_i2c driver > about which devices are handled by what drivers. Currently, both use > whitelists, which results in valid devices being unhandled by default, > when they should not be rejected by hid-quirks. This is quickly becoming > an issue. > > Since elan_i2c has a maintained whitelist of what devices it will handle, > use that to implement a blacklist in hid-quirks so that only the devices > that need to be handled by elan_i2c get rejected by hid-quirks, and > everything else is handled by default. The downside is the whitelist and > blacklist need to be kept in sync. > > Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > --- > drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------ > 1 file changed, 67 insertions(+), 11 deletions(-) > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c > index e5ca6fe2ca57..edebd0700e3d 100644 > --- a/drivers/hid/hid-quirks.c > +++ b/drivers/hid/hid-quirks.c > @@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = { > { } > }; > > +/* > + * List of device names that elan_i2c is handling and HID should ignore. Must > + * be kept in sync with elan_i2c > + */ > +static const char *hid_elan_i2c_ignore[] = { If this is a copy of elan whitelist, then, if we do not want to bother with sharing it in object form (as a elan-i2c-ids module), can we at least move it into include/linux/input/elan-i2c-ids.h and consume from hid-quirks.c? Thanks.
On 6/11/2019 6:35 PM, Dmitry Torokhov wrote: > On Thu, Jun 06, 2019 at 09:13:22AM -0700, Jeffrey Hugo wrote: >> There needs to be coordination between hid-quirks and the elan_i2c driver >> about which devices are handled by what drivers. Currently, both use >> whitelists, which results in valid devices being unhandled by default, >> when they should not be rejected by hid-quirks. This is quickly becoming >> an issue. >> >> Since elan_i2c has a maintained whitelist of what devices it will handle, >> use that to implement a blacklist in hid-quirks so that only the devices >> that need to be handled by elan_i2c get rejected by hid-quirks, and >> everything else is handled by default. The downside is the whitelist and >> blacklist need to be kept in sync. >> >> Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> >> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> >> --- >> drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------ >> 1 file changed, 67 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c >> index e5ca6fe2ca57..edebd0700e3d 100644 >> --- a/drivers/hid/hid-quirks.c >> +++ b/drivers/hid/hid-quirks.c >> @@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = { >> { } >> }; >> >> +/* >> + * List of device names that elan_i2c is handling and HID should ignore. Must >> + * be kept in sync with elan_i2c >> + */ >> +static const char *hid_elan_i2c_ignore[] = { > > If this is a copy of elan whitelist, then, if we do not want to bother > with sharing it in object form (as a elan-i2c-ids module), can we at > least move it into include/linux/input/elan-i2c-ids.h and consume from > hid-quirks.c? I can put it in a shared header file, however elan-i2c and hid-quirks would need to be updated in the same change to prevent a breakage, but that would seem to violate a concern Benjamin brought up in v4 given that elan-i2c is maintained in your input tree, and hid-quirks is maintained in his hid tree. Are you ok with the elan-i2c changes going through Benjamin's hid tree?
On Wed, 12 Jun 2019, Jeffrey Hugo wrote: > On 6/11/2019 6:35 PM, Dmitry Torokhov wrote: > > On Thu, Jun 06, 2019 at 09:13:22AM -0700, Jeffrey Hugo wrote: > > > There needs to be coordination between hid-quirks and the elan_i2c driver > > > about which devices are handled by what drivers. Currently, both use > > > whitelists, which results in valid devices being unhandled by default, > > > when they should not be rejected by hid-quirks. This is quickly becoming > > > an issue. > > > > > > Since elan_i2c has a maintained whitelist of what devices it will handle, > > > use that to implement a blacklist in hid-quirks so that only the devices > > > that need to be handled by elan_i2c get rejected by hid-quirks, and > > > everything else is handled by default. The downside is the whitelist and > > > blacklist need to be kept in sync. > > > > > > Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > > > --- > > > drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------ > > > 1 file changed, 67 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c > > > index e5ca6fe2ca57..edebd0700e3d 100644 > > > --- a/drivers/hid/hid-quirks.c > > > +++ b/drivers/hid/hid-quirks.c > > > @@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = { > > > { } > > > }; > > > +/* > > > + * List of device names that elan_i2c is handling and HID should ignore. Must > > > + * be kept in sync with elan_i2c > > > + */ > > > +static const char *hid_elan_i2c_ignore[] = { > > > > If this is a copy of elan whitelist, then, if we do not want to bother > > with sharing it in object form (as a elan-i2c-ids module), can we at > > least move it into include/linux/input/elan-i2c-ids.h and consume from > > hid-quirks.c? > > I can put it in a shared header file, however elan-i2c and hid-quirks > would need to be updated in the same change to prevent a breakage, but > that would seem to violate a concern Benjamin brought up in v4 given > that elan-i2c is maintained in your input tree, and hid-quirks is > maintained in his hid tree. > > Are you ok with the elan-i2c changes going through Benjamin's hid tree? We co-ordinate cross-subsystem merges all the time. That is never a reason to not do the 'right thing (tm)'. If this information can be held in a single, central place, without the need for constant re-alignment, I'm all for it.
On Tue, 11 Jun 2019, Dmitry Torokhov wrote: > > +static const char *hid_elan_i2c_ignore[] = { > > If this is a copy of elan whitelist, then, if we do not want to bother > with sharing it in object form (as a elan-i2c-ids module), can we at > least move it into include/linux/input/elan-i2c-ids.h and consume from > hid-quirks.c? Let's just not duplicate it in both objects. Why not properly export it from hid_quirks? Thanks,
On 6/12/2019 8:37 AM, Lee Jones wrote: > On Wed, 12 Jun 2019, Jeffrey Hugo wrote: > >> On 6/11/2019 6:35 PM, Dmitry Torokhov wrote: >>> On Thu, Jun 06, 2019 at 09:13:22AM -0700, Jeffrey Hugo wrote: >>>> There needs to be coordination between hid-quirks and the elan_i2c driver >>>> about which devices are handled by what drivers. Currently, both use >>>> whitelists, which results in valid devices being unhandled by default, >>>> when they should not be rejected by hid-quirks. This is quickly becoming >>>> an issue. >>>> >>>> Since elan_i2c has a maintained whitelist of what devices it will handle, >>>> use that to implement a blacklist in hid-quirks so that only the devices >>>> that need to be handled by elan_i2c get rejected by hid-quirks, and >>>> everything else is handled by default. The downside is the whitelist and >>>> blacklist need to be kept in sync. >>>> >>>> Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> >>>> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> >>>> --- >>>> drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------ >>>> 1 file changed, 67 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c >>>> index e5ca6fe2ca57..edebd0700e3d 100644 >>>> --- a/drivers/hid/hid-quirks.c >>>> +++ b/drivers/hid/hid-quirks.c >>>> @@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = { >>>> { } >>>> }; >>>> +/* >>>> + * List of device names that elan_i2c is handling and HID should ignore. Must >>>> + * be kept in sync with elan_i2c >>>> + */ >>>> +static const char *hid_elan_i2c_ignore[] = { >>> >>> If this is a copy of elan whitelist, then, if we do not want to bother >>> with sharing it in object form (as a elan-i2c-ids module), can we at >>> least move it into include/linux/input/elan-i2c-ids.h and consume from >>> hid-quirks.c? >> >> I can put it in a shared header file, however elan-i2c and hid-quirks >> would need to be updated in the same change to prevent a breakage, but >> that would seem to violate a concern Benjamin brought up in v4 given >> that elan-i2c is maintained in your input tree, and hid-quirks is >> maintained in his hid tree. >> >> Are you ok with the elan-i2c changes going through Benjamin's hid tree? > > We co-ordinate cross-subsystem merges all the time. That is never a > reason to not do the 'right thing (tm)'. If this information can be > held in a single, central place, without the need for constant > re-alignment, I'm all for it. > I'm aware coordination is a thing. I'm trying to elicit some coordination between Dmitry and Benjamin.
On Wed, Jun 12, 2019 at 7:45 AM Jiri Kosina <jikos@kernel.org> wrote: > > On Tue, 11 Jun 2019, Dmitry Torokhov wrote: > > > > +static const char *hid_elan_i2c_ignore[] = { > > > > If this is a copy of elan whitelist, then, if we do not want to bother > > with sharing it in object form (as a elan-i2c-ids module), can we at > > least move it into include/linux/input/elan-i2c-ids.h and consume from > > hid-quirks.c? > > Let's just not duplicate it in both objects. Why not properly export it > from hid_quirks? Strictly speaking Elan does not depend on HID; exporting it from quirks would mean adding this dependency. This also mean that you can't make Elan built-in while keeping HID as a module (I think this at least used to be config on some Chromebooks). Thanks.
On Wed, Jun 12, 2019 at 5:14 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Wed, Jun 12, 2019 at 7:45 AM Jiri Kosina <jikos@kernel.org> wrote: > > > > On Tue, 11 Jun 2019, Dmitry Torokhov wrote: > > > > > > +static const char *hid_elan_i2c_ignore[] = { > > > > > > If this is a copy of elan whitelist, then, if we do not want to bother > > > with sharing it in object form (as a elan-i2c-ids module), can we at > > > least move it into include/linux/input/elan-i2c-ids.h and consume from > > > hid-quirks.c? > > > > Let's just not duplicate it in both objects. Why not properly export it > > from hid_quirks? > > Strictly speaking Elan does not depend on HID; exporting it from > quirks would mean adding this dependency. This also mean that you > can't make Elan built-in while keeping HID as a module (I think this > at least used to be config on some Chromebooks). > I also think it would me things cleaner to have the list of devices in elan_i2c. If we put the list of devices supported by elan_i2c in a header, and have HID read this .h file directly, there will be no runtime dependency. I am sure we can work something out to remove Jeffrey's fears. Cheers, Benjamin
On 6/12/2019 9:21 AM, Benjamin Tissoires wrote: > On Wed, Jun 12, 2019 at 5:14 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: >> >> On Wed, Jun 12, 2019 at 7:45 AM Jiri Kosina <jikos@kernel.org> wrote: >>> >>> On Tue, 11 Jun 2019, Dmitry Torokhov wrote: >>> >>>>> +static const char *hid_elan_i2c_ignore[] = { >>>> >>>> If this is a copy of elan whitelist, then, if we do not want to bother >>>> with sharing it in object form (as a elan-i2c-ids module), can we at >>>> least move it into include/linux/input/elan-i2c-ids.h and consume from >>>> hid-quirks.c? >>> >>> Let's just not duplicate it in both objects. Why not properly export it >>> from hid_quirks? >> >> Strictly speaking Elan does not depend on HID; exporting it from >> quirks would mean adding this dependency. This also mean that you >> can't make Elan built-in while keeping HID as a module (I think this >> at least used to be config on some Chromebooks). >> > > I also think it would me things cleaner to have the list of devices in elan_i2c. > If we put the list of devices supported by elan_i2c in a header, and > have HID read this .h file directly, there will be no runtime > dependency. > > I am sure we can work something out to remove Jeffrey's fears. Works for me. I plan to have the next rev posted by the end of the week.
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c index e5ca6fe2ca57..edebd0700e3d 100644 --- a/drivers/hid/hid-quirks.c +++ b/drivers/hid/hid-quirks.c @@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = { { } }; +/* + * List of device names that elan_i2c is handling and HID should ignore. Must + * be kept in sync with elan_i2c + */ +static const char *hid_elan_i2c_ignore[] = { + "ELAN0000", + "ELAN0100", + "ELAN0600", + "ELAN0601", + "ELAN0602", + "ELAN0603", + "ELAN0604", + "ELAN0605", + "ELAN0606", + "ELAN0607", + "ELAN0608", + "ELAN0609", + "ELAN060B", + "ELAN060C", + "ELAN060F", + "ELAN0610", + "ELAN0611", + "ELAN0612", + "ELAN0613", + "ELAN0614", + "ELAN0615", + "ELAN0616", + "ELAN0617", + "ELAN0618", + "ELAN0619", + "ELAN061A", + "ELAN061B", + "ELAN061C", + "ELAN061D", + "ELAN061E", + "ELAN061F", + "ELAN0620", + "ELAN0621", + "ELAN0622", + "ELAN0623", + "ELAN0624", + "ELAN0625", + "ELAN0626", + "ELAN0627", + "ELAN0628", + "ELAN0629", + "ELAN062A", + "ELAN062B", + "ELAN062C", + "ELAN062D", + "ELAN0631", + "ELAN0632", + "ELAN1000", + "elan,ekth3000", + "" /* sentinel */ +}; bool hid_ignore(struct hid_device *hdev) { + int i; + if (hdev->quirks & HID_QUIRK_NO_IGNORE) return false; if (hdev->quirks & HID_QUIRK_IGNORE) @@ -978,18 +1036,16 @@ bool hid_ignore(struct hid_device *hdev) break; case USB_VENDOR_ID_ELAN: /* - * Many Elan devices have a product id of 0x0401 and are handled - * by the elan_i2c input driver. But the ACPI HID ELAN0800 dev - * is not (and cannot be) handled by that driver -> - * Ignore all 0x0401 devs except for the ELAN0800 dev. + * Blacklist of everything that gets handled by the elan_i2c + * input driver. This should be kept in sync with the whitelist + * that exists in that driver. This avoids disabling valid + * touchpads and other ELAN devices. */ - if (hdev->product == 0x0401 && - strncmp(hdev->name, "ELAN0800", 8) != 0) - return true; - /* Same with product id 0x0400 */ - if (hdev->product == 0x0400 && - strncmp(hdev->name, "QTEC0001", 8) != 0) - return true; + if ((hdev->product == 0x0401 || hdev->product == 0x0400)) + for (i = 0; strlen(hid_elan_i2c_ignore[i]); ++i) + if (!strncmp(hdev->name, hid_elan_i2c_ignore[i], + strlen(hid_elan_i2c_ignore[i]))) + return true; break; }
There needs to be coordination between hid-quirks and the elan_i2c driver about which devices are handled by what drivers. Currently, both use whitelists, which results in valid devices being unhandled by default, when they should not be rejected by hid-quirks. This is quickly becoming an issue. Since elan_i2c has a maintained whitelist of what devices it will handle, use that to implement a blacklist in hid-quirks so that only the devices that need to be handled by elan_i2c get rejected by hid-quirks, and everything else is handled by default. The downside is the whitelist and blacklist need to be kept in sync. Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> --- drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 11 deletions(-)