| Message ID | 20170722185537.12696-2-hdegoede@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
On Jul 22 2017 or thereabouts, Hans de Goede wrote: > The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, > it uses its own protocol which is handled by the chipone_icn8318 driver. > > If the i2c_hid_driver's probe functon gets called it will fail with a > "hid_descr_cmd failed" error. > > Worse, after the probe failure the i2c / ACPI core code will put the ACPI > device in D3 state and when the chipone_icn8318 driver then loads the > device is put back in D0 state, executing its PS0 ACPI method, which > resets the controller, causing the controller to loose its firmware > which was loaded by the BIOS. The chipone_icn8318 driver has a workaround > for this, but that requires it to be the only (or the first) driver to > probe the device. > > This commit adds a match callback and returns -ENODEV for i2c_client-s > with a CHPN0001 ACPI device id, so that the probe function never gets > called, fixing the controller losing its firmware. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > -Use the new i2c_driver match callback to only filter out the CHPN0001 > ACPI device, rather then use acpi_dev_present in probe and not > registering the driver at all when the system has a CHPN0001 device I like the v2 much more than the v1. Reviewed-By: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cheers, Benjamin > --- > drivers/hid/i2c-hid/i2c-hid.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c > index 046f692fd0a2..79bed9afc388 100644 > --- a/drivers/hid/i2c-hid/i2c-hid.c > +++ b/drivers/hid/i2c-hid/i2c-hid.c > @@ -891,6 +891,26 @@ static void i2c_hid_acpi_fix_up_power(struct device *dev) > acpi_device_fix_up_power(adev); > } > > +static const struct acpi_device_id i2c_hid_acpi_blacklist[] = { > + /* > + * The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID > + * compatible, just probing it puts the device in an unusable state due > + * to it also have ACPI power management issues. > + */ > + {"CHPN0001", 0 }, > + { }, > +}; > + > +static int i2c_hid_match(struct i2c_client *client) > +{ > + struct acpi_device *adev = ACPI_COMPANION(&client->dev); > + > + if (adev && acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0) > + return -ENODEV; > + > + return 0; > +} > + > static const struct acpi_device_id i2c_hid_acpi_match[] = { > {"ACPI0C50", 0 }, > {"PNP0C50", 0 }, > @@ -905,6 +925,7 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client, > } > > static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {} > +static int i2c_hid_match(struct i2c_client *client) { return 0; } > #endif > > #ifdef CONFIG_OF > @@ -1255,6 +1276,7 @@ static struct i2c_driver i2c_hid_driver = { > .of_match_table = of_match_ptr(i2c_hid_of_match), > }, > > + .match = i2c_hid_match, > .probe = i2c_hid_probe, > .remove = i2c_hid_remove, > .shutdown = i2c_hid_shutdown, > -- > 2.13.0 >
On Mon, 24 Jul 2017, Benjamin Tissoires wrote: > > The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, > > it uses its own protocol which is handled by the chipone_icn8318 driver. > > > > If the i2c_hid_driver's probe functon gets called it will fail with a > > "hid_descr_cmd failed" error. > > > > Worse, after the probe failure the i2c / ACPI core code will put the ACPI > > device in D3 state and when the chipone_icn8318 driver then loads the > > device is put back in D0 state, executing its PS0 ACPI method, which > > resets the controller, causing the controller to loose its firmware > > which was loaded by the BIOS. The chipone_icn8318 driver has a workaround > > for this, but that requires it to be the only (or the first) driver to > > probe the device. > > > > This commit adds a match callback and returns -ENODEV for i2c_client-s > > with a CHPN0001 ACPI device id, so that the probe function never gets > > called, fixing the controller losing its firmware. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > Changes in v2: > > -Use the new i2c_driver match callback to only filter out the CHPN0001 > > ACPI device, rather then use acpi_dev_present in probe and not > > registering the driver at all when the system has a CHPN0001 device > > I like the v2 much more than the v1. > > Reviewed-By: Benjamin Tissoires <benjamin.tissoires@redhat.com> Wolfram, what do we do with this patchset? I think it makes sense for both patches to go in via one tree. Either I can take it through hid.git with your Ack for the first patch, or vice versa. Please let me know what you'd prefer. Thanks,
> what do we do with this patchset? I think it makes sense for both patches > to go in via one tree. Yes. > Either I can take it through hid.git with your Ack for the first patch, or > vice versa. Please let me know what you'd prefer. I'd like use the i2c tree for it. The changes to i2c core are an API addition while the changes to the HID driver are basically boilerplate. Makes sense?
On Tue, 25 Jul 2017, Wolfram Sang wrote: > > Either I can take it through hid.git with your Ack for the first patch, or > > vice versa. Please let me know what you'd prefer. > > I'd like use the i2c tree for it. The changes to i2c core are an API > addition while the changes to the HID driver are basically boilerplate. > > Makes sense? Absolutely. Please feel free to add Reviewed-by: Jiri Kosina <jkosina@suse.cz> to patch 2/2. Thanks,
Hey guys, Sorry, I don't understand some of the stuff here. But I'd like to understand it before I add something to the I2C core. Especially as it feels a bit a the edge of the driver model to me. On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote: > The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, > it uses its own protocol which is handled by the chipone_icn8318 driver. > > If the i2c_hid_driver's probe functon gets called it will fail with a > "hid_descr_cmd failed" error. That sounds like it fails pretty late. I'd assume we could check the blacklist right at the beginning of probe and bail out immediately? > Worse, after the probe failure the i2c / ACPI core code will put the ACPI > device in D3 state Where does that happen? Sorry, I can't find it. Would it be an idea to add a flag somewhere telling the device should not be put into D3? That would be way more generic in case this happens outside I2C world, or? Disclaimer: I am brainstorming here, don't know super much about ACPI. > This commit adds a match callback and returns -ENODEV for i2c_client-s > with a CHPN0001 ACPI device id, so that the probe function never gets > called, fixing the controller losing its firmware. Do you know if something like a match-callback exists somewhere else in the kernel? Regards, Wolfram
On Thu, Aug 17, 2017 at 12:39 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > Hey guys, > > Sorry, I don't understand some of the stuff here. But I'd like to > understand it before I add something to the I2C core. Especially as it > feels a bit a the edge of the driver model to me. > > On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote: >> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, >> it uses its own protocol which is handled by the chipone_icn8318 driver. >> >> If the i2c_hid_driver's probe functon gets called it will fail with a >> "hid_descr_cmd failed" error. > > That sounds like it fails pretty late. I'd assume we could check the > blacklist right at the beginning of probe and bail out immediately? > >> Worse, after the probe failure the i2c / ACPI core code will put the ACPI >> device in D3 state So I guess this will also happen if I unbind/rebind chipone_icn8318 driver? > > Where does that happen? Sorry, I can't find it. Would it be an idea to > add a flag somewhere telling the device should not be put into D3? That > would be way more generic in case this happens outside I2C world, or? > Disclaimer: I am brainstorming here, don't know super much about ACPI. > >> This commit adds a match callback and returns -ENODEV for i2c_client-s >> with a CHPN0001 ACPI device id, so that the probe function never gets >> called, fixing the controller losing its firmware. > > Do you know if something like a match-callback exists somewhere else in > the kernel? Having blacklist means that we are failing at design somewhere... In this case what we have is wrong ACPI table. Instead of adding hooks to i2c core can we fix it (DSDT) up? I know people do not like it, but I'd say this is task for yet another platform driver to adjust ACPI properties (kill the wrong _CID, disable PS0) before instantiating the device. Thanks.
Hi, On 17-08-17 21:39, Wolfram Sang wrote: > Hey guys, > > Sorry, I don't understand some of the stuff here. But I'd like to > understand it before I add something to the I2C core. Especially as it > feels a bit a the edge of the driver model to me. > > On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote: >> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, >> it uses its own protocol which is handled by the chipone_icn8318 driver. >> >> If the i2c_hid_driver's probe functon gets called it will fail with a >> "hid_descr_cmd failed" error. > > That sounds like it fails pretty late. I'd assume we could check the > blacklist right at the beginning of probe and bail out immediately? The problem is that calling probe (and then failing) leads to the device-core powering up and then powering down (put in D0 / D3 state) the device, after which the touchscreen controller has lost its firmware. So the trick is to get the device-core to never call i2c_bus_type.probe for i2c-hid at all, which means that i2c_bus_type.match must return false in this case. >> Worse, after the probe failure the i2c / ACPI core code will put the ACPI >> device in D3 state > > Where does that happen? Sorry, I can't find it. Would it be an idea to > add a flag somewhere telling the device should not be put into D3? That > would be way more generic in case this happens outside I2C world, or? > Disclaimer: I am brainstorming here, don't know super much about ACPI. i2c_device_probe() calls dev_pm_domain_attach(&client->dev, true) which calls acpi_dev_pm_attach(dev, true) which then does a acpi_dev_pm_full_power(adev) moving the device to D0 (if it was not in D0 already). When the probe fails i2c_device_probe() then does dev_pm_domain_detach(&client->dev, true); which calls acpi_dev_pm_detach(dev, true) which does acpi_dev_pm_low_power(...) and now the device is in D3. >> This commit adds a match callback and returns -ENODEV for i2c_client-s >> with a CHPN0001 ACPI device id, so that the probe function never gets >> called, fixing the controller losing its firmware. > > Do you know if something like a match-callback exists somewhere else in > the kernel? No AFAIK there is no driver level match callback (only bus level) at other places in the kernel. Regards, Hans
Hi again, I realized I did not answer 1 of your questions: On 17-08-17 21:39, Wolfram Sang wrote: > Hey guys, > > Sorry, I don't understand some of the stuff here. But I'd like to > understand it before I add something to the I2C core. Especially as it > feels a bit a the edge of the driver model to me. > > On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote: >> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, >> it uses its own protocol which is handled by the chipone_icn8318 driver. >> >> If the i2c_hid_driver's probe functon gets called it will fail with a >> "hid_descr_cmd failed" error. > > That sounds like it fails pretty late. I'd assume we could check the > blacklist right at the beginning of probe and bail out immediately? > >> Worse, after the probe failure the i2c / ACPI core code will put the ACPI >> device in D3 state > > Where does that happen? Sorry, I can't find it. Would it be an idea to > add a flag somewhere telling the device should not be put into D3? It is already possible to do this and my patches for the icn8318 driver do this: struct acpi_device *adev; adev = ACPI_COMPANION(dev); /* * Disable ACPI power management the _PS3 method is empty, so * there is no powersaving when using ACPI power management. * The _PS0 method resets the controller causing it to loose its * firmware, which has been loaded by the BIOS and we do not * know how to restore the firmware. */ adev->flags.power_manageable = 0; The problem is that this happens in the probe() from the icn8318 driver and if the i2c-hid drivers probe() executes first we end up in the dev_pm_domain_detach() path of i2c_device_probe() and after that the touchscreen-controller no longer works (*), iow after that it is too late to disable acpi pm for the device. *) Unless we find a way to reload the firmware, which technically is doable, but then we get into the problem of now having to distribute the firmware in linux-firmware Regards, Hans
Hi, On 18-08-17 00:15, Dmitry Torokhov wrote: > On Thu, Aug 17, 2017 at 12:39 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> Hey guys, >> >> Sorry, I don't understand some of the stuff here. But I'd like to >> understand it before I add something to the I2C core. Especially as it >> feels a bit a the edge of the driver model to me. >> >> On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote: >>> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, >>> it uses its own protocol which is handled by the chipone_icn8318 driver. >>> >>> If the i2c_hid_driver's probe functon gets called it will fail with a >>> "hid_descr_cmd failed" error. >> >> That sounds like it fails pretty late. I'd assume we could check the >> blacklist right at the beginning of probe and bail out immediately? >> >>> Worse, after the probe failure the i2c / ACPI core code will put the ACPI >>> device in D3 state > > So I guess this will also happen if I unbind/rebind chipone_icn8318 driver? No because on probe the chipone_icn8318 driver does (with the patches I've pending) : struct acpi_device *adev; adev = ACPI_COMPANION(dev); /* * Disable ACPI power management the _PS3 method is empty, so * there is no powersaving when using ACPI power management. * The _PS0 method resets the controller causing it to loose its * firmware, which has been loaded by the BIOS and we do not * know how to restore the firmware. */ adev->flags.power_manageable = 0; So it disables ACPI pm for the device, keeping the controller in D0 so that it will never loose its firmware. >> Where does that happen? Sorry, I can't find it. Would it be an idea to >> add a flag somewhere telling the device should not be put into D3? That >> would be way more generic in case this happens outside I2C world, or? >> Disclaimer: I am brainstorming here, don't know super much about ACPI. >> >>> This commit adds a match callback and returns -ENODEV for i2c_client-s >>> with a CHPN0001 ACPI device id, so that the probe function never gets >>> called, fixing the controller losing its firmware. >> >> Do you know if something like a match-callback exists somewhere else in >> the kernel? > > Having blacklist means that we are failing at design somewhere... > > In this case what we have is wrong ACPI table. Instead of adding hooks > to i2c core can we fix it (DSDT) up? I know people do not like it, but > I'd say this is task for yet another platform driver to adjust ACPI > properties (kill the wrong _CID, disable PS0) before instantiating the > device. I don't think that is going to fly. Certainly DSDT patching is out of the question, we could modify the acpi_dev after enumeration, but that is not going to be pretty. This is going to be drivers/platform/x86/silead_dmi.c all over again, but where that made sense as to decouple the board-info from the driver this case is different as we are not talking board specific behavior here, but device specific so this really belongs in the drivers IMHO, also the reception of silead_dmi.c has not been 100% positive. I don't think the blacklist entry in i2c-hid is a problem per se, we've similar issues with USB devices where they claim a generic class but need a specific driver and we typically solve that with a blacklist for the specific vid:pid in the class driver. The nastiness here is not the blacklist, but the pm issues when the wrong driver gets its probe() method called first. Regards, Hans
Hi Hans, On Mon, Aug 28, 2017 at 03:04:04PM +0200, Hans de Goede wrote: > Hi, > > On 18-08-17 00:15, Dmitry Torokhov wrote: > >On Thu, Aug 17, 2017 at 12:39 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > >>Hey guys, > >> > >>Sorry, I don't understand some of the stuff here. But I'd like to > >>understand it before I add something to the I2C core. Especially as it > >>feels a bit a the edge of the driver model to me. > >> > >>On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote: > >>>The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, > >>>it uses its own protocol which is handled by the chipone_icn8318 driver. > >>> > >>>If the i2c_hid_driver's probe functon gets called it will fail with a > >>>"hid_descr_cmd failed" error. > >> > >>That sounds like it fails pretty late. I'd assume we could check the > >>blacklist right at the beginning of probe and bail out immediately? > >> > >>>Worse, after the probe failure the i2c / ACPI core code will put the ACPI > >>>device in D3 state > > > >So I guess this will also happen if I unbind/rebind chipone_icn8318 driver? > > No because on probe the chipone_icn8318 driver does (with the patches > I've pending) : > > struct acpi_device *adev; > > adev = ACPI_COMPANION(dev); > > /* > * Disable ACPI power management the _PS3 method is empty, so > * there is no powersaving when using ACPI power management. > * The _PS0 method resets the controller causing it to loose its > * firmware, which has been loaded by the BIOS and we do not > * know how to restore the firmware. > */ > adev->flags.power_manageable = 0; > > So it disables ACPI pm for the device, keeping the controller in D0 so that > it will never loose its firmware. > > > >>Where does that happen? Sorry, I can't find it. Would it be an idea to > >>add a flag somewhere telling the device should not be put into D3? That > >>would be way more generic in case this happens outside I2C world, or? > >>Disclaimer: I am brainstorming here, don't know super much about ACPI. > >> > >>>This commit adds a match callback and returns -ENODEV for i2c_client-s > >>>with a CHPN0001 ACPI device id, so that the probe function never gets > >>>called, fixing the controller losing its firmware. > >> > >>Do you know if something like a match-callback exists somewhere else in > >>the kernel? > > > >Having blacklist means that we are failing at design somewhere... > > > >In this case what we have is wrong ACPI table. Instead of adding hooks > >to i2c core can we fix it (DSDT) up? I know people do not like it, but > >I'd say this is task for yet another platform driver to adjust ACPI > >properties (kill the wrong _CID, disable PS0) before instantiating the > >device. > > I don't think that is going to fly. Certainly DSDT patching is out of the > question, we could modify the acpi_dev after enumeration, but that is not > going to be pretty. This is going to be drivers/platform/x86/silead_dmi.c > all over again, but where that made sense as to decouple the board-info > from the driver this case is different as we are not talking board > specific behavior here, but device specific so this really belongs in the > drivers IMHO, also the reception of silead_dmi.c has not been 100% positive. No, this is definitely board-specific as well. This is a brain dead firmware on a particular board that declares PS0 and wrong CID. The device itself can be made working perfectly well in another box. I think I also mentioned before that encoding particular platform behavior in the device driver is not the best option. I understand that nobody likes silead_dmi as it is nasty (as it shoudl be, it fixes missing/wrong data suplied by the platform), but I think the main objection is that we had to make it built-in. I wonder if we could work on making it usable as a module, by having driver core try loading "<device-alias>-quirk" module before doing probes so that quirk is guaranteed to be available? > > I don't think the blacklist entry in i2c-hid is a problem per se, we've > similar issues with USB devices where they claim a generic class but need > a specific driver and we typically solve that with a blacklist for the > specific vid:pid in the class driver. Yes, you do that for a particular _device_. You move this device to a brand new box and you still need the same quirk. Here it is the box that is deficient. > > The nastiness here is not the blacklist, but the pm issues when the wrong > driver gets its probe() method called first. BTW, how do we order probing? Should we try matching on _HID before trying _CID? Thanks.
Hi Dmitry, On 28-08-17 18:31, Dmitry Torokhov wrote: > Hi Hans, > > On Mon, Aug 28, 2017 at 03:04:04PM +0200, Hans de Goede wrote: >> Hi, >> >> On 18-08-17 00:15, Dmitry Torokhov wrote: >>> On Thu, Aug 17, 2017 at 12:39 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >>>> Hey guys, >>>> >>>> Sorry, I don't understand some of the stuff here. But I'd like to >>>> understand it before I add something to the I2C core. Especially as it >>>> feels a bit a the edge of the driver model to me. >>>> >>>> On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote: >>>>> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, >>>>> it uses its own protocol which is handled by the chipone_icn8318 driver. >>>>> >>>>> If the i2c_hid_driver's probe functon gets called it will fail with a >>>>> "hid_descr_cmd failed" error. >>>> >>>> That sounds like it fails pretty late. I'd assume we could check the >>>> blacklist right at the beginning of probe and bail out immediately? >>>> >>>>> Worse, after the probe failure the i2c / ACPI core code will put the ACPI >>>>> device in D3 state >>> >>> So I guess this will also happen if I unbind/rebind chipone_icn8318 driver? >> >> No because on probe the chipone_icn8318 driver does (with the patches >> I've pending) : >> >> struct acpi_device *adev; >> >> adev = ACPI_COMPANION(dev); >> >> /* >> * Disable ACPI power management the _PS3 method is empty, so >> * there is no powersaving when using ACPI power management. >> * The _PS0 method resets the controller causing it to loose its >> * firmware, which has been loaded by the BIOS and we do not >> * know how to restore the firmware. >> */ >> adev->flags.power_manageable = 0; >> >> So it disables ACPI pm for the device, keeping the controller in D0 so that >> it will never loose its firmware. >> >> >>>> Where does that happen? Sorry, I can't find it. Would it be an idea to >>>> add a flag somewhere telling the device should not be put into D3? That >>>> would be way more generic in case this happens outside I2C world, or? >>>> Disclaimer: I am brainstorming here, don't know super much about ACPI. >>>> >>>>> This commit adds a match callback and returns -ENODEV for i2c_client-s >>>>> with a CHPN0001 ACPI device id, so that the probe function never gets >>>>> called, fixing the controller losing its firmware. >>>> >>>> Do you know if something like a match-callback exists somewhere else in >>>> the kernel? >>> >>> Having blacklist means that we are failing at design somewhere... >>> >>> In this case what we have is wrong ACPI table. Instead of adding hooks >>> to i2c core can we fix it (DSDT) up? I know people do not like it, but >>> I'd say this is task for yet another platform driver to adjust ACPI >>> properties (kill the wrong _CID, disable PS0) before instantiating the >>> device. >> >> I don't think that is going to fly. Certainly DSDT patching is out of the >> question, we could modify the acpi_dev after enumeration, but that is not >> going to be pretty. This is going to be drivers/platform/x86/silead_dmi.c >> all over again, but where that made sense as to decouple the board-info >> from the driver this case is different as we are not talking board >> specific behavior here, but device specific so this really belongs in the >> drivers IMHO, also the reception of silead_dmi.c has not been 100% positive. > > No, this is definitely board-specific as well. This is a brain dead > firmware on a particular board that declares PS0 and wrong CID. The > device itself can be made working perfectly well in another box. I believe all x86 tablet models with this touchscreen controller have the same issues, so although technically this is a board problem we might just as well treat it as a device problem. > I think I also mentioned before that encoding particular platform > behavior in the device driver is not the best option. > > I understand that nobody likes silead_dmi as it is nasty (as it shoudl > be, it fixes missing/wrong data suplied by the platform), but I think > the main objection is that we had to make it built-in. I wonder if we > could work on making it usable as a module, by having driver core try > loading "<device-alias>-quirk" module before doing probes so that quirk > is guaranteed to be available? That is tricky when using an initrd, what if the initial attempt to load "<device-alias>-quirk" fails because we are running from the initrd should we retry ? When ? Etc. > >> >> I don't think the blacklist entry in i2c-hid is a problem per se, we've >> similar issues with USB devices where they claim a generic class but need >> a specific driver and we typically solve that with a blacklist for the >> specific vid:pid in the class driver. > > Yes, you do that for a particular _device_. You move this device to a > brand new box and you still need the same quirk. Here it is the box that > is deficient. See above to the best of my knowledge all models using this touchscreen controller have the same issue. >> The nastiness here is not the blacklist, but the pm issues when the wrong >> driver gets its probe() method called first. > > BTW, how do we order probing? Should we try matching on _HID before > trying _CID? That won't help much, udev ends up loading the modules based on a modalias so playing tricks with ordering is hard (and again we may have an initrd making things interesting, or have the wrong driver built in). Regards, Hans
Hi, On 28-08-17 14:50, Hans de Goede wrote: > Hi again, > > I realized I did not answer 1 of your questions: > > On 17-08-17 21:39, Wolfram Sang wrote: >> Hey guys, >> >> Sorry, I don't understand some of the stuff here. But I'd like to >> understand it before I add something to the I2C core. Especially as it >> feels a bit a the edge of the driver model to me. >> >> On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote: >>> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, >>> it uses its own protocol which is handled by the chipone_icn8318 driver. >>> >>> If the i2c_hid_driver's probe functon gets called it will fail with a >>> "hid_descr_cmd failed" error. >> >> That sounds like it fails pretty late. I'd assume we could check the >> blacklist right at the beginning of probe and bail out immediately? >> >>> Worse, after the probe failure the i2c / ACPI core code will put the ACPI >>> device in D3 state >> >> Where does that happen? Sorry, I can't find it. Would it be an idea to >> add a flag somewhere telling the device should not be put into D3? > > It is already possible to do this and my patches for the icn8318 driver > do this: > > struct acpi_device *adev; > > adev = ACPI_COMPANION(dev); > > /* > * Disable ACPI power management the _PS3 method is empty, so > * there is no powersaving when using ACPI power management. > * The _PS0 method resets the controller causing it to loose its > * firmware, which has been loaded by the BIOS and we do not > * know how to restore the firmware. > */ > adev->flags.power_manageable = 0; > > The problem is that this happens in the probe() from the icn8318 driver > and if the i2c-hid drivers probe() executes first we end up in the > dev_pm_domain_detach() path of i2c_device_probe() and after that the > touchscreen-controller no longer works (*), iow after that it is too late > to disable acpi pm for the device. So thinking more about this it might be cleaner to add a blacklist of _CID / _HID ACPI-ids for which power-management should be disabled to drivers/acpi/device_pm.c : acpi_bus_init_power(). When I've some time to look into this I will write a patch following that approach. So lets forget about the approach to add an i2c_driver match callback for now. Regards, Hans > *) Unless we find a way to reload the firmware, which technically is > doable, but then we get into the problem of now having to distribute the > firmware in linux-firmware
> So thinking more about this it might be cleaner to add a blacklist > of _CID / _HID ACPI-ids for which power-management should be disabled > to drivers/acpi/device_pm.c : acpi_bus_init_power(). > > When I've some time to look into this I will write a patch following > that approach. > > So lets forget about the approach to add an i2c_driver match callback > for now. Thanks for the update! That sounds better to me from the high level view that I have. Thanks to you and Dmitry for discussing the details throughly. Very much appreciated!
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 046f692fd0a2..79bed9afc388 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -891,6 +891,26 @@ static void i2c_hid_acpi_fix_up_power(struct device *dev) acpi_device_fix_up_power(adev); } +static const struct acpi_device_id i2c_hid_acpi_blacklist[] = { + /* + * The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID + * compatible, just probing it puts the device in an unusable state due + * to it also have ACPI power management issues. + */ + {"CHPN0001", 0 }, + { }, +}; + +static int i2c_hid_match(struct i2c_client *client) +{ + struct acpi_device *adev = ACPI_COMPANION(&client->dev); + + if (adev && acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0) + return -ENODEV; + + return 0; +} + static const struct acpi_device_id i2c_hid_acpi_match[] = { {"ACPI0C50", 0 }, {"PNP0C50", 0 }, @@ -905,6 +925,7 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client, } static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {} +static int i2c_hid_match(struct i2c_client *client) { return 0; } #endif #ifdef CONFIG_OF @@ -1255,6 +1276,7 @@ static struct i2c_driver i2c_hid_driver = { .of_match_table = of_match_ptr(i2c_hid_of_match), }, + .match = i2c_hid_match, .probe = i2c_hid_probe, .remove = i2c_hid_remove, .shutdown = i2c_hid_shutdown,
The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible, it uses its own protocol which is handled by the chipone_icn8318 driver. If the i2c_hid_driver's probe functon gets called it will fail with a "hid_descr_cmd failed" error. Worse, after the probe failure the i2c / ACPI core code will put the ACPI device in D3 state and when the chipone_icn8318 driver then loads the device is put back in D0 state, executing its PS0 ACPI method, which resets the controller, causing the controller to loose its firmware which was loaded by the BIOS. The chipone_icn8318 driver has a workaround for this, but that requires it to be the only (or the first) driver to probe the device. This commit adds a match callback and returns -ENODEV for i2c_client-s with a CHPN0001 ACPI device id, so that the probe function never gets called, fixing the controller losing its firmware. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -Use the new i2c_driver match callback to only filter out the CHPN0001 ACPI device, rather then use acpi_dev_present in probe and not registering the driver at all when the system has a CHPN0001 device --- drivers/hid/i2c-hid/i2c-hid.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)