| Message ID | 20170316161601.32267-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 2017-03-16 at 17:16 +0100, Hans de Goede wrote: > On some systems with axp288 PMIC the dsdt also exports an ACPI battery > device (PNP0C0A device). This leads to there being 2 battery > power_supply-s registed like this: > > ~$ acpi > Battery 0: Charging, 84%, 00:49:39 until charged > Battery 1: Unknown, 0%, rate information unavailable > > Note that the ACPI battery device does not work properly this is due > to Linux missing support for the vendor specific BMOP ACPI opregion. > But even if the ACPI battery where to function fine we still do not > want to export the same battery to userspace twice. > > Therefor this commit calls acpi_battery_unregister() after > successfully > registering the axp288-fuel-gauge power_supply to remove the duplicate > (and often broken) ACPI battery power_supply. > + # if ACPI_BATTERY=m, this can't be 'y' Driver dependencies? Deferred probe?
Hi, On 16-03-17 17:33, Andy Shevchenko wrote: > On Thu, 2017-03-16 at 17:16 +0100, Hans de Goede wrote: >> On some systems with axp288 PMIC the dsdt also exports an ACPI battery >> device (PNP0C0A device). This leads to there being 2 battery >> power_supply-s registed like this: >> >> ~$ acpi >> Battery 0: Charging, 84%, 00:49:39 until charged >> Battery 1: Unknown, 0%, rate information unavailable >> >> Note that the ACPI battery device does not work properly this is due >> to Linux missing support for the vendor specific BMOP ACPI opregion. >> But even if the ACPI battery where to function fine we still do not >> want to export the same battery to userspace twice. >> >> Therefor this commit calls acpi_battery_unregister() after >> successfully >> registering the axp288-fuel-gauge power_supply to remove the duplicate >> (and often broken) ACPI battery power_supply. > >> + # if ACPI_BATTERY=m, this can't be 'y' > > Driver dependencies? Deferred probe? axp288_fuel_gauge.ko will use a symbol from drivers/acpi/battery.c now. If ACPI_BATTERY is disabled include/linux/power/acpi.h provides a stub, but if it is enabled in any form then no stub, so then if ACPI_BATTERY=m axp288_fuel_gauge.ko needs to be a module too. Regards, Hans
On Thursday, March 16, 2017 05:16:00 PM Hans de Goede wrote: > On some systems with axp288 PMIC the dsdt also exports an ACPI battery > device (PNP0C0A device). This leads to there being 2 battery > power_supply-s registed like this: > > ~$ acpi > Battery 0: Charging, 84%, 00:49:39 until charged > Battery 1: Unknown, 0%, rate information unavailable > > Note that the ACPI battery device does not work properly this is due > to Linux missing support for the vendor specific BMOP ACPI opregion. > But even if the ACPI battery where to function fine we still do not > want to export the same battery to userspace twice. > > Therefor this commit calls acpi_battery_unregister() after successfully > registering the axp288-fuel-gauge power_supply to remove the duplicate > (and often broken) ACPI battery power_supply. > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=194811 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Tested-by: Sergei Trusov <t.rus76@ya.ru> > --- > drivers/power/supply/Kconfig | 2 ++ > drivers/power/supply/axp288_fuel_gauge.c | 3 +++ > 2 files changed, 5 insertions(+) > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index d0453ca..e504644 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -242,6 +242,8 @@ config AXP288_CHARGER > config AXP288_FUEL_GAUGE > tristate "X-Powers AXP288 Fuel Gauge" > depends on MFD_AXP20X && IIO > + # if ACPI_BATTERY=m, this can't be 'y' > + depends on ACPI_BATTERY || !ACPI_BATTERY > help > Say yes here to have support for X-Power power management IC (PMIC) > Fuel Gauge. The device provides battery statistics and status > diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c > index a8dcabc..15f10ce 100644 > --- a/drivers/power/supply/axp288_fuel_gauge.c > +++ b/drivers/power/supply/axp288_fuel_gauge.c > @@ -26,6 +26,7 @@ > #include <linux/mfd/axp20x.h> > #include <linux/platform_device.h> > #include <linux/power_supply.h> > +#include <linux/power/acpi.h> > #include <linux/iio/consumer.h> > #include <linux/debugfs.h> > #include <linux/seq_file.h> > @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev) > return ret; > } > > + acpi_battery_unregister(); > + What if the ACPI battery driver is loaded after this has been called already? > fuel_gauge_create_debugfs(info); > fuel_gauge_init_irq(info); > schedule_delayed_work(&info->status_monitor, STATUS_MON_DELAY_JIFFIES); > Thanks, Rafael
Hi, On 29-03-17 22:31, Rafael J. Wysocki wrote: > On Thursday, March 16, 2017 05:16:00 PM Hans de Goede wrote: >> On some systems with axp288 PMIC the dsdt also exports an ACPI battery >> device (PNP0C0A device). This leads to there being 2 battery >> power_supply-s registed like this: >> >> ~$ acpi >> Battery 0: Charging, 84%, 00:49:39 until charged >> Battery 1: Unknown, 0%, rate information unavailable >> >> Note that the ACPI battery device does not work properly this is due >> to Linux missing support for the vendor specific BMOP ACPI opregion. >> But even if the ACPI battery where to function fine we still do not >> want to export the same battery to userspace twice. >> >> Therefor this commit calls acpi_battery_unregister() after successfully >> registering the axp288-fuel-gauge power_supply to remove the duplicate >> (and often broken) ACPI battery power_supply. >> >> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=194811 >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> Tested-by: Sergei Trusov <t.rus76@ya.ru> >> --- >> drivers/power/supply/Kconfig | 2 ++ >> drivers/power/supply/axp288_fuel_gauge.c | 3 +++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig >> index d0453ca..e504644 100644 >> --- a/drivers/power/supply/Kconfig >> +++ b/drivers/power/supply/Kconfig >> @@ -242,6 +242,8 @@ config AXP288_CHARGER >> config AXP288_FUEL_GAUGE >> tristate "X-Powers AXP288 Fuel Gauge" >> depends on MFD_AXP20X && IIO >> + # if ACPI_BATTERY=m, this can't be 'y' >> + depends on ACPI_BATTERY || !ACPI_BATTERY >> help >> Say yes here to have support for X-Power power management IC (PMIC) >> Fuel Gauge. The device provides battery statistics and status >> diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c >> index a8dcabc..15f10ce 100644 >> --- a/drivers/power/supply/axp288_fuel_gauge.c >> +++ b/drivers/power/supply/axp288_fuel_gauge.c >> @@ -26,6 +26,7 @@ >> #include <linux/mfd/axp20x.h> >> #include <linux/platform_device.h> >> #include <linux/power_supply.h> >> +#include <linux/power/acpi.h> >> #include <linux/iio/consumer.h> >> #include <linux/debugfs.h> >> #include <linux/seq_file.h> >> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev) >> return ret; >> } >> >> + acpi_battery_unregister(); >> + > > What if the ACPI battery driver is loaded after this has been called already? The module exports that symbol so it must be loaded already. If both are built-in then the probe method of the axp288_fuel_gauge may run first, acpi_battery_unregister() sets a (mutex proteted) flag which gets checked by acpi_battery_probe() which will then exit with -ENODEV when it runs later. So this is already taken care of :) Also see the first patch in this series which implements acpi_battery_unregister(). Regards, Hans > >> fuel_gauge_create_debugfs(info); >> fuel_gauge_init_irq(info); >> schedule_delayed_work(&info->status_monitor, STATUS_MON_DELAY_JIFFIES); >> > > Thanks, > Rafael >
On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> wrote: [cut] >>> --- a/drivers/power/supply/axp288_fuel_gauge.c >>> +++ b/drivers/power/supply/axp288_fuel_gauge.c >>> @@ -26,6 +26,7 @@ >>> #include <linux/mfd/axp20x.h> >>> #include <linux/platform_device.h> >>> #include <linux/power_supply.h> >>> +#include <linux/power/acpi.h> >>> #include <linux/iio/consumer.h> >>> #include <linux/debugfs.h> >>> #include <linux/seq_file.h> >>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct >>> platform_device *pdev) >>> return ret; >>> } >>> >>> + acpi_battery_unregister(); >>> + >> >> >> What if the ACPI battery driver is loaded after this has been called >> already? > > > The module exports that symbol so it must be loaded already. But then it may be unloaded manually and loaded again, may it not? Thanks, Rafael
Hi, On 31-03-17 11:05, Rafael J. Wysocki wrote: > On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> wrote: > > [cut] > >>>> --- a/drivers/power/supply/axp288_fuel_gauge.c >>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c >>>> @@ -26,6 +26,7 @@ >>>> #include <linux/mfd/axp20x.h> >>>> #include <linux/platform_device.h> >>>> #include <linux/power_supply.h> >>>> +#include <linux/power/acpi.h> >>>> #include <linux/iio/consumer.h> >>>> #include <linux/debugfs.h> >>>> #include <linux/seq_file.h> >>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct >>>> platform_device *pdev) >>>> return ret; >>>> } >>>> >>>> + acpi_battery_unregister(); >>>> + >>> >>> >>> What if the ACPI battery driver is loaded after this has been called >>> already? >> >> >> The module exports that symbol so it must be loaded already. > > But then it may be unloaded manually and loaded again, may it not? Only if you first unload axp288_fuel_gauge.ko otherwise it will have a refcount > 0. Regards, Hans
On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 31-03-17 11:05, Rafael J. Wysocki wrote: >> >> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> >> wrote: >> >> [cut] >> >>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c >>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c >>>>> @@ -26,6 +26,7 @@ >>>>> #include <linux/mfd/axp20x.h> >>>>> #include <linux/platform_device.h> >>>>> #include <linux/power_supply.h> >>>>> +#include <linux/power/acpi.h> >>>>> #include <linux/iio/consumer.h> >>>>> #include <linux/debugfs.h> >>>>> #include <linux/seq_file.h> >>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct >>>>> platform_device *pdev) >>>>> return ret; >>>>> } >>>>> >>>>> + acpi_battery_unregister(); >>>>> + >>>> >>>> >>>> >>>> What if the ACPI battery driver is loaded after this has been called >>>> already? >>> >>> >>> >>> The module exports that symbol so it must be loaded already. >> >> >> But then it may be unloaded manually and loaded again, may it not? > > > Only if you first unload axp288_fuel_gauge.ko otherwise it will > have a refcount > 0. OK Anyway, I'd prefer blacklists in the battery and ac drivers to be honest. Thanks, Rafael
Hi, On 31-03-17 11:11, Rafael J. Wysocki wrote: > On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 31-03-17 11:05, Rafael J. Wysocki wrote: >>> >>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> >>> wrote: >>> >>> [cut] >>> >>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c >>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c >>>>>> @@ -26,6 +26,7 @@ >>>>>> #include <linux/mfd/axp20x.h> >>>>>> #include <linux/platform_device.h> >>>>>> #include <linux/power_supply.h> >>>>>> +#include <linux/power/acpi.h> >>>>>> #include <linux/iio/consumer.h> >>>>>> #include <linux/debugfs.h> >>>>>> #include <linux/seq_file.h> >>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct >>>>>> platform_device *pdev) >>>>>> return ret; >>>>>> } >>>>>> >>>>>> + acpi_battery_unregister(); >>>>>> + >>>>> >>>>> >>>>> >>>>> What if the ACPI battery driver is loaded after this has been called >>>>> already? >>>> >>>> >>>> >>>> The module exports that symbol so it must be loaded already. >>> >>> >>> But then it may be unloaded manually and loaded again, may it not? >> >> >> Only if you first unload axp288_fuel_gauge.ko otherwise it will >> have a refcount > 0. > > OK > > Anyway, I'd prefer blacklists in the battery and ac drivers to be honest. As I explained in my reply to the discussion around the first patch that is somewhat hard to do and requires encoding knowledge in those drivers which really does not belong there: "The problem is that Intel re-uses HIDs between generations and for the Whiskey Cove PMIC we want to not use the ACPI battery and ac drivers on Cherry Trail (where they are known to be broken) but things are different on Apollo Lake. Yet both use the same HID for their companion Whiskey Cove PMIC even though they are 2 completely different revisions of the PMIC (e.g. one uses i2c the other does not). The 2 native drivers have code to detect which revision they are dealing with and exit with -ENODEV if it is not the revision they were written for, but this means that simple HID blacklisting will not work. So IMHO the decision to unregister the ACPI battery / ac interface really belongs in the native driver as that has all the nitty gritty detail needed to make that decision." Regards, Hans
On Friday, March 31, 2017 11:57:47 AM Hans de Goede wrote: > Hi, Hi, [cut] > > > > Anyway, I'd prefer blacklists in the battery and ac drivers to be honest. > > As I explained in my reply to the discussion around the first patch that > is somewhat hard to do and requires encoding knowledge in those drivers > which really does not belong there: > > "The problem is that Intel re-uses HIDs between generations and > for the Whiskey Cove PMIC we want to not use the ACPI battery > and ac drivers on Cherry Trail (where they are known to be > broken) but things are different on Apollo Lake. Yet both > use the same HID for their companion Whiskey Cove PMIC even > though they are 2 completely different revisions of the PMIC > (e.g. one uses i2c the other does not). > > The 2 native drivers have code to detect which revision they > are dealing with and exit with -ENODEV if it is not the > revision they were written for, but this means that simple > HID blacklisting will not work. So IMHO the decision to > unregister the ACPI battery / ac interface really belongs > in the native driver as that has all the nitty gritty detail > needed to make that decision." Do the native drivers bind to platform devices? Thanks, Rafael
Hi, On 01-04-17 00:30, Rafael J. Wysocki wrote: > On Friday, March 31, 2017 11:57:47 AM Hans de Goede wrote: >> Hi, > > Hi, > > [cut] > >>> >>> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest. >> >> As I explained in my reply to the discussion around the first patch that >> is somewhat hard to do and requires encoding knowledge in those drivers >> which really does not belong there: >> >> "The problem is that Intel re-uses HIDs between generations and >> for the Whiskey Cove PMIC we want to not use the ACPI battery >> and ac drivers on Cherry Trail (where they are known to be >> broken) but things are different on Apollo Lake. Yet both >> use the same HID for their companion Whiskey Cove PMIC even >> though they are 2 completely different revisions of the PMIC >> (e.g. one uses i2c the other does not). >> >> The 2 native drivers have code to detect which revision they >> are dealing with and exit with -ENODEV if it is not the >> revision they were written for, but this means that simple >> HID blacklisting will not work. So IMHO the decision to >> unregister the ACPI battery / ac interface really belongs >> in the native driver as that has all the nitty gritty detail >> needed to make that decision." > > Do the native drivers bind to platform devices? No, (yes, but not really) they are i2c drivers, but for cells of mfd i2c devices, so under the hood they bind to platform devices. Regards, Hans
Hi, On 31-03-17 11:57, Hans de Goede wrote: > Hi, > > On 31-03-17 11:11, Rafael J. Wysocki wrote: >> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> Hi, >>> >>> On 31-03-17 11:05, Rafael J. Wysocki wrote: >>>> >>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> >>>> wrote: >>>> >>>> [cut] >>>> >>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c >>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c >>>>>>> @@ -26,6 +26,7 @@ >>>>>>> #include <linux/mfd/axp20x.h> >>>>>>> #include <linux/platform_device.h> >>>>>>> #include <linux/power_supply.h> >>>>>>> +#include <linux/power/acpi.h> >>>>>>> #include <linux/iio/consumer.h> >>>>>>> #include <linux/debugfs.h> >>>>>>> #include <linux/seq_file.h> >>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct >>>>>>> platform_device *pdev) >>>>>>> return ret; >>>>>>> } >>>>>>> >>>>>>> + acpi_battery_unregister(); >>>>>>> + >>>>>> >>>>>> >>>>>> >>>>>> What if the ACPI battery driver is loaded after this has been called >>>>>> already? >>>>> >>>>> >>>>> >>>>> The module exports that symbol so it must be loaded already. >>>> >>>> >>>> But then it may be unloaded manually and loaded again, may it not? >>> >>> >>> Only if you first unload axp288_fuel_gauge.ko otherwise it will >>> have a refcount > 0. >> >> OK >> >> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest. > > As I explained in my reply to the discussion around the first patch that > is somewhat hard to do and requires encoding knowledge in those drivers > which really does not belong there: > > "The problem is that Intel re-uses HIDs between generations and > for the Whiskey Cove PMIC we want to not use the ACPI battery > and ac drivers on Cherry Trail (where they are known to be > broken) but things are different on Apollo Lake. Yet both > use the same HID for their companion Whiskey Cove PMIC even > though they are 2 completely different revisions of the PMIC > (e.g. one uses i2c the other does not). > > The 2 native drivers have code to detect which revision they > are dealing with and exit with -ENODEV if it is not the > revision they were written for, but this means that simple > HID blacklisting will not work. So IMHO the decision to > unregister the ACPI battery / ac interface really belongs > in the native driver as that has all the nitty gritty detail > needed to make that decision." So thinking more about this, esp. after receiving a bug report from a user getting ACPI errors because of Linux not implementing the proprietary undocumented BMOP opregion before the ACPI battery driver gets unregistered by the native one, I thing we do indeed need to go with a blacklist. This means also being able to match by _HRV, as Some HIDs are re-used for different hardware between Bay Trail / Cherry Trail / Broxton with just a bump of _HRV. I'm currently working on respinning my "acpi: utils: Add new acpi_dev_present helper" to address the review comments on it. I'm going to give it an optional hrv function argument for this use, so as to not having to implement hrv checking code in both ac.c and battery.c . So self-nack for this series as is. Regards, Hans
Hi, On 07-04-17 09:18, Hans de Goede wrote: > Hi, > > On 31-03-17 11:57, Hans de Goede wrote: >> Hi, >> >> On 31-03-17 11:11, Rafael J. Wysocki wrote: >>> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi, >>>> >>>> On 31-03-17 11:05, Rafael J. Wysocki wrote: >>>>> >>>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> >>>>> wrote: >>>>> >>>>> [cut] >>>>> >>>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c >>>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c >>>>>>>> @@ -26,6 +26,7 @@ >>>>>>>> #include <linux/mfd/axp20x.h> >>>>>>>> #include <linux/platform_device.h> >>>>>>>> #include <linux/power_supply.h> >>>>>>>> +#include <linux/power/acpi.h> >>>>>>>> #include <linux/iio/consumer.h> >>>>>>>> #include <linux/debugfs.h> >>>>>>>> #include <linux/seq_file.h> >>>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct >>>>>>>> platform_device *pdev) >>>>>>>> return ret; >>>>>>>> } >>>>>>>> >>>>>>>> + acpi_battery_unregister(); >>>>>>>> + >>>>>>> >>>>>>> >>>>>>> >>>>>>> What if the ACPI battery driver is loaded after this has been called >>>>>>> already? >>>>>> >>>>>> >>>>>> >>>>>> The module exports that symbol so it must be loaded already. >>>>> >>>>> >>>>> But then it may be unloaded manually and loaded again, may it not? >>>> >>>> >>>> Only if you first unload axp288_fuel_gauge.ko otherwise it will >>>> have a refcount > 0. >>> >>> OK >>> >>> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest. >> >> As I explained in my reply to the discussion around the first patch that >> is somewhat hard to do and requires encoding knowledge in those drivers >> which really does not belong there: >> >> "The problem is that Intel re-uses HIDs between generations and >> for the Whiskey Cove PMIC we want to not use the ACPI battery >> and ac drivers on Cherry Trail (where they are known to be >> broken) but things are different on Apollo Lake. Yet both >> use the same HID for their companion Whiskey Cove PMIC even >> though they are 2 completely different revisions of the PMIC >> (e.g. one uses i2c the other does not). >> >> The 2 native drivers have code to detect which revision they >> are dealing with and exit with -ENODEV if it is not the >> revision they were written for, but this means that simple >> HID blacklisting will not work. So IMHO the decision to >> unregister the ACPI battery / ac interface really belongs >> in the native driver as that has all the nitty gritty detail >> needed to make that decision." > > So thinking more about this, esp. after receiving a bug report > from a user getting ACPI errors because of Linux not implementing > the proprietary undocumented BMOP opregion before the ACPI battery > driver gets unregistered by the native one, I thing we do indeed > need to go with a blacklist. > > This means also being able to match by _HRV, as Some HIDs are > re-used for different hardware between Bay Trail / Cherry Trail / > Broxton with just a bump of _HRV. > > I'm currently working on respinning my > "acpi: utils: Add new acpi_dev_present helper" to address the > review comments on it. I'm going to give it an optional hrv > function argument for this use, so as to not having to implement > hrv checking code in both ac.c and battery.c . So a quick copy paste from another thread, the black-list approach causes regressions even before hitting -next and seeing any substantial testing, so we're back to adding unregister functions and calling those from native PMIC power_supply drivers when the native power_supply has been successfully registered. Some Bay Trail / Cherry Trail users are running kernels build from my personal tree to get early access to various fixes in there and I got a regression report on the DELL 5855, where the blacklisting of the ACPI battery driver if INT33F4 is present caused the battery monitoring to stop working, that devices has an INT33F4 node with _STA returning 15 yet it is not using an axp288 PMIC at all, I'm still gathering more info, but I believe atm that Dell simply disabled the i2c controller to which the axp288 would be connected if present and left the other bits of the DTSD unmodified. One option which comes to mind would be to only count devices as present if all their deps are met, but that will only work if the blacklist check is done after all other drivers have loaded which is not how things work. So I believe that my earlier attempts at fixing the double power_supply registration by unregistering the ACPI one when the native one has successfully loaded is better. That guarantees regressions like this one will not happen, because the ACPI power_supply does not get unregistered until the native one has loaded. Regards, Hans
Hi, On 10-04-17 09:31, Hans de Goede wrote: > Hi, > > On 07-04-17 09:18, Hans de Goede wrote: >> Hi, >> >> On 31-03-17 11:57, Hans de Goede wrote: >>> Hi, >>> >>> On 31-03-17 11:11, Rafael J. Wysocki wrote: >>>> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> Hi, >>>>> >>>>> On 31-03-17 11:05, Rafael J. Wysocki wrote: >>>>>> >>>>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> >>>>>> wrote: >>>>>> >>>>>> [cut] >>>>>> >>>>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c >>>>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c >>>>>>>>> @@ -26,6 +26,7 @@ >>>>>>>>> #include <linux/mfd/axp20x.h> >>>>>>>>> #include <linux/platform_device.h> >>>>>>>>> #include <linux/power_supply.h> >>>>>>>>> +#include <linux/power/acpi.h> >>>>>>>>> #include <linux/iio/consumer.h> >>>>>>>>> #include <linux/debugfs.h> >>>>>>>>> #include <linux/seq_file.h> >>>>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct >>>>>>>>> platform_device *pdev) >>>>>>>>> return ret; >>>>>>>>> } >>>>>>>>> >>>>>>>>> + acpi_battery_unregister(); >>>>>>>>> + >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> What if the ACPI battery driver is loaded after this has been called >>>>>>>> already? >>>>>>> >>>>>>> >>>>>>> >>>>>>> The module exports that symbol so it must be loaded already. >>>>>> >>>>>> >>>>>> But then it may be unloaded manually and loaded again, may it not? >>>>> >>>>> >>>>> Only if you first unload axp288_fuel_gauge.ko otherwise it will >>>>> have a refcount > 0. >>>> >>>> OK >>>> >>>> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest. >>> >>> As I explained in my reply to the discussion around the first patch that >>> is somewhat hard to do and requires encoding knowledge in those drivers >>> which really does not belong there: >>> >>> "The problem is that Intel re-uses HIDs between generations and >>> for the Whiskey Cove PMIC we want to not use the ACPI battery >>> and ac drivers on Cherry Trail (where they are known to be >>> broken) but things are different on Apollo Lake. Yet both >>> use the same HID for their companion Whiskey Cove PMIC even >>> though they are 2 completely different revisions of the PMIC >>> (e.g. one uses i2c the other does not). >>> >>> The 2 native drivers have code to detect which revision they >>> are dealing with and exit with -ENODEV if it is not the >>> revision they were written for, but this means that simple >>> HID blacklisting will not work. So IMHO the decision to >>> unregister the ACPI battery / ac interface really belongs >>> in the native driver as that has all the nitty gritty detail >>> needed to make that decision." >> >> So thinking more about this, esp. after receiving a bug report >> from a user getting ACPI errors because of Linux not implementing >> the proprietary undocumented BMOP opregion before the ACPI battery >> driver gets unregistered by the native one, I thing we do indeed >> need to go with a blacklist. >> >> This means also being able to match by _HRV, as Some HIDs are >> re-used for different hardware between Bay Trail / Cherry Trail / >> Broxton with just a bump of _HRV. >> >> I'm currently working on respinning my >> "acpi: utils: Add new acpi_dev_present helper" to address the >> review comments on it. I'm going to give it an optional hrv >> function argument for this use, so as to not having to implement >> hrv checking code in both ac.c and battery.c . > > So a quick copy paste from another thread, the black-list approach > causes regressions even before hitting -next and seeing any > substantial testing, so we're back to adding unregister functions > and calling those from native PMIC power_supply drivers when the > native power_supply has been successfully registered. > > Some Bay Trail / Cherry Trail users are running kernels build > from my personal tree to get early access to various fixes > in there and I got a regression report on the DELL 5855, where > the blacklisting of the ACPI battery driver if INT33F4 is > present caused the battery monitoring to stop working, that > devices has an INT33F4 node with _STA returning 15 yet it > is not using an axp288 PMIC at all, I'm still gathering more > info, but I believe atm that Dell simply disabled the i2c > controller to which the axp288 would be connected if present > and left the other bits of the DTSD unmodified. > > One option which comes to mind would be to only count devices > as present if all their deps are met, but that will only > work if the blacklist check is done after all other drivers > have loaded which is not how things work. > > So I believe that my earlier attempts at fixing the double > power_supply registration by unregistering the ACPI one when > the native one has successfully loaded is better. That guarantees > regressions like this one will not happen, because the ACPI > power_supply does not get unregistered until the native one > has loaded. Ok scrap that, I've some more info and the INT33F4 node on the Dell 5855 is returning 0 from _STA and the blacklist is causing problems on that machine for other reason, it could be the user was using an older version with the uninitialized .cls match info problem, I've asked the user to test the latest version. So assuming that does work, we are good to go with the blacklist approach (which seems to be the solution everyone prefers), sorry about the noise. Regards, Hans
On Mon, Apr 10, 2017 at 8:13 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 10-04-17 09:31, Hans de Goede wrote: >> >> Hi, >> >> On 07-04-17 09:18, Hans de Goede wrote: >>> >>> Hi, >>> >>> On 31-03-17 11:57, Hans de Goede wrote: >>>> >>>> Hi, >>>> >>>> On 31-03-17 11:11, Rafael J. Wysocki wrote: >>>>> >>>>> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> >>>>> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 31-03-17 11:05, Rafael J. Wysocki wrote: >>>>>>> >>>>>>> >>>>>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> >>>>>>> wrote: >>>>>>> >>>>>>> [cut] >>>>>>> >>>>>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c >>>>>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c >>>>>>>>>> @@ -26,6 +26,7 @@ >>>>>>>>>> #include <linux/mfd/axp20x.h> >>>>>>>>>> #include <linux/platform_device.h> >>>>>>>>>> #include <linux/power_supply.h> >>>>>>>>>> +#include <linux/power/acpi.h> >>>>>>>>>> #include <linux/iio/consumer.h> >>>>>>>>>> #include <linux/debugfs.h> >>>>>>>>>> #include <linux/seq_file.h> >>>>>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct >>>>>>>>>> platform_device *pdev) >>>>>>>>>> return ret; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> + acpi_battery_unregister(); >>>>>>>>>> + >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> What if the ACPI battery driver is loaded after this has been >>>>>>>>> called >>>>>>>>> already? >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> The module exports that symbol so it must be loaded already. >>>>>>> >>>>>>> >>>>>>> >>>>>>> But then it may be unloaded manually and loaded again, may it not? >>>>>> >>>>>> >>>>>> >>>>>> Only if you first unload axp288_fuel_gauge.ko otherwise it will >>>>>> have a refcount > 0. >>>>> >>>>> >>>>> OK >>>>> >>>>> Anyway, I'd prefer blacklists in the battery and ac drivers to be >>>>> honest. >>>> >>>> >>>> As I explained in my reply to the discussion around the first patch that >>>> is somewhat hard to do and requires encoding knowledge in those drivers >>>> which really does not belong there: >>>> >>>> "The problem is that Intel re-uses HIDs between generations and >>>> for the Whiskey Cove PMIC we want to not use the ACPI battery >>>> and ac drivers on Cherry Trail (where they are known to be >>>> broken) but things are different on Apollo Lake. Yet both >>>> use the same HID for their companion Whiskey Cove PMIC even >>>> though they are 2 completely different revisions of the PMIC >>>> (e.g. one uses i2c the other does not). >>>> >>>> The 2 native drivers have code to detect which revision they >>>> are dealing with and exit with -ENODEV if it is not the >>>> revision they were written for, but this means that simple >>>> HID blacklisting will not work. So IMHO the decision to >>>> unregister the ACPI battery / ac interface really belongs >>>> in the native driver as that has all the nitty gritty detail >>>> needed to make that decision." >>> >>> >>> So thinking more about this, esp. after receiving a bug report >>> from a user getting ACPI errors because of Linux not implementing >>> the proprietary undocumented BMOP opregion before the ACPI battery >>> driver gets unregistered by the native one, I thing we do indeed >>> need to go with a blacklist. >>> >>> This means also being able to match by _HRV, as Some HIDs are >>> re-used for different hardware between Bay Trail / Cherry Trail / >>> Broxton with just a bump of _HRV. >>> >>> I'm currently working on respinning my >>> "acpi: utils: Add new acpi_dev_present helper" to address the >>> review comments on it. I'm going to give it an optional hrv >>> function argument for this use, so as to not having to implement >>> hrv checking code in both ac.c and battery.c . >> >> >> So a quick copy paste from another thread, the black-list approach >> causes regressions even before hitting -next and seeing any >> substantial testing, so we're back to adding unregister functions >> and calling those from native PMIC power_supply drivers when the >> native power_supply has been successfully registered. >> >> Some Bay Trail / Cherry Trail users are running kernels build >> from my personal tree to get early access to various fixes >> in there and I got a regression report on the DELL 5855, where >> the blacklisting of the ACPI battery driver if INT33F4 is >> present caused the battery monitoring to stop working, that >> devices has an INT33F4 node with _STA returning 15 yet it >> is not using an axp288 PMIC at all, I'm still gathering more >> info, but I believe atm that Dell simply disabled the i2c >> controller to which the axp288 would be connected if present >> and left the other bits of the DTSD unmodified. >> >> One option which comes to mind would be to only count devices >> as present if all their deps are met, but that will only >> work if the blacklist check is done after all other drivers >> have loaded which is not how things work. >> >> So I believe that my earlier attempts at fixing the double >> power_supply registration by unregistering the ACPI one when >> the native one has successfully loaded is better. That guarantees >> regressions like this one will not happen, because the ACPI >> power_supply does not get unregistered until the native one >> has loaded. > > > Ok scrap that, I've some more info and the INT33F4 node on > the Dell 5855 is returning 0 from _STA and the blacklist is > causing problems on that machine for other reason, it could > be the user was using an older version with the uninitialized > .cls match info problem, I've asked the user to test the latest > version. > > So assuming that does work, we are good to go with the blacklist > approach (which seems to be the solution everyone prefers), > sorry about the noise. OK, but there's one more possibility to consider. Instead of unregistering the ACPI battery (or AC) driver from the platform device driver superseding it, you could clear the match_driver flag for the ACPI companion device and call device_release_driver() on it, like acpi_bus_trim(). Then (because the match_driver flag is unset) the driver core will not try to attach the driver to the thing again - until the next invocation of acpi_bus_attach() on it, which I think is a bug, because there seems to be a flags.visited check missing in there (I need to go back and recall why it is not there). Arguably, that would be a more lightweight way of getting what you want without using a blacklist. Thanks, Rafael
Hi, On 10-04-17 22:01, Rafael J. Wysocki wrote: > On Mon, Apr 10, 2017 at 8:13 PM, Hans de Goede <hdegoede@redhat.com> wrote: <snip> >>>>>> OK >>>>>> >>>>>> Anyway, I'd prefer blacklists in the battery and ac drivers to be >>>>>> honest. >>>>> >>>>> >>>>> As I explained in my reply to the discussion around the first patch that >>>>> is somewhat hard to do and requires encoding knowledge in those drivers >>>>> which really does not belong there: >>>>> >>>>> "The problem is that Intel re-uses HIDs between generations and >>>>> for the Whiskey Cove PMIC we want to not use the ACPI battery >>>>> and ac drivers on Cherry Trail (where they are known to be >>>>> broken) but things are different on Apollo Lake. Yet both >>>>> use the same HID for their companion Whiskey Cove PMIC even >>>>> though they are 2 completely different revisions of the PMIC >>>>> (e.g. one uses i2c the other does not). >>>>> >>>>> The 2 native drivers have code to detect which revision they >>>>> are dealing with and exit with -ENODEV if it is not the >>>>> revision they were written for, but this means that simple >>>>> HID blacklisting will not work. So IMHO the decision to >>>>> unregister the ACPI battery / ac interface really belongs >>>>> in the native driver as that has all the nitty gritty detail >>>>> needed to make that decision." >>>> >>>> >>>> So thinking more about this, esp. after receiving a bug report >>>> from a user getting ACPI errors because of Linux not implementing >>>> the proprietary undocumented BMOP opregion before the ACPI battery >>>> driver gets unregistered by the native one, I thing we do indeed >>>> need to go with a blacklist. >>>> >>>> This means also being able to match by _HRV, as Some HIDs are >>>> re-used for different hardware between Bay Trail / Cherry Trail / >>>> Broxton with just a bump of _HRV. >>>> >>>> I'm currently working on respinning my >>>> "acpi: utils: Add new acpi_dev_present helper" to address the >>>> review comments on it. I'm going to give it an optional hrv >>>> function argument for this use, so as to not having to implement >>>> hrv checking code in both ac.c and battery.c . >>> >>> >>> So a quick copy paste from another thread, the black-list approach >>> causes regressions even before hitting -next and seeing any >>> substantial testing, so we're back to adding unregister functions >>> and calling those from native PMIC power_supply drivers when the >>> native power_supply has been successfully registered. >>> >>> Some Bay Trail / Cherry Trail users are running kernels build >>> from my personal tree to get early access to various fixes >>> in there and I got a regression report on the DELL 5855, where >>> the blacklisting of the ACPI battery driver if INT33F4 is >>> present caused the battery monitoring to stop working, that >>> devices has an INT33F4 node with _STA returning 15 yet it >>> is not using an axp288 PMIC at all, I'm still gathering more >>> info, but I believe atm that Dell simply disabled the i2c >>> controller to which the axp288 would be connected if present >>> and left the other bits of the DTSD unmodified. >>> >>> One option which comes to mind would be to only count devices >>> as present if all their deps are met, but that will only >>> work if the blacklist check is done after all other drivers >>> have loaded which is not how things work. >>> >>> So I believe that my earlier attempts at fixing the double >>> power_supply registration by unregistering the ACPI one when >>> the native one has successfully loaded is better. That guarantees >>> regressions like this one will not happen, because the ACPI >>> power_supply does not get unregistered until the native one >>> has loaded. >> >> >> Ok scrap that, I've some more info and the INT33F4 node on >> the Dell 5855 is returning 0 from _STA and the blacklist is >> causing problems on that machine for other reason, it could >> be the user was using an older version with the uninitialized >> .cls match info problem, I've asked the user to test the latest >> version. >> >> So assuming that does work, we are good to go with the blacklist >> approach (which seems to be the solution everyone prefers), >> sorry about the noise. > > OK, but there's one more possibility to consider. > > Instead of unregistering the ACPI battery (or AC) driver from the > platform device driver superseding it, you could clear the > match_driver flag for the ACPI companion device and call > device_release_driver() on it, like acpi_bus_trim(). Then (because > the match_driver flag is unset) the driver core will not try to attach > the driver to the thing again - until the next invocation of > acpi_bus_attach() on it, which I think is a bug, because there seems > to be a flags.visited check missing in there (I need to go back and > recall why it is not there). > > Arguably, that would be a more lightweight way of getting what you > want without using a blacklist. The reason I changed my mind on the blacklist approach and went from unregistering on native-power-supply register to the blacklist is because some users where seeing a ton of ACPI errors from the ACPI battery driver trying to use the broken ACPI battery dev in the DTSD before the native PMIC driver loaded and unregistered the battery dev. So I really believe the blacklist is the right approach. I just got confirmation from the user that the regression on the Dell 5855 was indeed a false positive. So from my pov we are good to go with v5 of patch 1 of this set and v2 (also send as v3 and v4 but unchanged) of patches 2-4 of this set. Regards, Hans
On Tue, Apr 11, 2017 at 11:18 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 10-04-17 22:01, Rafael J. Wysocki wrote: >> >> On Mon, Apr 10, 2017 at 8:13 PM, Hans de Goede <hdegoede@redhat.com> >> wrote: > [cut] > > The reason I changed my mind on the blacklist approach and went > from unregistering on native-power-supply register to the blacklist > is because some users where seeing a ton of ACPI errors from the > ACPI battery driver trying to use the broken ACPI battery dev in > the DTSD before the native PMIC driver loaded and unregistered > the battery dev. So I really believe the blacklist is the right > approach. Fair enough. > I just got confirmation from the user that the regression on the > Dell 5855 was indeed a false positive. > > So from my pov we are good to go with v5 of patch 1 of this > set and v2 (also send as v3 and v4 but unchanged) of patches > 2-4 of this set. OK, but please resend the patches you're happy with afresh and let me know what is superseded by them, so that I don't have to carry out any research on that. Thanks, Rafael
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index d0453ca..e504644 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -242,6 +242,8 @@ config AXP288_CHARGER config AXP288_FUEL_GAUGE tristate "X-Powers AXP288 Fuel Gauge" depends on MFD_AXP20X && IIO + # if ACPI_BATTERY=m, this can't be 'y' + depends on ACPI_BATTERY || !ACPI_BATTERY help Say yes here to have support for X-Power power management IC (PMIC) Fuel Gauge. The device provides battery statistics and status diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c index a8dcabc..15f10ce 100644 --- a/drivers/power/supply/axp288_fuel_gauge.c +++ b/drivers/power/supply/axp288_fuel_gauge.c @@ -26,6 +26,7 @@ #include <linux/mfd/axp20x.h> #include <linux/platform_device.h> #include <linux/power_supply.h> +#include <linux/power/acpi.h> #include <linux/iio/consumer.h> #include <linux/debugfs.h> #include <linux/seq_file.h> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev) return ret; } + acpi_battery_unregister(); + fuel_gauge_create_debugfs(info); fuel_gauge_init_irq(info); schedule_delayed_work(&info->status_monitor, STATUS_MON_DELAY_JIFFIES);