| Message ID | 20210225174041.405739-3-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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.
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. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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(-)