On Wed, Jun 12, 2019 at 04:20:42PM -0600, Jeffrey Hugo wrote: > On 6/12/2019 3:46 PM, Dmitry Torokhov wrote: > > On Wed, Jun 12, 2019 at 02:27:21PM -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, > > > which is now in a header file that hid-quirks can access, 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. > > > > > > Suggested-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> > > > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@xxxxxxxxx> > > > --- > > > drivers/hid/hid-quirks.c | 27 ++++++++++++++++----------- > > > 1 file changed, 16 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c > > > index e5ca6fe2ca57..bd81bb090222 100644 > > > --- a/drivers/hid/hid-quirks.c > > > +++ b/drivers/hid/hid-quirks.c > > > @@ -16,6 +16,7 @@ > > > #include <linux/export.h> > > > #include <linux/slab.h> > > > #include <linux/mutex.h> > > > +#include <linux/input/elan-i2c-ids.h> > > > #include "hid-ids.h" > > > @@ -914,6 +915,8 @@ static const struct hid_device_id hid_mouse_ignore_list[] = { > > > 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 +981,20 @@ 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 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(elan_acpi_id[i].id); ++i) > > > + if (!strncmp(hdev->name, elan_acpi_id[i].id, > > > + strlen(elan_acpi_id[i].id))) > > > + return true; > > > + for (i = 0; strlen(elan_of_match[i].name); ++i) > > > + if (!strncmp(hdev->name, elan_of_match[i].name, > > > + strlen(elan_of_match[i].name))) > > > + return true; > > > > Do we really need to blacklist the OF case here? I thought that in ACPI > > case we have clashes as HID gets matched by elan_i2c and CID is matched > > by i2c-hid, but I do not believe we'll run into the same situation on OF > > systems. > > I think its the safer approach. > > On an OF system, such as patch 3 in the series, the "hid-over-i2c" will end > up running through this (kind of the whole reason why this series exists). > The vendor and product ids will still match, so we'll end up going through > the lists to see if the hdev->name (the compatible string) will match the > blacklist. "hid-over-i2c" won't match the blacklist, but if there is a more > specific compatible, it might. > > In that case, not matching OF would work, however how it could break today > is if both "hid-over-i2c" and "elan,ekth3000" were listed for the same > device, and elan_i2c was not compiled. In that case, if we skip the OF part > of the black list, hid-quirks will not reject the device, and you'll > probably have some odd behavior instead of the obvious "the device doesn't > work because the correct driver isn't present" behavior. > > While that scenario might be far fetched since having both "hid-over-i2c" > and "elan,ekth3000" probably violates the OF bindings, its still safer to > include the OF case in the blacklist against future scenarios. Yes, I believe it is quite far fetched. We are talking about someone setting compatible sting to something that is decidedly not compatible. I.e. we know that devices driven by elan_i2c are not compatible with hi-over-i2c driver/protocol, so why do we expect that they both will be specified in the same compatible string? I know ACPI case is messier in this regard as 2 drivers look at the different data items when evaluating whether they should bind to the device, but here we are dealing with the same string. Thanks. -- Dmitry