| Message ID | 20210225174041.405739-3-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] PCI: Introduce quirk hook after driver shutdown callback | expand |
Kai-Heng Feng <kai.heng.feng@canonical.com> writes: > Now we have a generic D3 shutdown quirk, so convert the original > approach to a PCI quirk. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/net/wireless/realtek/rtw88/pci.c | 2 -- > drivers/pci/quirks.c | 6 ++++++ > 2 files changed, 6 insertions(+), 2 deletions(-) It would have been nice to CC linux-wireless also on patches 1-2. I only saw patch 3 and had to search the rest of patches from lkml. I assume this goes via the PCI tree so: Acked-by: Kalle Valo <kvalo@codeaurora.org>
On 26.02.2021 08:12, Kalle Valo wrote: > Kai-Heng Feng <kai.heng.feng@canonical.com> writes: > >> Now we have a generic D3 shutdown quirk, so convert the original >> approach to a PCI quirk. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> drivers/net/wireless/realtek/rtw88/pci.c | 2 -- >> drivers/pci/quirks.c | 6 ++++++ >> 2 files changed, 6 insertions(+), 2 deletions(-) > > It would have been nice to CC linux-wireless also on patches 1-2. I only > saw patch 3 and had to search the rest of patches from lkml. > > I assume this goes via the PCI tree so: > > Acked-by: Kalle Valo <kvalo@codeaurora.org> > To me it looks odd to (mis-)use the quirk mechanism to set a device to D3cold on shutdown. As I see it the quirk mechanism is used to work around certain device misbehavior. And setting a device to a D3 state on shutdown is a normal activity, and the shutdown() callback seems to be a good place for it. I miss an explanation what the actual benefit of the change is.
On Fri, Feb 26, 2021 at 8:10 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 26.02.2021 08:12, Kalle Valo wrote: > > Kai-Heng Feng <kai.heng.feng@canonical.com> writes: > > > >> Now we have a generic D3 shutdown quirk, so convert the original > >> approach to a PCI quirk. > >> > >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > >> --- > >> drivers/net/wireless/realtek/rtw88/pci.c | 2 -- > >> drivers/pci/quirks.c | 6 ++++++ > >> 2 files changed, 6 insertions(+), 2 deletions(-) > > > > It would have been nice to CC linux-wireless also on patches 1-2. I only > > saw patch 3 and had to search the rest of patches from lkml. > > > > I assume this goes via the PCI tree so: > > > > Acked-by: Kalle Valo <kvalo@codeaurora.org> > > > > To me it looks odd to (mis-)use the quirk mechanism to set a device > to D3cold on shutdown. As I see it the quirk mechanism is used to work > around certain device misbehavior. And setting a device to a D3 > state on shutdown is a normal activity, and the shutdown() callback > seems to be a good place for it. > I miss an explanation what the actual benefit of the change is. To make putting device to D3 more generic, as there are more than one device need the quirk. Here's the discussion: https://lore.kernel.org/linux-usb/00de6927-3fa6-a9a3-2d65-2b4d4e8f0012@linux.intel.com/ Kai-Heng
On 26.02.2021 13:18, Kai-Heng Feng wrote: > On Fri, Feb 26, 2021 at 8:10 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >> On 26.02.2021 08:12, Kalle Valo wrote: >>> Kai-Heng Feng <kai.heng.feng@canonical.com> writes: >>> >>>> Now we have a generic D3 shutdown quirk, so convert the original >>>> approach to a PCI quirk. >>>> >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> --- >>>> drivers/net/wireless/realtek/rtw88/pci.c | 2 -- >>>> drivers/pci/quirks.c | 6 ++++++ >>>> 2 files changed, 6 insertions(+), 2 deletions(-) >>> >>> It would have been nice to CC linux-wireless also on patches 1-2. I only >>> saw patch 3 and had to search the rest of patches from lkml. >>> >>> I assume this goes via the PCI tree so: >>> >>> Acked-by: Kalle Valo <kvalo@codeaurora.org> >>> >> >> To me it looks odd to (mis-)use the quirk mechanism to set a device >> to D3cold on shutdown. As I see it the quirk mechanism is used to work >> around certain device misbehavior. And setting a device to a D3 >> state on shutdown is a normal activity, and the shutdown() callback >> seems to be a good place for it. >> I miss an explanation what the actual benefit of the change is. > > To make putting device to D3 more generic, as there are more than one > device need the quirk. > > Here's the discussion: > https://lore.kernel.org/linux-usb/00de6927-3fa6-a9a3-2d65-2b4d4e8f0012@linux.intel.com/ > Thanks for the link. For the AMD USB use case I don't have a strong opinion, what's considered the better option may be a question of personal taste. For rtw88 however I'd still consider it over-engineering to replace a simple call to pci_set_power_state() with a PCI quirk. I may be biased here because I find it sometimes bothering if I want to look up how a device is handled and in addition to checking the respective driver I also have to grep through quirks.c whether there's any special handling. > Kai-Heng > Heiner
Heiner Kallweit <hkallweit1@gmail.com> writes: > On 26.02.2021 13:18, Kai-Heng Feng wrote: >> On Fri, Feb 26, 2021 at 8:10 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >>> >>> On 26.02.2021 08:12, Kalle Valo wrote: >>>> Kai-Heng Feng <kai.heng.feng@canonical.com> writes: >>>> >>>>> Now we have a generic D3 shutdown quirk, so convert the original >>>>> approach to a PCI quirk. >>>>> >>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>>> --- >>>>> drivers/net/wireless/realtek/rtw88/pci.c | 2 -- >>>>> drivers/pci/quirks.c | 6 ++++++ >>>>> 2 files changed, 6 insertions(+), 2 deletions(-) >>>> >>>> It would have been nice to CC linux-wireless also on patches 1-2. I only >>>> saw patch 3 and had to search the rest of patches from lkml. >>>> >>>> I assume this goes via the PCI tree so: >>>> >>>> Acked-by: Kalle Valo <kvalo@codeaurora.org> >>>> >>> >>> To me it looks odd to (mis-)use the quirk mechanism to set a device >>> to D3cold on shutdown. As I see it the quirk mechanism is used to work >>> around certain device misbehavior. And setting a device to a D3 >>> state on shutdown is a normal activity, and the shutdown() callback >>> seems to be a good place for it. >>> I miss an explanation what the actual benefit of the change is. >> >> To make putting device to D3 more generic, as there are more than one >> device need the quirk. >> >> Here's the discussion: >> https://lore.kernel.org/linux-usb/00de6927-3fa6-a9a3-2d65-2b4d4e8f0012@linux.intel.com/ >> > > Thanks for the link. For the AMD USB use case I don't have a strong opinion, > what's considered the better option may be a question of personal taste. > For rtw88 however I'd still consider it over-engineering to replace a simple > call to pci_set_power_state() with a PCI quirk. > I may be biased here because I find it sometimes bothering if I want to > look up how a device is handled and in addition to checking the respective > driver I also have to grep through quirks.c whether there's any special > handling. Good point about rtw88. And if there's a new PCI id for rtw88 we need to also update the quirk in the PCI subsystem, which will be most likely forgotten.
On Fri, Feb 26, 2021 at 02:31:31PM +0100, Heiner Kallweit wrote: > On 26.02.2021 13:18, Kai-Heng Feng wrote: > > On Fri, Feb 26, 2021 at 8:10 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> > >> On 26.02.2021 08:12, Kalle Valo wrote: > >>> Kai-Heng Feng <kai.heng.feng@canonical.com> writes: > >>> > >>>> Now we have a generic D3 shutdown quirk, so convert the original > >>>> approach to a PCI quirk. > >>>> > >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > >>>> --- > >>>> drivers/net/wireless/realtek/rtw88/pci.c | 2 -- > >>>> drivers/pci/quirks.c | 6 ++++++ > >>>> 2 files changed, 6 insertions(+), 2 deletions(-) > >>> > >>> It would have been nice to CC linux-wireless also on patches 1-2. I only > >>> saw patch 3 and had to search the rest of patches from lkml. > >>> > >>> I assume this goes via the PCI tree so: > >>> > >>> Acked-by: Kalle Valo <kvalo@codeaurora.org> > >> > >> To me it looks odd to (mis-)use the quirk mechanism to set a device > >> to D3cold on shutdown. As I see it the quirk mechanism is used to work > >> around certain device misbehavior. And setting a device to a D3 > >> state on shutdown is a normal activity, and the shutdown() callback > >> seems to be a good place for it. > >> I miss an explanation what the actual benefit of the change is. > > > > To make putting device to D3 more generic, as there are more than one > > device need the quirk. > > > > Here's the discussion: > > https://lore.kernel.org/linux-usb/00de6927-3fa6-a9a3-2d65-2b4d4e8f0012@linux.intel.com/ > > > > Thanks for the link. For the AMD USB use case I don't have a strong opinion, > what's considered the better option may be a question of personal taste. > For rtw88 however I'd still consider it over-engineering to replace a simple > call to pci_set_power_state() with a PCI quirk. > I may be biased here because I find it sometimes bothering if I want to > look up how a device is handled and in addition to checking the respective > driver I also have to grep through quirks.c whether there's any special > handling. I haven't looked at these patches carefully, but in general, I agree that quirks should be used to work around hardware defects in the device. If the device behaves correctly per spec, we should use a different mechanism so the code remains generic and all devices get the benefit. If we do add quirks, the commit log should explain what the device defect is. Bjorn
On Sat, Feb 27, 2021 at 2:17 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Feb 26, 2021 at 02:31:31PM +0100, Heiner Kallweit wrote: > > On 26.02.2021 13:18, Kai-Heng Feng wrote: > > > On Fri, Feb 26, 2021 at 8:10 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > >> > > >> On 26.02.2021 08:12, Kalle Valo wrote: > > >>> Kai-Heng Feng <kai.heng.feng@canonical.com> writes: > > >>> > > >>>> Now we have a generic D3 shutdown quirk, so convert the original > > >>>> approach to a PCI quirk. > > >>>> > > >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > >>>> --- > > >>>> drivers/net/wireless/realtek/rtw88/pci.c | 2 -- > > >>>> drivers/pci/quirks.c | 6 ++++++ > > >>>> 2 files changed, 6 insertions(+), 2 deletions(-) > > >>> > > >>> It would have been nice to CC linux-wireless also on patches 1-2. I only > > >>> saw patch 3 and had to search the rest of patches from lkml. > > >>> > > >>> I assume this goes via the PCI tree so: > > >>> > > >>> Acked-by: Kalle Valo <kvalo@codeaurora.org> > > >> > > >> To me it looks odd to (mis-)use the quirk mechanism to set a device > > >> to D3cold on shutdown. As I see it the quirk mechanism is used to work > > >> around certain device misbehavior. And setting a device to a D3 > > >> state on shutdown is a normal activity, and the shutdown() callback > > >> seems to be a good place for it. > > >> I miss an explanation what the actual benefit of the change is. > > > > > > To make putting device to D3 more generic, as there are more than one > > > device need the quirk. > > > > > > Here's the discussion: > > > https://lore.kernel.org/linux-usb/00de6927-3fa6-a9a3-2d65-2b4d4e8f0012@linux.intel.com/ > > > > > > > Thanks for the link. For the AMD USB use case I don't have a strong opinion, > > what's considered the better option may be a question of personal taste. > > For rtw88 however I'd still consider it over-engineering to replace a simple > > call to pci_set_power_state() with a PCI quirk. > > I may be biased here because I find it sometimes bothering if I want to > > look up how a device is handled and in addition to checking the respective > > driver I also have to grep through quirks.c whether there's any special > > handling. > > I haven't looked at these patches carefully, but in general, I agree > that quirks should be used to work around hardware defects in the > device. If the device behaves correctly per spec, we should use a > different mechanism so the code remains generic and all devices get > the benefit. > > If we do add quirks, the commit log should explain what the device > defect is. So maybe it's reasonable to put all PCI devices to D3 at shutdown? Kai-Heng > > Bjorn
[+cc Rafael, linux-pm] On Thu, Mar 04, 2021 at 02:07:18PM +0800, Kai-Heng Feng wrote: > On Sat, Feb 27, 2021 at 2:17 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Feb 26, 2021 at 02:31:31PM +0100, Heiner Kallweit wrote: > > > On 26.02.2021 13:18, Kai-Heng Feng wrote: > > > > On Fri, Feb 26, 2021 at 8:10 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > > >> > > > >> On 26.02.2021 08:12, Kalle Valo wrote: > > > >>> Kai-Heng Feng <kai.heng.feng@canonical.com> writes: > > > >>> > > > >>>> Now we have a generic D3 shutdown quirk, so convert the original > > > >>>> approach to a PCI quirk. > > > >>>> > > > >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > >>>> --- > > > >>>> drivers/net/wireless/realtek/rtw88/pci.c | 2 -- > > > >>>> drivers/pci/quirks.c | 6 ++++++ > > > >>>> 2 files changed, 6 insertions(+), 2 deletions(-) > > > >>> > > > >>> It would have been nice to CC linux-wireless also on patches 1-2. I only > > > >>> saw patch 3 and had to search the rest of patches from lkml. > > > >>> > > > >>> I assume this goes via the PCI tree so: > > > >>> > > > >>> Acked-by: Kalle Valo <kvalo@codeaurora.org> > > > >> > > > >> To me it looks odd to (mis-)use the quirk mechanism to set a device > > > >> to D3cold on shutdown. As I see it the quirk mechanism is used to work > > > >> around certain device misbehavior. And setting a device to a D3 > > > >> state on shutdown is a normal activity, and the shutdown() callback > > > >> seems to be a good place for it. > > > >> I miss an explanation what the actual benefit of the change is. > > > > > > > > To make putting device to D3 more generic, as there are more than one > > > > device need the quirk. > > > > > > > > Here's the discussion: > > > > https://lore.kernel.org/linux-usb/00de6927-3fa6-a9a3-2d65-2b4d4e8f0012@linux.intel.com/ > > > > > > > > > > Thanks for the link. For the AMD USB use case I don't have a strong opinion, > > > what's considered the better option may be a question of personal taste. > > > For rtw88 however I'd still consider it over-engineering to replace a simple > > > call to pci_set_power_state() with a PCI quirk. > > > I may be biased here because I find it sometimes bothering if I want to > > > look up how a device is handled and in addition to checking the respective > > > driver I also have to grep through quirks.c whether there's any special > > > handling. > > > > I haven't looked at these patches carefully, but in general, I agree > > that quirks should be used to work around hardware defects in the > > device. If the device behaves correctly per spec, we should use a > > different mechanism so the code remains generic and all devices get > > the benefit. > > > > If we do add quirks, the commit log should explain what the device > > defect is. > > So maybe it's reasonable to put all PCI devices to D3 at shutdown? I don't know off-hand. I added Rafael and linux-pm in case they do. If not, I suggest working up a patch to do that and a commit log that explains why that's a good idea and then we can have a discussion about it. This thread really doesn't have that justification. It says "putting device X in D3cold at shutdown saves 0.03w while in S5", but doesn't explain why that's safe or desirable for all devices. Bjorn
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index 786a48649946..cddc9b09bb1f 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -1709,8 +1709,6 @@ void rtw_pci_shutdown(struct pci_dev *pdev) if (chip->ops->shutdown) chip->ops->shutdown(rtwdev); - - pci_set_power_state(pdev, PCI_D3hot); } EXPORT_SYMBOL(rtw_pci_shutdown); diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 0a848ef0b7db..dfb8746e3b72 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5627,3 +5627,9 @@ static void pci_fixup_shutdown_d3(struct pci_dev *pdev) pci_set_power_state(pdev, PCI_D3cold); } DECLARE_PCI_FIXUP_SHUTDOWN(PCI_VENDOR_ID_AMD, 0x1639, pci_fixup_shutdown_d3); +DECLARE_PCI_FIXUP_SHUTDOWN(PCI_VENDOR_ID_REALTEK, 0xd723, pci_fixup_shutdown_d3); +DECLARE_PCI_FIXUP_SHUTDOWN(PCI_VENDOR_ID_REALTEK, 0xc821, pci_fixup_shutdown_d3); +DECLARE_PCI_FIXUP_SHUTDOWN(PCI_VENDOR_ID_REALTEK, 0xb822, pci_fixup_shutdown_d3); +DECLARE_PCI_FIXUP_SHUTDOWN(PCI_VENDOR_ID_REALTEK, 0xb822, pci_fixup_shutdown_d3); +DECLARE_PCI_FIXUP_SHUTDOWN(PCI_VENDOR_ID_REALTEK, 0xc822, pci_fixup_shutdown_d3); +DECLARE_PCI_FIXUP_SHUTDOWN(PCI_VENDOR_ID_REALTEK, 0xc82f, pci_fixup_shutdown_d3);
Now we have a generic D3 shutdown quirk, so convert the original approach to a PCI quirk. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/net/wireless/realtek/rtw88/pci.c | 2 -- drivers/pci/quirks.c | 6 ++++++ 2 files changed, 6 insertions(+), 2 deletions(-)