| Message ID | 8879480.rMLUfLXkoz@kreacher (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | PCI: ACPI: Get rid of struct pci_platform_pm_ops and clean up code | expand |
Hi, Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Using struct pci_platform_pm_ops for ACPI adds unnecessary > indirection to the interactions between the PCI core and ACPI PM, > which is also subject to retpolines. > > Moreover, it is not particularly clear from the current code that, > as far as PCI PM is concerned, "platform" really means just ACPI > except for the special casess when Intel MID PCI PM is used or when > ACPI support is disabled (through the kernel config or command line, > or because there are no usable ACPI tables on the system). > > To address the above, rework the PCI PM code to invoke ACPI PM > functions directly as needed and drop the acpi_pci_platform_pm > object that is not necessary any more. > > Accordingly, update some of the ACPI PM functions in question to do > extra checks in case the ACPI support is disabled (which previously > was taken care of by avoiding to set the pci_platform_ops pointer > in those cases). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: > * Rebase on top of the new [1/7] and move dropping struct > pci_platform_pm_ops to a separate patch. I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't apply (after 1/7 applied). Should I apply this on another tree? > --- > drivers/pci/pci-acpi.c | 42 ++++++++++++++++++++---------------------- > drivers/pci/pci.c | 22 +++++++++------------- > drivers/pci/pci.h | 38 +++++++++++++++++++++++++++++++++++++- > 3 files changed, 66 insertions(+), 36 deletions(-) > > Index: linux-pm/drivers/pci/pci-acpi.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-acpi.c > +++ linux-pm/drivers/pci/pci-acpi.c > @@ -906,10 +906,13 @@ acpi_status pci_acpi_add_pm_notifier(str > * choose highest power _SxD or any lower power > */ > > -static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) > +pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) > { > int acpi_state, d_max; > > + if (acpi_pci_disabled) > + return PCI_POWER_ERROR; > + > if (pdev->no_d3cold) > d_max = ACPI_STATE_D3_HOT; > else > @@ -965,7 +968,7 @@ int pci_dev_acpi_reset(struct pci_dev *d > return 0; > } > > -static bool acpi_pci_power_manageable(struct pci_dev *dev) > +bool acpi_pci_power_manageable(struct pci_dev *dev) > { > struct acpi_device *adev = ACPI_COMPANION(&dev->dev); > > @@ -974,13 +977,13 @@ static bool acpi_pci_power_manageable(st > return acpi_device_power_manageable(adev); > } > > -static bool acpi_pci_bridge_d3(struct pci_dev *dev) > +bool acpi_pci_bridge_d3(struct pci_dev *dev) > { > const union acpi_object *obj; > struct acpi_device *adev; > struct pci_dev *rpdev; > > - if (!dev->is_hotplug_bridge) > + if (acpi_pci_disabled || !dev->is_hotplug_bridge) > return false; > > /* Assume D3 support if the bridge is power-manageable by ACPI. */ > @@ -1008,7 +1011,7 @@ static bool acpi_pci_bridge_d3(struct pc > return obj->integer.value == 1; > } > > -static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) > +int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) > { > struct acpi_device *adev = ACPI_COMPANION(&dev->dev); > static const u8 state_conv[] = { > @@ -1046,7 +1049,7 @@ static int acpi_pci_set_power_state(stru > return error; > } > > -static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev) > +pci_power_t acpi_pci_get_power_state(struct pci_dev *dev) > { > struct acpi_device *adev = ACPI_COMPANION(&dev->dev); > static const pci_power_t state_conv[] = { > @@ -1068,7 +1071,7 @@ static pci_power_t acpi_pci_get_power_st > return state_conv[state]; > } > > -static void acpi_pci_refresh_power_state(struct pci_dev *dev) > +void acpi_pci_refresh_power_state(struct pci_dev *dev) > { > struct acpi_device *adev = ACPI_COMPANION(&dev->dev); > > @@ -1093,17 +1096,23 @@ static int acpi_pci_propagate_wakeup(str > return 0; > } > > -static int acpi_pci_wakeup(struct pci_dev *dev, bool enable) > +int acpi_pci_wakeup(struct pci_dev *dev, bool enable) > { > + if (acpi_pci_disabled) > + return 0; > + > if (acpi_pm_device_can_wakeup(&dev->dev)) > return acpi_pm_set_device_wakeup(&dev->dev, enable); > > return acpi_pci_propagate_wakeup(dev->bus, enable); > } > > -static bool acpi_pci_need_resume(struct pci_dev *dev) > +bool acpi_pci_need_resume(struct pci_dev *dev) > { > - struct acpi_device *adev = ACPI_COMPANION(&dev->dev); > + struct acpi_device *adev; > + > + if (acpi_pci_disabled) > + return false; > > /* > * In some cases (eg. Samsung 305V4A) leaving a bridge in suspend over > @@ -1118,6 +1127,7 @@ static bool acpi_pci_need_resume(struct > if (!adev || !acpi_device_power_manageable(adev)) > return false; > > + adev = ACPI_COMPANION(&dev->dev); > if (adev->wakeup.flags.valid && > device_may_wakeup(&dev->dev) != !!adev->wakeup.prepare_count) > return true; > @@ -1128,17 +1138,6 @@ static bool acpi_pci_need_resume(struct > return !!adev->power.flags.dsw_present; > } > > -static const struct pci_platform_pm_ops acpi_pci_platform_pm = { > - .bridge_d3 = acpi_pci_bridge_d3, > - .is_manageable = acpi_pci_power_manageable, > - .set_state = acpi_pci_set_power_state, > - .get_state = acpi_pci_get_power_state, > - .refresh_state = acpi_pci_refresh_power_state, > - .choose_state = acpi_pci_choose_state, > - .set_wakeup = acpi_pci_wakeup, > - .need_resume = acpi_pci_need_resume, > -}; > - > void acpi_pci_add_bus(struct pci_bus *bus) > { > union acpi_object *obj; > @@ -1448,7 +1447,6 @@ static int __init acpi_pci_init(void) > if (acpi_pci_disabled) > return 0; > > - pci_set_platform_pm(&acpi_pci_platform_pm); > acpi_pci_slot_init(); > acpiphp_init(); > > Index: linux-pm/drivers/pci/pci.h > =================================================================== > --- linux-pm.orig/drivers/pci/pci.h > +++ linux-pm/drivers/pci/pci.h > @@ -725,17 +725,53 @@ int pci_acpi_program_hp_params(struct pc > extern const struct attribute_group pci_dev_acpi_attr_group; > void pci_set_acpi_fwnode(struct pci_dev *dev); > int pci_dev_acpi_reset(struct pci_dev *dev, bool probe); > +bool acpi_pci_power_manageable(struct pci_dev *dev); > +bool acpi_pci_bridge_d3(struct pci_dev *dev); > +int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state); > +pci_power_t acpi_pci_get_power_state(struct pci_dev *dev); > +void acpi_pci_refresh_power_state(struct pci_dev *dev); > +int acpi_pci_wakeup(struct pci_dev *dev, bool enable); > +bool acpi_pci_need_resume(struct pci_dev *dev); > +pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); > #else > static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) > { > return -ENOTTY; > } > - > static inline void pci_set_acpi_fwnode(struct pci_dev *dev) {} > static inline int pci_acpi_program_hp_params(struct pci_dev *dev) > { > return -ENODEV; > } > +static inline bool acpi_pci_power_manageable(struct pci_dev *dev) > +{ > + return false; > +} > +static inline bool acpi_pci_bridge_d3(struct pci_dev *dev) > +{ > + return false; > +} > +static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) > +{ > + return -ENODEV; > +} > +static inline pci_power_t acpi_pci_get_power_state(struct pci_dev *dev) > +{ > + return PCI_UNKNOWN; > +} > +static inline void acpi_pci_refresh_power_state(struct pci_dev *dev) {} > +static inline int acpi_pci_wakeup(struct pci_dev *dev, bool enable) > +{ > + return -ENODEV; > +} > +static inline bool acpi_pci_need_resume(struct pci_dev *dev) > +{ > + return false; > +} > +static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) > +{ > + return PCI_POWER_ERROR; > +} > #endif > > #ifdef CONFIG_PCIEASPM > Index: linux-pm/drivers/pci/pci.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci.c > +++ linux-pm/drivers/pci/pci.c > @@ -988,7 +988,7 @@ static inline bool platform_pci_power_ma > if (pci_use_mid_pm()) > return true; > > - return pci_platform_pm ? pci_platform_pm->is_manageable(dev) : false; > + return acpi_pci_power_manageable(dev); > } > > static inline int platform_pci_set_power_state(struct pci_dev *dev, > @@ -997,7 +997,7 @@ static inline int platform_pci_set_power > if (pci_use_mid_pm()) > return mid_pci_set_power_state(dev, t); > > - return pci_platform_pm ? pci_platform_pm->set_state(dev, t) : -ENOSYS; > + return acpi_pci_set_power_state(dev, t); > } > > static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev) > @@ -1005,13 +1005,13 @@ static inline pci_power_t platform_pci_g > if (pci_use_mid_pm()) > return mid_pci_get_power_state(dev); > > - return pci_platform_pm ? pci_platform_pm->get_state(dev) : PCI_UNKNOWN; > + return acpi_pci_get_power_state(dev); > } > > static inline void platform_pci_refresh_power_state(struct pci_dev *dev) > { > - if (!pci_use_mid_pm() && pci_platform_pm && pci_platform_pm->refresh_state) > - pci_platform_pm->refresh_state(dev); > + if (!pci_use_mid_pm()) > + acpi_pci_refresh_power_state(dev); > } > > static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev) > @@ -1019,8 +1019,7 @@ static inline pci_power_t platform_pci_c > if (pci_use_mid_pm()) > return PCI_POWER_ERROR; > > - return pci_platform_pm ? > - pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR; > + return acpi_pci_choose_state(dev); > } > > static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) > @@ -1028,8 +1027,7 @@ static inline int platform_pci_set_wakeu > if (pci_use_mid_pm()) > return PCI_POWER_ERROR; > > - return pci_platform_pm ? > - pci_platform_pm->set_wakeup(dev, enable) : -ENODEV; > + return acpi_pci_wakeup(dev, enable); > } > > static inline bool platform_pci_need_resume(struct pci_dev *dev) > @@ -1037,7 +1035,7 @@ static inline bool platform_pci_need_res > if (pci_use_mid_pm()) > return false; > > - return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false; > + return acpi_pci_need_resume(dev); > } > > static inline bool platform_pci_bridge_d3(struct pci_dev *dev) > @@ -1045,9 +1043,7 @@ static inline bool platform_pci_bridge_d > if (pci_use_mid_pm()) > return false; > > - if (pci_platform_pm && pci_platform_pm->bridge_d3) > - return pci_platform_pm->bridge_d3(dev); > - return false; > + return acpi_pci_bridge_d3(dev); > } > > /** > > >
On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth <fntoth@gmail.com> wrote: > > Hi, > Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Using struct pci_platform_pm_ops for ACPI adds unnecessary > > indirection to the interactions between the PCI core and ACPI PM, > > which is also subject to retpolines. > > > > Moreover, it is not particularly clear from the current code that, > > as far as PCI PM is concerned, "platform" really means just ACPI > > except for the special casess when Intel MID PCI PM is used or when > > ACPI support is disabled (through the kernel config or command line, > > or because there are no usable ACPI tables on the system). > > > > To address the above, rework the PCI PM code to invoke ACPI PM > > functions directly as needed and drop the acpi_pci_platform_pm > > object that is not necessary any more. > > > > Accordingly, update some of the ACPI PM functions in question to do > > extra checks in case the ACPI support is disabled (which previously > > was taken care of by avoiding to set the pci_platform_ops pointer > > in those cases). > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > > > v1 -> v2: > > * Rebase on top of the new [1/7] and move dropping struct > > pci_platform_pm_ops to a separate patch. > > I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't > apply (after 1/7 applied). Should I apply this on another tree? This is on top of https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ which is not yet in any tree. Sorry for the confusion.
On Thu, Sep 23, 2021 at 3:26 PM Ferry Toth <fntoth@gmail.com> wrote: > > Hi, > > Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki: > > On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth <fntoth@gmail.com> wrote: > > Hi, > Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Using struct pci_platform_pm_ops for ACPI adds unnecessary > indirection to the interactions between the PCI core and ACPI PM, > which is also subject to retpolines. > > Moreover, it is not particularly clear from the current code that, > as far as PCI PM is concerned, "platform" really means just ACPI > except for the special casess when Intel MID PCI PM is used or when > ACPI support is disabled (through the kernel config or command line, > or because there are no usable ACPI tables on the system). > > To address the above, rework the PCI PM code to invoke ACPI PM > functions directly as needed and drop the acpi_pci_platform_pm > object that is not necessary any more. > > Accordingly, update some of the ACPI PM functions in question to do > extra checks in case the ACPI support is disabled (which previously > was taken care of by avoiding to set the pci_platform_ops pointer > in those cases). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: > * Rebase on top of the new [1/7] and move dropping struct > pci_platform_pm_ops to a separate patch. > > I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't > apply (after 1/7 applied). Should I apply this on another tree? > > This is on top of > https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ > which is not yet in any tree. > > Sorry for the confusion. > > No problem at all. If I can I will try to report back tonight. Else, will be delayed 2 due to a short break. Thank you!
Repost (with formatting removed, sorry for the noise) Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki: > On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com> wrote: >> Hi, >> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki: >>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com> >>> >>> Using struct pci_platform_pm_ops for ACPI adds unnecessary >>> indirection to the interactions between the PCI core and ACPI PM, >>> which is also subject to retpolines. >>> >>> Moreover, it is not particularly clear from the current code that, >>> as far as PCI PM is concerned, "platform" really means just ACPI >>> except for the special casess when Intel MID PCI PM is used or when >>> ACPI support is disabled (through the kernel config or command line, >>> or because there are no usable ACPI tables on the system). >>> >>> To address the above, rework the PCI PM code to invoke ACPI PM >>> functions directly as needed and drop the acpi_pci_platform_pm >>> object that is not necessary any more. >>> >>> Accordingly, update some of the ACPI PM functions in question to do >>> extra checks in case the ACPI support is disabled (which previously >>> was taken care of by avoiding to set the pci_platform_ops pointer >>> in those cases). >>> >>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com> >>> --- >>> >>> v1 -> v2: >>> * Rebase on top of the new [1/7] and move dropping struct >>> pci_platform_pm_ops to a separate patch. >> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't >> apply (after 1/7 applied). Should I apply this on another tree? > This is on top of > https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ > which is not yet in any tree. > > Sorry for the confusion. No problem at all. If I can I will try to report back tonight. Else, will be delayed 2 due to a short break.
Hi Op 23-09-2021 om 15:51 schreef Ferry Toth: > Repost (with formatting removed, sorry for the noise) > Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki: >> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com> wrote: >>> Hi, >>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki: >>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com> >>>> >>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary >>>> indirection to the interactions between the PCI core and ACPI PM, >>>> which is also subject to retpolines. >>>> >>>> Moreover, it is not particularly clear from the current code that, >>>> as far as PCI PM is concerned, "platform" really means just ACPI >>>> except for the special casess when Intel MID PCI PM is used or when >>>> ACPI support is disabled (through the kernel config or command line, >>>> or because there are no usable ACPI tables on the system). >>>> >>>> To address the above, rework the PCI PM code to invoke ACPI PM >>>> functions directly as needed and drop the acpi_pci_platform_pm >>>> object that is not necessary any more. >>>> >>>> Accordingly, update some of the ACPI PM functions in question to do >>>> extra checks in case the ACPI support is disabled (which previously >>>> was taken care of by avoiding to set the pci_platform_ops pointer >>>> in those cases). >>>> >>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com> >>>> --- >>>> >>>> v1 -> v2: >>>> * Rebase on top of the new [1/7] and move dropping struct >>>> pci_platform_pm_ops to a separate patch. >>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't >>> apply (after 1/7 applied). Should I apply this on another tree? >> This is on top of >> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ >> >> which is not yet in any tree. >> >> Sorry for the confusion. > No problem at all. If I can I will try to report back tonight. Else, > will be delayed 2 due to a short break. With those 3 extra patches followed by 7 from this series it builds. But on boot I get: dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core Then after this it reboots. Nothing in the logs. Nothing else on console, I guess something goes wrong early. I tried both in host / device mode - no change. Normally in host mode I have: usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 5.14 Only ref to dwc3 is: tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup / using lookup tables for GPIO lookup / No GPIO consumer cs found
On Thu, Sep 23, 2021 at 10:32 PM Ferry Toth <fntoth@gmail.com> wrote: > > Hi > > Op 23-09-2021 om 15:51 schreef Ferry Toth: > > Repost (with formatting removed, sorry for the noise) > > Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki: > >> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com> wrote: > >>> Hi, > >>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki: > >>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com> > >>>> > >>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary > >>>> indirection to the interactions between the PCI core and ACPI PM, > >>>> which is also subject to retpolines. > >>>> > >>>> Moreover, it is not particularly clear from the current code that, > >>>> as far as PCI PM is concerned, "platform" really means just ACPI > >>>> except for the special casess when Intel MID PCI PM is used or when > >>>> ACPI support is disabled (through the kernel config or command line, > >>>> or because there are no usable ACPI tables on the system). > >>>> > >>>> To address the above, rework the PCI PM code to invoke ACPI PM > >>>> functions directly as needed and drop the acpi_pci_platform_pm > >>>> object that is not necessary any more. > >>>> > >>>> Accordingly, update some of the ACPI PM functions in question to do > >>>> extra checks in case the ACPI support is disabled (which previously > >>>> was taken care of by avoiding to set the pci_platform_ops pointer > >>>> in those cases). > >>>> > >>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com> > >>>> --- > >>>> > >>>> v1 -> v2: > >>>> * Rebase on top of the new [1/7] and move dropping struct > >>>> pci_platform_pm_ops to a separate patch. > >>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't > >>> apply (after 1/7 applied). Should I apply this on another tree? > >> This is on top of > >> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ > >> > >> which is not yet in any tree. > >> > >> Sorry for the confusion. > > No problem at all. If I can I will try to report back tonight. Else, > > will be delayed 2 due to a short break. > > With those 3 extra patches followed by 7 from this series it builds. But > on boot I get: > dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core > Then after this it reboots. Nothing in the logs. Nothing else on > console, I guess something goes wrong early. It appears so. Can you please try just the 3 extra patches this series is on top of? The problem is more likely to be located in one of them.
Hi, Op 24-09-2021 om 14:02 schreef Rafael J. Wysocki: > On Thu, Sep 23, 2021 at 10:32 PM Ferry Toth <fntoth@gmail.com> wrote: >> Hi >> >> Op 23-09-2021 om 15:51 schreef Ferry Toth: >>> Repost (with formatting removed, sorry for the noise) >>> Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki: >>>> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com> wrote: >>>>> Hi, >>>>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki: >>>>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com> >>>>>> >>>>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary >>>>>> indirection to the interactions between the PCI core and ACPI PM, >>>>>> which is also subject to retpolines. >>>>>> >>>>>> Moreover, it is not particularly clear from the current code that, >>>>>> as far as PCI PM is concerned, "platform" really means just ACPI >>>>>> except for the special casess when Intel MID PCI PM is used or when >>>>>> ACPI support is disabled (through the kernel config or command line, >>>>>> or because there are no usable ACPI tables on the system). >>>>>> >>>>>> To address the above, rework the PCI PM code to invoke ACPI PM >>>>>> functions directly as needed and drop the acpi_pci_platform_pm >>>>>> object that is not necessary any more. >>>>>> >>>>>> Accordingly, update some of the ACPI PM functions in question to do >>>>>> extra checks in case the ACPI support is disabled (which previously >>>>>> was taken care of by avoiding to set the pci_platform_ops pointer >>>>>> in those cases). >>>>>> >>>>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com> >>>>>> --- >>>>>> >>>>>> v1 -> v2: >>>>>> * Rebase on top of the new [1/7] and move dropping struct >>>>>> pci_platform_pm_ops to a separate patch. >>>>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't >>>>> apply (after 1/7 applied). Should I apply this on another tree? >>>> This is on top of >>>> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ >>>> >>>> which is not yet in any tree. >>>> >>>> Sorry for the confusion. >>> No problem at all. If I can I will try to report back tonight. Else, >>> will be delayed 2 due to a short break. >> With those 3 extra patches followed by 7 from this series it builds. But >> on boot I get: >> dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core >> Then after this it reboots. Nothing in the logs. Nothing else on >> console, I guess something goes wrong early. > It appears so. > > Can you please try just the 3 extra patches this series is on top of? > The problem is more likely to be located in one of them. Yes, I hope to be able to that this evening.
Hi Op 24-09-2021 om 14:02 schreef Rafael J. Wysocki: > On Thu, Sep 23, 2021 at 10:32 PM Ferry Toth <fntoth@gmail.com> wrote: >> Hi >> >> Op 23-09-2021 om 15:51 schreef Ferry Toth: >>> Repost (with formatting removed, sorry for the noise) >>> Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki: >>>> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com> wrote: >>>>> Hi, >>>>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki: >>>>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com> >>>>>> >>>>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary >>>>>> indirection to the interactions between the PCI core and ACPI PM, >>>>>> which is also subject to retpolines. >>>>>> >>>>>> Moreover, it is not particularly clear from the current code that, >>>>>> as far as PCI PM is concerned, "platform" really means just ACPI >>>>>> except for the special casess when Intel MID PCI PM is used or when >>>>>> ACPI support is disabled (through the kernel config or command line, >>>>>> or because there are no usable ACPI tables on the system). >>>>>> >>>>>> To address the above, rework the PCI PM code to invoke ACPI PM >>>>>> functions directly as needed and drop the acpi_pci_platform_pm >>>>>> object that is not necessary any more. >>>>>> >>>>>> Accordingly, update some of the ACPI PM functions in question to do >>>>>> extra checks in case the ACPI support is disabled (which previously >>>>>> was taken care of by avoiding to set the pci_platform_ops pointer >>>>>> in those cases). >>>>>> >>>>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com> >>>>>> --- >>>>>> >>>>>> v1 -> v2: >>>>>> * Rebase on top of the new [1/7] and move dropping struct >>>>>> pci_platform_pm_ops to a separate patch. >>>>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't >>>>> apply (after 1/7 applied). Should I apply this on another tree? >>>> This is on top of >>>> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ >>>> >>>> which is not yet in any tree. >>>> >>>> Sorry for the confusion. >>> No problem at all. If I can I will try to report back tonight. Else, >>> will be delayed 2 due to a short break. >> With those 3 extra patches followed by 7 from this series it builds. But >> on boot I get: >> dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core >> Then after this it reboots. Nothing in the logs. Nothing else on >> console, I guess something goes wrong early. > It appears so. > > Can you please try just the 3 extra patches this series is on top of? > The problem is more likely to be located in one of them. Boots fine with just the 3 so up to and including "ACPI: glue: Look for ACPI bus type only if ACPI companion is not known". From the log I get: Intel MID platform detected, using MID PCI ops PCI: Using configuration type 1 for base access .. PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug .. PCI: Probing PCI hardware PCI: root bus 00: using default resources PCI: Probing PCI hardware (bus 00) PCI: pci_cache_line_size set to 64 bytes .. pnp: PnP ACPI init .. pnp: PnP ACPI: found 2 devices .. xhci-hcd xhci-hcd.1.auto: xHCI Host Controller xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 1 xhci-hcd xhci-hcd.1.auto: hcc params 0x0220f06c hci version 0x100 quirks 0x0000000002010010 xhci-hcd xhci-hcd.1.auto: irq 14, io mem 0xf9100000 usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 5.15 usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1 usb usb1: Product: xHCI Host Controller usb usb1: Manufacturer: Linux 5.15.0-rc2-edison-acpi-standard xhci-hcd usb usb1: SerialNumber: xhci-hcd.1.auto hub 1-0:1.0: USB hub found hub 1-0:1.0: 1 port detected I continued up to "PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI", still boots. In the logs I still see "Intel MID platform detected, using MID PCI ops". Unfortunately no more time today, and tomorrow short holiday starts. I will continue after returning next Sat.
On Fri, Sep 24, 2021 at 11:17 PM Ferry Toth <fntoth@gmail.com> wrote: > > Hi > > Op 24-09-2021 om 14:02 schreef Rafael J. Wysocki: > > On Thu, Sep 23, 2021 at 10:32 PM Ferry Toth <fntoth@gmail.com> wrote: > >> Hi > >> > >> Op 23-09-2021 om 15:51 schreef Ferry Toth: > >>> Repost (with formatting removed, sorry for the noise) > >>> Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki: > >>>> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com> wrote: > >>>>> Hi, > >>>>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki: > >>>>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com> > >>>>>> > >>>>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary > >>>>>> indirection to the interactions between the PCI core and ACPI PM, > >>>>>> which is also subject to retpolines. > >>>>>> > >>>>>> Moreover, it is not particularly clear from the current code that, > >>>>>> as far as PCI PM is concerned, "platform" really means just ACPI > >>>>>> except for the special casess when Intel MID PCI PM is used or when > >>>>>> ACPI support is disabled (through the kernel config or command line, > >>>>>> or because there are no usable ACPI tables on the system). > >>>>>> > >>>>>> To address the above, rework the PCI PM code to invoke ACPI PM > >>>>>> functions directly as needed and drop the acpi_pci_platform_pm > >>>>>> object that is not necessary any more. > >>>>>> > >>>>>> Accordingly, update some of the ACPI PM functions in question to do > >>>>>> extra checks in case the ACPI support is disabled (which previously > >>>>>> was taken care of by avoiding to set the pci_platform_ops pointer > >>>>>> in those cases). > >>>>>> > >>>>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com> > >>>>>> --- > >>>>>> > >>>>>> v1 -> v2: > >>>>>> * Rebase on top of the new [1/7] and move dropping struct > >>>>>> pci_platform_pm_ops to a separate patch. > >>>>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't > >>>>> apply (after 1/7 applied). Should I apply this on another tree? > >>>> This is on top of > >>>> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ > >>>> > >>>> which is not yet in any tree. > >>>> > >>>> Sorry for the confusion. > >>> No problem at all. If I can I will try to report back tonight. Else, > >>> will be delayed 2 due to a short break. > >> With those 3 extra patches followed by 7 from this series it builds. But > >> on boot I get: > >> dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core > >> Then after this it reboots. Nothing in the logs. Nothing else on > >> console, I guess something goes wrong early. > > It appears so. > > > > Can you please try just the 3 extra patches this series is on top of? > > The problem is more likely to be located in one of them. > Boots fine with just the 3 so up to and including "ACPI: glue: Look for > ACPI bus type only if ACPI companion is not known". From the log I get: > > > Intel MID platform detected, using MID PCI ops > PCI: Using configuration type 1 for base access > .. > PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" > and report a bug > .. > PCI: Probing PCI hardware > PCI: root bus 00: using default resources > PCI: Probing PCI hardware (bus 00) > PCI: pci_cache_line_size set to 64 bytes > .. > pnp: PnP ACPI init > .. > pnp: PnP ACPI: found 2 devices > .. > xhci-hcd xhci-hcd.1.auto: xHCI Host Controller > xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 1 > xhci-hcd xhci-hcd.1.auto: hcc params 0x0220f06c hci version 0x100 quirks > 0x0000000002010010 > xhci-hcd xhci-hcd.1.auto: irq 14, io mem 0xf9100000 > usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, > bcdDevice= 5.15 > usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > usb usb1: Product: xHCI Host Controller > usb usb1: Manufacturer: Linux 5.15.0-rc2-edison-acpi-standard xhci-hcd > usb usb1: SerialNumber: xhci-hcd.1.auto > hub 1-0:1.0: USB hub found > hub 1-0:1.0: 1 port detected > > I continued up to "PCI: ACPI: PM: Do not use pci_platform_pm_ops for > ACPI", still boots. > > In the logs I still see "Intel MID platform detected, using MID PCI ops". > > Unfortunately no more time today, and tomorrow short holiday starts. I > will continue after returning next Sat. Thanks for the testing and feedback, much appreciated! I'm going to queue up the patches that you have tested with a Tested-by tag from you if that's not an issue. Also patches [3/7] ("PCI: PM: Drop struct pci_platform_pm_ops") and [7/7] ("PCI: PM: Simplify acpi_pci_power_manageable()") are not likely to introduce functional issue, because the former removes unused code and the latter simply rearranges some computations, so I'm going to queue up these two as well. Patches [4-5/7] change behavior, if only slightly, so they need to be double checked. In turn, patch [6/7[ contains a bug - it makes pci_platform_power_transition() call acpi_pci_set_power_state() instead of platform_pci_set_power_state() which is probably why you see the problem (nobody with an ACPI platform of any platform other then MID would see it). Sorry about that. After queuing up the patches that are not problematic I'll prepare a new 3-patch series to test on top of them. Thanks!
On Mon, Sep 20, 2021 at 09:17:08PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Using struct pci_platform_pm_ops for ACPI adds unnecessary > indirection to the interactions between the PCI core and ACPI PM, > which is also subject to retpolines. > > Moreover, it is not particularly clear from the current code that, > as far as PCI PM is concerned, "platform" really means just ACPI > except for the special casess when Intel MID PCI PM is used or when > ACPI support is disabled (through the kernel config or command line, > or because there are no usable ACPI tables on the system). > > To address the above, rework the PCI PM code to invoke ACPI PM > functions directly as needed and drop the acpi_pci_platform_pm > object that is not necessary any more. > > Accordingly, update some of the ACPI PM functions in question to do > extra checks in case the ACPI support is disabled (which previously > was taken care of by avoiding to set the pci_platform_ops pointer > in those cases). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> This patch as commit 9896a58cdd59 ("PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI") in -next causes the following build error when compiling x86_64 allmodconfig with clang: drivers/pci/pci-acpi.c:1125:7: error: variable 'adev' is uninitialized when used here [-Werror,-Wuninitialized] if (!adev || !acpi_device_power_manageable(adev)) ^~~~ drivers/pci/pci-acpi.c:1110:26: note: initialize the variable 'adev' to silence this warning struct acpi_device *adev; ^ = NULL 1 error generated. Should the adev assignment be moved up or is there a different fix necessary? Cheers, Nathan
On Wed, Sep 29, 2021 at 9:08 PM Nathan Chancellor <nathan@kernel.org> wrote: > > On Mon, Sep 20, 2021 at 09:17:08PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Using struct pci_platform_pm_ops for ACPI adds unnecessary > > indirection to the interactions between the PCI core and ACPI PM, > > which is also subject to retpolines. > > > > Moreover, it is not particularly clear from the current code that, > > as far as PCI PM is concerned, "platform" really means just ACPI > > except for the special casess when Intel MID PCI PM is used or when > > ACPI support is disabled (through the kernel config or command line, > > or because there are no usable ACPI tables on the system). > > > > To address the above, rework the PCI PM code to invoke ACPI PM > > functions directly as needed and drop the acpi_pci_platform_pm > > object that is not necessary any more. > > > > Accordingly, update some of the ACPI PM functions in question to do > > extra checks in case the ACPI support is disabled (which previously > > was taken care of by avoiding to set the pci_platform_ops pointer > > in those cases). > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > This patch as commit 9896a58cdd59 ("PCI: ACPI: PM: Do not use > pci_platform_pm_ops for ACPI") in -next causes the following build error > when compiling x86_64 allmodconfig with clang: > > drivers/pci/pci-acpi.c:1125:7: error: variable 'adev' is uninitialized when used here [-Werror,-Wuninitialized] > if (!adev || !acpi_device_power_manageable(adev)) > ^~~~ > drivers/pci/pci-acpi.c:1110:26: note: initialize the variable 'adev' to silence this warning > struct acpi_device *adev; > ^ > = NULL > 1 error generated. > > Should the adev assignment be moved up Yes, thanks! I'll fix it up in the tree.
Index: linux-pm/drivers/pci/pci-acpi.c =================================================================== --- linux-pm.orig/drivers/pci/pci-acpi.c +++ linux-pm/drivers/pci/pci-acpi.c @@ -906,10 +906,13 @@ acpi_status pci_acpi_add_pm_notifier(str * choose highest power _SxD or any lower power */ -static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) +pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) { int acpi_state, d_max; + if (acpi_pci_disabled) + return PCI_POWER_ERROR; + if (pdev->no_d3cold) d_max = ACPI_STATE_D3_HOT; else @@ -965,7 +968,7 @@ int pci_dev_acpi_reset(struct pci_dev *d return 0; } -static bool acpi_pci_power_manageable(struct pci_dev *dev) +bool acpi_pci_power_manageable(struct pci_dev *dev) { struct acpi_device *adev = ACPI_COMPANION(&dev->dev); @@ -974,13 +977,13 @@ static bool acpi_pci_power_manageable(st return acpi_device_power_manageable(adev); } -static bool acpi_pci_bridge_d3(struct pci_dev *dev) +bool acpi_pci_bridge_d3(struct pci_dev *dev) { const union acpi_object *obj; struct acpi_device *adev; struct pci_dev *rpdev; - if (!dev->is_hotplug_bridge) + if (acpi_pci_disabled || !dev->is_hotplug_bridge) return false; /* Assume D3 support if the bridge is power-manageable by ACPI. */ @@ -1008,7 +1011,7 @@ static bool acpi_pci_bridge_d3(struct pc return obj->integer.value == 1; } -static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) +int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) { struct acpi_device *adev = ACPI_COMPANION(&dev->dev); static const u8 state_conv[] = { @@ -1046,7 +1049,7 @@ static int acpi_pci_set_power_state(stru return error; } -static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev) +pci_power_t acpi_pci_get_power_state(struct pci_dev *dev) { struct acpi_device *adev = ACPI_COMPANION(&dev->dev); static const pci_power_t state_conv[] = { @@ -1068,7 +1071,7 @@ static pci_power_t acpi_pci_get_power_st return state_conv[state]; } -static void acpi_pci_refresh_power_state(struct pci_dev *dev) +void acpi_pci_refresh_power_state(struct pci_dev *dev) { struct acpi_device *adev = ACPI_COMPANION(&dev->dev); @@ -1093,17 +1096,23 @@ static int acpi_pci_propagate_wakeup(str return 0; } -static int acpi_pci_wakeup(struct pci_dev *dev, bool enable) +int acpi_pci_wakeup(struct pci_dev *dev, bool enable) { + if (acpi_pci_disabled) + return 0; + if (acpi_pm_device_can_wakeup(&dev->dev)) return acpi_pm_set_device_wakeup(&dev->dev, enable); return acpi_pci_propagate_wakeup(dev->bus, enable); } -static bool acpi_pci_need_resume(struct pci_dev *dev) +bool acpi_pci_need_resume(struct pci_dev *dev) { - struct acpi_device *adev = ACPI_COMPANION(&dev->dev); + struct acpi_device *adev; + + if (acpi_pci_disabled) + return false; /* * In some cases (eg. Samsung 305V4A) leaving a bridge in suspend over @@ -1118,6 +1127,7 @@ static bool acpi_pci_need_resume(struct if (!adev || !acpi_device_power_manageable(adev)) return false; + adev = ACPI_COMPANION(&dev->dev); if (adev->wakeup.flags.valid && device_may_wakeup(&dev->dev) != !!adev->wakeup.prepare_count) return true; @@ -1128,17 +1138,6 @@ static bool acpi_pci_need_resume(struct return !!adev->power.flags.dsw_present; } -static const struct pci_platform_pm_ops acpi_pci_platform_pm = { - .bridge_d3 = acpi_pci_bridge_d3, - .is_manageable = acpi_pci_power_manageable, - .set_state = acpi_pci_set_power_state, - .get_state = acpi_pci_get_power_state, - .refresh_state = acpi_pci_refresh_power_state, - .choose_state = acpi_pci_choose_state, - .set_wakeup = acpi_pci_wakeup, - .need_resume = acpi_pci_need_resume, -}; - void acpi_pci_add_bus(struct pci_bus *bus) { union acpi_object *obj; @@ -1448,7 +1447,6 @@ static int __init acpi_pci_init(void) if (acpi_pci_disabled) return 0; - pci_set_platform_pm(&acpi_pci_platform_pm); acpi_pci_slot_init(); acpiphp_init(); Index: linux-pm/drivers/pci/pci.h =================================================================== --- linux-pm.orig/drivers/pci/pci.h +++ linux-pm/drivers/pci/pci.h @@ -725,17 +725,53 @@ int pci_acpi_program_hp_params(struct pc extern const struct attribute_group pci_dev_acpi_attr_group; void pci_set_acpi_fwnode(struct pci_dev *dev); int pci_dev_acpi_reset(struct pci_dev *dev, bool probe); +bool acpi_pci_power_manageable(struct pci_dev *dev); +bool acpi_pci_bridge_d3(struct pci_dev *dev); +int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state); +pci_power_t acpi_pci_get_power_state(struct pci_dev *dev); +void acpi_pci_refresh_power_state(struct pci_dev *dev); +int acpi_pci_wakeup(struct pci_dev *dev, bool enable); +bool acpi_pci_need_resume(struct pci_dev *dev); +pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); #else static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) { return -ENOTTY; } - static inline void pci_set_acpi_fwnode(struct pci_dev *dev) {} static inline int pci_acpi_program_hp_params(struct pci_dev *dev) { return -ENODEV; } +static inline bool acpi_pci_power_manageable(struct pci_dev *dev) +{ + return false; +} +static inline bool acpi_pci_bridge_d3(struct pci_dev *dev) +{ + return false; +} +static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) +{ + return -ENODEV; +} +static inline pci_power_t acpi_pci_get_power_state(struct pci_dev *dev) +{ + return PCI_UNKNOWN; +} +static inline void acpi_pci_refresh_power_state(struct pci_dev *dev) {} +static inline int acpi_pci_wakeup(struct pci_dev *dev, bool enable) +{ + return -ENODEV; +} +static inline bool acpi_pci_need_resume(struct pci_dev *dev) +{ + return false; +} +static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) +{ + return PCI_POWER_ERROR; +} #endif #ifdef CONFIG_PCIEASPM Index: linux-pm/drivers/pci/pci.c =================================================================== --- linux-pm.orig/drivers/pci/pci.c +++ linux-pm/drivers/pci/pci.c @@ -988,7 +988,7 @@ static inline bool platform_pci_power_ma if (pci_use_mid_pm()) return true; - return pci_platform_pm ? pci_platform_pm->is_manageable(dev) : false; + return acpi_pci_power_manageable(dev); } static inline int platform_pci_set_power_state(struct pci_dev *dev, @@ -997,7 +997,7 @@ static inline int platform_pci_set_power if (pci_use_mid_pm()) return mid_pci_set_power_state(dev, t); - return pci_platform_pm ? pci_platform_pm->set_state(dev, t) : -ENOSYS; + return acpi_pci_set_power_state(dev, t); } static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev) @@ -1005,13 +1005,13 @@ static inline pci_power_t platform_pci_g if (pci_use_mid_pm()) return mid_pci_get_power_state(dev); - return pci_platform_pm ? pci_platform_pm->get_state(dev) : PCI_UNKNOWN; + return acpi_pci_get_power_state(dev); } static inline void platform_pci_refresh_power_state(struct pci_dev *dev) { - if (!pci_use_mid_pm() && pci_platform_pm && pci_platform_pm->refresh_state) - pci_platform_pm->refresh_state(dev); + if (!pci_use_mid_pm()) + acpi_pci_refresh_power_state(dev); } static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev) @@ -1019,8 +1019,7 @@ static inline pci_power_t platform_pci_c if (pci_use_mid_pm()) return PCI_POWER_ERROR; - return pci_platform_pm ? - pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR; + return acpi_pci_choose_state(dev); } static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) @@ -1028,8 +1027,7 @@ static inline int platform_pci_set_wakeu if (pci_use_mid_pm()) return PCI_POWER_ERROR; - return pci_platform_pm ? - pci_platform_pm->set_wakeup(dev, enable) : -ENODEV; + return acpi_pci_wakeup(dev, enable); } static inline bool platform_pci_need_resume(struct pci_dev *dev) @@ -1037,7 +1035,7 @@ static inline bool platform_pci_need_res if (pci_use_mid_pm()) return false; - return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false; + return acpi_pci_need_resume(dev); } static inline bool platform_pci_bridge_d3(struct pci_dev *dev) @@ -1045,9 +1043,7 @@ static inline bool platform_pci_bridge_d if (pci_use_mid_pm()) return false; - if (pci_platform_pm && pci_platform_pm->bridge_d3) - return pci_platform_pm->bridge_d3(dev); - return false; + return acpi_pci_bridge_d3(dev); } /**