linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] PCI: Introduce quirk hook after driver shutdown callback
@ 2021-02-25 17:40 Kai-Heng Feng
  2021-02-25 17:40 ` [PATCH 2/3] PCI: Set AMD Renoir USB controller to D3 when shutdown Kai-Heng Feng
  2021-02-25 17:40 ` [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk Kai-Heng Feng
  0 siblings, 2 replies; 11+ messages in thread
From: Kai-Heng Feng @ 2021-02-25 17:40 UTC (permalink / raw)
  To: bhelgaas
  Cc: Kai-Heng Feng, Aaron Ma, Arnd Bergmann, open list:PCI SUBSYSTEM,
	open list, open list:GENERIC INCLUDE/ASM HEADER FILES

It can be useful to apply quirk after device shutdown callback, like
putting device into a different power state.

This will be used by later patches.

Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pci-driver.c          | 2 ++
 drivers/pci/quirks.c              | 7 +++++++
 include/asm-generic/vmlinux.lds.h | 3 +++
 include/linux/pci.h               | 4 ++++
 4 files changed, 16 insertions(+)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index ec44a79e951a..7941f6190815 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -498,6 +498,8 @@ static void pci_device_shutdown(struct device *dev)
 	 */
 	if (kexec_in_progress && (pci_dev->current_state <= PCI_D3hot))
 		pci_clear_master(pci_dev);
+
+	pci_fixup_device(pci_fixup_shutdown, pci_dev);
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..1f94fafc6920 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -93,6 +93,8 @@ extern struct pci_fixup __start_pci_fixups_suspend[];
 extern struct pci_fixup __end_pci_fixups_suspend[];
 extern struct pci_fixup __start_pci_fixups_suspend_late[];
 extern struct pci_fixup __end_pci_fixups_suspend_late[];
+extern struct pci_fixup __start_pci_fixups_shutdown[];
+extern struct pci_fixup __end_pci_fixups_shutdown[];
 
 static bool pci_apply_fixup_final_quirks;
 
@@ -143,6 +145,11 @@ void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev)
 		end = __end_pci_fixups_suspend_late;
 		break;
 
+	case pci_fixup_shutdown:
+		start = __start_pci_fixups_shutdown;
+		end = __end_pci_fixups_shutdown;
+		break;
+
 	default:
 		/* stupid compiler warning, you would think with an enum... */
 		return;
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index c54adce8f6f6..aba43fc2f7b1 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -472,6 +472,9 @@
 		__start_pci_fixups_suspend_late = .;			\
 		KEEP(*(.pci_fixup_suspend_late))			\
 		__end_pci_fixups_suspend_late = .;			\
+		__start_pci_fixups_shutdown = .;			\
+		KEEP(*(.pci_fixup_shutdown))				\
+		__end_pci_fixups_shutdown = .;				\
 	}								\
 									\
 	/* Built-in firmware blobs */					\
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 86c799c97b77..7cbe9b21e049 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1923,6 +1923,7 @@ enum pci_fixup_pass {
 	pci_fixup_suspend,	/* pci_device_suspend() */
 	pci_fixup_resume_early, /* pci_device_resume_early() */
 	pci_fixup_suspend_late,	/* pci_device_suspend_late() */
+	pci_fixup_shutdown,	/* pci_device_shutdown() */
 };
 
 #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
@@ -2028,6 +2029,9 @@ enum pci_fixup_pass {
 #define DECLARE_PCI_FIXUP_SUSPEND_LATE(vendor, device, hook)		\
 	DECLARE_PCI_FIXUP_SECTION(.pci_fixup_suspend_late,		\
 		suspend_late##hook, vendor, device, PCI_ANY_ID, 0, hook)
+#define DECLARE_PCI_FIXUP_SHUTDOWN(vendor, device, hook)		\
+	DECLARE_PCI_FIXUP_SECTION(.pci_fixup_shutdown,			\
+		shutdown##hook, vendor, device, PCI_ANY_ID, 0, hook)
 
 #ifdef CONFIG_PCI_QUIRKS
 void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev);
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] PCI: Set AMD Renoir USB controller to D3 when shutdown
  2021-02-25 17:40 [PATCH 1/3] PCI: Introduce quirk hook after driver shutdown callback Kai-Heng Feng
@ 2021-02-25 17:40 ` Kai-Heng Feng
  2021-02-25 17:40 ` [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk Kai-Heng Feng
  1 sibling, 0 replies; 11+ messages in thread
From: Kai-Heng Feng @ 2021-02-25 17:40 UTC (permalink / raw)
  To: bhelgaas; +Cc: Aaron Ma, Kai-Heng Feng, open list:PCI SUBSYSTEM, open list

From: Aaron Ma <aaron.ma@canonical.com>

On AMD Renoir/Cezanne platforms, when set "Always on USB" to "On" in BIOS,
USB controller will consume more power than 0.03w.

Set it to D3cold when shutdown, S5 power consumption will be 0.03w lower.
The USB can charge other devices as before.
USB controller works fine after power on and reboot.

Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/quirks.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 1f94fafc6920..0a848ef0b7db 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -28,6 +28,7 @@
 #include <linux/platform_data/x86/apple.h>
 #include <linux/pm_runtime.h>
 #include <linux/switchtec.h>
+#include <linux/kexec.h>
 #include <asm/dma.h>	/* isa_dma_bridge_buggy */
 #include "pci.h"
 
@@ -5619,3 +5620,10 @@ static void apex_pci_fixup_class(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_CLASS_HEADER(0x1ac1, 0x089a,
 			       PCI_CLASS_NOT_DEFINED, 8, apex_pci_fixup_class);
+
+static void pci_fixup_shutdown_d3(struct pci_dev *pdev)
+{
+	if (!kexec_in_progress)
+		pci_set_power_state(pdev, PCI_D3cold);
+}
+DECLARE_PCI_FIXUP_SHUTDOWN(PCI_VENDOR_ID_AMD, 0x1639, pci_fixup_shutdown_d3);
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
  2021-02-25 17:40 [PATCH 1/3] PCI: Introduce quirk hook after driver shutdown callback Kai-Heng Feng
  2021-02-25 17:40 ` [PATCH 2/3] PCI: Set AMD Renoir USB controller to D3 when shutdown Kai-Heng Feng
@ 2021-02-25 17:40 ` Kai-Heng Feng
  2021-02-26  7:12   ` Kalle Valo
  1 sibling, 1 reply; 11+ messages in thread
From: Kai-Heng Feng @ 2021-02-25 17:40 UTC (permalink / raw)
  To: bhelgaas
  Cc: Kai-Heng Feng, Yan-Hsuan Chuang, Kalle Valo, David S. Miller,
	Jakub Kicinski, open list:REALTEK WIRELESS DRIVER (rtw88),
	open list:NETWORKING DRIVERS, open list, open list:PCI SUBSYSTEM

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(-)

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);
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
  2021-02-25 17:40 ` [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk Kai-Heng Feng
@ 2021-02-26  7:12   ` Kalle Valo
  2021-02-26 12:10     ` Heiner Kallweit
  0 siblings, 1 reply; 11+ messages in thread
From: Kalle Valo @ 2021-02-26  7:12 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: bhelgaas, Yan-Hsuan Chuang, David S. Miller, Jakub Kicinski,
	linux-wireless, netdev, open list, linux-pci

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>

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
  2021-02-26  7:12   ` Kalle Valo
@ 2021-02-26 12:10     ` Heiner Kallweit
  2021-02-26 12:18       ` Kai-Heng Feng
  0 siblings, 1 reply; 11+ messages in thread
From: Heiner Kallweit @ 2021-02-26 12:10 UTC (permalink / raw)
  To: Kalle Valo, Kai-Heng Feng
  Cc: bhelgaas, Yan-Hsuan Chuang, David S. Miller, Jakub Kicinski,
	linux-wireless, netdev, open list, linux-pci

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
  2021-02-26 12:10     ` Heiner Kallweit
@ 2021-02-26 12:18       ` Kai-Heng Feng
  2021-02-26 13:31         ` Heiner Kallweit
  0 siblings, 1 reply; 11+ messages in thread
From: Kai-Heng Feng @ 2021-02-26 12:18 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Kalle Valo, Bjorn Helgaas, Yan-Hsuan Chuang, David S. Miller,
	Jakub Kicinski, linux-wireless, Linux Netdev List, open list,
	Linux PCI

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
  2021-02-26 12:18       ` Kai-Heng Feng
@ 2021-02-26 13:31         ` Heiner Kallweit
  2021-02-26 13:40           ` Kalle Valo
  2021-02-26 18:16           ` Bjorn Helgaas
  0 siblings, 2 replies; 11+ messages in thread
From: Heiner Kallweit @ 2021-02-26 13:31 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: Kalle Valo, Bjorn Helgaas, Yan-Hsuan Chuang, David S. Miller,
	Jakub Kicinski, linux-wireless, Linux Netdev List, open list,
	Linux PCI

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
  2021-02-26 13:31         ` Heiner Kallweit
@ 2021-02-26 13:40           ` Kalle Valo
  2021-02-26 18:16           ` Bjorn Helgaas
  1 sibling, 0 replies; 11+ messages in thread
From: Kalle Valo @ 2021-02-26 13:40 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Kai-Heng Feng, Bjorn Helgaas, Yan-Hsuan Chuang, David S. Miller,
	Jakub Kicinski, linux-wireless, Linux Netdev List, open list,
	Linux PCI

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
  2021-02-26 13:31         ` Heiner Kallweit
  2021-02-26 13:40           ` Kalle Valo
@ 2021-02-26 18:16           ` Bjorn Helgaas
  2021-03-04  6:07             ` Kai-Heng Feng
  1 sibling, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2021-02-26 18:16 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Kai-Heng Feng, Kalle Valo, Bjorn Helgaas, Yan-Hsuan Chuang,
	David S. Miller, Jakub Kicinski, linux-wireless,
	Linux Netdev List, open list, Linux PCI

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
  2021-02-26 18:16           ` Bjorn Helgaas
@ 2021-03-04  6:07             ` Kai-Heng Feng
  2021-03-04 16:02               ` Bjorn Helgaas
  0 siblings, 1 reply; 11+ messages in thread
From: Kai-Heng Feng @ 2021-03-04  6:07 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Heiner Kallweit, Kalle Valo, Bjorn Helgaas, Yan-Hsuan Chuang,
	David S. Miller, Jakub Kicinski, linux-wireless,
	Linux Netdev List, open list, Linux PCI

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
  2021-03-04  6:07             ` Kai-Heng Feng
@ 2021-03-04 16:02               ` Bjorn Helgaas
  0 siblings, 0 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2021-03-04 16:02 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: Heiner Kallweit, Kalle Valo, Bjorn Helgaas, Yan-Hsuan Chuang,
	David S. Miller, Jakub Kicinski, linux-wireless,
	Linux Netdev List, open list, Linux PCI, Rafael J. Wysocki,
	linux-pm

[+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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-03-04 16:04 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-25 17:40 [PATCH 1/3] PCI: Introduce quirk hook after driver shutdown callback Kai-Heng Feng
2021-02-25 17:40 ` [PATCH 2/3] PCI: Set AMD Renoir USB controller to D3 when shutdown Kai-Heng Feng
2021-02-25 17:40 ` [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk Kai-Heng Feng
2021-02-26  7:12   ` Kalle Valo
2021-02-26 12:10     ` Heiner Kallweit
2021-02-26 12:18       ` Kai-Heng Feng
2021-02-26 13:31         ` Heiner Kallweit
2021-02-26 13:40           ` Kalle Valo
2021-02-26 18:16           ` Bjorn Helgaas
2021-03-04  6:07             ` Kai-Heng Feng
2021-03-04 16:02               ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).