[PATCH] ALSA: hda/realtek: Enable mute/micmute LEDs and limit mic boost on EliteBook 845 G8
On HP EliteBook 845 G8, the audio LEDs can be enabled by
ALC285_FIXUP_HP_MUTE_LED. So use it accordingly.
In addition to that, the mic captures lots of noises, so also limits the
mic boost. The quality of capture audio becomes crystal clear after
limiting the mic boost.
Signed-off-by: Kai-Heng Feng
---
sound/pci/hda/patch_realtek.c | 8
1 file changed, 8 insertions(+)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index a7544b77d3f7..c71f1f2b6a8c 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6427,6 +6427,7 @@ enum {
ALC282_FIXUP_ACER_DISABLE_LINEOUT,
ALC255_FIXUP_ACER_LIMIT_INT_MIC_BOOST,
ALC256_FIXUP_ACER_HEADSET_MIC,
+ ALC285_FIXUP_HP_LIMIT_INT_MIC_BOOST,
};
static const struct hda_fixup alc269_fixups[] = {
@@ -7901,6 +7902,12 @@ static const struct hda_fixup alc269_fixups[] = {
.chained = true,
.chain_id = ALC269_FIXUP_HEADSET_MODE_NO_HP_MIC
},
+ [ALC285_FIXUP_HP_LIMIT_INT_MIC_BOOST] = {
+ .type = HDA_FIXUP_FUNC,
+ .v.func = alc269_fixup_limit_int_mic_boost,
+ .chained = true,
+ .chain_id = ALC285_FIXUP_HP_MUTE_LED,
+ },
};
static const struct snd_pci_quirk alc269_fixup_tbl[] = {
@@ -8079,6 +8086,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x103c, 0x87f7, "HP Spectre x360 14",
ALC245_FIXUP_HP_X360_AMP),
SND_PCI_QUIRK(0x103c, 0x8846, "HP EliteBook 850 G8 Notebook PC",
ALC285_FIXUP_HP_GPIO_LED),
SND_PCI_QUIRK(0x103c, 0x884c, "HP EliteBook 840 G8 Notebook PC",
ALC285_FIXUP_HP_GPIO_LED),
+ SND_PCI_QUIRK(0x103c, 0x8898, "HP EliteBook 845 G8 Notebook PC",
ALC285_FIXUP_HP_LIMIT_INT_MIC_BOOST),
SND_PCI_QUIRK(0x1043, 0x103e, "ASUS X540SA", ALC256_FIXUP_ASUS_MIC),
SND_PCI_QUIRK(0x1043, 0x103f, "ASUS TX300", ALC282_FIXUP_ASUS_TX300),
SND_PCI_QUIRK(0x1043, 0x106d, "Asus K53BE",
ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
--
2.30.2
Re: [PATCH] efifb: Fix runtime pm calls for non PCI efifb device
Hi Sudeep,
On Tue, Apr 20, 2021 at 3:53 PM Sudeep Holla wrote:
>
> Gentle Ping! There is boot failure because of this issue with linux-next
> on few arm platforms with non PCIe efifb. Please review and get the fix
> merged ASAP so the testing on these platforms can continue with linux-next.
It was merged in drm-tip as d510c88cfbb2 ("efifb: Check efifb_pci_dev
before using it").
Kai-Heng
>
> On Thu, Apr 15, 2021 at 11:22:24AM +0100, Sudeep Holla wrote:
> > Commit a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI
> > D0")
> > added runtime pm calls to probe and remove routines to ensure the PCI
> > device for efifb stays in D0 state. However not ever efifb is based on
> > PCI device and efifb_pci_dev can be NULL if that is the case.
> >
> > In such cases, we will get a boot splat like below due to NULL dereference:
> > -->8
> > Console: switching to colour frame buffer device 240x67
> > fb0: EFI VGA frame buffer device
> > Unable to handle kernel NULL pointer dereference at virtual address
> > 0270
> > Mem abort info:
> >ESR = 0x9604
> >EC = 0x25: DABT (current EL), IL = 32 bits
> >SET = 0, FnV = 0
> >EA = 0, S1PTW = 0
> > Data abort info:
> >ISV = 0, ISS = 0x0004
> >CM = 0, WnR = 0
> > [0270] user address but active_mm is swapper
> > Internal error: Oops: 9604 [#1] PREEMPT SMP
> > Modules linked in:
> > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc7-next-20210413 #1
> > Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development
> > Platform
> > pstate: 6005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
> > pc : pm_runtime_drop_link+0x12c/0x338
> > lr : efifb_probe+0x7bc/0x7f0
> > Call trace:
> > pm_runtime_drop_link+0x12c/0x338
> > efifb_probe+0x7bc/0x7f0
> > platform_probe+0x68/0xd8
> > really_probe+0xe4/0x3a8
> > driver_probe_device+0x64/0xc8
> > device_driver_attach+0x74/0x80
> > __driver_attach+0x64/0xf0
> > bus_for_each_dev+0x70/0xc0
> > driver_attach+0x24/0x30
> > bus_add_driver+0x150/0x1f8
> > driver_register+0x64/0x120
> > __platform_driver_register+0x28/0x38
> > efifb_driver_init+0x1c/0x28
> > do_one_initcall+0x48/0x2b0
> > kernel_init_freeable+0x1e8/0x258
> > kernel_init+0x14/0x118
> > ret_from_fork+0x10/0x30
> > Code: 88027c01 35a2 17fff706 f9800051 (885f7c40)
> > ---[ end trace 17d8da630bf8ff77 ]---
> > Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
> > -->8
> >
> > Fix the issue by checking for non-NULL efifb_pci_dev before dereferencing
> > for runtime pm calls in probe and remove routines.
> >
> > Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI
> > D0")
> > Cc: Kai-Heng Feng
> > Cc: Alex Deucher
> > Cc: Thomas Zimmermann
> > Cc: Peter Jones
> > Signed-off-by: Sudeep Holla
> > ---
> > drivers/video/fbdev/efifb.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > index f58a545b3bf3..8ea8f079cde2 100644
> > --- a/drivers/video/fbdev/efifb.c
> > +++ b/drivers/video/fbdev/efifb.c
> > @@ -575,7 +575,8 @@ static int efifb_probe(struct platform_device *dev)
> > goto err_fb_dealoc;
> > }
> > fb_info(info, "%s frame buffer device\n", info->fix.id);
> > - pm_runtime_get_sync(&efifb_pci_dev->dev);
> > + if (efifb_pci_dev)
> > + pm_runtime_get_sync(&efifb_pci_dev->dev);
> > return 0;
> >
> > err_fb_dealoc:
> > @@ -602,7 +603,8 @@ static int efifb_remove(struct platform_device *pdev)
> > unregister_framebuffer(info);
> > sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
> > framebuffer_release(info);
> > - pm_runtime_put(&efifb_pci_dev->dev);
> > + if (efifb_pci_dev)
> > + pm_runtime_put(&efifb_pci_dev->dev);
> >
> > return 0;
> > }
> > --
> > 2.25.1
> >
>
> --
> Regards,
> Sudeep
Re: [PATCH] ACPI: PM: s2idle: Invoke _PTS for s2idle
On Mon, Apr 19, 2021 at 7:35 PM Rafael J. Wysocki wrote:
>
> On Mon, Apr 19, 2021 at 11:08 AM Kai-Heng Feng
> wrote:
> >
> > HP EliteBook 840 G8 reboots on s2idle resume, and HP EliteBook 845 G8
> > wakes up immediately on s2idle. Both are caused by the XMM7360 WWAN PCI
> > card.
> >
> > There's a WWAN specific method to really turn off the WWAN via EC:
> > Method (_PTS, 1, NotSerialized) // _PTS: Prepare To Sleep
> > {
> > ...
> > If (CondRefOf (\_SB.PCI0.GP12.PTS))
> > {
> > \_SB.PCI0.GP12.PTS (Arg0)
> > }
> > ...
> > }
> >
> > Scope (_SB.PCI0.GP12)
> > {
> > ...
> > Method (PTS, 1, Serialized)
> > {
> > If (^^LPCB.EC0.ECRG)
> > {
> > If ((PDID == 0x))
> > {
> > Return (Zero)
> > }
> >
> > POFF ()
> > SGIO (WWBR, One)
> > Sleep (0x1E)
> > Acquire (^^LPCB.EC0.ECMX, 0x)
> > ^^LPCB.EC0.WWP = One
> > Release (^^LPCB.EC0.ECMX)
> > Sleep (0x01F4)
> > }
> >
> > Return (Zero)
> > }
> > ...
> > }
> >
> > So let's also invok _PTS for s2idle.
> >
> > Signed-off-by: Kai-Heng Feng
> > ---
> > drivers/acpi/sleep.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index 09fd13757b65..7e84b4b09919 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -698,6 +698,7 @@ int acpi_s2idle_prepare(void)
> > }
> >
> > acpi_enable_wakeup_devices(ACPI_STATE_S0);
> > + acpi_enter_sleep_state_prep(ACPI_STATE_S0);
>
> The system is in S0 already at this point, so not really.
Ok, indeed ACPI spec only states _PTS can be used for S1 to S5.
> Please use a quirk to address this.
Let me discuss with HP folks. Right now it looks like we need to apply
this to all HP systems...
Kai-Heng
>
> >
> > /* Change the configuration of GPEs to avoid spurious wakeup. */
> > acpi_enable_all_wakeup_gpes();
> > --
Re: [PATCH v2] PCI: Disable D3cold support on Intel XMM7360
On Mon, Apr 12, 2021 at 4:18 PM Kai-Heng Feng
wrote:
>
> On Mon, Mar 29, 2021 at 1:23 PM Kai-Heng Feng
> wrote:
> >
> > On some platforms, the root port for Intel XMM7360 WWAN supports D3cold.
> > When the root port is put to D3cold by system suspend or runtime
> > suspend, attempt to systems resume or runtime resume will freeze the
> > laptop for a while, then it automatically shuts down.
> >
> > The root cause is unclear for now, as the Intel XMM7360 doesn't have a
> > driver yet.
> >
> > So disable D3cold for XMM7360 as a workaround, until proper device
> > driver is in place.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212419
> > Signed-off-by: Kai-Heng Feng
>
> A gentle ping...
Ok, I think I found the root cause:
https://lore.kernel.org/lkml/20210419090750.1272562-1-kai.heng.f...@canonical.com/
So we can ignore this patch.
Kai-Heng
>
> > ---
> > v2:
> > - Add comment.
> >
> > drivers/pci/quirks.c | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 653660e3ba9e..c48b0b4a4164 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -5612,3 +5612,16 @@ 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);
> > +
> > +/*
> > + * Device [8086:7360]
> > + * When it resumes from D3cold, system freeze and shutdown happens.
> > + * Currently there's no driver for XMM7360, so add it as a PCI quirk.
> > + * https://bugzilla.kernel.org/show_bug.cgi?id=212419
> > + */
> > +static void pci_fixup_no_d3cold(struct pci_dev *pdev)
> > +{
> > + pci_info(pdev, "disable D3cold\n");
> > + pci_d3cold_disable(pdev);
> > +}
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x7360, pci_fixup_no_d3cold);
> > --
> > 2.30.2
> >
[PATCH] ACPI: PM: s2idle: Invoke _PTS for s2idle
HP EliteBook 840 G8 reboots on s2idle resume, and HP EliteBook 845 G8
wakes up immediately on s2idle. Both are caused by the XMM7360 WWAN PCI
card.
There's a WWAN specific method to really turn off the WWAN via EC:
Method (_PTS, 1, NotSerialized) // _PTS: Prepare To Sleep
{
...
If (CondRefOf (\_SB.PCI0.GP12.PTS))
{
\_SB.PCI0.GP12.PTS (Arg0)
}
...
}
Scope (_SB.PCI0.GP12)
{
...
Method (PTS, 1, Serialized)
{
If (^^LPCB.EC0.ECRG)
{
If ((PDID == 0x))
{
Return (Zero)
}
POFF ()
SGIO (WWBR, One)
Sleep (0x1E)
Acquire (^^LPCB.EC0.ECMX, 0x)
^^LPCB.EC0.WWP = One
Release (^^LPCB.EC0.ECMX)
Sleep (0x01F4)
}
Return (Zero)
}
...
}
So let's also invok _PTS for s2idle.
Signed-off-by: Kai-Heng Feng
---
drivers/acpi/sleep.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 09fd13757b65..7e84b4b09919 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -698,6 +698,7 @@ int acpi_s2idle_prepare(void)
}
acpi_enable_wakeup_devices(ACPI_STATE_S0);
+ acpi_enter_sleep_state_prep(ACPI_STATE_S0);
/* Change the configuration of GPEs to avoid spurious wakeup. */
acpi_enable_all_wakeup_gpes();
--
2.30.2
Re: [PATCH v2] nvme: Favor D3cold for suspend if NVMe device supports it
On Mon, Apr 19, 2021 at 2:50 PM Christoph Hellwig wrote: > > On Fri, Apr 16, 2021 at 05:13:44PM +0800, Kai-Heng Feng wrote: > > On AMD platforms that use s2idle, NVMe timeouts on s2idle resume, > > because their SMU FW may cut off NVMe power during sleep. > > We're already have a discussion on a proper quirk for thse broken > platforms on the linux-nvme list, please take part in that discussion. Thanks. I didn't notice v5 was sent the to mailing list. As of now, AMD folks are also reviewing this, and I believe this approach is less quirky. Kai-Heng
[PATCH v2] nvme: Favor D3cold for suspend if NVMe device supports it
On AMD platforms that use s2idle, NVMe timeouts on s2idle resume,
because their SMU FW may cut off NVMe power during sleep.
Unlike Intel platforms where the power resources are generally
controlled by root port, the power resources is controlled by NVMe
device itself on recent AMD platforms:
...
Scope (\_SB.PCI0.GP24.NVME)
{
Name (_PR0, Package (0x01) // _PR0: Power Resources for D0
{
P0NV
})
Name (_PR2, Package (0x01) // _PR2: Power Resources for D2
{
P0NV
})
Name (_PR3, Package (0x01) // _PR3: Power Resources for D3hot
{
P0NV
})
Method (_PS0, 0, NotSerialized) // _PS0: Power State 0
{
}
Method (_PS3, 0, NotSerialized) // _PS3: Power State 3
{
}
}
...
And it's a great indication that the NVMe should use D3cold for sleep
instead of staying at D0.
So use NVME_QUIRK_SIMPLE_SUSPEND if the ACPI counterpart of NVMe device
supports D3cold.
Tested on HP EliteBook 845 G7/G8.
BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1230
References:
https://lore.kernel.org/linux-nvme/1618458725-17164-1-git-send-email-prike.li...@amd.com/
Cc: Alex Deucher
Cc: Prike Liang
Cc: Shyam Sundar S K
Signed-off-by: Kai-Heng Feng
---
v2:
- Typo. It's EliteBook 845, not EliteBook 840.
drivers/nvme/host/pci.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 7249ae74f71f..cc190324a919 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2840,6 +2840,13 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
acpi_status status;
u8 val;
+ /*
+* If the device itself supports D3cold, use that instead of D0 ASPM +
+* NVMe APST.
+*/
+ if (pci_pr3_present(dev))
+ return true;
+
/*
* Look for _DSD property specifying that the storage device on the port
* must use D3 to support deep platform power savings during
--
2.30.2
[PATCH] nvme: Favor D3cold for suspend if NVMe device supports it
On AMD platforms that use s2idle, NVMe timeouts on s2idle resume,
because their SMU FW may cut off NVMe power during sleep.
Unlike Intel platforms where the power resources are generally
controlled by root port, the power resources is controlled by NVMe
device itself on recent AMD platforms:
...
Scope (\_SB.PCI0.GP24.NVME)
{
Name (_PR0, Package (0x01) // _PR0: Power Resources for D0
{
P0NV
})
Name (_PR2, Package (0x01) // _PR2: Power Resources for D2
{
P0NV
})
Name (_PR3, Package (0x01) // _PR3: Power Resources for D3hot
{
P0NV
})
Method (_PS0, 0, NotSerialized) // _PS0: Power State 0
{
}
Method (_PS3, 0, NotSerialized) // _PS3: Power State 3
{
}
}
...
And it's a great indication that the NVMe should use D3cold for sleep
instead of staying at D0.
So use NVME_QUIRK_SIMPLE_SUSPEND if the ACPI counterpart of NVMe device
supports D3cold.
Tested on HP EliteBook 840 G7/G8.
BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1230
References:
https://lore.kernel.org/linux-nvme/1618458725-17164-1-git-send-email-prike.li...@amd.com/
Cc: Alex Deucher
Cc: Prike Liang
Cc: Shyam Sundar S K
Signed-off-by: Kai-Heng Feng
---
drivers/nvme/host/pci.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 7249ae74f71f..cc190324a919 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2840,6 +2840,13 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
acpi_status status;
u8 val;
+ /*
+* If the device itself supports D3cold, use that instead of D0 ASPM +
+* NVMe APST.
+*/
+ if (pci_pr3_present(dev))
+ return true;
+
/*
* Look for _DSD property specifying that the storage device on the port
* must use D3 to support deep platform power savings during
--
2.30.2
[PATCH] drm/i915/dp: Use slow and wide link training for DPCP rev < 1.4
Screen flickers on Innolux panel when clock rate 54 is in use. According to the panel vendor, though clock rate 54 is advertised, but the max clock rate it really supports is 27. So use slow and wide training for panels with DPCP rev < 1.4 to resolve the issue. User also confirmed the new strategy doesn't introduce regression on XPS 9380. Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3384 References: https://gitlab.freedesktop.org/drm/intel/-/issues/272 Signed-off-by: Kai-Heng Feng --- drivers/gpu/drm/i915/display/intel_dp.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 775d89b6c3fc..ca73e2179659 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -1461,12 +1461,12 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, intel_dp_can_bigjoiner(intel_dp)) pipe_config->bigjoiner = true; - if (intel_dp_is_edp(intel_dp)) + if (intel_dp_is_edp(intel_dp) && intel_dp->dpcd[DP_DPCD_REV] > 0x13) /* -* Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4 -* section A.1: "It is recommended that the minimum number of -* lanes be used, using the minimum link rate allowed for that -* lane configuration." +* Optimize for fast and narrow on DP 1.4. eDP 1.3 section 3.3 +* and eDP 1.4 section A.1: "It is recommended that the minimum +* number of lanes be used, using the minimum link rate allowed +* for that lane configuration." * * Note that we fall back to the max clock and lane count for eDP * panels that fail with the fast optimal settings (see -- 2.30.2
[PATCH] efifb: Check efifb_pci_dev before using it
On some platforms like Hyper-V and RPi4 with UEFI firmware, efifb is not
a PCI device.
So make sure efifb_pci_dev is found before using it.
Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI D0")
BugLink: https://bugs.launchpad.net/bugs/1922403
Signed-off-by: Kai-Heng Feng
---
drivers/video/fbdev/efifb.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index f58a545b3bf3..8ea8f079cde2 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -575,7 +575,8 @@ static int efifb_probe(struct platform_device *dev)
goto err_fb_dealoc;
}
fb_info(info, "%s frame buffer device\n", info->fix.id);
- pm_runtime_get_sync(&efifb_pci_dev->dev);
+ if (efifb_pci_dev)
+ pm_runtime_get_sync(&efifb_pci_dev->dev);
return 0;
err_fb_dealoc:
@@ -602,7 +603,8 @@ static int efifb_remove(struct platform_device *pdev)
unregister_framebuffer(info);
sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
framebuffer_release(info);
- pm_runtime_put(&efifb_pci_dev->dev);
+ if (efifb_pci_dev)
+ pm_runtime_put(&efifb_pci_dev->dev);
return 0;
}
--
2.30.2
[PATCH v3] USB: Add LPM quirk for Lenovo ThinkPad USB-C Dock Gen2 Ethernet
This is another branded 8153 device that doesn't work well with LPM
enabled:
[ 400.597506] r8152 5-1.1:1.0 enx482ae3a2a6f0: Tx status -71
So disable LPM to resolve the issue.
BugLink: https://bugs.launchpad.net/bugs/1922651
Signed-off-by: Kai-Heng Feng
---
v3:
- _Really_ put the quirk in the right order.
v2:
- Put the quirk in the right order.
drivers/usb/core/quirks.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 76ac5d6555ae..6114cf83bb44 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -438,6 +438,9 @@ static const struct usb_device_id usb_quirk_list[] = {
{ USB_DEVICE(0x17ef, 0xa012), .driver_info =
USB_QUIRK_DISCONNECT_SUSPEND },
+ /* Lenovo ThinkPad USB-C Dock Gen2 Ethernet (RTL8153 GigE) */
+ { USB_DEVICE(0x17ef, 0xa387), .driver_info = USB_QUIRK_NO_LPM },
+
/* BUILDWIN Photo Frame */
{ USB_DEVICE(0x1908, 0x1315), .driver_info =
USB_QUIRK_HONOR_BNUMINTERFACES },
--
2.30.2
Re: [PATCH v2] USB: Add LPM quirk for Lenovo ThinkPad USB-C Dock Gen2 Ethernet
On Mon, Apr 12, 2021 at 9:09 PM Johan Hovold wrote:
>
> On Mon, Apr 12, 2021 at 09:05:20PM +0800, Kai-Heng Feng wrote:
> > This is another branded 8153 device that doesn't work well with LPM
> > enabled:
> > [ 400.597506] r8152 5-1.1:1.0 enx482ae3a2a6f0: Tx status -71
> >
> > So disable LPM to resolve the issue.
> >
> > BugLink: https://bugs.launchpad.net/bugs/1922651
> > Signed-off-by: Kai-Heng Feng
> > ---
> > v2:
> > - Put the quirk in the right order.
>
> Seriously... You sent the exact same patch again. Still not ordered.
Oops, sorry for that, I mistyped the patch name :(
Will send a correct one.
Kai-Heng
>
> >
> > drivers/usb/core/quirks.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> > index 76ac5d6555ae..dfedb51cf832 100644
> > --- a/drivers/usb/core/quirks.c
> > +++ b/drivers/usb/core/quirks.c
> > @@ -434,6 +434,9 @@ static const struct usb_device_id usb_quirk_list[] = {
> > { USB_DEVICE(0x1532, 0x0116), .driver_info =
> > USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL },
> >
> > + /* Lenovo ThinkPad USB-C Dock Gen2 Ethernet (RTL8153 GigE) */
> > + { USB_DEVICE(0x17ef, 0xa387), .driver_info = USB_QUIRK_NO_LPM },
> > +
> > /* Lenovo ThinkCenter A630Z TI024Gen3 usb-audio */
> > { USB_DEVICE(0x17ef, 0xa012), .driver_info =
> > USB_QUIRK_DISCONNECT_SUSPEND },
>
> Johan
[PATCH v2] USB: Add LPM quirk for Lenovo ThinkPad USB-C Dock Gen2 Ethernet
This is another branded 8153 device that doesn't work well with LPM
enabled:
[ 400.597506] r8152 5-1.1:1.0 enx482ae3a2a6f0: Tx status -71
So disable LPM to resolve the issue.
BugLink: https://bugs.launchpad.net/bugs/1922651
Signed-off-by: Kai-Heng Feng
---
v2:
- Put the quirk in the right order.
drivers/usb/core/quirks.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 76ac5d6555ae..dfedb51cf832 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -434,6 +434,9 @@ static const struct usb_device_id usb_quirk_list[] = {
{ USB_DEVICE(0x1532, 0x0116), .driver_info =
USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL },
+ /* Lenovo ThinkPad USB-C Dock Gen2 Ethernet (RTL8153 GigE) */
+ { USB_DEVICE(0x17ef, 0xa387), .driver_info = USB_QUIRK_NO_LPM },
+
/* Lenovo ThinkCenter A630Z TI024Gen3 usb-audio */
{ USB_DEVICE(0x17ef, 0xa012), .driver_info =
USB_QUIRK_DISCONNECT_SUSPEND },
--
2.30.2
[PATCH] USB: Add LPM quirk for Lenovo ThinkPad USB-C Dock Gen2 Ethernet
This is another branded 8153 device that doesn't work well with LPM
enabled:
[ 400.597506] r8152 5-1.1:1.0 enx482ae3a2a6f0: Tx status -71
So disable LPM to resolve the issue.
BugLink: https://bugs.launchpad.net/bugs/1922651
Signed-off-by: Kai-Heng Feng
---
drivers/usb/core/quirks.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 76ac5d6555ae..dfedb51cf832 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -434,6 +434,9 @@ static const struct usb_device_id usb_quirk_list[] = {
{ USB_DEVICE(0x1532, 0x0116), .driver_info =
USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL },
+ /* Lenovo ThinkPad USB-C Dock Gen2 Ethernet (RTL8153 GigE) */
+ { USB_DEVICE(0x17ef, 0xa387), .driver_info = USB_QUIRK_NO_LPM },
+
/* Lenovo ThinkCenter A630Z TI024Gen3 usb-audio */
{ USB_DEVICE(0x17ef, 0xa012), .driver_info =
USB_QUIRK_DISCONNECT_SUSPEND },
--
2.30.2
Re: [PATCH v2] PCI: Disable D3cold support on Intel XMM7360
On Mon, Mar 29, 2021 at 1:23 PM Kai-Heng Feng
wrote:
>
> On some platforms, the root port for Intel XMM7360 WWAN supports D3cold.
> When the root port is put to D3cold by system suspend or runtime
> suspend, attempt to systems resume or runtime resume will freeze the
> laptop for a while, then it automatically shuts down.
>
> The root cause is unclear for now, as the Intel XMM7360 doesn't have a
> driver yet.
>
> So disable D3cold for XMM7360 as a workaround, until proper device
> driver is in place.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212419
> Signed-off-by: Kai-Heng Feng
A gentle ping...
> ---
> v2:
> - Add comment.
>
> drivers/pci/quirks.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 653660e3ba9e..c48b0b4a4164 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5612,3 +5612,16 @@ 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);
> +
> +/*
> + * Device [8086:7360]
> + * When it resumes from D3cold, system freeze and shutdown happens.
> + * Currently there's no driver for XMM7360, so add it as a PCI quirk.
> + * https://bugzilla.kernel.org/show_bug.cgi?id=212419
> + */
> +static void pci_fixup_no_d3cold(struct pci_dev *pdev)
> +{
> + pci_info(pdev, "disable D3cold\n");
> + pci_d3cold_disable(pdev);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x7360, pci_fixup_no_d3cold);
> --
> 2.30.2
>
[PATCH v2] PCI: Coalesce contiguous regions for host bridges
Built-in graphics on HP EliteDesk 805 G6 doesn't work because graphics
can't get the BAR it needs:
[0.611504] pci_bus :00: root bus resource [mem
0x1002020-0x100303f window]
[0.611505] pci_bus :00: root bus resource [mem
0x1003040-0x100401f window]
...
[0.638083] pci :00:08.1: bridge window [mem 0xd200-0xd23f]
[0.638086] pci :00:08.1: bridge window [mem
0x1003000-0x100401f 64bit pref]
[0.962086] pci :00:08.1: can't claim BAR 15 [mem
0x1003000-0x100401f 64bit pref]: no compatible bridge window
[0.962086] pci :00:08.1: [mem 0x1003000-0x100401f 64bit pref]
clipped to [mem 0x1003000-0x100303f 64bit pref]
[0.962086] pci :00:08.1: bridge window [mem
0x1003000-0x100303f 64bit pref]
[0.962086] pci :07:00.0: can't claim BAR 0 [mem
0x1003000-0x1003fff 64bit pref]: no compatible bridge window
[0.962086] pci :07:00.0: can't claim BAR 2 [mem
0x1004000-0x100401f 64bit pref]: no compatible bridge window
However, the root bus has two contiguous regions that can contain the
child resource requested.
Bjorn Helgaas pointed out that we can simply coalesce contiguous regions
for host bridges, since host bridge don't have _SRS. So do that
accordingly to make child resource can be contained. This change makes
the graphics works on the system in question.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212013
Suggested-by: Bjorn Helgaas
Signed-off-by: Kai-Heng Feng
---
v2:
- Coalesce all contiguous regresion in pci_register_host_bridge(), if
conditions are met.
drivers/pci/probe.c | 49 +
1 file changed, 45 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 953f15abc850..3607ce7402b4 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include "pci.h"
#define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
@@ -874,14 +875,30 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
dev_set_msi_domain(&bus->dev, d);
}
+static int res_cmp(void *priv, struct list_head *a, struct list_head *b)
+{
+ struct resource_entry *entry1, *entry2;
+
+ entry1 = container_of(a, struct resource_entry, node);
+ entry2 = container_of(b, struct resource_entry, node);
+
+ if (entry1->res->flags != entry2->res->flags)
+ return entry1->res->flags > entry2->res->flags;
+
+ if (entry1->offset != entry2->offset)
+ return entry1->offset > entry2->offset;
+
+ return entry1->res->start > entry2->res->start;
+}
+
static int pci_register_host_bridge(struct pci_host_bridge *bridge)
{
struct device *parent = bridge->dev.parent;
- struct resource_entry *window, *n;
+ struct resource_entry *window, *next, *n;
struct pci_bus *bus, *b;
- resource_size_t offset;
+ resource_size_t offset, next_offset;
LIST_HEAD(resources);
- struct resource *res;
+ struct resource *res, *next_res;
char addr[64], *fmt;
const char *name;
int err;
@@ -959,11 +976,35 @@ static int pci_register_host_bridge(struct
pci_host_bridge *bridge)
if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE)
dev_warn(&bus->dev, "Unknown NUMA node; performance will be
reduced\n");
+ /* Sort and coalesce contiguous windows */
+ list_sort(NULL, &resources, res_cmp);
+ resource_list_for_each_entry_safe(window, n, &resources) {
+ if (list_is_last(&window->node, &resources))
+ break;
+
+ next = list_next_entry(window, node);
+ offset = window->offset;
+ res = window->res;
+ next_offset = next->offset;
+ next_res = next->res;
+
+ if (res->flags != next_res->flags || offset != next_offset)
+ continue;
+
+ if (res->end + 1 == next_res->start) {
+ next_res->start = res->start;
+ res->flags = res->start = res->end = 0;
+ }
+ }
+
/* Add initial resources to the bus */
resource_list_for_each_entry_safe(window, n, &resources) {
- list_move_tail(&window->node, &bridge->windows);
offset = window->offset;
res = window->res;
+ if (!res->end)
+ continue;
+
+ list_move_tail(&window->node, &bridge->windows);
if (res->flags & IORESOURCE_BUS)
pci_bus_insert_busn_res(bus, bus->number, res->end);
--
2.30.2
Re: [PATCH] PCI: Try to find two continuous regions for child resource
On Tue, Mar 30, 2021 at 12:23 AM Bjorn Helgaas wrote:
>
> On Mon, Mar 29, 2021 at 04:47:59PM +0800, Kai-Heng Feng wrote:
> > Built-in grahpics on HP EliteDesk 805 G6 doesn't work because graphics
> > can't get the BAR it needs:
> > [0.611504] pci_bus :00: root bus resource [mem
> > 0x1002020-0x100303f window]
> > [0.611505] pci_bus :00: root bus resource [mem
> > 0x1003040-0x100401f window]
> > ...
> > [0.638083] pci :00:08.1: bridge window [mem 0xd200-0xd23f]
> > [0.638086] pci :00:08.1: bridge window [mem
> > 0x1003000-0x100401f 64bit pref]
> > [0.962086] pci :00:08.1: can't claim BAR 15 [mem
> > 0x1003000-0x100401f 64bit pref]: no compatible bridge window
> > [0.962086] pci :00:08.1: [mem 0x1003000-0x100401f 64bit
> > pref] clipped to [mem 0x1003000-0x100303f 64bit pref]
> > [0.962086] pci :00:08.1: bridge window [mem
> > 0x1003000-0x100303f 64bit pref]
> > [0.962086] pci :07:00.0: can't claim BAR 0 [mem
> > 0x1003000-0x1003fff 64bit pref]: no compatible bridge window
> > [0.962086] pci :07:00.0: can't claim BAR 2 [mem
> > 0x1004000-0x100401f 64bit pref]: no compatible bridge window
> >
> > However, the root bus has two continuous regions that can contain the
> > child resource requested.
> >
> > So try to find another parent region if two regions are continuous and
> > can contain child resource. This change makes the grahpics works on the
> > system in question.
>
> The BIOS description of PCI0 is interesting:
>
> pci_bus :00: root bus resource [mem 0x100-0x100201f window]
> pci_bus :00: root bus resource [mem 0x1002020-0x100303f window]
> pci_bus :00: root bus resource [mem 0x1003040-0x100401f window]
>
> So the PCI0 _CRS apparently gave us:
>
> [mem 0x100-0x100201f] size 0x2020 (512MB + 2MB)
> [mem 0x1002020-0x100303f] size 0x1020 (256MB + 2MB)
> [mem 0x1003040-0x100401f] size 0x0fe0 (254MB)
>
> These are all contiguous, so we'd have no problem if we coalesced them
> into a single window:
>
> [mem 0x100-0x100401f window] size 0x4020 (1GB + 2MB)
>
> I think we currently keep these root bus resources separate because if
> we ever support _SRS for host bridges, the argument we give to _SRS
> must be exactly the same format as what we got from _CRS (see ACPI
> v6.3, sec 6.2.16, and pnpacpi_set_resources()).
>
> pnpacpi_encode_resources() is currently very simple-minded and copies
> each device resource back into a single _SRS entry. But (1) we don't
> support _SRS for host bridges, and (2) if we ever do, we can make
> pnpacpi_encode_resources() smarter so it breaks things back up.
>
> So I think we should try to fix this by coalescing these adjacent
> resources from _CRS so we end up with a single root bus resource that
> covers all contiguous regions.
Thanks for the tip! Working on v2 patch.
>
> Typos, etc:
> - No need for the timestamps; they're not relevant to the problem.
> - s/grahpics/graphics/ (two occurrences above)
> - s/continuous/contiguous/ (three occurrences above)
Will also update those in v2.
Kai-Heng
>
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212013
> > Signed-off-by: Kai-Heng Feng
> > ---
> > arch/microblaze/pci/pci-common.c | 4 +--
> > arch/powerpc/kernel/pci-common.c | 8 ++---
> > arch/sparc/kernel/pci.c | 4 +--
> > drivers/pci/pci.c| 60 +++-
> > drivers/pci/setup-res.c | 21 +++
> > drivers/pcmcia/rsrc_nonstatic.c | 4 +--
> > include/linux/pci.h | 6 ++--
> > 7 files changed, 80 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/microblaze/pci/pci-common.c
> > b/arch/microblaze/pci/pci-common.c
> > index 557585f1be41..8e65832fb510 100644
> > --- a/arch/microblaze/pci/pci-common.c
> > +++ b/arch/microblaze/pci/pci-common.c
> > @@ -669,7 +669,7 @@ static void pcibios_allocate_bus_resources(struct
> > pci_bus *bus)
> > {
> > struct pci_bus *b;
> > int i;
> > - struct resource *res, *pr;
> > + struct resource *res, *pr = NULL;
> >
> > pr_debug("PCI: Allocating bus resources for %04x:%02x...\n",
> >pci_domain_nr(bus), bus->number);
> > @@ -688,7 +688,7 @@ static void pcibios_allocate_bus_resources(struct
> > pci_bus *bus)
> >
[PATCH] PCI: Try to find two continuous regions for child resource
Built-in grahpics on HP EliteDesk 805 G6 doesn't work because graphics
can't get the BAR it needs:
[0.611504] pci_bus :00: root bus resource [mem
0x1002020-0x100303f window]
[0.611505] pci_bus :00: root bus resource [mem
0x1003040-0x100401f window]
...
[0.638083] pci :00:08.1: bridge window [mem 0xd200-0xd23f]
[0.638086] pci :00:08.1: bridge window [mem
0x1003000-0x100401f 64bit pref]
[0.962086] pci :00:08.1: can't claim BAR 15 [mem
0x1003000-0x100401f 64bit pref]: no compatible bridge window
[0.962086] pci :00:08.1: [mem 0x1003000-0x100401f 64bit pref]
clipped to [mem 0x1003000-0x100303f 64bit pref]
[0.962086] pci :00:08.1: bridge window [mem
0x1003000-0x100303f 64bit pref]
[0.962086] pci :07:00.0: can't claim BAR 0 [mem
0x1003000-0x1003fff 64bit pref]: no compatible bridge window
[0.962086] pci :07:00.0: can't claim BAR 2 [mem
0x1004000-0x100401f 64bit pref]: no compatible bridge window
However, the root bus has two continuous regions that can contain the
child resource requested.
So try to find another parent region if two regions are continuous and
can contain child resource. This change makes the grahpics works on the
system in question.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212013
Signed-off-by: Kai-Heng Feng
---
arch/microblaze/pci/pci-common.c | 4 +--
arch/powerpc/kernel/pci-common.c | 8 ++---
arch/sparc/kernel/pci.c | 4 +--
drivers/pci/pci.c| 60 +++-
drivers/pci/setup-res.c | 21 +++
drivers/pcmcia/rsrc_nonstatic.c | 4 +--
include/linux/pci.h | 6 ++--
7 files changed, 80 insertions(+), 27 deletions(-)
diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
index 557585f1be41..8e65832fb510 100644
--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -669,7 +669,7 @@ static void pcibios_allocate_bus_resources(struct pci_bus
*bus)
{
struct pci_bus *b;
int i;
- struct resource *res, *pr;
+ struct resource *res, *pr = NULL;
pr_debug("PCI: Allocating bus resources for %04x:%02x...\n",
pci_domain_nr(bus), bus->number);
@@ -688,7 +688,7 @@ static void pcibios_allocate_bus_resources(struct pci_bus
*bus)
* and as such ensure proper re-allocation
* later.
*/
- pr = pci_find_parent_resource(bus->self, res);
+ pci_find_parent_resource(bus->self, res, &pr, NULL);
if (pr == res) {
/* this happens when the generic PCI
* code (wrongly) decides that this
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 001e90cd8948..f865354b746d 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1196,7 +1196,7 @@ static void pcibios_allocate_bus_resources(struct pci_bus
*bus)
{
struct pci_bus *b;
int i;
- struct resource *res, *pr;
+ struct resource *res, *pr = NULL;
pr_debug("PCI: Allocating bus resources for %04x:%02x...\n",
pci_domain_nr(bus), bus->number);
@@ -1213,7 +1213,7 @@ static void pcibios_allocate_bus_resources(struct pci_bus
*bus)
pr = (res->flags & IORESOURCE_IO) ?
&ioport_resource : &iomem_resource;
else {
- pr = pci_find_parent_resource(bus->self, res);
+ pci_find_parent_resource(bus->self, res, &pr, NULL);
if (pr == res) {
/* this happens when the generic PCI
* code (wrongly) decides that this
@@ -1265,12 +1265,12 @@ static void pcibios_allocate_bus_resources(struct
pci_bus *bus)
static inline void alloc_resource(struct pci_dev *dev, int idx)
{
- struct resource *pr, *r = &dev->resource[idx];
+ struct resource *pr = NULL, *r = &dev->resource[idx];
pr_debug("PCI: Allocating %s: Resource %d: %pR\n",
pci_name(dev), idx, r);
- pr = pci_find_parent_resource(dev, r);
+ pci_find_parent_resource(dev, r, &pr, NULL);
if (!pr || (pr->flags & IORESOURCE_UNSET) ||
request_resource(pr, r) < 0) {
printk(KERN_WARNING "PCI: Cannot allocate resource region %d"
diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
index 9c2b720bfd20..b4006798e4e1 100644
--- a/arch/sparc/kernel/pci.c
+++ b/arch/sparc/kernel/pci.c
@@ -621,7 +621,7 @@ static void pci_bus_register_of_sysfs(str
[PATCH v2] PCI: Disable D3cold support on Intel XMM7360
On some platforms, the root port for Intel XMM7360 WWAN supports D3cold.
When the root port is put to D3cold by system suspend or runtime
suspend, attempt to systems resume or runtime resume will freeze the
laptop for a while, then it automatically shuts down.
The root cause is unclear for now, as the Intel XMM7360 doesn't have a
driver yet.
So disable D3cold for XMM7360 as a workaround, until proper device
driver is in place.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212419
Signed-off-by: Kai-Heng Feng
---
v2:
- Add comment.
drivers/pci/quirks.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..c48b0b4a4164 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5612,3 +5612,16 @@ 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);
+
+/*
+ * Device [8086:7360]
+ * When it resumes from D3cold, system freeze and shutdown happens.
+ * Currently there's no driver for XMM7360, so add it as a PCI quirk.
+ * https://bugzilla.kernel.org/show_bug.cgi?id=212419
+ */
+static void pci_fixup_no_d3cold(struct pci_dev *pdev)
+{
+ pci_info(pdev, "disable D3cold\n");
+ pci_d3cold_disable(pdev);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x7360, pci_fixup_no_d3cold);
--
2.30.2
[PATCH v3 1/2] ALSA: usb-audio: Carve out connector value checking into a helper
This is preparation for next patch, no functional change intended.
Signed-off-by: Kai-Heng Feng
---
v3:
- No change.
v2:
- Only return early when ret < 0.
sound/usb/mixer.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index b004b2e63a5d..5a2d9a768f70 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1446,13 +1446,11 @@ static int mixer_ctl_master_bool_get(struct
snd_kcontrol *kcontrol,
return 0;
}
-/* get the connectors status and report it as boolean type */
-static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+static int get_connector_value(struct usb_mixer_elem_info *cval,
+ char *name, int *val)
{
- struct usb_mixer_elem_info *cval = kcontrol->private_data;
struct snd_usb_audio *chip = cval->head.mixer->chip;
- int idx = 0, validx, ret, val;
+ int idx = 0, validx, ret;
validx = cval->control << 8 | 0;
@@ -1467,21 +1465,24 @@ static int mixer_ctl_connector_get(struct snd_kcontrol
*kcontrol,
ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
UAC2_CS_CUR,
USB_RECIP_INTERFACE | USB_TYPE_CLASS |
USB_DIR_IN,
validx, idx, &uac2_conn,
sizeof(uac2_conn));
- val = !!uac2_conn.bNrChannels;
+ if (val)
+ *val = !!uac2_conn.bNrChannels;
} else { /* UAC_VERSION_3 */
struct uac3_insertion_ctl_blk uac3_conn;
ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
UAC2_CS_CUR,
USB_RECIP_INTERFACE | USB_TYPE_CLASS |
USB_DIR_IN,
validx, idx, &uac3_conn,
sizeof(uac3_conn));
- val = !!uac3_conn.bmConInserted;
+ if (val)
+ *val = !!uac3_conn.bmConInserted;
}
snd_usb_unlock_shutdown(chip);
if (ret < 0) {
- if (strstr(kcontrol->id.name, "Speaker")) {
- ucontrol->value.integer.value[0] = 1;
+ if (name && strstr(name, "Speaker")) {
+ if (val)
+ *val = 1;
return 0;
}
error:
@@ -1491,6 +1492,21 @@ static int mixer_ctl_connector_get(struct snd_kcontrol
*kcontrol,
return filter_error(cval, ret);
}
+ return ret;
+}
+
+/* get the connectors status and report it as boolean type */
+static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct usb_mixer_elem_info *cval = kcontrol->private_data;
+ int ret, val;
+
+ ret = get_connector_value(cval, kcontrol->id.name, &val);
+
+ if (ret < 0)
+ return ret;
+
ucontrol->value.integer.value[0] = val;
return 0;
}
--
2.30.2
[PATCH v3 2/2] ALSA: usb-audio: Check connector value on resume
Rear Mic on Lenovo P620 cannot record after S3, despite that there's no
error and the other two functions of the USB audio, Line In and Line
Out, work just fine.
The mic starts to work again after running userspace app like "alsactl
store". Following the lead, the evidence shows that as soon as connector
status is queried, the mic can work again.
So also check connector value on resume to "wake up" the USB audio to
make it functional.
This can be device specific, however I think this generic approach may
benefit more than one device.
Now the resume callback checks connector, and a new callback,
reset_resume, to also restore switches and volumes.
Suggested-by: Takashi Iwai
Signed-off-by: Kai-Heng Feng
---
v3:
- New callback to handle resume and reset-resume separately.
v2:
- Remove reset-resume.
- Fold the connector checking to the mixer resume callback.
sound/usb/mixer.c| 44 +++-
sound/usb/mixer.h| 1 +
sound/usb/mixer_quirks.c | 2 +-
3 files changed, 36 insertions(+), 11 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 5a2d9a768f70..2faf5767c7f8 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -3631,20 +3631,43 @@ static int restore_mixer_value(struct
usb_mixer_elem_list *list)
return 0;
}
+static int default_mixer_resume(struct usb_mixer_elem_list *list)
+{
+ struct usb_mixer_elem_info *cval = mixer_elem_list_to_info(list);
+
+ /* get connector value to "wake up" the USB audio */
+ if (cval->val_type == USB_MIXER_BOOLEAN && cval->channels == 1)
+ get_connector_value(cval, NULL, NULL);
+
+ return 0;
+}
+
+static int default_mixer_reset_resume(struct usb_mixer_elem_list *list)
+{
+ int err = default_mixer_resume(list);
+
+ if (err < 0)
+ return err;
+ return restore_mixer_value(list);
+}
+
int snd_usb_mixer_resume(struct usb_mixer_interface *mixer, bool reset_resume)
{
struct usb_mixer_elem_list *list;
+ usb_mixer_elem_resume_func_t f;
int id, err;
- if (reset_resume) {
- /* restore cached mixer values */
- for (id = 0; id < MAX_ID_ELEMS; id++) {
- for_each_mixer_elem(list, mixer, id) {
- if (list->resume) {
- err = list->resume(list);
- if (err < 0)
- return err;
- }
+ /* restore cached mixer values */
+ for (id = 0; id < MAX_ID_ELEMS; id++) {
+ for_each_mixer_elem(list, mixer, id) {
+ if (reset_resume)
+ f = list->reset_resume;
+ else
+ f = list->resume;
+ if (f) {
+ err = f(list);
+ if (err < 0)
+ return err;
}
}
}
@@ -3663,6 +3686,7 @@ void snd_usb_mixer_elem_init_std(struct
usb_mixer_elem_list *list,
list->id = unitid;
list->dump = snd_usb_mixer_dump_cval;
#ifdef CONFIG_PM
- list->resume = restore_mixer_value;
+ list->resume = default_mixer_resume;
+ list->reset_resume = default_mixer_reset_resume;
#endif
}
diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
index c29e27ac43a7..e5a01f17bf3c 100644
--- a/sound/usb/mixer.h
+++ b/sound/usb/mixer.h
@@ -69,6 +69,7 @@ struct usb_mixer_elem_list {
bool is_std_info;
usb_mixer_elem_dump_func_t dump;
usb_mixer_elem_resume_func_t resume;
+ usb_mixer_elem_resume_func_t reset_resume;
};
/* iterate over mixer element list of the given unit id */
diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index ffd922327ae4..b7f9c2fded05 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -151,7 +151,7 @@ static int add_single_ctl_with_resume(struct
usb_mixer_interface *mixer,
*listp = list;
list->mixer = mixer;
list->id = id;
- list->resume = resume;
+ list->reset_resume = resume;
kctl = snd_ctl_new1(knew, list);
if (!kctl) {
kfree(list);
--
2.30.2
Re: [PATCH v2 2/2] ALSA: usb-audio: Check connector value on resume
On Thu, Mar 25, 2021 at 9:55 PM Kai-Heng Feng
wrote:
>
> On Thu, Mar 25, 2021 at 9:41 PM Takashi Iwai wrote:
> >
> > On Thu, 25 Mar 2021 13:12:48 +0100,
> > Kai-Heng Feng wrote:
> > >
> > > Rear Mic on Lenovo P620 cannot record after S3, despite that there's no
> > > error and the other two functions of the USB audio, Line In and Line
> > > Out, work just fine.
> > >
> > > The mic starts to work again after running userspace app like "alsactl
> > > store". Following the lead, the evidence shows that as soon as connector
> > > status is queried, the mic can work again.
> > >
> > > So also check connector value on resume to "wake up" the USB audio to
> > > make it functional.
> > >
> > > This can be device specific, however I think this generic approach may
> > > benefit more than one device.
> > >
> > > While at it, also remove reset-resume path to consolidate mixer resume
> > > path.
> > >
> > > Signed-off-by: Kai-Heng Feng
> > > ---
> > > v2:
> > > - Remove reset-resume.
> > > - Fold the connector checking to the mixer resume callback.
> >
> > That's not what I meant exactly... I meant to put both into the
> > single resume callback, but handle each part conditionally depending
> > on reset_resume argument.
>
> OK, I get what you mean now.
>
> >
> > But this turned out to need more changes in mixer_quirks.c
> > unnecessarily. Maybe adding the two resume functions is a better
> > approach in the end, but not for the specific connection thing but
> > generically both resume and reset_resume callbacks. Something like
> > below.
>
> This approach looks good. Let me send another one.
Actually, it works really well and I don't I think I should send the
code you wrote.
Is it possible to push your version with my commit log, and with my tested tag?
Tested-by: Kai-Heng Feng
> Kai-Heng
>
> >
> >
> > thanks,
> >
> > Takashi
> >
> >
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index b004b2e63a5d..1dab281bb269 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -3615,20 +3615,43 @@ static int restore_mixer_value(struct
> > usb_mixer_elem_list *list)
> > return 0;
> > }
> >
> > +static int default_mixer_resume(struct usb_mixer_elem_list *list)
> > +{
> > + struct usb_mixer_elem_info *cval = mixer_elem_list_to_info(list);
> > +
> > + /* get connector value to "wake up" the USB audio */
> > + if (cval->val_type == USB_MIXER_BOOLEAN && cval->channels == 1)
> > + get_connector_value(cval, NULL, NULL);
> > +
> > + return 0;
> > +}
> > +
> > +static int default_mixer_reset_resume(struct usb_mixer_elem_list *list)
> > +{
> > + int err = default_mixer_resume(list);
> > +
> > + if (err < 0)
> > + return err;
> > + return restore_mixer_value(list);
> > +}
> > +
> > int snd_usb_mixer_resume(struct usb_mixer_interface *mixer, bool
> > reset_resume)
> > {
> > struct usb_mixer_elem_list *list;
> > + usb_mixer_elem_resume_func_t f;
> > int id, err;
> >
> > - if (reset_resume) {
> > - /* restore cached mixer values */
> > - for (id = 0; id < MAX_ID_ELEMS; id++) {
> > - for_each_mixer_elem(list, mixer, id) {
> > - if (list->resume) {
> > - err = list->resume(list);
> > - if (err < 0)
> > - return err;
> > - }
> > + /* restore cached mixer values */
> > + for (id = 0; id < MAX_ID_ELEMS; id++) {
> > + for_each_mixer_elem(list, mixer, id) {
> > + if (reset_resume)
> > + f = list->reset_resume;
> > + else
> > + f = list->resume;
> > + if (f) {
> > + err = list->resume(list);
> > + if (err < 0)
> > + return err;
> > }
> > }
> > }
> > @@ -3647,6 +3670,7 @@ void snd_usb_mixer_elem_
Re: [PATCH v2 2/2] ALSA: usb-audio: Check connector value on resume
On Thu, Mar 25, 2021 at 9:41 PM Takashi Iwai wrote:
>
> On Thu, 25 Mar 2021 13:12:48 +0100,
> Kai-Heng Feng wrote:
> >
> > Rear Mic on Lenovo P620 cannot record after S3, despite that there's no
> > error and the other two functions of the USB audio, Line In and Line
> > Out, work just fine.
> >
> > The mic starts to work again after running userspace app like "alsactl
> > store". Following the lead, the evidence shows that as soon as connector
> > status is queried, the mic can work again.
> >
> > So also check connector value on resume to "wake up" the USB audio to
> > make it functional.
> >
> > This can be device specific, however I think this generic approach may
> > benefit more than one device.
> >
> > While at it, also remove reset-resume path to consolidate mixer resume
> > path.
> >
> > Signed-off-by: Kai-Heng Feng
> > ---
> > v2:
> > - Remove reset-resume.
> > - Fold the connector checking to the mixer resume callback.
>
> That's not what I meant exactly... I meant to put both into the
> single resume callback, but handle each part conditionally depending
> on reset_resume argument.
OK, I get what you mean now.
>
> But this turned out to need more changes in mixer_quirks.c
> unnecessarily. Maybe adding the two resume functions is a better
> approach in the end, but not for the specific connection thing but
> generically both resume and reset_resume callbacks. Something like
> below.
This approach looks good. Let me send another one.
Kai-Heng
>
>
> thanks,
>
> Takashi
>
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index b004b2e63a5d..1dab281bb269 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -3615,20 +3615,43 @@ static int restore_mixer_value(struct
> usb_mixer_elem_list *list)
> return 0;
> }
>
> +static int default_mixer_resume(struct usb_mixer_elem_list *list)
> +{
> + struct usb_mixer_elem_info *cval = mixer_elem_list_to_info(list);
> +
> + /* get connector value to "wake up" the USB audio */
> + if (cval->val_type == USB_MIXER_BOOLEAN && cval->channels == 1)
> + get_connector_value(cval, NULL, NULL);
> +
> + return 0;
> +}
> +
> +static int default_mixer_reset_resume(struct usb_mixer_elem_list *list)
> +{
> + int err = default_mixer_resume(list);
> +
> + if (err < 0)
> + return err;
> + return restore_mixer_value(list);
> +}
> +
> int snd_usb_mixer_resume(struct usb_mixer_interface *mixer, bool
> reset_resume)
> {
> struct usb_mixer_elem_list *list;
> + usb_mixer_elem_resume_func_t f;
> int id, err;
>
> - if (reset_resume) {
> - /* restore cached mixer values */
> - for (id = 0; id < MAX_ID_ELEMS; id++) {
> - for_each_mixer_elem(list, mixer, id) {
> - if (list->resume) {
> - err = list->resume(list);
> - if (err < 0)
> - return err;
> - }
> + /* restore cached mixer values */
> + for (id = 0; id < MAX_ID_ELEMS; id++) {
> + for_each_mixer_elem(list, mixer, id) {
> + if (reset_resume)
> + f = list->reset_resume;
> + else
> + f = list->resume;
> + if (f) {
> + err = list->resume(list);
> + if (err < 0)
> + return err;
> }
> }
> }
> @@ -3647,6 +3670,7 @@ void snd_usb_mixer_elem_init_std(struct
> usb_mixer_elem_list *list,
> list->id = unitid;
> list->dump = snd_usb_mixer_dump_cval;
> #ifdef CONFIG_PM
> - list->resume = restore_mixer_value;
> + list->resume = default_mixer_resume;
> + list->reset_resume = default_mixer_reset_resume;
> #endif
> }
> diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
> index c29e27ac43a7..e5a01f17bf3c 100644
> --- a/sound/usb/mixer.h
> +++ b/sound/usb/mixer.h
> @@ -69,6 +69,7 @@ struct usb_mixer_elem_list {
> bool is_std_info;
> usb_mixer_elem_dump_func_t dump;
> usb_mixer_elem_resume_func_t resume;
> + usb_mixer_elem_resume_func_t reset_resume;
> };
>
> /* iterate over mixer element list of the
[PATCH v2 2/2] ALSA: usb-audio: Check connector value on resume
Rear Mic on Lenovo P620 cannot record after S3, despite that there's no
error and the other two functions of the USB audio, Line In and Line
Out, work just fine.
The mic starts to work again after running userspace app like "alsactl
store". Following the lead, the evidence shows that as soon as connector
status is queried, the mic can work again.
So also check connector value on resume to "wake up" the USB audio to
make it functional.
This can be device specific, however I think this generic approach may
benefit more than one device.
While at it, also remove reset-resume path to consolidate mixer resume
path.
Signed-off-by: Kai-Heng Feng
---
v2:
- Remove reset-resume.
- Fold the connector checking to the mixer resume callback.
sound/usb/card.c | 16 +++-
sound/usb/mixer.c | 27 +++
sound/usb/mixer.h | 2 +-
3 files changed, 19 insertions(+), 26 deletions(-)
diff --git a/sound/usb/card.c b/sound/usb/card.c
index 0826a437f8fc..552a57c00799 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -1014,7 +1014,7 @@ static int usb_audio_suspend(struct usb_interface *intf,
pm_message_t message)
return 0;
}
-static int __usb_audio_resume(struct usb_interface *intf, bool reset_resume)
+static int usb_audio_resume(struct usb_interface *intf)
{
struct snd_usb_audio *chip = usb_get_intfdata(intf);
struct snd_usb_stream *as;
@@ -1040,7 +1040,7 @@ static int __usb_audio_resume(struct usb_interface *intf,
bool reset_resume)
* we just notify and restart the mixers
*/
list_for_each_entry(mixer, &chip->mixer_list, list) {
- err = snd_usb_mixer_resume(mixer, reset_resume);
+ err = snd_usb_mixer_resume(mixer);
if (err < 0)
goto err_out;
}
@@ -1060,16 +1060,6 @@ static int __usb_audio_resume(struct usb_interface
*intf, bool reset_resume)
atomic_dec(&chip->active); /* allow autopm after this point */
return err;
}
-
-static int usb_audio_resume(struct usb_interface *intf)
-{
- return __usb_audio_resume(intf, false);
-}
-
-static int usb_audio_reset_resume(struct usb_interface *intf)
-{
- return __usb_audio_resume(intf, true);
-}
#else
#define usb_audio_suspend NULL
#define usb_audio_resume NULL
@@ -1095,7 +1085,7 @@ static struct usb_driver usb_audio_driver = {
.disconnect = usb_audio_disconnect,
.suspend = usb_audio_suspend,
.resume = usb_audio_resume,
- .reset_resume = usb_audio_reset_resume,
+ .reset_resume = usb_audio_resume,
.id_table = usb_audio_ids,
.supports_autosuspend = 1,
};
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 5a2d9a768f70..7468c8d2b158 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -3601,11 +3601,16 @@ int snd_usb_mixer_suspend(struct usb_mixer_interface
*mixer)
return 0;
}
-static int restore_mixer_value(struct usb_mixer_elem_list *list)
+static int resume_mixer(struct usb_mixer_elem_list *list)
{
struct usb_mixer_elem_info *cval = mixer_elem_list_to_info(list);
int c, err, idx;
+ /* get connector value to "wake up" the USB audio */
+ if (cval->val_type == USB_MIXER_BOOLEAN && cval->channels == 1)
+ get_connector_value(cval, NULL, NULL);
+
+ /* restore other mixer value */
if (cval->cmask) {
idx = 0;
for (c = 0; c < MAX_CHANNELS; c++) {
@@ -3631,20 +3636,18 @@ static int restore_mixer_value(struct
usb_mixer_elem_list *list)
return 0;
}
-int snd_usb_mixer_resume(struct usb_mixer_interface *mixer, bool reset_resume)
+int snd_usb_mixer_resume(struct usb_mixer_interface *mixer)
{
struct usb_mixer_elem_list *list;
int id, err;
- if (reset_resume) {
- /* restore cached mixer values */
- for (id = 0; id < MAX_ID_ELEMS; id++) {
- for_each_mixer_elem(list, mixer, id) {
- if (list->resume) {
- err = list->resume(list);
- if (err < 0)
- return err;
- }
+ /* restore cached mixer values */
+ for (id = 0; id < MAX_ID_ELEMS; id++) {
+ for_each_mixer_elem(list, mixer, id) {
+ if (list->resume) {
+ err = list->resume(list);
+ if (err < 0)
+ return err;
}
}
}
@@ -3663,6 +3666,6 @@ void snd_usb_mixer_elem_init_std(struct
usb_mixer_elem_list *list,
list->id = unitid;
list->dump = snd_usb_mixer_dump_cval;
#ifdef CONFIG_PM
- list
[PATCH v2 1/2] ALSA: usb-audio: Carve out connector value checking into a helper
This is preparation for next patch, no functional change intended.
Signed-off-by: Kai-Heng Feng
---
v2:
- Only return early when ret < 0.
sound/usb/mixer.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index b004b2e63a5d..5a2d9a768f70 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1446,13 +1446,11 @@ static int mixer_ctl_master_bool_get(struct
snd_kcontrol *kcontrol,
return 0;
}
-/* get the connectors status and report it as boolean type */
-static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+static int get_connector_value(struct usb_mixer_elem_info *cval,
+ char *name, int *val)
{
- struct usb_mixer_elem_info *cval = kcontrol->private_data;
struct snd_usb_audio *chip = cval->head.mixer->chip;
- int idx = 0, validx, ret, val;
+ int idx = 0, validx, ret;
validx = cval->control << 8 | 0;
@@ -1467,21 +1465,24 @@ static int mixer_ctl_connector_get(struct snd_kcontrol
*kcontrol,
ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
UAC2_CS_CUR,
USB_RECIP_INTERFACE | USB_TYPE_CLASS |
USB_DIR_IN,
validx, idx, &uac2_conn,
sizeof(uac2_conn));
- val = !!uac2_conn.bNrChannels;
+ if (val)
+ *val = !!uac2_conn.bNrChannels;
} else { /* UAC_VERSION_3 */
struct uac3_insertion_ctl_blk uac3_conn;
ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
UAC2_CS_CUR,
USB_RECIP_INTERFACE | USB_TYPE_CLASS |
USB_DIR_IN,
validx, idx, &uac3_conn,
sizeof(uac3_conn));
- val = !!uac3_conn.bmConInserted;
+ if (val)
+ *val = !!uac3_conn.bmConInserted;
}
snd_usb_unlock_shutdown(chip);
if (ret < 0) {
- if (strstr(kcontrol->id.name, "Speaker")) {
- ucontrol->value.integer.value[0] = 1;
+ if (name && strstr(name, "Speaker")) {
+ if (val)
+ *val = 1;
return 0;
}
error:
@@ -1491,6 +1492,21 @@ static int mixer_ctl_connector_get(struct snd_kcontrol
*kcontrol,
return filter_error(cval, ret);
}
+ return ret;
+}
+
+/* get the connectors status and report it as boolean type */
+static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct usb_mixer_elem_info *cval = kcontrol->private_data;
+ int ret, val;
+
+ ret = get_connector_value(cval, kcontrol->id.name, &val);
+
+ if (ret < 0)
+ return ret;
+
ucontrol->value.integer.value[0] = val;
return 0;
}
--
2.30.2
Re: [PATCH 2/2] ALSA: usb-audio: Check connector value on resume
On Thu, Mar 25, 2021 at 3:19 PM Takashi Iwai wrote:
>
> On Wed, 24 Mar 2021 18:14:08 +0100,
> Kai-Heng Feng wrote:
> >
> > Rear Mic on Lenovo P620 cannot record after S3, despite that there's no
> > error and the other two functions of the USB audio, Line In and Line
> > Out, work just fine.
> >
> > The mic starts to work again after running userspace app like "alsactl
> > store". Following the lead, the evidence shows that as soon as connector
> > status is queried, the mic can work again.
> >
> > So also check connector value on resume to "wake up" the USB audio to
> > make it functional.
> >
> > This can be device specific, however I think this generic approach may
> > benefit more than one device.
> >
> > Signed-off-by: Kai-Heng Feng
>
> Just to be sure: this workaround is always needed no matter whether
> reset_resume is set or not?
Yes, reset_resume is irrelevant for this issue. Getting connector
status is the key.
> If so, it's better to change the resume
> callback to take reset_resume argument and call it always. The
> resume_connector() can be folded into there.
OK, will send V2.
Kai-Heng
>
>
> thanks,
>
> Takashi
>
> > ---
> > sound/usb/mixer.c | 18 ++
> > sound/usb/mixer.h | 1 +
> > 2 files changed, 19 insertions(+)
> >
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index 98f5417a70e4..6a553d891b0f 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -3631,11 +3631,28 @@ static int restore_mixer_value(struct
> > usb_mixer_elem_list *list)
> > return 0;
> > }
> >
> > +static int resume_connector(struct usb_mixer_elem_list *list)
> > +{
> > + struct usb_mixer_elem_info *cval = mixer_elem_list_to_info(list);
> > +
> > + if (cval->val_type != USB_MIXER_BOOLEAN || cval->channels != 1)
> > + return 0;
> > +
> > + return get_connector_value(cval, NULL, NULL);
> > +}
> > +
> > int snd_usb_mixer_resume(struct usb_mixer_interface *mixer, bool
> > reset_resume)
> > {
> > struct usb_mixer_elem_list *list;
> > int id, err;
> >
> > + for (id = 0; id < MAX_ID_ELEMS; id++) {
> > + for_each_mixer_elem(list, mixer, id) {
> > + if (list->resume_connector)
> > + list->resume_connector(list);
> > + }
> > + }
> > +
> > if (reset_resume) {
> > /* restore cached mixer values */
> > for (id = 0; id < MAX_ID_ELEMS; id++) {
> > @@ -3664,5 +3681,6 @@ void snd_usb_mixer_elem_init_std(struct
> > usb_mixer_elem_list *list,
> > list->dump = snd_usb_mixer_dump_cval;
> > #ifdef CONFIG_PM
> > list->resume = restore_mixer_value;
> > + list->resume_connector = resume_connector;
> > #endif
> > }
> > diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
> > index c29e27ac43a7..843ccff0eea3 100644
> > --- a/sound/usb/mixer.h
> > +++ b/sound/usb/mixer.h
> > @@ -69,6 +69,7 @@ struct usb_mixer_elem_list {
> > bool is_std_info;
> > usb_mixer_elem_dump_func_t dump;
> > usb_mixer_elem_resume_func_t resume;
> > + usb_mixer_elem_resume_func_t resume_connector;
> > };
> >
> > /* iterate over mixer element list of the given unit id */
> > --
> > 2.30.2
> >
[PATCH 2/2] ALSA: usb-audio: Check connector value on resume
Rear Mic on Lenovo P620 cannot record after S3, despite that there's no
error and the other two functions of the USB audio, Line In and Line
Out, work just fine.
The mic starts to work again after running userspace app like "alsactl
store". Following the lead, the evidence shows that as soon as connector
status is queried, the mic can work again.
So also check connector value on resume to "wake up" the USB audio to
make it functional.
This can be device specific, however I think this generic approach may
benefit more than one device.
Signed-off-by: Kai-Heng Feng
---
sound/usb/mixer.c | 18 ++
sound/usb/mixer.h | 1 +
2 files changed, 19 insertions(+)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 98f5417a70e4..6a553d891b0f 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -3631,11 +3631,28 @@ static int restore_mixer_value(struct
usb_mixer_elem_list *list)
return 0;
}
+static int resume_connector(struct usb_mixer_elem_list *list)
+{
+ struct usb_mixer_elem_info *cval = mixer_elem_list_to_info(list);
+
+ if (cval->val_type != USB_MIXER_BOOLEAN || cval->channels != 1)
+ return 0;
+
+ return get_connector_value(cval, NULL, NULL);
+}
+
int snd_usb_mixer_resume(struct usb_mixer_interface *mixer, bool reset_resume)
{
struct usb_mixer_elem_list *list;
int id, err;
+ for (id = 0; id < MAX_ID_ELEMS; id++) {
+ for_each_mixer_elem(list, mixer, id) {
+ if (list->resume_connector)
+ list->resume_connector(list);
+ }
+ }
+
if (reset_resume) {
/* restore cached mixer values */
for (id = 0; id < MAX_ID_ELEMS; id++) {
@@ -3664,5 +3681,6 @@ void snd_usb_mixer_elem_init_std(struct
usb_mixer_elem_list *list,
list->dump = snd_usb_mixer_dump_cval;
#ifdef CONFIG_PM
list->resume = restore_mixer_value;
+ list->resume_connector = resume_connector;
#endif
}
diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
index c29e27ac43a7..843ccff0eea3 100644
--- a/sound/usb/mixer.h
+++ b/sound/usb/mixer.h
@@ -69,6 +69,7 @@ struct usb_mixer_elem_list {
bool is_std_info;
usb_mixer_elem_dump_func_t dump;
usb_mixer_elem_resume_func_t resume;
+ usb_mixer_elem_resume_func_t resume_connector;
};
/* iterate over mixer element list of the given unit id */
--
2.30.2
[PATCH 1/2] ALSA: usb-audio: Carve out connector value checking into a helper
This is preparation for next patch, no functional change intended.
Signed-off-by: Kai-Heng Feng
---
sound/usb/mixer.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index b004b2e63a5d..98f5417a70e4 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1446,13 +1446,11 @@ static int mixer_ctl_master_bool_get(struct
snd_kcontrol *kcontrol,
return 0;
}
-/* get the connectors status and report it as boolean type */
-static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+static int get_connector_value(struct usb_mixer_elem_info *cval,
+ char *name, int *val)
{
- struct usb_mixer_elem_info *cval = kcontrol->private_data;
struct snd_usb_audio *chip = cval->head.mixer->chip;
- int idx = 0, validx, ret, val;
+ int idx = 0, validx, ret;
validx = cval->control << 8 | 0;
@@ -1467,21 +1465,24 @@ static int mixer_ctl_connector_get(struct snd_kcontrol
*kcontrol,
ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
UAC2_CS_CUR,
USB_RECIP_INTERFACE | USB_TYPE_CLASS |
USB_DIR_IN,
validx, idx, &uac2_conn,
sizeof(uac2_conn));
- val = !!uac2_conn.bNrChannels;
+ if (val)
+ *val = !!uac2_conn.bNrChannels;
} else { /* UAC_VERSION_3 */
struct uac3_insertion_ctl_blk uac3_conn;
ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
UAC2_CS_CUR,
USB_RECIP_INTERFACE | USB_TYPE_CLASS |
USB_DIR_IN,
validx, idx, &uac3_conn,
sizeof(uac3_conn));
- val = !!uac3_conn.bmConInserted;
+ if (val)
+ *val = !!uac3_conn.bmConInserted;
}
snd_usb_unlock_shutdown(chip);
if (ret < 0) {
- if (strstr(kcontrol->id.name, "Speaker")) {
- ucontrol->value.integer.value[0] = 1;
+ if (name && strstr(name, "Speaker")) {
+ if (val)
+ *val = 1;
return 0;
}
error:
@@ -1491,6 +1492,21 @@ static int mixer_ctl_connector_get(struct snd_kcontrol
*kcontrol,
return filter_error(cval, ret);
}
+ return ret;
+}
+
+/* get the connectors status and report it as boolean type */
+static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct usb_mixer_elem_info *cval = kcontrol->private_data;
+ int ret, val;
+
+ ret = get_connector_value(cval, kcontrol->id.name, &val);
+
+ if (ret)
+ return ret;
+
ucontrol->value.integer.value[0] = val;
return 0;
}
--
2.30.2
[PATCH] PCI: Disable D3cold support on Intel XMM7360
On some platforms, the root port for Intel XMM7360 WWAN supports D3cold.
When the root port is put to D3cold by system suspend or runtime
suspend, attempt to systems resume or runtime resume will freeze the
laptop for a while, then it automatically shuts down.
The root cause is unclear for now, as the Intel XMM7360 doesn't have a
driver yet.
So disable D3cold for XMM7360 as a workaround, until proper device
driver is in place.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212419
Signed-off-by: Kai-Heng Feng
---
drivers/pci/quirks.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..8ca8153e4a19 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5612,3 +5612,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_no_d3cold(struct pci_dev *pdev)
+{
+ pci_info(pdev, "disable D3cold\n");
+ pci_d3cold_disable(pdev);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x7360, pci_fixup_no_d3cold);
--
2.30.2
Re: [PATCH][RESEND] Revert "PM: ACPI: reboot: Use S5 for reboot"
On Fri, Mar 19, 2021 at 3:02 AM Josef Bacik wrote: > > On 3/18/21 1:42 AM, Kai-Heng Feng wrote: > > On Thu, Mar 18, 2021 at 1:25 AM Josef Bacik wrote: > > [snipped] > >> "shutdown now" works fine with and without your patch. Thanks, > > > > Rafael, > > Please revert the patch while we are working on it. > > > > Josef, > > Can you please test the following patch: > > > > That made it work fine. Thanks, So there are things depend on reboot or shutdown to carry out different behaviors. Can you please find whether there is any code path uses SYSTEM_RESTART or SYSTEM_POWER_OFF that leads to different behavior? Most likely happen in a driver's .shutdown callback. Kai-Heng > > Josef
Re: [PATCH][RESEND] Revert "PM: ACPI: reboot: Use S5 for reboot"
On Thu, Mar 18, 2021 at 1:25 AM Josef Bacik wrote:
[snipped]
> "shutdown now" works fine with and without your patch. Thanks,
Rafael,
Please revert the patch while we are working on it.
Josef,
Can you please test the following patch:
diff --git a/kernel/reboot.c b/kernel/reboot.c
index eb1b15850761..263444a3fb38 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -233,6 +233,15 @@ void migrate_to_reboot_cpu(void)
set_cpus_allowed_ptr(current, cpumask_of(cpu));
}
+static void kernel_shutdown_prepare(enum system_states state)
+{
+ blocking_notifier_call_chain(&reboot_notifier_list,
+ (state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
+ system_state = state;
+ usermodehelper_disable();
+ device_shutdown();
+}
+
/**
* kernel_restart - reboot the system
* @cmd: pointer to buffer containing command to execute for restart
@@ -243,7 +252,7 @@ void migrate_to_reboot_cpu(void)
*/
void kernel_restart(char *cmd)
{
- kernel_restart_prepare(cmd);
+ kernel_shutdown_prepare(SYSTEM_POWER_OFF);
if (pm_power_off_prepare)
pm_power_off_prepare();
migrate_to_reboot_cpu();
@@ -257,14 +266,6 @@ void kernel_restart(char *cmd)
}
EXPORT_SYMBOL_GPL(kernel_restart);
-static void kernel_shutdown_prepare(enum system_states state)
-{
- blocking_notifier_call_chain(&reboot_notifier_list,
- (state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
- system_state = state;
- usermodehelper_disable();
- device_shutdown();
-}
/**
* kernel_halt - halt the system
*
>
> Josef
Re: [PATCH][RESEND] Revert "PM: ACPI: reboot: Use S5 for reboot"
On Wed, Mar 17, 2021 at 11:19 PM Josef Bacik wrote:
>
> On 3/16/21 10:50 PM, Kai-Heng Feng wrote:
> > Hi,
> >
> > On Wed, Mar 17, 2021 at 10:17 AM Josef Bacik wrote:
> >>
> >> This reverts commit d60cd06331a3566d3305b3c7b566e79edf4e2095.
> >>
> >> This patch causes a panic when rebooting my Dell Poweredge r440. I do
> >> not have the full panic log as it's lost at that stage of the reboot and
> >> I do not have a serial console. Reverting this patch makes my system
> >> able to reboot again.
> >
> > But this patch also helps many HP laptops, so maybe we should figure
> > out what's going on on Poweredge r440.
> > Does it also panic on shutdown?
> >
>
> Sure I'll test whatever to get it fixed, but I just wasted 3 days bisecting
> and
> lost a weekend of performance testing on btrfs because of this regression, so
> until you figure out how it broke it needs to be reverted so people don't have
> to figure out why reboot suddenly isn't working.
That's unfortunate to hear. However, I've been spending tons of time
on bisecting kernels. To me it's just a normal part of kernel
development so I won't call it "wasted".
Feel free to revert the patch though.
>
> Running "halt" has the same effect with and without your patch, it gets to
> "system halted" and just sits there without powering off. Not entirely sure
> why
> that is, but there's no panic.
What about shutdown? pm_power_off_prepare() is used by shutdown but
it's not used by halt.
Kai-Heng
>
> The panic itself is lost, but I see there's an NMI and I have the RIP
>
> (gdb) list *('mwait_idle_with_hints.constprop.0'+0x4b)
> 0x816dabdb is in mwait_idle_with_hints
> (./arch/x86/include/asm/current.h:15).
> 10
> 11 DECLARE_PER_CPU(struct task_struct *, current_task);
> 12
> 13 static __always_inline struct task_struct *get_current(void)
> 14 {
> 15 return this_cpu_read_stable(current_task);
> 16 }
> 17
> 18 #define current get_current()
> 19
>
> :jmp0x936dac02
>
> :nopl (%rax)
> :jmp0x936dabac
>
> :nopl (%rax)
> :mfence
> :mov%gs:0x17bc0,%rax
> : clflush (%rax)
> : mfence
> : xor%edx,%edx
> : mov%rdx,%rcx
> : mov%gs:0x17bc0,%rax
> : monitor %rax,%rcx,%rdx
> : mov(%rax),%rax
> : test $0x8,%al
> : jne0x936dabdb
>
> : jmpq 0x936dabd0
>
> : verw 0x9f9fec(%rip)#
> 0x940d4bbc
> : mov$0x1,%ecx
> : mov%rdi,%rax
> : mwait %rax,%rcx
> : mov%gs:0x17bc0,%rax
> : lock andb $0xdf,0x2(%rax)
> : lock addl $0x0,-0x4(%rsp)
> : mov(%rax),%rax
> : test $0x8,%al
> : je 0x936dac01
>
> : andl
> $0x7fff,%gs:0x6c93cf7f(%rip)# 0x17b80
> : retq
> : mov%gs:0x17bc0,%rax
> : lock orb $0x20,0x2(%rax)
> : mov(%rax),%rax
> : test $0x8,%al
> : jne0x936dabdb
>
> : jmpq 0x936dab95
>
> : nopl 0x0(%rax)
>
> 0x4b is after the mwait, which means we're panicing in the
> current_clr_polling(), where we do clear_thread_flag(TIF_POLLING_NRFLAG).
> Thanks,
>
> Josef
Re: [PATCH][RESEND] Revert "PM: ACPI: reboot: Use S5 for reboot"
Hi,
On Wed, Mar 17, 2021 at 10:17 AM Josef Bacik wrote:
>
> This reverts commit d60cd06331a3566d3305b3c7b566e79edf4e2095.
>
> This patch causes a panic when rebooting my Dell Poweredge r440. I do
> not have the full panic log as it's lost at that stage of the reboot and
> I do not have a serial console. Reverting this patch makes my system
> able to reboot again.
But this patch also helps many HP laptops, so maybe we should figure
out what's going on on Poweredge r440.
Does it also panic on shutdown?
Kai-Heng
>
> Signed-off-by: Josef Bacik
> ---
> - apologies, I mistyped the lkml list email.
>
> kernel/reboot.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index eb1b15850761..a6ad5eb2fa73 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -244,8 +244,6 @@ void migrate_to_reboot_cpu(void)
> void kernel_restart(char *cmd)
> {
> kernel_restart_prepare(cmd);
> - if (pm_power_off_prepare)
> - pm_power_off_prepare();
> migrate_to_reboot_cpu();
> syscore_shutdown();
> if (!cmd)
> --
> 2.26.2
>
Re: [PATCH] ALSA: usb-audio: Disable USB autosuspend properly in setup_disable_autosuspend()
Hi Joakim,
On Thu, Mar 4, 2021 at 5:50 PM Joakim Tjernlund
wrote:
>
> On Thu, 2021-03-04 at 12:34 +0800, Kai-Heng Feng wrote:
> > Rear audio on Lenovo ThinkStation P620 stops working after commit
> > 1965c4364bdd ("ALSA: usb-audio: Disable autosuspend for Lenovo
> > ThinkStation P620"):
> > [6.013526] usbcore: registered new interface driver snd-usb-audio
> > [6.023064] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
> > wIndex = 0x0, type = 1
> > [6.023083] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
> > wIndex = 0x0, type = 4
> > [6.023090] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
> > wIndex = 0x0, type = 1
> > [6.023098] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
> > wIndex = 0x0, type = 4
> > [6.023103] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
> > wIndex = 0x0, type = 1
> > [6.023110] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
> > wIndex = 0x0, type = 4
> > [6.045846] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
> > wIndex = 0x0, type = 1
> > [6.045866] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
> > wIndex = 0x0, type = 4
> > [6.045877] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
> > wIndex = 0x0, type = 1
> > [6.045886] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
> > wIndex = 0x0, type = 4
> > [6.045894] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
> > wIndex = 0x0, type = 1
> > [6.045908] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
> > wIndex = 0x0, type = 4
> >
> > I overlooked the issue because when I was working on the said commit,
> > only the front audio is tested. Apology for that.
> >
> > Changing supports_autosuspend in driver is too late for disabling
> > autosuspend, because it was already used by USB probe routine, so it can
> > break the balance on the following code that depends on
> > supports_autosuspend.
> >
> > Fix it by using usb_disable_autosuspend() helper, and balance the
> > suspend count in disconnect callback.
> >
> > Fixes: 1965c4364bdd ("ALSA: usb-audio: Disable autosuspend for Lenovo
> > ThinkStation P620")
> > Signed-off-by: Kai-Heng Feng
>
> I got an report from a co-worker who has no USB sound from a Lenovo ThinkPad
> in a Ultra Dock.
> USB HS is connected to Dock USB jack.
> Could this be the same problem?
It's a different issue. Please file a separate bug report.
Kai-Heng
>
> Jocke
Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
On Sat, Feb 27, 2021 at 2:17 AM Bjorn Helgaas 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 > > > wrote: > > >> > > >> On 26.02.2021 08:12, Kalle Valo wrote: > > >>> Kai-Heng Feng writes: > > >>> > > >>>> Now we have a generic D3 shutdown quirk, so convert the original > > >>>> approach to a PCI quirk. > > >>>> > > >>>> Signed-off-by: Kai-Heng Feng > > >>>> --- > > >>>> 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 > > >> > > >> 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-2b4d4e8f0...@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
[PATCH] ALSA: usb-audio: Disable USB autosuspend properly in setup_disable_autosuspend()
Rear audio on Lenovo ThinkStation P620 stops working after commit
1965c4364bdd ("ALSA: usb-audio: Disable autosuspend for Lenovo
ThinkStation P620"):
[6.013526] usbcore: registered new interface driver snd-usb-audio
[6.023064] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
wIndex = 0x0, type = 1
[6.023083] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
wIndex = 0x0, type = 4
[6.023090] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
wIndex = 0x0, type = 1
[6.023098] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
wIndex = 0x0, type = 4
[6.023103] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
wIndex = 0x0, type = 1
[6.023110] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
wIndex = 0x0, type = 4
[6.045846] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
wIndex = 0x0, type = 1
[6.045866] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
wIndex = 0x0, type = 4
[6.045877] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
wIndex = 0x0, type = 1
[6.045886] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
wIndex = 0x0, type = 4
[6.045894] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x100,
wIndex = 0x0, type = 1
[6.045908] usb 3-6: cannot get ctl value: req = 0x81, wValue = 0x202,
wIndex = 0x0, type = 4
I overlooked the issue because when I was working on the said commit,
only the front audio is tested. Apology for that.
Changing supports_autosuspend in driver is too late for disabling
autosuspend, because it was already used by USB probe routine, so it can
break the balance on the following code that depends on
supports_autosuspend.
Fix it by using usb_disable_autosuspend() helper, and balance the
suspend count in disconnect callback.
Fixes: 1965c4364bdd ("ALSA: usb-audio: Disable autosuspend for Lenovo
ThinkStation P620")
Signed-off-by: Kai-Heng Feng
---
sound/usb/card.c | 5 +
sound/usb/quirks.c | 2 +-
sound/usb/usbaudio.h | 1 +
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/usb/card.c b/sound/usb/card.c
index 85ed8507e41a..08c794883299 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -830,6 +830,8 @@ static int usb_audio_probe(struct usb_interface *intf,
snd_media_device_create(chip, intf);
}
+ chip->quirk_type = quirk->type;
+
usb_chip[chip->index] = chip;
chip->intf[chip->num_interfaces] = intf;
chip->num_interfaces++;
@@ -912,6 +914,9 @@ static void usb_audio_disconnect(struct usb_interface *intf)
} else {
mutex_unlock(®ister_mutex);
}
+
+ if (chip->quirk_type & QUIRK_SETUP_DISABLE_AUTOSUSPEND)
+ usb_enable_autosuspend(interface_to_usbdev(intf));
}
/* lock the shutdown (disconnect) task and autoresume */
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index 9ba4682ebc48..ef5ee899db26 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -547,7 +547,7 @@ static int setup_disable_autosuspend(struct snd_usb_audio
*chip,
struct usb_driver *driver,
const struct snd_usb_audio_quirk *quirk)
{
- driver->supports_autosuspend = 0;
+ usb_disable_autosuspend(interface_to_usbdev(iface));
return 1; /* Continue with creating streams and mixer */
}
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index 215c1771dd57..60b9dd7df6bb 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -27,6 +27,7 @@ struct snd_usb_audio {
struct snd_card *card;
struct usb_interface *intf[MAX_CARD_INTERFACES];
u32 usb_id;
+ uint16_t quirk_type;
struct mutex mutex;
unsigned int system_suspend;
atomic_t active;
--
2.30.0
Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
On Fri, Feb 26, 2021 at 8:10 PM Heiner Kallweit wrote: > > On 26.02.2021 08:12, Kalle Valo wrote: > > Kai-Heng Feng writes: > > > >> Now we have a generic D3 shutdown quirk, so convert the original > >> approach to a PCI quirk. > >> > >> Signed-off-by: Kai-Heng Feng > >> --- > >> 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 > > > > 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-2b4d4e8f0...@linux.intel.com/ Kai-Heng
[PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
Now we have a generic D3 shutdown quirk, so convert the original approach to a PCI quirk. Signed-off-by: Kai-Heng Feng --- 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
[PATCH 2/3] PCI: Set AMD Renoir USB controller to D3 when shutdown
From: Aaron Ma
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
Signed-off-by: Kai-Heng Feng
---
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
#include
#include
+#include
#include/* 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
[PATCH 1/3] PCI: Introduce quirk hook after driver shutdown callback
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
Signed-off-by: Kai-Heng Feng
---
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
Re: [PATCH] efifb: Ensure graphics device for efifb stays at PCI D0
On Mon, Feb 1, 2021 at 11:21 PM Alex Deucher wrote: > > On Sat, Jan 30, 2021 at 6:27 AM Kai-Heng Feng > wrote: > > > > We are seeing root ports on some desktop boards support D3cold for > > discrete graphics card. So when efifb is in use while graphics device > > isn't bound to a driver, PCI and ACPI will put the graphics to D3cold > > when runtime suspend kicks in, makes efifb stop working. > > > > So ensure the graphics device won't be runtime suspended, to keep efifb > > work all the time. > > > > Signed-off-by: Kai-Heng Feng > > Reviewed-by: Alex Deucher A gentle ping... > > > --- > > drivers/video/fbdev/efifb.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c > > index e57c00824965..19edd7206409 100644 > > --- a/drivers/video/fbdev/efifb.c > > +++ b/drivers/video/fbdev/efifb.c > > @@ -16,6 +16,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include /* For drm_get_panel_orientation_quirk */ > > @@ -575,6 +576,7 @@ static int efifb_probe(struct platform_device *dev) > > goto err_fb_dealoc; > > } > > fb_info(info, "%s frame buffer device\n", info->fix.id); > > + pm_runtime_get_sync(&efifb_pci_dev->dev); > > return 0; > > > > err_fb_dealoc: > > @@ -601,6 +603,7 @@ static int efifb_remove(struct platform_device *pdev) > > unregister_framebuffer(info); > > sysfs_remove_groups(&pdev->dev.kobj, efifb_groups); > > framebuffer_release(info); > > + pm_runtime_put(&efifb_pci_dev->dev); > > > > return 0; > > } > > -- > > 2.29.2 > > > > ___ > > dri-devel mailing list > > dri-de...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] xhci-pci: Set AMD Renoir USB controller to D3 when shutdown
On Fri, Feb 19, 2021 at 4:07 PM Aaron Ma wrote: > > > > On 2/11/21 8:50 PM, Greg Kroah-Hartman wrote: > > On Wed, Feb 10, 2021 at 03:13:30PM +0200, Mathias Nyman wrote: > >> On 9.2.2021 10.37, Greg Kroah-Hartman wrote: > >>> On Fri, Feb 05, 2021 at 02:50:15PM +0800, Kai-Heng Feng wrote: > >>>> On Fri, Feb 5, 2021 at 2:45 PM Aaron Ma wrote: > >>>>> > >>>>> > >>>>> On 2/5/21 12:27 PM, Kai-Heng Feng wrote: > >>>>>> Can you please test the following patch, which should address the root > >>>>>> cause: > >>>>>> https://lore.kernel.org/linux-acpi/20201201213019.1558738-1-furq...@google.com/ > >>>>>> > >>>>>> It also helps another AMD laptop on S5: > >>>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1912935 > >>>>>> > >>>>> > >>>>> No, this patch doesn't help on ThinkPad AMD platform. > >>>> > >>>> Thanks for the confirmation! > >>>> > >>>> Acked-by: Kai-Heng Feng > >>> > >>> Mathias, want me to take this in my tree now, or are you going to send > >>> me more patches for 5.12-rc1? > >>> > >> > >> Nothing more for 5.12-rc1 from me. > >> > >> Could this be a PCI quirk instead of xhci? > >> Maybe there is some PCI flag for this already, haven't checked yet. > >> > >> We want a specific PCI device to go to PCI D3cold at PCI shutdown... > > > > There probably is. Kay-Heng, can you look into doing that instead? > > > > There is no such PCI quirk, usually it calls driver to shutdown. Let me work on it. There are other devices need to be in D3 for shutdown, a generic approach across all devices will be better. Kai-Heng > > Regards, > Aaron > > > thanks, > > > > greg k-h > >
Re: [PATCH 1/2] PCI/AER: Disable AER interrupt during suspend
On Fri, Feb 5, 2021 at 7:28 AM Bjorn Helgaas wrote:
>
> [+cc Alex]
>
> On Thu, Jan 28, 2021 at 12:09:37PM +0800, Kai-Heng Feng wrote:
> > On Thu, Jan 28, 2021 at 4:51 AM Bjorn Helgaas wrote:
> > > On Thu, Jan 28, 2021 at 01:31:00AM +0800, Kai-Heng Feng wrote:
> > > > Commit 50310600ebda ("iommu/vt-d: Enable PCI ACS for platform opt in
> > > > hint") enables ACS, and some platforms lose its NVMe after resume from
> > > > firmware:
> > > > [ 50.947816] pcieport :00:1b.0: DPC: containment event,
> > > > status:0x1f01 source:0x
> > > > [ 50.947817] pcieport :00:1b.0: DPC: unmasked uncorrectable error
> > > > detected
> > > > [ 50.947829] pcieport :00:1b.0: PCIe Bus Error:
> > > > severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Receiver ID)
> > > > [ 50.947830] pcieport :00:1b.0: device [8086:06ac] error
> > > > status/mask=0020/0001
> > > > [ 50.947831] pcieport :00:1b.0:[21] ACSViol
> > > > (First)
> > > > [ 50.947841] pcieport :00:1b.0: AER: broadcast error_detected
> > > > message
> > > > [ 50.947843] nvme nvme0: frozen state error detected, reset controller
> > > >
> > > > It happens right after ACS gets enabled during resume.
> > > >
> > > > To prevent that from happening, disable AER interrupt and enable it on
> > > > system suspend and resume, respectively.
> > >
> > > Lots of questions here. Maybe this is what we'll end up doing, but I
> > > am curious about why the error is reported in the first place.
> > >
> > > Is this a consequence of the link going down and back up?
> >
> > Could be. From the observations, it only happens when firmware suspend
> > (S3) is used.
> > Maybe it happens when it's gets powered up, but I don't have equipment
> > to debug at hardware level.
> >
> > If we use non-firmware suspend method, enabling ACS after resume won't
> > trip AER and DPC.
> >
> > > Is it consequence of the device doing a DMA when it shouldn't?
> >
> > If it's doing DMA while suspending, the same error should also happen
> > after NVMe is suspended and before PCIe port suspending.
> > Furthermore, if non-firmware suspend method is used, there's so such
> > issue, so less likely to be any DMA operation.
> >
> > > Are we doing something in the wrong order during suspend? Or maybe
> > > resume, since I assume the error is reported during resume?
> >
> > Yes the error is reported during resume. The suspend/resume order
> > seems fine as non-firmware suspend doesn't have this issue.
>
> I really feel like we need a better understanding of what's going on
> here. Disabling the AER interrupt is like closing our eyes and
> pretending that because we don't see it, it didn't happen.
>
> An ACS error is triggered by a DMA, right? I'm assuming an MMIO
> access from the CPU wouldn't trigger this error. And it sounds like
> the error is triggered before we even start running the driver after
> resume.
>
> If we're powering up an NVMe device from D3cold and it DMAs before the
> driver touches it, something would be seriously broken. I doubt
> that's what's happening. Maybe a device could resume some previously
> programmed DMA after powering up from D3hot.
I am not that familiar with PCIe ACS/AER/DPC, so I can't really answer
questions you raised.
PCIe spec doesn't say the suspend/resume order is also not helping here.
However, I really think it's a system firmware issue.
I've seen some suspend-to-idle platforms with NVMe can reach D3cold,
those are unaffected.
>
> Or maybe the error occurred on suspend, like if the device wasn't
> quiesced or something, but we didn't notice it until resume? The
> AER error status bits are RW1CS, which means they can be preserved
> across hot/warm/cold resets.
>
> Can you instrument the code to see whether the AER error status bit is
> set before enabling ACS? I'm not sure that merely enabling ACS (I
> assume you mean pci_std_enable_acs(), where we write PCI_ACS_CTRL)
> should cause an interrupt for a previously-logged error. I suspect
> that could happen when enabling *AER*, but I wouldn't think it would
> happen when enabling *ACS*.
Diff to print AER status:
https://bugzilla.kernel.org/show_bug.cgi?id=209149#c11
And dmesg:
https://bugzilla.kernel.org/show_bug.cgi?id=209149#c12
Looks like the read before suspend and after r
Re: [PATCH] xhci-pci: Set AMD Renoir USB controller to D3 when shutdown
On Fri, Feb 5, 2021 at 2:45 PM Aaron Ma wrote: > > > On 2/5/21 12:27 PM, Kai-Heng Feng wrote: > > Can you please test the following patch, which should address the root > > cause: > > https://lore.kernel.org/linux-acpi/20201201213019.1558738-1-furq...@google.com/ > > > > It also helps another AMD laptop on S5: > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1912935 > > > > No, this patch doesn't help on ThinkPad AMD platform. Thanks for the confirmation! Acked-by: Kai-Heng Feng > > Aaron > > > We don't need to put bandage on drivers one by one once the patch with > > alternative approach is in upstream. > > > > Kai-Heng
Re: [PATCH] xhci-pci: Set AMD Renoir USB controller to D3 when shutdown
On Thu, Feb 4, 2021 at 1:20 PM Aaron Ma wrote:
>
> 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.
Can you please test the following patch, which should address the root cause:
https://lore.kernel.org/linux-acpi/20201201213019.1558738-1-furq...@google.com/
It also helps another AMD laptop on S5:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1912935
We don't need to put bandage on drivers one by one once the patch with
alternative approach is in upstream.
Kai-Heng
>
> Signed-off-by: Aaron Ma
> ---
> drivers/usb/host/xhci-pci.c | 8
> drivers/usb/host/xhci.h | 1 +
> 2 files changed, 9 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 84da8406d5b4..a31be1ba927f 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -62,6 +62,7 @@
> #define PCI_DEVICE_ID_AMD_PROMONTORYA_30x43ba
> #define PCI_DEVICE_ID_AMD_PROMONTORYA_20x43bb
> #define PCI_DEVICE_ID_AMD_PROMONTORYA_10x43bc
> +#define PCI_DEVICE_ID_AMD_RENOIR_USB31 0x1639
> #define PCI_DEVICE_ID_ASMEDIA_1042_XHCI0x1042
> #define PCI_DEVICE_ID_ASMEDIA_1042A_XHCI 0x1142
> #define PCI_DEVICE_ID_ASMEDIA_1142_XHCI0x1242
> @@ -171,6 +172,10 @@ static void xhci_pci_quirks(struct device *dev, struct
> xhci_hcd *xhci)
> if (pdev->vendor == PCI_VENDOR_ID_AMD)
> xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>
> + if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> + pdev->device == PCI_DEVICE_ID_AMD_RENOIR_USB31)
> + xhci->quirks |= XHCI_SHUTDOWN_D3COLD;
> +
> if ((pdev->vendor == PCI_VENDOR_ID_AMD) &&
> ((pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_4) ||
> (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_3) ||
> @@ -594,6 +599,9 @@ static void xhci_pci_shutdown(struct usb_hcd *hcd)
> /* Yet another workaround for spurious wakeups at shutdown with HSW */
> if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
> pci_set_power_state(pdev, PCI_D3hot);
> +
> + if (xhci->quirks & XHCI_SHUTDOWN_D3COLD)
> + pci_set_power_state(pdev, PCI_D3cold);
> }
> #endif /* CONFIG_PM */
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 25e57bc9c3cc..0684193da4bd 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1883,6 +1883,7 @@ struct xhci_hcd {
> #define XHCI_SKIP_PHY_INIT BIT_ULL(37)
> #define XHCI_DISABLE_SPARSEBIT_ULL(38)
> #define XHCI_SG_TRB_CACHE_SIZE_QUIRK BIT_ULL(39)
> +#define XHCI_SHUTDOWN_D3COLD BIT_ULL(40)
>
> unsigned intnum_active_eps;
> unsigned intlimit_active_eps;
> --
> 2.30.0
>
Re: [PATCH] drivers: core: Detach device from power domain on shutdown
On Thu, Feb 4, 2021 at 12:09 PM Furquan Shaikh wrote: > > On Wed, Feb 3, 2021 at 6:37 PM Kai-Heng Feng > wrote: > > > > Hi Furquan, > > > > On Wed, Jan 13, 2021 at 10:31 PM Dmitry Osipenko wrote: > > [snipped] > > > Thank you all for addressing this problem! > > > > Are you still working on the alternate solution? > > Yes, it is in my pipeline, but I have been distracted because of some > other high priority tasks. I plan to push something for review in ~3-4 > weeks. Please Cc me in the revised patch. Thanks for your work. Kai-Heng > > > This patch can > > address S5 power consumption issue for some laptops: > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1912935 > > > > Kai-Heng
Re: [PATCH] drivers: core: Detach device from power domain on shutdown
Hi Furquan, On Wed, Jan 13, 2021 at 10:31 PM Dmitry Osipenko wrote: [snipped] > Thank you all for addressing this problem! Are you still working on the alternate solution? This patch can address S5 power consumption issue for some laptops: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1912935 Kai-Heng
Re: [PATCH] rtw88: 8821c: Add RFE 2 support
On Tue, Feb 2, 2021 at 3:02 PM Larry Finger wrote: > > On 2/2/21 12:29 AM, Kalle Valo wrote: > > Kai-Heng Feng writes: > > > >> On Wed, Aug 5, 2020 at 7:24 PM Kai-Heng Feng > >> wrote: > >>> > >>> Hi Tony, > >>> > >>>> On Aug 5, 2020, at 19:18, Tony Chuang wrote: > >>>> > >>>>> 8821CE with RFE 2 isn't supported: > >>>>> [ 12.404834] rtw_8821ce :02:00.0: rfe 2 isn't supported > >>>>> [ 12.404937] rtw_8821ce :02:00.0: failed to setup chip efuse info > >>>>> [ 12.404939] rtw_8821ce :02:00.0: failed to setup chip information > >>>>> > >>>> > >>>> NACK > >>>> > >>>> The RFE type 2 should be working with some additional fixes. > >>>> Did you tested connecting to AP with BT paired? > >>> > >>> No, I only tested WiFi. > >>> > >>>> The antenna configuration is different with RFE type 0. > >>>> I will ask someone else to fix them. > >>>> Then the RFE type 2 modules can be supported. > >>> > >>> Good to know that, I'll be patient and wait for a real fix. > >> > >> It's been quite some time, is support for RFE type 2 ready now? > > > > It looks like this patch should add it: > > > > https://patchwork.kernel.org/project/linux-wireless/patch/20210202055012.8296-4-pks...@realtek.com/ > > > New firmware (rtw8821c_fw.bin) is also needed. That is available at > https://github.com/lwfinger/rtw88.git. Thanks. RFE2 works with the new firmware. Kai-Heng > > Larry >
[PATCH v2] r8169: Add support for another RTL8168FP
According to the vendor driver, the new chip with XID 0x54b is
essentially the same as the one with XID 0x54a, but it doesn't need the
firmware.
So add support accordingly.
Signed-off-by: Kai-Heng Feng
---
v2:
- Add phy support.
- Rebase on net-next.
drivers/net/ethernet/realtek/r8169.h| 1 +
drivers/net/ethernet/realtek/r8169_main.c | 17 +++--
drivers/net/ethernet/realtek/r8169_phy_config.c | 1 +
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.h
b/drivers/net/ethernet/realtek/r8169.h
index 7be86ef5a584..2728df46ec41 100644
--- a/drivers/net/ethernet/realtek/r8169.h
+++ b/drivers/net/ethernet/realtek/r8169.h
@@ -63,6 +63,7 @@ enum mac_version {
RTL_GIGA_MAC_VER_50,
RTL_GIGA_MAC_VER_51,
RTL_GIGA_MAC_VER_52,
+ RTL_GIGA_MAC_VER_53,
RTL_GIGA_MAC_VER_60,
RTL_GIGA_MAC_VER_61,
RTL_GIGA_MAC_VER_63,
diff --git a/drivers/net/ethernet/realtek/r8169_main.c
b/drivers/net/ethernet/realtek/r8169_main.c
index 475e6f01ea10..48697ec36e55 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -146,6 +146,7 @@ static const struct {
[RTL_GIGA_MAC_VER_50] = {"RTL8168ep/8111ep" },
[RTL_GIGA_MAC_VER_51] = {"RTL8168ep/8111ep" },
[RTL_GIGA_MAC_VER_52] = {"RTL8168fp/RTL8117", FIRMWARE_8168FP_3},
+ [RTL_GIGA_MAC_VER_53] = {"RTL8168fp/RTL8117", },
[RTL_GIGA_MAC_VER_60] = {"RTL8125A" },
[RTL_GIGA_MAC_VER_61] = {"RTL8125A",FIRMWARE_8125A_3},
/* reserve 62 for CFG_METHOD_4 in the vendor driver */
@@ -696,7 +697,7 @@ static bool rtl_is_8168evl_up(struct rtl8169_private *tp)
{
return tp->mac_version >= RTL_GIGA_MAC_VER_34 &&
tp->mac_version != RTL_GIGA_MAC_VER_39 &&
- tp->mac_version <= RTL_GIGA_MAC_VER_52;
+ tp->mac_version <= RTL_GIGA_MAC_VER_53;
}
static bool rtl_supports_eee(struct rtl8169_private *tp)
@@ -763,7 +764,9 @@ static bool name ## _check(struct rtl8169_private *tp)
static void r8168fp_adjust_ocp_cmd(struct rtl8169_private *tp, u32 *cmd, int
type)
{
/* based on RTL8168FP_OOBMAC_BASE in vendor driver */
- if (tp->mac_version == RTL_GIGA_MAC_VER_52 && type == ERIAR_OOB)
+ if (type == ERIAR_OOB &&
+ (tp->mac_version == RTL_GIGA_MAC_VER_52 ||
+tp->mac_version == RTL_GIGA_MAC_VER_53))
*cmd |= 0x7f0 << 18;
}
@@ -1238,7 +1241,7 @@ static enum rtl_dash_type rtl_check_dash(struct
rtl8169_private *tp)
case RTL_GIGA_MAC_VER_28:
case RTL_GIGA_MAC_VER_31:
return r8168dp_check_dash(tp) ? RTL_DASH_DP : RTL_DASH_NONE;
- case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
+ case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
return r8168ep_check_dash(tp) ? RTL_DASH_EP : RTL_DASH_NONE;
default:
return RTL_DASH_NONE;
@@ -1962,6 +1965,7 @@ static enum mac_version rtl8169_get_mac_version(u16 xid,
bool gmii)
{ 0x7c8, 0x608, RTL_GIGA_MAC_VER_61 },
/* RTL8117 */
+ { 0x7cf, 0x54b, RTL_GIGA_MAC_VER_53 },
{ 0x7cf, 0x54a, RTL_GIGA_MAC_VER_52 },
/* 8168EP family. */
@@ -2236,7 +2240,7 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
case RTL_GIGA_MAC_VER_38:
RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN |
RX_DMA_BURST);
break;
- case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_52:
+ case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST
| RX_EARLY_OFF);
break;
case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
@@ -2410,7 +2414,7 @@ DECLARE_RTL_COND(rtl_rxtx_empty_cond_2)
static void rtl_wait_txrx_fifo_empty(struct rtl8169_private *tp)
{
switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_52:
+ case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
rtl_loop_wait_high(tp, &rtl_txcfg_empty_cond, 100, 42);
rtl_loop_wait_high(tp, &rtl_rxtx_empty_cond, 100, 42);
break;
@@ -3669,6 +3673,7 @@ static void rtl_hw_config(struct rtl8169_private *tp)
[RTL_GIGA_MAC_VER_50] = rtl_hw_start_8168ep_2,
[RTL_GIGA_MAC_VER_51] = rtl_hw_start_8168ep_3,
[RTL_GIGA_MAC_VER_52] = rtl_hw_start_8117,
+ [RTL_GIGA_MAC_VER_53] = rtl_hw_start_8117,
[RTL_GIGA_MAC_VER_60] = rtl_hw_start_8125a_1,
[RTL_GIGA_MAC_VER_61] = rtl_hw_start_8125a_2,
[RTL_GIGA_MAC_VER_63] = rtl_hw_s
Re: [PATCH] r8169: Add support for another RTL8168FP
On Tue, Feb 2, 2021 at 2:54 AM Heiner Kallweit wrote:
>
> On 01.02.2021 17:47, Kai-Heng Feng wrote:
> > According to the vendor driver, the new chip with XID 0x54b is
> > essentially the same as the one with XID 0x54a, but it doesn't need the
> > firmware.
> >
> > So add support accordingly.
> >
> > Signed-off-by: Kai-Heng Feng
> > ---
> > drivers/net/ethernet/realtek/r8169.h | 1 +
> > drivers/net/ethernet/realtek/r8169_main.c | 21 +
> > 2 files changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169.h
> > b/drivers/net/ethernet/realtek/r8169.h
> > index 7be86ef5a584..2728df46ec41 100644
> > --- a/drivers/net/ethernet/realtek/r8169.h
> > +++ b/drivers/net/ethernet/realtek/r8169.h
> > @@ -63,6 +63,7 @@ enum mac_version {
> > RTL_GIGA_MAC_VER_50,
> > RTL_GIGA_MAC_VER_51,
> > RTL_GIGA_MAC_VER_52,
> > + RTL_GIGA_MAC_VER_53,
> > RTL_GIGA_MAC_VER_60,
> > RTL_GIGA_MAC_VER_61,
> > RTL_GIGA_MAC_VER_63,
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > b/drivers/net/ethernet/realtek/r8169_main.c
> > index a569abe7f5ef..ee1f72a9d453 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -145,6 +145,7 @@ static const struct {
> > [RTL_GIGA_MAC_VER_50] = {"RTL8168ep/8111ep" },
> > [RTL_GIGA_MAC_VER_51] = {"RTL8168ep/8111ep" },
> > [RTL_GIGA_MAC_VER_52] = {"RTL8168fp/RTL8117", FIRMWARE_8168FP_3},
> > + [RTL_GIGA_MAC_VER_53] = {"RTL8168fp/RTL8117", },
> > [RTL_GIGA_MAC_VER_60] = {"RTL8125A" },
> > [RTL_GIGA_MAC_VER_61] = {"RTL8125A",FIRMWARE_8125A_3},
> > /* reserve 62 for CFG_METHOD_4 in the vendor driver */
> > @@ -682,7 +683,7 @@ static bool rtl_is_8168evl_up(struct rtl8169_private
> > *tp)
> > {
> > return tp->mac_version >= RTL_GIGA_MAC_VER_34 &&
> > tp->mac_version != RTL_GIGA_MAC_VER_39 &&
> > -tp->mac_version <= RTL_GIGA_MAC_VER_52;
> > +tp->mac_version <= RTL_GIGA_MAC_VER_53;
> > }
> >
> > static bool rtl_supports_eee(struct rtl8169_private *tp)
> > @@ -1012,7 +1013,9 @@ static u16 rtl_ephy_read(struct rtl8169_private *tp,
> > int reg_addr)
> > static void r8168fp_adjust_ocp_cmd(struct rtl8169_private *tp, u32 *cmd,
> > int type)
> > {
> > /* based on RTL8168FP_OOBMAC_BASE in vendor driver */
> > - if (tp->mac_version == RTL_GIGA_MAC_VER_52 && type == ERIAR_OOB)
> > + if (type == ERIAR_OOB &&
> > + (tp->mac_version == RTL_GIGA_MAC_VER_52 ||
> > + tp->mac_version == RTL_GIGA_MAC_VER_53))
> > *cmd |= 0x7f0 << 18;
> > }
> >
> > @@ -1164,7 +1167,7 @@ static void rtl8168_driver_start(struct
> > rtl8169_private *tp)
> > case RTL_GIGA_MAC_VER_31:
> > rtl8168dp_driver_start(tp);
> > break;
> > - case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
> > + case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
> > rtl8168ep_driver_start(tp);
> > break;
> > default:
> > @@ -1195,7 +1198,7 @@ static void rtl8168_driver_stop(struct
> > rtl8169_private *tp)
> > case RTL_GIGA_MAC_VER_31:
> > rtl8168dp_driver_stop(tp);
> > break;
> > - case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
> > + case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
> > rtl8168ep_driver_stop(tp);
> > break;
> > default:
> > @@ -1223,7 +1226,7 @@ static bool r8168_check_dash(struct rtl8169_private
> > *tp)
> > case RTL_GIGA_MAC_VER_28:
> > case RTL_GIGA_MAC_VER_31:
> > return r8168dp_check_dash(tp);
> > - case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
> > + case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
> > return r8168ep_check_dash(tp);
> > default:
> > return false;
> > @@ -1930,6 +1933,7 @@ static enum mac_version rtl8169_get_mac_version(u16
> > xid, bool gmii)
> > { 0x7c8, 0x608, RTL_GIGA_MAC_VER_61 },
> >
> > /* RTL8117 */
> > + { 0x7cf, 0x54b, RTL_GIGA_MAC_VER_53
[PATCH] r8169: Add support for another RTL8168FP
According to the vendor driver, the new chip with XID 0x54b is
essentially the same as the one with XID 0x54a, but it doesn't need the
firmware.
So add support accordingly.
Signed-off-by: Kai-Heng Feng
---
drivers/net/ethernet/realtek/r8169.h | 1 +
drivers/net/ethernet/realtek/r8169_main.c | 21 +
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.h
b/drivers/net/ethernet/realtek/r8169.h
index 7be86ef5a584..2728df46ec41 100644
--- a/drivers/net/ethernet/realtek/r8169.h
+++ b/drivers/net/ethernet/realtek/r8169.h
@@ -63,6 +63,7 @@ enum mac_version {
RTL_GIGA_MAC_VER_50,
RTL_GIGA_MAC_VER_51,
RTL_GIGA_MAC_VER_52,
+ RTL_GIGA_MAC_VER_53,
RTL_GIGA_MAC_VER_60,
RTL_GIGA_MAC_VER_61,
RTL_GIGA_MAC_VER_63,
diff --git a/drivers/net/ethernet/realtek/r8169_main.c
b/drivers/net/ethernet/realtek/r8169_main.c
index a569abe7f5ef..ee1f72a9d453 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -145,6 +145,7 @@ static const struct {
[RTL_GIGA_MAC_VER_50] = {"RTL8168ep/8111ep" },
[RTL_GIGA_MAC_VER_51] = {"RTL8168ep/8111ep" },
[RTL_GIGA_MAC_VER_52] = {"RTL8168fp/RTL8117", FIRMWARE_8168FP_3},
+ [RTL_GIGA_MAC_VER_53] = {"RTL8168fp/RTL8117", },
[RTL_GIGA_MAC_VER_60] = {"RTL8125A" },
[RTL_GIGA_MAC_VER_61] = {"RTL8125A",FIRMWARE_8125A_3},
/* reserve 62 for CFG_METHOD_4 in the vendor driver */
@@ -682,7 +683,7 @@ static bool rtl_is_8168evl_up(struct rtl8169_private *tp)
{
return tp->mac_version >= RTL_GIGA_MAC_VER_34 &&
tp->mac_version != RTL_GIGA_MAC_VER_39 &&
- tp->mac_version <= RTL_GIGA_MAC_VER_52;
+ tp->mac_version <= RTL_GIGA_MAC_VER_53;
}
static bool rtl_supports_eee(struct rtl8169_private *tp)
@@ -1012,7 +1013,9 @@ static u16 rtl_ephy_read(struct rtl8169_private *tp, int
reg_addr)
static void r8168fp_adjust_ocp_cmd(struct rtl8169_private *tp, u32 *cmd, int
type)
{
/* based on RTL8168FP_OOBMAC_BASE in vendor driver */
- if (tp->mac_version == RTL_GIGA_MAC_VER_52 && type == ERIAR_OOB)
+ if (type == ERIAR_OOB &&
+ (tp->mac_version == RTL_GIGA_MAC_VER_52 ||
+tp->mac_version == RTL_GIGA_MAC_VER_53))
*cmd |= 0x7f0 << 18;
}
@@ -1164,7 +1167,7 @@ static void rtl8168_driver_start(struct rtl8169_private
*tp)
case RTL_GIGA_MAC_VER_31:
rtl8168dp_driver_start(tp);
break;
- case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
+ case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
rtl8168ep_driver_start(tp);
break;
default:
@@ -1195,7 +1198,7 @@ static void rtl8168_driver_stop(struct rtl8169_private
*tp)
case RTL_GIGA_MAC_VER_31:
rtl8168dp_driver_stop(tp);
break;
- case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
+ case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
rtl8168ep_driver_stop(tp);
break;
default:
@@ -1223,7 +1226,7 @@ static bool r8168_check_dash(struct rtl8169_private *tp)
case RTL_GIGA_MAC_VER_28:
case RTL_GIGA_MAC_VER_31:
return r8168dp_check_dash(tp);
- case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
+ case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
return r8168ep_check_dash(tp);
default:
return false;
@@ -1930,6 +1933,7 @@ static enum mac_version rtl8169_get_mac_version(u16 xid,
bool gmii)
{ 0x7c8, 0x608, RTL_GIGA_MAC_VER_61 },
/* RTL8117 */
+ { 0x7cf, 0x54b, RTL_GIGA_MAC_VER_53 },
{ 0x7cf, 0x54a, RTL_GIGA_MAC_VER_52 },
/* 8168EP family. */
@@ -2274,7 +2278,7 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
case RTL_GIGA_MAC_VER_38:
RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN |
RX_DMA_BURST);
break;
- case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_52:
+ case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST
| RX_EARLY_OFF);
break;
case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
@@ -2449,7 +2453,7 @@ DECLARE_RTL_COND(rtl_rxtx_empty_cond_2)
static void rtl_wait_txrx_fifo_empty(struct rtl8169_private *tp)
{
switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_52:
+ case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
rtl_loop_wait_high(tp, &rtl_txcfg_empty_cond, 100, 42);
Re: [PATCH] PM: sleep: core: Resume suspended device if direct-complete is disabled
On Thu, Jan 28, 2021 at 4:09 PM Takashi Iwai wrote:
>
> On Thu, 31 Dec 2020 07:03:19 +0100,
> Kai-Heng Feng wrote:
> >
> > HDA controller can't be runtime-suspended after commit 215a22ed31a1
> > ("ALSA: hda: Refactor codjc PM to use direct-complete optimization"),
> > which enables direct-complete for HDA codec.
> >
> > The HDA codec driver doesn't expect direct-complete will be disabled
> > after it returns a positive value from prepare() callback. So freeze()
> > is called directly when it's runtime-suspended, breaks the balance of
> > its internal codec_powered counting.
> >
> > So if a device is prepared for direct-complete but PM core breaks the
> > assumption, resume the device to keep PM operations balanced.
> >
> > Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
> > optimization")
> > Signed-off-by: Kai-Heng Feng
>
> Kai-Heng, is this fix still needed for 5.11?
No it's not needed anymore because "ALSA: hda: Balance runtime/system
PM if direct-complete is disabled" is in place.
>
> The description mentions about HD-audio controller, while the recent
> revert was the HD-audio codec, so I suppose it's still affected?
Not affected anymore if above mentioned patch is applied.
Kai-Heng
>
>
> thanks,
>
> Takashi
>
> > ---
> > drivers/base/power/main.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index 46793276598d..9c0e25a92ad0 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1849,6 +1849,10 @@ static int device_prepare(struct device *dev,
> > pm_message_t state)
> > (ret > 0 || dev->power.no_pm_callbacks) &&
> > !dev_pm_test_driver_flags(dev, DPM_FLAG_NO_DIRECT_COMPLETE);
> > spin_unlock_irq(&dev->power.lock);
> > +
> > + if (ret > 0 && !dev->power.direct_complete)
> > + pm_runtime_resume(dev);
> > +
> > return 0;
> > }
> >
> > --
> > 2.29.2
> >
Re: [PATCH 1/2] PCI/AER: Disable AER interrupt during suspend
On Thu, Jan 28, 2021 at 4:51 AM Bjorn Helgaas wrote:
>
> On Thu, Jan 28, 2021 at 01:31:00AM +0800, Kai-Heng Feng wrote:
> > Commit 50310600ebda ("iommu/vt-d: Enable PCI ACS for platform opt in
> > hint") enables ACS, and some platforms lose its NVMe after resume from
> > firmware:
> > [ 50.947816] pcieport :00:1b.0: DPC: containment event, status:0x1f01
> > source:0x
> > [ 50.947817] pcieport :00:1b.0: DPC: unmasked uncorrectable error
> > detected
> > [ 50.947829] pcieport :00:1b.0: PCIe Bus Error: severity=Uncorrected
> > (Non-Fatal), type=Transaction Layer, (Receiver ID)
> > [ 50.947830] pcieport :00:1b.0: device [8086:06ac] error
> > status/mask=0020/0001
> > [ 50.947831] pcieport :00:1b.0:[21] ACSViol(First)
> > [ 50.947841] pcieport :00:1b.0: AER: broadcast error_detected message
> > [ 50.947843] nvme nvme0: frozen state error detected, reset controller
> >
> > It happens right after ACS gets enabled during resume.
> >
> > To prevent that from happening, disable AER interrupt and enable it on
> > system suspend and resume, respectively.
>
> Lots of questions here. Maybe this is what we'll end up doing, but I
> am curious about why the error is reported in the first place.
>
> Is this a consequence of the link going down and back up?
Could be. From the observations, it only happens when firmware suspend
(S3) is used.
Maybe it happens when it's gets powered up, but I don't have equipment
to debug at hardware level.
If we use non-firmware suspend method, enabling ACS after resume won't
trip AER and DPC.
>
> Is it consequence of the device doing a DMA when it shouldn't?
If it's doing DMA while suspending, the same error should also happen
after NVMe is suspended and before PCIe port suspending.
Furthermore, if non-firmware suspend method is used, there's so such
issue, so less likely to be any DMA operation.
>
> Are we doing something in the wrong order during suspend? Or maybe
> resume, since I assume the error is reported during resume?
Yes the error is reported during resume. The suspend/resume order
seems fine as non-firmware suspend doesn't have this issue.
>
> If we *do* take the error, why doesn't DPC recovery work?
It works for the root port, but not for the NVMe drive:
[ 50.947816] pcieport :00:1b.0: DPC: containment event,
status:0x1f01 source:0x
[ 50.947817] pcieport :00:1b.0: DPC: unmasked uncorrectable error detected
[ 50.947829] pcieport :00:1b.0: PCIe Bus Error:
severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Receiver
ID)
[ 50.947830] pcieport :00:1b.0: device [8086:06ac] error
status/mask=0020/0001
[ 50.947831] pcieport :00:1b.0:[21] ACSViol(First)
[ 50.947841] pcieport :00:1b.0: AER: broadcast error_detected message
[ 50.947843] nvme nvme0: frozen state error detected, reset controller
[ 50.948400] ACPI: EC: event unblocked
[ 50.948432] xhci_hcd :00:14.0: PME# disabled
[ 50.948444] xhci_hcd :00:14.0: enabling bus mastering
[ 50.949056] pcieport :00:1b.0: PME# disabled
[ 50.949068] pcieport :00:1c.0: PME# disabled
[ 50.949416] e1000e :00:1f.6: PME# disabled
[ 50.949463] e1000e :00:1f.6: enabling bus mastering
[ 50.951606] sd 0:0:0:0: [sda] Starting disk
[ 50.951610] nvme :01:00.0: can't change power state from D3hot
to D0 (config space inaccessible)
[ 50.951730] nvme nvme0: Removing after probe failure status: -19
[ 50.952360] nvme nvme0: failed to set APST feature (-19)
[ 50.971136] snd_hda_intel :00:1f.3: PME# disabled
[ 51.089330] pcieport :00:1b.0: AER: broadcast resume message
[ 51.089345] pcieport :00:1b.0: AER: device recovery successful
But I think why recovery doesn't work for NVMe is for another discussion...
Kai-Heng
>
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209149
> > Fixes: 50310600ebda ("iommu/vt-d: Enable PCI ACS for platform opt in hint")
> > Signed-off-by: Kai-Heng Feng
> > ---
> > drivers/pci/pcie/aer.c | 18 ++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index 77b0f2c45bc0..0e9a85530ae6 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -1365,6 +1365,22 @@ static int aer_probe(struct pcie_device *dev)
> > return 0;
> > }
> >
> > +static int aer_suspend(struct pcie_device *dev)
> > +{
> > + struct aer_rpc *rpc = get_service_data(dev);
> > +
> > + aer_disable_rootport(rpc);
> > + return 0;
> > +}
> > +
> > +static
[PATCH 2/2] PCI/DPC: Disable DPC interrupt during suspend
Commit 50310600ebda ("iommu/vt-d: Enable PCI ACS for platform opt in
hint") enables ACS, and some platforms lose its NVMe after resume from
firmware:
[ 50.947816] pcieport :00:1b.0: DPC: containment event, status:0x1f01
source:0x
[ 50.947817] pcieport :00:1b.0: DPC: unmasked uncorrectable error detected
[ 50.947829] pcieport :00:1b.0: PCIe Bus Error: severity=Uncorrected
(Non-Fatal), type=Transaction Layer, (Receiver ID)
[ 50.947830] pcieport :00:1b.0: device [8086:06ac] error
status/mask=0020/0001
[ 50.947831] pcieport :00:1b.0:[21] ACSViol(First)
[ 50.947841] pcieport :00:1b.0: AER: broadcast error_detected message
[ 50.947843] nvme nvme0: frozen state error detected, reset controller
Like what previous patch does to AER, introduce new helpers to disable
DPC interrupt and enable it on system suspend and resume, respectively.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209149
Fixes: 50310600ebda ("iommu/vt-d: Enable PCI ACS for platform opt in hint")
Signed-off-by: Kai-Heng Feng
---
drivers/pci/pcie/dpc.c | 49 --
1 file changed, 38 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index e05aba86a317..d12289cb5d44 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -279,6 +279,28 @@ void pci_dpc_init(struct pci_dev *pdev)
}
}
+static void dpc_enable(struct pcie_device *dev)
+{
+ struct pci_dev *pdev = dev->port;
+ u16 cap, ctl;
+
+ pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
+ pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
+
+ ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL |
PCI_EXP_DPC_CTL_INT_EN;
+ pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
+}
+
+static void dpc_disable(struct pcie_device *dev)
+{
+ struct pci_dev *pdev = dev->port;
+ u16 ctl;
+
+ pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
+ ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
+ pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
+}
+
#define FLAG(x, y) (((x) & (y)) ? '+' : '-')
static int dpc_probe(struct pcie_device *dev)
{
@@ -299,11 +321,7 @@ static int dpc_probe(struct pcie_device *dev)
return status;
}
- pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
- pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
-
- ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL |
PCI_EXP_DPC_CTL_INT_EN;
- pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
+ dpc_enable(dev);
pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c
PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
@@ -316,14 +334,21 @@ static int dpc_probe(struct pcie_device *dev)
return status;
}
-static void dpc_remove(struct pcie_device *dev)
+static int dpc_suspend(struct pcie_device *dev)
{
- struct pci_dev *pdev = dev->port;
- u16 ctl;
+ dpc_disable(dev);
+ return 0;
+}
- pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
- ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
- pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
+static int dpc_resume(struct pcie_device *dev)
+{
+ dpc_enable(dev);
+ return 0;
+}
+
+static void dpc_remove(struct pcie_device *dev)
+{
+ dpc_disable(dev);
}
static struct pcie_port_service_driver dpcdriver = {
@@ -331,6 +356,8 @@ static struct pcie_port_service_driver dpcdriver = {
.port_type = PCIE_ANY_PORT,
.service= PCIE_PORT_SERVICE_DPC,
.probe = dpc_probe,
+ .suspend= dpc_suspend,
+ .resume = dpc_resume,
.remove = dpc_remove,
};
--
2.29.2
[PATCH 1/2] PCI/AER: Disable AER interrupt during suspend
Commit 50310600ebda ("iommu/vt-d: Enable PCI ACS for platform opt in
hint") enables ACS, and some platforms lose its NVMe after resume from
firmware:
[ 50.947816] pcieport :00:1b.0: DPC: containment event, status:0x1f01
source:0x
[ 50.947817] pcieport :00:1b.0: DPC: unmasked uncorrectable error detected
[ 50.947829] pcieport :00:1b.0: PCIe Bus Error: severity=Uncorrected
(Non-Fatal), type=Transaction Layer, (Receiver ID)
[ 50.947830] pcieport :00:1b.0: device [8086:06ac] error
status/mask=0020/0001
[ 50.947831] pcieport :00:1b.0:[21] ACSViol(First)
[ 50.947841] pcieport :00:1b.0: AER: broadcast error_detected message
[ 50.947843] nvme nvme0: frozen state error detected, reset controller
It happens right after ACS gets enabled during resume.
To prevent that from happening, disable AER interrupt and enable it on
system suspend and resume, respectively.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209149
Fixes: 50310600ebda ("iommu/vt-d: Enable PCI ACS for platform opt in hint")
Signed-off-by: Kai-Heng Feng
---
drivers/pci/pcie/aer.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 77b0f2c45bc0..0e9a85530ae6 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1365,6 +1365,22 @@ static int aer_probe(struct pcie_device *dev)
return 0;
}
+static int aer_suspend(struct pcie_device *dev)
+{
+ struct aer_rpc *rpc = get_service_data(dev);
+
+ aer_disable_rootport(rpc);
+ return 0;
+}
+
+static int aer_resume(struct pcie_device *dev)
+{
+ struct aer_rpc *rpc = get_service_data(dev);
+
+ aer_enable_rootport(rpc);
+ return 0;
+}
+
/**
* aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
* @dev: pointer to Root Port, RCEC, or RCiEP
@@ -1437,6 +1453,8 @@ static struct pcie_port_service_driver aerdriver = {
.service= PCIE_PORT_SERVICE_AER,
.probe = aer_probe,
+ .suspend= aer_suspend,
+ .resume = aer_resume,
.remove = aer_remove,
};
--
2.29.2
[PATCH] ACPI / device_sysfs: Prefer "compatible" modalias
Commit 8765c5ba1949 ("ACPI / scan: Rework modalias creation when
"compatible" is present") may create two "MODALIAS=" in uevent file if
conditions are met.
This breaks systemd-udevd, which assumes each "key" in uevent file is
unique. The internal implementation of systemd-udevd overwrites the
first MODALIAS with the second one, so its kmod rule doesn't load driver
for the first MODALIAS.
So if both ACPI modalias and OF modalias are present, use the latter
one to ensure there's only one MODALIAS.
Reference: https://github.com/systemd/systemd/pull/18163
Cc: AceLan Kao
Cc: "Rafael J. Wysocki"
Cc: Greg Kroah-Hartman
Cc: Andy Shevchenko
Suggested-by: Mika Westerberg
Fixes: 8765c5ba1949 ("ACPI / scan: Rework modalias creation when "compatible"
is present")
Signed-off-by: Kai-Heng Feng
---
drivers/acpi/device_sysfs.c | 20 ++--
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index 96869f1538b9..bfca116482b8 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -251,20 +251,12 @@ int __acpi_device_uevent_modalias(struct acpi_device
*adev,
if (add_uevent_var(env, "MODALIAS="))
return -ENOMEM;
- len = create_pnp_modalias(adev, &env->buf[env->buflen - 1],
- sizeof(env->buf) - env->buflen);
- if (len < 0)
- return len;
-
- env->buflen += len;
- if (!adev->data.of_compatible)
- return 0;
-
- if (len > 0 && add_uevent_var(env, "MODALIAS="))
- return -ENOMEM;
-
- len = create_of_modalias(adev, &env->buf[env->buflen - 1],
-sizeof(env->buf) - env->buflen);
+ if (adev->data.of_compatible)
+ len = create_of_modalias(adev, &env->buf[env->buflen - 1],
+sizeof(env->buf) - env->buflen);
+ else
+ len = create_pnp_modalias(adev, &env->buf[env->buflen - 1],
+ sizeof(env->buf) - env->buflen);
if (len < 0)
return len;
--
2.29.2
Re: [PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias
On Tue, Jan 19, 2021 at 6:34 PM Greg Kroah-Hartman wrote: > > On Tue, Jan 19, 2021 at 11:41:59AM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 04:41:48PM +0800, Kai-Heng Feng wrote: > > > On Tue, Jan 19, 2021 at 4:27 PM Greg Kroah-Hartman > > > wrote: > > > > On Tue, Jan 19, 2021 at 04:15:13PM +0800, Kai-Heng Feng wrote: > > > > ... > > > > > > Who will use OF_MODALIAS and where have you documented it? > > > > > > After this lands in mainline, I'll modify the pull request for systemd > > > to add a new rule for OF_MODALIAS. > > > I'll modify the comment on the function to document the change. > > > > I'm wondering why to have two fixes in two places instead of fixing udev to > > understand multiple MODALIAS= events? > > It's not a matter of multiple events, it's a single event with a > key/value pair with duplicate keys and different values. > > What is this event with different values supposed to be doing in > userspace? Do you want multiple invocations of `modprobe` or something > else? > > Usually a "device" only has a single "signature" that modprobe uses to > look up the correct module for. Modules can support any number of > device signatures, but traditionally it is odd to think that a device > itself can be supported by multiple modules, which is what you are > saying is happening here. > > So what should userspace do with this, and why does a device need to > have multiple module alias signatures? >From the original use case [1], I think the "compatible" modalias should be enough. Andy and Mika, what do you think? Can we remove the ACPI modalias for this case? [1] https://lwn.net/Articles/612062/ Kai-Heng > > thanks, > > greg k-h
Re: [PATCH] HID: multitouch: Apply MT_QUIRK_CONFIDENCE quirk for multi-input devices
On Tue, Jan 19, 2021 at 1:45 AM Kai-Heng Feng
wrote:
>
> Hi,
>
> On Mon, Jan 18, 2021 at 10:41 PM Benjamin Tissoires
> wrote:
> >
> > Hi,
> >
> > On Mon, Jan 18, 2021 at 2:45 PM Kai-Heng Feng
> > wrote:
> > >
> > > Palm ejection stops working on some Elan and Synaptics touchpad after
> > > commit 40d5bb87377a ("HID: multitouch: enable multi-input as a quirk for
> > > some devices").
> > >
> > > The commit changes the mt_class from MT_CLS_WIN_8 to
> > > MT_CLS_WIN_8_FORCE_MULTI_INPUT, so MT_QUIRK_CONFIDENCE isn't applied
> > > anymore.
> > >
> > > So also apply the quirk since MT_CLS_WIN_8_FORCE_MULTI_INPUT is
> > > essentially MT_CLS_WIN_8.
> > >
> > > Fixes: 40d5bb87377a ("HID: multitouch: enable multi-input as a quirk for
> > > some devices")
> > > Signed-off-by: Kai-Heng Feng
> >
> > Thanks for the patch.
> >
> > IIt seems I was too lazy to write a regression test for it, and this
> > strikes back.
> > Can you also work on a regression test for this at
> > https://gitlab.freedesktop.org/libevdev/hid-tools ?
>
> Of course. I'll do it later this week. Currently I have both affected
> Elan and Synaptics touchpad in hand.
> Will this be a blocker of getting the patch merged?
I made a pull request:
https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/105
The tests don't pass for both Elan and Synaptics device, but let's fix it there.
Kai-Heng
>
> Kai-Heng
>
> >
> > Cheers,
> > Benjamin
> >
> >
> >
> >
> > > ---
> > > drivers/hid/hid-multitouch.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > > index 0743ef51d3b2..8429ebe7097e 100644
> > > --- a/drivers/hid/hid-multitouch.c
> > > +++ b/drivers/hid/hid-multitouch.c
> > > @@ -758,7 +758,8 @@ static int mt_touch_input_mapping(struct hid_device
> > > *hdev, struct hid_input *hi,
> > > MT_STORE_FIELD(inrange_state);
> > > return 1;
> > > case HID_DG_CONFIDENCE:
> > > - if (cls->name == MT_CLS_WIN_8 &&
> > > + if ((cls->name == MT_CLS_WIN_8 ||
> > > +cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT)
> > > &&
> > > (field->application == HID_DG_TOUCHPAD ||
> > > field->application ==
> > > HID_DG_TOUCHSCREEN))
> > > app->quirks |= MT_QUIRK_CONFIDENCE;
> > > --
> > > 2.29.2
> > >
> >
[PATCH v2] ALSA: hda: Balance runtime/system PM if direct-complete is disabled
After hibernation, HDA controller can't be runtime-suspended after
commit 215a22ed31a1 ("ALSA: hda: Refactor codjc PM to use
direct-complete optimization"), which enables direct-complete for HDA
codec.
The HDA codec driver didn't expect direct-complete will be disabled
after it returns a positive value from prepare() callback. However,
there are some places that PM core can disable direct-complete. For
instance, system hibernation or when codec has subordinates like LEDs.
So if the codec is prepared for direct-complete but PM core still calls
codec's suspend or freeze callback, partially revert the commit and take
the original approach, which uses pm_runtime_force_*() helpers to
ensure PM refcount are balanced. Meanwhile, still keep prepare() and
complete() callbacks to enable direct-complete and request a resume for
jack detection, respectively.
Reported-by: Kenneth R. Crudup
Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
optimization")
Signed-off-by: Kai-Heng Feng
---
v2:
- Use pm_runtime_force_*() helpers to avoid suspend/resume ping pong.
sound/pci/hda/hda_codec.c | 24 +++-
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 687216e74526..eec1775dfffe 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2934,7 +2934,7 @@ static void hda_call_codec_resume(struct hda_codec *codec)
snd_hdac_leave_pm(&codec->core);
}
-static int hda_codec_suspend(struct device *dev)
+static int hda_codec_runtime_suspend(struct device *dev)
{
struct hda_codec *codec = dev_to_hda_codec(dev);
unsigned int state;
@@ -2953,7 +2953,7 @@ static int hda_codec_suspend(struct device *dev)
return 0;
}
-static int hda_codec_resume(struct device *dev)
+static int hda_codec_runtime_resume(struct device *dev)
{
struct hda_codec *codec = dev_to_hda_codec(dev);
@@ -2968,16 +2968,6 @@ static int hda_codec_resume(struct device *dev)
return 0;
}
-static int hda_codec_runtime_suspend(struct device *dev)
-{
- return hda_codec_suspend(dev);
-}
-
-static int hda_codec_runtime_resume(struct device *dev)
-{
- return hda_codec_resume(dev);
-}
-
#endif /* CONFIG_PM */
#ifdef CONFIG_PM_SLEEP
@@ -2998,31 +2988,31 @@ static void hda_codec_pm_complete(struct device *dev)
static int hda_codec_pm_suspend(struct device *dev)
{
dev->power.power_state = PMSG_SUSPEND;
- return hda_codec_suspend(dev);
+ return pm_runtime_force_suspend(dev);
}
static int hda_codec_pm_resume(struct device *dev)
{
dev->power.power_state = PMSG_RESUME;
- return hda_codec_resume(dev);
+ return pm_runtime_force_resume(dev);
}
static int hda_codec_pm_freeze(struct device *dev)
{
dev->power.power_state = PMSG_FREEZE;
- return hda_codec_suspend(dev);
+ return pm_runtime_force_suspend(dev);
}
static int hda_codec_pm_thaw(struct device *dev)
{
dev->power.power_state = PMSG_THAW;
- return hda_codec_resume(dev);
+ return pm_runtime_force_resume(dev);
}
static int hda_codec_pm_restore(struct device *dev)
{
dev->power.power_state = PMSG_RESTORE;
- return hda_codec_resume(dev);
+ return pm_runtime_force_resume(dev);
}
#endif /* CONFIG_PM_SLEEP */
--
2.29.2
[PATCH v2] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias
Commit 8765c5ba1949 ("ACPI / scan: Rework modalias creation when
"compatible" is present") may create two "MODALIAS=" in uevent file if
conditions are met.
This breaks systemd-udevd, which assumes each "key" in uevent file is
unique. The internal implementation of systemd-udevd overwrites the
first MODALIAS with the second one, so its kmod rule doesn't load driver
for the first MODALIAS.
Right now it doesn't seem to have any user relies on the second
MODALIAS, so change it to OF_MODALIAS to workaround the issue.
Reference: https://github.com/systemd/systemd/pull/18163
Fixes: 8765c5ba1949 ("ACPI / scan: Rework modalias creation when "compatible"
is present")
Signed-off-by: Kai-Heng Feng
---
v2:
Add a comment to document why it's changed.
drivers/acpi/device_sysfs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index 96869f1538b9..17483c40deeb 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -260,7 +260,11 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev,
if (!adev->data.of_compatible)
return 0;
- if (len > 0 && add_uevent_var(env, "MODALIAS="))
+ /* Two "MODALIAS=" breaks how systemd-udevd handles uevent file.
+* As userspace may not be able to handle duplicated keys, add prefix
+* "OF_" to avoid the key collision.
+*/
+ if (len > 0 && add_uevent_var(env, "OF_MODALIAS="))
return -ENOMEM;
len = create_of_modalias(adev, &env->buf[env->buflen - 1],
--
2.29.2
Re: [PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias
On Tue, Jan 19, 2021 at 4:27 PM Greg Kroah-Hartman
wrote:
>
> On Tue, Jan 19, 2021 at 04:15:13PM +0800, Kai-Heng Feng wrote:
> > Commit 8765c5ba1949 ("ACPI / scan: Rework modalias creation when
> > "compatible" is present") may create two "MODALIAS=" in uevent file if
> > conditions are met.
> >
> > This breaks systemd-udevd, which assumes each "key" in uevent file is
> > unique. The internal implementation of systemd-udevd overwrites the
> > first MODALIAS with the second one, so its kmod rule doesn't load driver
> > for the first MODALIAS.
> >
> > Right now it doesn't seem to have any user relies on the second
> > MODALIAS, so change it to OF_MODALIAS to workaround the issue.
> >
> > Reference: https://github.com/systemd/systemd/pull/18163
> > Fixes: 8765c5ba1949 ("ACPI / scan: Rework modalias creation when
> > "compatible" is present")
> > Cc: AceLan Kao
> > Cc: "Rafael J. Wysocki"
> > Cc: Greg Kroah-Hartman ,
> > Cc: Mika Westerberg ,
> > Cc: Andy Shevchenko
> > Signed-off-by: Kai-Heng Feng
> > ---
> > drivers/acpi/device_sysfs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > index 96869f1538b9..c92b671cb816 100644
> > --- a/drivers/acpi/device_sysfs.c
> > +++ b/drivers/acpi/device_sysfs.c
> > @@ -260,7 +260,7 @@ int __acpi_device_uevent_modalias(struct acpi_device
> > *adev,
> > if (!adev->data.of_compatible)
> > return 0;
> >
> > - if (len > 0 && add_uevent_var(env, "MODALIAS="))
> > + if (len > 0 && add_uevent_var(env, "OF_MODALIAS="))
>
> Who will use OF_MODALIAS and where have you documented it?
After this lands in mainline, I'll modify the pull request for systemd
to add a new rule for OF_MODALIAS.
I'll modify the comment on the function to document the change.
Kai-Heng
>
> thanks,
>
> greg k-h
[PATCH] ACPI / device_sysfs: Use OF_MODALIAS for "compatible" modalias
Commit 8765c5ba1949 ("ACPI / scan: Rework modalias creation when
"compatible" is present") may create two "MODALIAS=" in uevent file if
conditions are met.
This breaks systemd-udevd, which assumes each "key" in uevent file is
unique. The internal implementation of systemd-udevd overwrites the
first MODALIAS with the second one, so its kmod rule doesn't load driver
for the first MODALIAS.
Right now it doesn't seem to have any user relies on the second
MODALIAS, so change it to OF_MODALIAS to workaround the issue.
Reference: https://github.com/systemd/systemd/pull/18163
Fixes: 8765c5ba1949 ("ACPI / scan: Rework modalias creation when "compatible"
is present")
Cc: AceLan Kao
Cc: "Rafael J. Wysocki"
Cc: Greg Kroah-Hartman ,
Cc: Mika Westerberg ,
Cc: Andy Shevchenko
Signed-off-by: Kai-Heng Feng
---
drivers/acpi/device_sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index 96869f1538b9..c92b671cb816 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -260,7 +260,7 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev,
if (!adev->data.of_compatible)
return 0;
- if (len > 0 && add_uevent_var(env, "MODALIAS="))
+ if (len > 0 && add_uevent_var(env, "OF_MODALIAS="))
return -ENOMEM;
len = create_of_modalias(adev, &env->buf[env->buflen - 1],
--
2.29.2
Re: [PATCH] HID: multitouch: Apply MT_QUIRK_CONFIDENCE quirk for multi-input devices
Hi,
On Mon, Jan 18, 2021 at 10:41 PM Benjamin Tissoires
wrote:
>
> Hi,
>
> On Mon, Jan 18, 2021 at 2:45 PM Kai-Heng Feng
> wrote:
> >
> > Palm ejection stops working on some Elan and Synaptics touchpad after
> > commit 40d5bb87377a ("HID: multitouch: enable multi-input as a quirk for
> > some devices").
> >
> > The commit changes the mt_class from MT_CLS_WIN_8 to
> > MT_CLS_WIN_8_FORCE_MULTI_INPUT, so MT_QUIRK_CONFIDENCE isn't applied
> > anymore.
> >
> > So also apply the quirk since MT_CLS_WIN_8_FORCE_MULTI_INPUT is
> > essentially MT_CLS_WIN_8.
> >
> > Fixes: 40d5bb87377a ("HID: multitouch: enable multi-input as a quirk for
> > some devices")
> > Signed-off-by: Kai-Heng Feng
>
> Thanks for the patch.
>
> IIt seems I was too lazy to write a regression test for it, and this
> strikes back.
> Can you also work on a regression test for this at
> https://gitlab.freedesktop.org/libevdev/hid-tools ?
Of course. I'll do it later this week. Currently I have both affected
Elan and Synaptics touchpad in hand.
Will this be a blocker of getting the patch merged?
Kai-Heng
>
> Cheers,
> Benjamin
>
>
>
>
> > ---
> > drivers/hid/hid-multitouch.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > index 0743ef51d3b2..8429ebe7097e 100644
> > --- a/drivers/hid/hid-multitouch.c
> > +++ b/drivers/hid/hid-multitouch.c
> > @@ -758,7 +758,8 @@ static int mt_touch_input_mapping(struct hid_device
> > *hdev, struct hid_input *hi,
> > MT_STORE_FIELD(inrange_state);
> > return 1;
> > case HID_DG_CONFIDENCE:
> > - if (cls->name == MT_CLS_WIN_8 &&
> > + if ((cls->name == MT_CLS_WIN_8 ||
> > +cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT) &&
> > (field->application == HID_DG_TOUCHPAD ||
> > field->application == HID_DG_TOUCHSCREEN))
> > app->quirks |= MT_QUIRK_CONFIDENCE;
> > --
> > 2.29.2
> >
>
Re: [PATCH] ALSA: hda: Balance runtime/system PM if direct-complete is disabled
On Mon, Jan 18, 2021 at 9:21 PM Takashi Iwai wrote:
>
> On Mon, 18 Jan 2021 14:09:36 +0100,
> Kai-Heng Feng wrote:
> >
> > HDA controller can't be runtime-suspended after commit 215a22ed31a1
> > ("ALSA: hda: Refactor codjc PM to use direct-complete optimization"),
> > which enables direct-complete for HDA codec.
> >
> > The HDA codec driver didn't expect direct-complete will be disabled
> > after it returns a positive value from prepare() callback. However,
> > there are some places that PM core can disable direct-complete. For
> > instance, system hibernation or when codec has subordinates like LEDs.
>
> Hmm. This sounds rather like the approach using the direct-complete
> isn't well suited for the purpose? The increasing number of
> regression reports worries me.
Direct-complete works fine on HDA controller but so far not so on HDA codec.
I think the main reason is that the codec doesn't have the middle
layer to handle the detail, while HDA controller has PCI bus to deal
with them.
>
> > So if a device is prepared for direct-complete but PM core still calls
> > codec's suspend or freeze callback, resume the device to keep PM
> > operations balanced.
>
> I find the ping-pong of the resume/suspend there a bit odd. It's no
> refcount management but it invokes the real resume there, which is
> involved with lots of operations.
Yes. I'll find a better approach to address this.
>
> Can we rather skip the hda_codec_suspend() call instead (while
> changing dev->power.power_state)?
Maybe we can revert the most of the commit, and just leave
hda_codec_pm_complete(), which is the most relevant part of the patch.
Let me test a bit and send a new patch. Let me know if you don't like
this approach.
A question a bit unrelated to the discussion - how does
snd_hdac_power_up_pm() work for concurrent resume?
It can work for recursive call, but what if there are concurrent
resume request, but pm_runtime_get_sync() is still running?
The second call may access the codec which hasn't completed resume.
Kai-Heng
>
>
> thanks,
>
> Takashi
>
> > Reported-by: Kenneth R. Crudup
> > Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
> > optimization")
> > Signed-off-by: Kai-Heng Feng
> > ---
> > sound/pci/hda/hda_codec.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index 687216e74526..0afbced979df 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -2997,6 +2997,9 @@ static void hda_codec_pm_complete(struct device *dev)
> >
> > static int hda_codec_pm_suspend(struct device *dev)
> > {
> > + if (pm_runtime_status_suspended(dev))
> > + pm_runtime_resume(dev);
> > +
> > dev->power.power_state = PMSG_SUSPEND;
> > return hda_codec_suspend(dev);
> > }
> > @@ -3009,6 +3012,9 @@ static int hda_codec_pm_resume(struct device *dev)
> >
> > static int hda_codec_pm_freeze(struct device *dev)
> > {
> > + if (pm_runtime_status_suspended(dev))
> > + pm_runtime_resume(dev);
> > +
> > dev->power.power_state = PMSG_FREEZE;
> > return hda_codec_suspend(dev);
> > }
> > --
> > 2.29.2
> >
[PATCH] HID: multitouch: Apply MT_QUIRK_CONFIDENCE quirk for multi-input devices
Palm ejection stops working on some Elan and Synaptics touchpad after
commit 40d5bb87377a ("HID: multitouch: enable multi-input as a quirk for
some devices").
The commit changes the mt_class from MT_CLS_WIN_8 to
MT_CLS_WIN_8_FORCE_MULTI_INPUT, so MT_QUIRK_CONFIDENCE isn't applied
anymore.
So also apply the quirk since MT_CLS_WIN_8_FORCE_MULTI_INPUT is
essentially MT_CLS_WIN_8.
Fixes: 40d5bb87377a ("HID: multitouch: enable multi-input as a quirk for some
devices")
Signed-off-by: Kai-Heng Feng
---
drivers/hid/hid-multitouch.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 0743ef51d3b2..8429ebe7097e 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -758,7 +758,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev,
struct hid_input *hi,
MT_STORE_FIELD(inrange_state);
return 1;
case HID_DG_CONFIDENCE:
- if (cls->name == MT_CLS_WIN_8 &&
+ if ((cls->name == MT_CLS_WIN_8 ||
+cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT) &&
(field->application == HID_DG_TOUCHPAD ||
field->application == HID_DG_TOUCHSCREEN))
app->quirks |= MT_QUIRK_CONFIDENCE;
--
2.29.2
[PATCH] ALSA: hda: Balance runtime/system PM if direct-complete is disabled
HDA controller can't be runtime-suspended after commit 215a22ed31a1
("ALSA: hda: Refactor codjc PM to use direct-complete optimization"),
which enables direct-complete for HDA codec.
The HDA codec driver didn't expect direct-complete will be disabled
after it returns a positive value from prepare() callback. However,
there are some places that PM core can disable direct-complete. For
instance, system hibernation or when codec has subordinates like LEDs.
So if a device is prepared for direct-complete but PM core still calls
codec's suspend or freeze callback, resume the device to keep PM
operations balanced.
Reported-by: Kenneth R. Crudup
Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
optimization")
Signed-off-by: Kai-Heng Feng
---
sound/pci/hda/hda_codec.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 687216e74526..0afbced979df 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2997,6 +2997,9 @@ static void hda_codec_pm_complete(struct device *dev)
static int hda_codec_pm_suspend(struct device *dev)
{
+ if (pm_runtime_status_suspended(dev))
+ pm_runtime_resume(dev);
+
dev->power.power_state = PMSG_SUSPEND;
return hda_codec_suspend(dev);
}
@@ -3009,6 +3012,9 @@ static int hda_codec_pm_resume(struct device *dev)
static int hda_codec_pm_freeze(struct device *dev)
{
+ if (pm_runtime_status_suspended(dev))
+ pm_runtime_resume(dev);
+
dev->power.power_state = PMSG_FREEZE;
return hda_codec_suspend(dev);
}
--
2.29.2
Re: Multiple MODALIAS= in uevent file confuses userspace
On Sat, Jan 9, 2021 at 12:25 AM Kai-Heng Feng
wrote:
>
> Commit 8765c5ba19490 ("ACPI / scan: Rework modalias creation when
> "compatible" is present") creates two modaliases for certain ACPI
> devices. However userspace (systemd-udevd in this case) assumes uevent
> file doesn't have duplicated keys, so two "MODALIAS=" breaks the
> assumption.
>
> Based on the assumption, systemd-udevd internally uses hashmap to
> store each line of uevent file, so the second modalias always replaces
> the first modalias.
>
> My attempt [1] is to add a new key, "MODALIAS1" for the second
> modalias. This brings up the question of whether each key in uevent
> file is unique. If it's no unique, this may break may userspace.
Does anyone know if there's any user of the second modalias?
If there's no user of the second one, can we change it to OF_MODALIAS
or COMPAT_MODALIAS?
Kai-Heng
>
> [1] https://github.com/systemd/systemd/pull/18163
>
> Kai-Heng
[PATCH v4 2/3] ASoC: SOF: Intel: hda: Modify existing helper to disable WAKEEN
Modify hda_codec_jack_wake_enable() to also support disable WAKEEN.
In addition, this patch also moves the WAKEEN disablement call out of
hda_codec_jack_check() into hda_codec_jack_wake_enable().
This is a preparation for next patch.
No functional change intended.
Signed-off-by: Kai-Heng Feng
---
v3 v4:
No change.
v2:
Mention it moves the disabling part into another function.
sound/soc/sof/intel/hda-codec.c | 16 +++-
sound/soc/sof/intel/hda-dsp.c | 6 --
sound/soc/sof/intel/hda.h | 2 +-
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index df59c79cfdfc..b7e9931ead57 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -63,16 +63,18 @@ static int hda_codec_load_module(struct hda_codec *codec)
}
/* enable controller wake up event for all codecs with jack connectors */
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable)
{
struct hda_bus *hbus = sof_to_hbus(sdev);
struct hdac_bus *bus = sof_to_bus(sdev);
struct hda_codec *codec;
unsigned int mask = 0;
- list_for_each_codec(codec, hbus)
- if (codec->jacktbl.used)
- mask |= BIT(codec->core.addr);
+ if (enable) {
+ list_for_each_codec(codec, hbus)
+ if (codec->jacktbl.used)
+ mask |= BIT(codec->core.addr);
+ }
snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, mask);
}
@@ -81,12 +83,8 @@ void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
void hda_codec_jack_check(struct snd_sof_dev *sdev)
{
struct hda_bus *hbus = sof_to_hbus(sdev);
- struct hdac_bus *bus = sof_to_bus(sdev);
struct hda_codec *codec;
- /* disable controller Wake Up event*/
- snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0);
-
list_for_each_codec(codec, hbus)
/*
* Wake up all jack-detecting codecs regardless whether an event
@@ -96,7 +94,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev)
pm_request_resume(&codec->core.dev);
}
#else
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {}
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable) {}
void hda_codec_jack_check(struct snd_sof_dev *sdev) {}
#endif /* CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC */
EXPORT_SYMBOL_NS(hda_codec_jack_wake_enable, SND_SOC_SOF_HDA_AUDIO_CODEC);
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index 2b001151fe37..7d00107cf3b2 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -617,7 +617,7 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool
runtime_suspend)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
if (runtime_suspend)
- hda_codec_jack_wake_enable(sdev);
+ hda_codec_jack_wake_enable(sdev, true);
/* power down all hda link */
snd_hdac_ext_bus_link_power_down_all(bus);
@@ -683,8 +683,10 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
runtime_resume)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/* check jack status */
- if (runtime_resume)
+ if (runtime_resume) {
+ hda_codec_jack_wake_enable(sdev, false);
hda_codec_jack_check(sdev);
+ }
/* turn off the links that were off before suspend */
list_for_each_entry(hlink, &bus->hlink_list, list) {
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 9ec8ae0fd649..a3b6f3e9121c 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -650,7 +650,7 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device
*dev);
*/
void hda_codec_probe_bus(struct snd_sof_dev *sdev,
bool hda_codec_use_common_hdmi);
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev);
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable);
void hda_codec_jack_check(struct snd_sof_dev *sdev);
#endif /* CONFIG_SND_SOC_SOF_HDA */
--
2.29.2
[PATCH v4 3/3] ASoC: SOF: Intel: hda: Avoid checking jack on system suspend
System takes a very long time to suspend after commit 215a22ed31a1
("ALSA: hda: Refactor codec PM to use direct-complete optimization"):
[ 90.065964] PM: suspend entry (s2idle)
[ 90.067337] Filesystems sync: 0.001 seconds
[ 90.185758] Freezing user space processes ... (elapsed 0.002 seconds) done.
[ 90.188713] OOM killer disabled.
[ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001 seconds)
done.
[ 90.190024] printk: Suspending console(s) (use no_console_suspend to debug)
[ 90.904912] intel_pch_thermal :00:12.0: CPU-PCH is cool [49C], continue
to suspend
[ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 329.490933] ACPI: EC: interrupt blocked
That commit keeps the codec suspended during the system suspend. However,
mute/micmute LED will clear codec's direct-complete flag by
dpm_clear_superiors_direct_complete().
This doesn't play well with SOF driver. When its runtime resume is
called for system suspend, hda_codec_jack_check() schedules
jackpoll_work which uses snd_hdac_is_power_on() to check whether codec
is suspended. Because the direct-complete path isn't taken,
pm_runtime_disable() isn't called so snd_hdac_is_power_on() returns
false and jackpoll continues to run, and snd_hda_power_up_pm() cannot
power up an already suspended codec in multiple attempts, causes the
long delay on system suspend:
if (dev->power.direct_complete) {
if (pm_runtime_status_suspended(dev)) {
pm_runtime_disable(dev);
if (pm_runtime_status_suspended(dev)) {
pm_dev_dbg(dev, state, "direct-complete ");
goto Complete;
}
pm_runtime_enable(dev);
}
dev->power.direct_complete = false;
}
When direct-complete path is taken, snd_hdac_is_power_on() returns true
and hda_jackpoll_work() is skipped by accident. So this is still not
correct.
If we were to use snd_hdac_is_power_on() in system PM path,
pm_runtime_status_suspended() should be used instead of
pm_runtime_suspended(), otherwise pm_runtime_{enable,disable}() may
change the outcome of snd_hdac_is_power_on().
Because devices suspend in reverse order (i.e. child first), it doesn't
make much sense to resume an already suspended codec from audio
controller. So avoid the issue by making sure jackpoll isn't used in
system PM process.
Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
optimization")
Signed-off-by: Kai-Heng Feng
---
v4:
Clarify why direct-complete path isn't take.
v3:
Clarify the root cause and why it's needed.
v2:
No change.
sound/soc/sof/intel/hda-dsp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index 7d00107cf3b2..1c5e05b88a90 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -685,7 +685,8 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
runtime_resume)
/* check jack status */
if (runtime_resume) {
hda_codec_jack_wake_enable(sdev, false);
- hda_codec_jack_check(sdev);
+ if (sdev->system_suspend_target == SOF_SUSPEND_NONE)
+ hda_codec_jack_check(sdev);
}
/* turn off the links that were off before suspend */
--
2.29.2
[PATCH v4 1/3] ASoC: SOF: Intel: hda: Resume codec to do jack detection
Instead of queueing jackpoll_work, runtime resume the codec to let it
use different jack detection methods based on jackpoll_interval.
This partially matches SOF driver's behavior with commit a6e7d0a4bdb0
("ALSA: hda: fix jack detection with Realtek codecs when in D3"), the
difference is SOF unconditionally resumes the codec.
Signed-off-by: Kai-Heng Feng
---
v4:
No change.
v3:
Remove wrong assumption that only Realtek codec is used by SOF.
v2:
No change.
sound/soc/sof/intel/hda-codec.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 6875fa570c2c..df59c79cfdfc 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -93,8 +93,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev)
* has been recorded in STATESTS
*/
if (codec->jacktbl.used)
- schedule_delayed_work(&codec->jackpoll_work,
- codec->jackpoll_interval);
+ pm_request_resume(&codec->core.dev);
}
#else
void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {}
--
2.29.2
Re: [PATCH v3 1/4] ALSA: hda/realtek: Power up codec when setting LED via COEF and GPIO
On Tue, Jan 12, 2021 at 9:17 PM Takashi Iwai wrote:
>
> On Tue, 12 Jan 2021 14:06:59 +0100,
> Kai-Heng Feng wrote:
> >
> > System takes a very long time to suspend after commit 215a22ed31a1
> > ("ALSA: hda: Refactor codec PM to use direct-complete optimization"):
> > [ 90.065964] PM: suspend entry (s2idle)
> > [ 90.067337] Filesystems sync: 0.001 seconds
> > [ 90.185758] Freezing user space processes ... (elapsed 0.002 seconds)
> > done.
> > [ 90.188713] OOM killer disabled.
> > [ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001
> > seconds) done.
> > [ 90.190024] printk: Suspending console(s) (use no_console_suspend to
> > debug)
> > [ 90.904912] intel_pch_thermal :00:12.0: CPU-PCH is cool [49C],
> > continue to suspend
> > [ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
> > 0x2b8000. -5
> > [ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
> > 0x2b8000. -5
> > [ 329.490933] ACPI: EC: interrupt blocked
> >
> > That commit keeps the codec suspended during the system suspend. However,
> > led_suspend() for mute and micmute led writes codec register, triggers
> > a pending wake up. The wakeup is then handled in __device_suspend() by
> > the following:
> > - pm_runtime_disable() to handle the wakeup event.
> > - device is no longer is suspended state, direct-complete isn't taken.
> > - pm_runtime_enable() to balance disable_depth.
> >
> > if (dev->power.direct_complete) {
> > if (pm_runtime_status_suspended(dev)) {
> > pm_runtime_disable(dev);
> > if (pm_runtime_status_suspended(dev)) {
> > pm_dev_dbg(dev, state, "direct-complete ");
> > goto Complete;
> > }
> >
> > pm_runtime_enable(dev);
> > }
> > dev->power.direct_complete = false;
> > }
> >
> > Since direct-complete doens't apply anymore, the codec's system suspend
> > routine is used.
> >
> > This doesn't play well with SOF driver. When its runtime resume is
> > called for system suspend, hda_codec_jack_check() schedules
> > jackpoll_work which uses snd_hdac_is_power_on() to check whether codec
> > is suspended. Because of the previous pm_runtime_enable(),
> > snd_hdac_is_power_on() returns false and jackpoll continues to run, and
> > snd_hda_power_up_pm() cannot power up an already suspended codec in
> > multiple attempts, causes the long delay on system suspend.
> >
> > When direct-complete path is taken, snd_hdac_is_power_on() returns true
> > and hda_jackpoll_work() is skipped by accident. This is still not
> > correct, and it will be addressed by later patch.
> >
> > Explicitly runtime resume codec on setting LED to solve the issue.
> >
> > Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
> > optimization")
>
> Hmm, I really don't get this.
>
> alc_update_gpio_data() calls snd_hda_codec_write() in the end, and
> this function goes over bus->exec_verb call. And for the legacy HDA
> codec, it's codec_exec_verb in hda_codec.c, which has already
> snd_hda_power_up_pm().
>
> What's the missing piece?
Thanks for pointing this out!
The missing piece here is that the issue only happens when both mute
and micmute LED are off, so alc_update_gpio_data() doesn't do anything
to turn the LED off during suspend.
If LED is on, turning off LED will indeed resume the codec and the
issue doesn't happen.
So this patch is unnecessary. Will send v4.
Kai-Heng
>
>
>
> thanks,
>
> Takashi
>
> > ---
> > v3:
> > New patch.
> >
> > sound/pci/hda/patch_realtek.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> > index 3c1d2a3fb1a4..304a7bc89fcd 100644
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -4164,7 +4164,10 @@ static void alc_update_gpio_led(struct hda_codec
> > *codec, unsigned int mask,
> > {
> > if (polarity)
> > enabled = !enabled;
> > + /* temporarily power up/down for setting GPIO */
> > + snd_hda_power_up_pm(codec);
> > alc_update_gpio_data(codec, mask, !enabled); /* muted -> LED on */
> > + snd_hda_power_down_pm(codec);
> > }
> >
> > /* turn on/off mute LED via GPIO per vmaster hook */
> > @@ -4287,8 +4290,10 @@ static void alc_update_coef_led(struct hda_codec
> > *codec,
> > if (polarity)
> > on = !on;
> > /* temporarily power up/down for setting COEF bit */
> > + snd_hda_power_up_pm(codec);
> > alc_update_coef_idx(codec, led->idx, led->mask,
> > on ? led->on : led->off);
> > + snd_hda_power_down_pm(codec);
> > }
> >
> > /* update mute-LED according to the speaker mute state via COEF bit */
> > --
> > 2.29.2
> >
[PATCH v3 3/4] ASoC: SOF: Intel: hda: Modify existing helper to disable WAKEEN
Modify hda_codec_jack_wake_enable() to also support disable WAKEEN.
In addition, this patch also moves the WAKEEN disablement call out of
hda_codec_jack_check() into hda_codec_jack_wake_enable().
This is a preparation for next patch.
No functional change intended.
---
v3:
No change.
v2:
Mention it moves the disabling part into another function.
sound/soc/sof/intel/hda-codec.c | 16 +++-
sound/soc/sof/intel/hda-dsp.c | 6 --
sound/soc/sof/intel/hda.h | 2 +-
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index df59c79cfdfc..b7e9931ead57 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -63,16 +63,18 @@ static int hda_codec_load_module(struct hda_codec *codec)
}
/* enable controller wake up event for all codecs with jack connectors */
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable)
{
struct hda_bus *hbus = sof_to_hbus(sdev);
struct hdac_bus *bus = sof_to_bus(sdev);
struct hda_codec *codec;
unsigned int mask = 0;
- list_for_each_codec(codec, hbus)
- if (codec->jacktbl.used)
- mask |= BIT(codec->core.addr);
+ if (enable) {
+ list_for_each_codec(codec, hbus)
+ if (codec->jacktbl.used)
+ mask |= BIT(codec->core.addr);
+ }
snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, mask);
}
@@ -81,12 +83,8 @@ void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
void hda_codec_jack_check(struct snd_sof_dev *sdev)
{
struct hda_bus *hbus = sof_to_hbus(sdev);
- struct hdac_bus *bus = sof_to_bus(sdev);
struct hda_codec *codec;
- /* disable controller Wake Up event*/
- snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0);
-
list_for_each_codec(codec, hbus)
/*
* Wake up all jack-detecting codecs regardless whether an event
@@ -96,7 +94,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev)
pm_request_resume(&codec->core.dev);
}
#else
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {}
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable) {}
void hda_codec_jack_check(struct snd_sof_dev *sdev) {}
#endif /* CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC */
EXPORT_SYMBOL_NS(hda_codec_jack_wake_enable, SND_SOC_SOF_HDA_AUDIO_CODEC);
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index 2b001151fe37..7d00107cf3b2 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -617,7 +617,7 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool
runtime_suspend)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
if (runtime_suspend)
- hda_codec_jack_wake_enable(sdev);
+ hda_codec_jack_wake_enable(sdev, true);
/* power down all hda link */
snd_hdac_ext_bus_link_power_down_all(bus);
@@ -683,8 +683,10 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
runtime_resume)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/* check jack status */
- if (runtime_resume)
+ if (runtime_resume) {
+ hda_codec_jack_wake_enable(sdev, false);
hda_codec_jack_check(sdev);
+ }
/* turn off the links that were off before suspend */
list_for_each_entry(hlink, &bus->hlink_list, list) {
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 9ec8ae0fd649..a3b6f3e9121c 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -650,7 +650,7 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device
*dev);
*/
void hda_codec_probe_bus(struct snd_sof_dev *sdev,
bool hda_codec_use_common_hdmi);
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev);
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable);
void hda_codec_jack_check(struct snd_sof_dev *sdev);
#endif /* CONFIG_SND_SOC_SOF_HDA */
--
2.29.2
[PATCH v3 4/4] ASoC: SOF: Intel: hda: Avoid checking jack on system suspend
When runtime resume is for system suspend, hda_codec_jack_check()
schedules jackpoll_work which uses snd_hdac_is_power_on() to check
whether codec is suspended.
If we were to use snd_hdac_is_power_on() in system PM path,
pm_runtime_status_suspended() should be used instead of
pm_runtime_suspended(), otherwise pm_runtime_{enable,disable}() changes
the result of snd_hdac_is_power_on().
Because devices suspend in reverse order (i.e. child first), it doesn't
make much sense to resume already suspended codec from audio controller.
So instead of using pm_runtime_status_suspended(), the better approach
here is to make sure jackpoll isn't used in system PM process.
Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
optimization")
---
v3:
Clarify the root cause and why it's needed.
v2:
No change.
sound/soc/sof/intel/hda-dsp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index 7d00107cf3b2..1c5e05b88a90 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -685,7 +685,8 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
runtime_resume)
/* check jack status */
if (runtime_resume) {
hda_codec_jack_wake_enable(sdev, false);
- hda_codec_jack_check(sdev);
+ if (sdev->system_suspend_target == SOF_SUSPEND_NONE)
+ hda_codec_jack_check(sdev);
}
/* turn off the links that were off before suspend */
--
2.29.2
[PATCH v3 2/4] ASoC: SOF: Intel: hda: Resume codec to do jack detection
Instead of queueing jackpoll_work, runtime resume the codec to let it
use different jack detection methods based on jackpoll_interval.
This partially matches SOF driver's behavior with commit a6e7d0a4bdb0
("ALSA: hda: fix jack detection with Realtek codecs when in D3"), the
difference is SOF unconditionally resumes the codec.
---
v3:
Remove wrong assumption that only Realtek codec is used by SOF.
v2:
No change.
sound/soc/sof/intel/hda-codec.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 6875fa570c2c..df59c79cfdfc 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -93,8 +93,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev)
* has been recorded in STATESTS
*/
if (codec->jacktbl.used)
- schedule_delayed_work(&codec->jackpoll_work,
- codec->jackpoll_interval);
+ pm_request_resume(&codec->core.dev);
}
#else
void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {}
--
2.29.2
[PATCH v3 1/4] ALSA: hda/realtek: Power up codec when setting LED via COEF and GPIO
System takes a very long time to suspend after commit 215a22ed31a1
("ALSA: hda: Refactor codec PM to use direct-complete optimization"):
[ 90.065964] PM: suspend entry (s2idle)
[ 90.067337] Filesystems sync: 0.001 seconds
[ 90.185758] Freezing user space processes ... (elapsed 0.002 seconds) done.
[ 90.188713] OOM killer disabled.
[ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001 seconds)
done.
[ 90.190024] printk: Suspending console(s) (use no_console_suspend to debug)
[ 90.904912] intel_pch_thermal :00:12.0: CPU-PCH is cool [49C], continue
to suspend
[ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 329.490933] ACPI: EC: interrupt blocked
That commit keeps the codec suspended during the system suspend. However,
led_suspend() for mute and micmute led writes codec register, triggers
a pending wake up. The wakeup is then handled in __device_suspend() by
the following:
- pm_runtime_disable() to handle the wakeup event.
- device is no longer is suspended state, direct-complete isn't taken.
- pm_runtime_enable() to balance disable_depth.
if (dev->power.direct_complete) {
if (pm_runtime_status_suspended(dev)) {
pm_runtime_disable(dev);
if (pm_runtime_status_suspended(dev)) {
pm_dev_dbg(dev, state, "direct-complete ");
goto Complete;
}
pm_runtime_enable(dev);
}
dev->power.direct_complete = false;
}
Since direct-complete doens't apply anymore, the codec's system suspend
routine is used.
This doesn't play well with SOF driver. When its runtime resume is
called for system suspend, hda_codec_jack_check() schedules
jackpoll_work which uses snd_hdac_is_power_on() to check whether codec
is suspended. Because of the previous pm_runtime_enable(),
snd_hdac_is_power_on() returns false and jackpoll continues to run, and
snd_hda_power_up_pm() cannot power up an already suspended codec in
multiple attempts, causes the long delay on system suspend.
When direct-complete path is taken, snd_hdac_is_power_on() returns true
and hda_jackpoll_work() is skipped by accident. This is still not
correct, and it will be addressed by later patch.
Explicitly runtime resume codec on setting LED to solve the issue.
Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
optimization")
---
v3:
New patch.
sound/pci/hda/patch_realtek.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 3c1d2a3fb1a4..304a7bc89fcd 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4164,7 +4164,10 @@ static void alc_update_gpio_led(struct hda_codec *codec,
unsigned int mask,
{
if (polarity)
enabled = !enabled;
+ /* temporarily power up/down for setting GPIO */
+ snd_hda_power_up_pm(codec);
alc_update_gpio_data(codec, mask, !enabled); /* muted -> LED on */
+ snd_hda_power_down_pm(codec);
}
/* turn on/off mute LED via GPIO per vmaster hook */
@@ -4287,8 +4290,10 @@ static void alc_update_coef_led(struct hda_codec *codec,
if (polarity)
on = !on;
/* temporarily power up/down for setting COEF bit */
+ snd_hda_power_up_pm(codec);
alc_update_coef_idx(codec, led->idx, led->mask,
on ? led->on : led->off);
+ snd_hda_power_down_pm(codec);
}
/* update mute-LED according to the speaker mute state via COEF bit */
--
2.29.2
Re: [PATCH v2 1/2] thermal: int340x: Fix unexpected shutdown at critical temperature
On Tue, Dec 22, 2020 at 1:23 AM Kai-Heng Feng
wrote:
>
> We are seeing thermal shutdown on Intel based mobile workstations, the
> shutdown happens during the first trip handle in
> thermal_zone_device_register():
> kernel: thermal thermal_zone15: critical temperature reached (101 C),
> shutting down
>
> However, we shouldn't do a thermal shutdown here, since
> 1) We may want to use a dedicated daemon, Intel's thermald in this case,
> to handle thermal shutdown.
>
> 2) For ACPI based system, _CRT doesn't mean shutdown unless it's inside
> ThermalZone namespace. ACPI Spec, 11.4.4 _CRT (Critical Temperature):
> "... If this object it present under a device, the device’s driver
> evaluates this object to determine the device’s critical cooling
> temperature trip point. This value may then be used by the device’s
> driver to program an internal device temperature sensor trip point."
>
> So a "critical trip" here merely means we should take a more aggressive
> cooling method.
>
> As int340x device isn't present under ACPI ThermalZone, override the
> default .critical callback to prevent surprising thermal shutdown.
>
> Signed-off-by: Kai-Heng Feng
A gentle ping...
> ---
> v2:
> - Amend subject.
> - Remove int3400 device.
>
> .../thermal/intel/int340x_thermal/int340x_thermal_zone.c| 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> index 6e479deff76b..d1248ba943a4 100644
> --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> @@ -146,12 +146,18 @@ static int int340x_thermal_get_trip_hyst(struct
> thermal_zone_device *zone,
> return 0;
> }
>
> +static void int340x_thermal_critical(struct thermal_zone_device *zone)
> +{
> + dev_dbg(&zone->device, "%s: critical temperature reached\n",
> zone->type);
> +}
> +
> static struct thermal_zone_device_ops int340x_thermal_zone_ops = {
> .get_temp = int340x_thermal_get_zone_temp,
> .get_trip_temp = int340x_thermal_get_trip_temp,
> .get_trip_type = int340x_thermal_get_trip_type,
> .set_trip_temp = int340x_thermal_set_trip_temp,
> .get_trip_hyst = int340x_thermal_get_trip_hyst,
> + .critical = int340x_thermal_critical,
> };
>
> static int int340x_thermal_get_trip_config(acpi_handle handle, char *name,
> --
> 2.29.2
>
Multiple MODALIAS= in uevent file confuses userspace
Commit 8765c5ba19490 ("ACPI / scan: Rework modalias creation when
"compatible" is present") creates two modaliases for certain ACPI
devices. However userspace (systemd-udevd in this case) assumes uevent
file doesn't have duplicated keys, so two "MODALIAS=" breaks the
assumption.
Based on the assumption, systemd-udevd internally uses hashmap to
store each line of uevent file, so the second modalias always replaces
the first modalias.
My attempt [1] is to add a new key, "MODALIAS1" for the second
modalias. This brings up the question of whether each key in uevent
file is unique. If it's no unique, this may break may userspace.
[1] https://github.com/systemd/systemd/pull/18163
Kai-Heng
Re: [PATCH v2 3/3] ASoC: SOF: Intel: hda: Avoid checking jack on system suspend
On Tue, Jan 5, 2021 at 8:28 PM Kai Vehmanen
wrote:
>
> Hey,
>
> On Mon, 4 Jan 2021, Kai-Heng Feng wrote:
>
> > System takes a very long time to suspend after commit 215a22ed31a1
> > ("ALSA: hda: Refactor codec PM to use direct-complete optimization"):
> > [ 90.065964] PM: suspend entry (s2idle)
>
> the patch itself looks good, but can you explain a bit more in what
> conditions you hit the delay?
If both controller and codec are suspended, I can 100% reproduce the issue.
>
> I tried to reproduce the delay on multiple systems (with tip of
> tiwai/master), but with no luck. I can see hda_jackpoll_work() called, but
> at this point runtime pm has been disabled already (via
> __device_suspend()) and snd_hdac_is_power_on() will return true even when
> pm_runtime_suspended() is true as well (which is expected as runtime-pm is
> disabled at this point for system suspend). End result is codec is not
> powered up in hda_jackpoll_work() and suspend is not delayed.
On my system snd_hdac_is_power_on() calls hda_set_power_state() which
takes long time to write to (suspended) codec.
I am not sure why it doesn't power up codec on your system.
>
> The patch still seems correct. You would hit the problem you describe if
> jackpoll_interval was set to a non-zero value (not the case on most
> systems supported by SOF, but still a possibility). I'm still curious how
> you hit the problem. At minimum, we are missing a scenario in our testing.
The issue happens with zero jackpoll_interval.
Kai-Heng
>
> Br, Kai
Re: [PATCH] rtw88: 8821c: Add RFE 2 support
On Wed, Aug 5, 2020 at 7:24 PM Kai-Heng Feng wrote: > > Hi Tony, > > > On Aug 5, 2020, at 19:18, Tony Chuang wrote: > > > >> 8821CE with RFE 2 isn't supported: > >> [ 12.404834] rtw_8821ce :02:00.0: rfe 2 isn't supported > >> [ 12.404937] rtw_8821ce :02:00.0: failed to setup chip efuse info > >> [ 12.404939] rtw_8821ce :02:00.0: failed to setup chip information > >> > > > > NACK > > > > The RFE type 2 should be working with some additional fixes. > > Did you tested connecting to AP with BT paired? > > No, I only tested WiFi. > > > The antenna configuration is different with RFE type 0. > > I will ask someone else to fix them. > > Then the RFE type 2 modules can be supported. > > Good to know that, I'll be patient and wait for a real fix. It's been quite some time, is support for RFE type 2 ready now? Kai-Heng > > Kai-Heng > > > > > Yen-Hsuan >
Re: [PATCH v2 1/3] ASoC: SOF: Intel: hda: Resume codec to do jack detection
On Tue, Jan 5, 2021 at 9:00 PM Kai Vehmanen
wrote:
>
> Hi,
>
> On Mon, 4 Jan 2021, Kai-Heng Feng wrote:
>
> > Instead of queueing jackpoll_work, runtime resume the codec to let it
> > use different jack detection methods based on jackpoll_interval.
>
> hmm, but jackpoll_work() does the same thing, right? So end result should
> be the same.
It depends on the jackpoll_interval value. But yes the end result
should be the same.
>
> > This matches SOF driver's behavior with commit a6e7d0a4bdb0 ("ALSA: hda:
> > fix jack detection with Realtek codecs when in D3"). Since SOF only uses
> > Realtek codec, we don't need any additional check for the resume.
>
> This is not quite correct. First, SOF does support any HDA codec, not just
> Realteks (see e.g. https://github.com/thesofproject/linux/issues/1807),
> and second, this doesn't really match the hda_intel.c patch you mention.
> SOF implements a more conservative approach where we basicly assume
> codec->forced_resume=1 to always apply, and do not implement support for
> codec->relaxed_resume. So the above patch doesn't fully apply to SOF as
> the design is not same.
OK, I assumed SOF always use Realtek codec, so codec->forced_resume=1
is always applied like the other patch.
I'll remove this section.
>
> > diff --git a/sound/soc/sof/intel/hda-codec.c
> > b/sound/soc/sof/intel/hda-codec.c
> > index 6875fa570c2c..df59c79cfdfc 100644
> > --- a/sound/soc/sof/intel/hda-codec.c
> > +++ b/sound/soc/sof/intel/hda-codec.c
> > @@ -93,8 +93,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev)
> >* has been recorded in STATESTS
> >*/
> > if (codec->jacktbl.used)
> > - schedule_delayed_work(&codec->jackpoll_work,
> > - codec->jackpoll_interval);
> > + pm_request_resume(&codec->core.dev);
>
> I think this change is still good. Just drop the but about Realtek
> codecs from commit message and maybe s/matches/aligns/.
OK, will do.
Kai-Heng
>
> Br, Kai
[PATCH v2 2/3] ASoC: SOF: Intel: hda: Modify existing helper to disable WAKEEN
Modify hda_codec_jack_wake_enable() to also support disable WAKEEN.
In addition, this patch also moves the WAKEEN disablement call out of
hda_codec_jack_check() into hda_codec_jack_wake_enable().
This is a preparation for next patch.
No functional change intended.
Signed-off-by: Kai-Heng Feng
---
v2:
Mention it moves the disabling part into another function.
sound/soc/sof/intel/hda-codec.c | 16 +++-
sound/soc/sof/intel/hda-dsp.c | 6 --
sound/soc/sof/intel/hda.h | 2 +-
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index df59c79cfdfc..b7e9931ead57 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -63,16 +63,18 @@ static int hda_codec_load_module(struct hda_codec *codec)
}
/* enable controller wake up event for all codecs with jack connectors */
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable)
{
struct hda_bus *hbus = sof_to_hbus(sdev);
struct hdac_bus *bus = sof_to_bus(sdev);
struct hda_codec *codec;
unsigned int mask = 0;
- list_for_each_codec(codec, hbus)
- if (codec->jacktbl.used)
- mask |= BIT(codec->core.addr);
+ if (enable) {
+ list_for_each_codec(codec, hbus)
+ if (codec->jacktbl.used)
+ mask |= BIT(codec->core.addr);
+ }
snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, mask);
}
@@ -81,12 +83,8 @@ void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
void hda_codec_jack_check(struct snd_sof_dev *sdev)
{
struct hda_bus *hbus = sof_to_hbus(sdev);
- struct hdac_bus *bus = sof_to_bus(sdev);
struct hda_codec *codec;
- /* disable controller Wake Up event*/
- snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0);
-
list_for_each_codec(codec, hbus)
/*
* Wake up all jack-detecting codecs regardless whether an event
@@ -96,7 +94,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev)
pm_request_resume(&codec->core.dev);
}
#else
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {}
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable) {}
void hda_codec_jack_check(struct snd_sof_dev *sdev) {}
#endif /* CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC */
EXPORT_SYMBOL_NS(hda_codec_jack_wake_enable, SND_SOC_SOF_HDA_AUDIO_CODEC);
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index 2b001151fe37..7d00107cf3b2 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -617,7 +617,7 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool
runtime_suspend)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
if (runtime_suspend)
- hda_codec_jack_wake_enable(sdev);
+ hda_codec_jack_wake_enable(sdev, true);
/* power down all hda link */
snd_hdac_ext_bus_link_power_down_all(bus);
@@ -683,8 +683,10 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
runtime_resume)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/* check jack status */
- if (runtime_resume)
+ if (runtime_resume) {
+ hda_codec_jack_wake_enable(sdev, false);
hda_codec_jack_check(sdev);
+ }
/* turn off the links that were off before suspend */
list_for_each_entry(hlink, &bus->hlink_list, list) {
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 9ec8ae0fd649..a3b6f3e9121c 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -650,7 +650,7 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device
*dev);
*/
void hda_codec_probe_bus(struct snd_sof_dev *sdev,
bool hda_codec_use_common_hdmi);
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev);
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable);
void hda_codec_jack_check(struct snd_sof_dev *sdev);
#endif /* CONFIG_SND_SOC_SOF_HDA */
--
2.29.2
[PATCH v2 1/3] ASoC: SOF: Intel: hda: Resume codec to do jack detection
Instead of queueing jackpoll_work, runtime resume the codec to let it
use different jack detection methods based on jackpoll_interval.
This matches SOF driver's behavior with commit a6e7d0a4bdb0 ("ALSA: hda:
fix jack detection with Realtek codecs when in D3"). Since SOF only uses
Realtek codec, we don't need any additional check for the resume.
Signed-off-by: Kai-Heng Feng
---
v2:
No change.
sound/soc/sof/intel/hda-codec.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 6875fa570c2c..df59c79cfdfc 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -93,8 +93,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev)
* has been recorded in STATESTS
*/
if (codec->jacktbl.used)
- schedule_delayed_work(&codec->jackpoll_work,
- codec->jackpoll_interval);
+ pm_request_resume(&codec->core.dev);
}
#else
void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {}
--
2.29.2
[PATCH v2 3/3] ASoC: SOF: Intel: hda: Avoid checking jack on system suspend
System takes a very long time to suspend after commit 215a22ed31a1
("ALSA: hda: Refactor codec PM to use direct-complete optimization"):
[ 90.065964] PM: suspend entry (s2idle)
[ 90.067337] Filesystems sync: 0.001 seconds
[ 90.185758] Freezing user space processes ... (elapsed 0.002 seconds) done.
[ 90.188713] OOM killer disabled.
[ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001 seconds)
done.
[ 90.190024] printk: Suspending console(s) (use no_console_suspend to debug)
[ 90.904912] intel_pch_thermal :00:12.0: CPU-PCH is cool [49C], continue
to suspend
[ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 329.490933] ACPI: EC: interrupt blocked
That commit keeps codec suspended during the system suspend. However,
SOF driver's runtime resume, which is for system suspend, calls
hda_codec_jack_check() and schedules jackpoll_work. The jackpoll
work uses snd_hda_power_up_pm() which tries to resume the codec in
system suspend path, hence blocking the suspend process.
So only check jack status if it's not in system PM process.
Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
optimization")
Signed-off-by: Kai-Heng Feng
---
v2:
No change.
sound/soc/sof/intel/hda-dsp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index 7d00107cf3b2..1c5e05b88a90 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -685,7 +685,8 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
runtime_resume)
/* check jack status */
if (runtime_resume) {
hda_codec_jack_wake_enable(sdev, false);
- hda_codec_jack_check(sdev);
+ if (sdev->system_suspend_target == SOF_SUSPEND_NONE)
+ hda_codec_jack_check(sdev);
}
/* turn off the links that were off before suspend */
--
2.29.2
Re: [PATCH 2/2] ASoC: SOF: Intel: hda: Avoid checking jack on system suspend
On Fri, Jan 1, 2021 at 4:07 PM Takashi Iwai wrote:
>
> On Thu, 31 Dec 2020 19:06:43 +0100,
> Kai-Heng Feng wrote:
> >
> > On Thu, Dec 31, 2020 at 6:55 PM Takashi Iwai wrote:
> > >
> > > On Tue, 29 Dec 2020 14:38:15 +0100,
> > > Kai-Heng Feng wrote:
> > > >
> > > > System takes a very long time to suspend after commit 215a22ed31a1
> > > > ("ALSA: hda: Refactor codec PM to use direct-complete optimization"):
> > > > [ 90.065964] PM: suspend entry (s2idle)
> > > > [ 90.067337] Filesystems sync: 0.001 seconds
> > > > [ 90.185758] Freezing user space processes ... (elapsed 0.002
> > > > seconds) done.
> > > > [ 90.188713] OOM killer disabled.
> > > > [ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001
> > > > seconds) done.
> > > > [ 90.190024] printk: Suspending console(s) (use no_console_suspend to
> > > > debug)
> > > > [ 90.904912] intel_pch_thermal :00:12.0: CPU-PCH is cool [49C],
> > > > continue to suspend
> > > > [ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync
> > > > register 0x2b8000. -5
> > > > [ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync
> > > > register 0x2b8000. -5
> > > > [ 329.490933] ACPI: EC: interrupt blocked
> > > >
> > > > That commit keeps codec suspended during the system suspend. However,
> > > > SOF driver's runtime resume, which is for system suspend, calls
> > > > hda_codec_jack_check() and schedules jackpoll_work. The jackpoll
> > > > work uses snd_hda_power_up_pm() which tries to resume the codec in
> > > > system suspend path, hence blocking the suspend process.
> > > >
> > > > So only check jack status if it's not in system PM process.
> > >
> > > After your previous patch set, the legacy HDA does queue the
> > > jackpoll_work only if jackpoll_interval is set. I suppose rather the
> > > same rule would be applied?
> >
> > It's queued in hda_codec_pm_complete(), which happens at the end of PM
> > process.
> > This one is queued in the middle of PM suspend, so it's not the same here.
>
> But why do we need the jack status check explicitly there if
> hda_codec_pm_complete() already does it (via re-queuing the resume)?
Because hda_resume() is called for both system and runtime resume, but
hda_codec_pm_complete() only gets called for system resume. So we
still need to cover the runtime resume case.
However, we can use pm_request_resume() to wakeup the codec and do the
jack detection, instead of queueing jackpoll_work directly. I'll do
the change in v2.
Kai-Heng
>
> thanks,
>
> Takashi
>
> >
> > Kai-Heng
> >
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> > >
> > > >
> > > > Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use
> > > > direct-complete optimization")
> > > > Signed-off-by: Kai-Heng Feng
> > > > ---
> > > > sound/soc/sof/intel/hda-dsp.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/sound/soc/sof/intel/hda-dsp.c
> > > > b/sound/soc/sof/intel/hda-dsp.c
> > > > index 7d00107cf3b2..1c5e05b88a90 100644
> > > > --- a/sound/soc/sof/intel/hda-dsp.c
> > > > +++ b/sound/soc/sof/intel/hda-dsp.c
> > > > @@ -685,7 +685,8 @@ static int hda_resume(struct snd_sof_dev *sdev,
> > > > bool runtime_resume)
> > > > /* check jack status */
> > > > if (runtime_resume) {
> > > > hda_codec_jack_wake_enable(sdev, false);
> > > > - hda_codec_jack_check(sdev);
> > > > + if (sdev->system_suspend_target == SOF_SUSPEND_NONE)
> > > > + hda_codec_jack_check(sdev);
> > > > }
> > > >
> > > > /* turn off the links that were off before suspend */
> > > > --
> > > > 2.29.2
> > > >
> >
Re: [PATCH 1/2] ASoC: SOF: Intel: hda: Modify existing helper to disable WAKEEN
On Thu, Dec 31, 2020 at 6:52 PM Takashi Iwai wrote:
>
> On Tue, 29 Dec 2020 14:38:14 +0100,
> Kai-Heng Feng wrote:
> >
> > Modify hda_codec_jack_wake_enable() to also support disable WAKEEN.
> > This is a preparation for next patch.
> >
> > No functional change intended.
>
> Maybe it's better to mention that this patch moves the WAKEEN
> disablement call out of hda_codec_jack_check() into
> hda_codec_jack_wake_enable(), too.
Ok, will update the commit log in v2.
Kai-Heng
>
>
> thanks,
>
> Takashi
>
> >
> > Signed-off-by: Kai-Heng Feng
> > ---
> > sound/soc/sof/intel/hda-codec.c | 16 +++-
> > sound/soc/sof/intel/hda-dsp.c | 6 --
> > sound/soc/sof/intel/hda.h | 2 +-
> > 3 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/sound/soc/sof/intel/hda-codec.c
> > b/sound/soc/sof/intel/hda-codec.c
> > index 6875fa570c2c..bc9ac4abdab5 100644
> > --- a/sound/soc/sof/intel/hda-codec.c
> > +++ b/sound/soc/sof/intel/hda-codec.c
> > @@ -63,16 +63,18 @@ static int hda_codec_load_module(struct hda_codec
> > *codec)
> > }
> >
> > /* enable controller wake up event for all codecs with jack connectors */
> > -void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
> > +void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable)
> > {
> > struct hda_bus *hbus = sof_to_hbus(sdev);
> > struct hdac_bus *bus = sof_to_bus(sdev);
> > struct hda_codec *codec;
> > unsigned int mask = 0;
> >
> > - list_for_each_codec(codec, hbus)
> > - if (codec->jacktbl.used)
> > - mask |= BIT(codec->core.addr);
> > + if (enable) {
> > + list_for_each_codec(codec, hbus)
> > + if (codec->jacktbl.used)
> > + mask |= BIT(codec->core.addr);
> > + }
> >
> > snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, mask);
> > }
> > @@ -81,12 +83,8 @@ void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
> > void hda_codec_jack_check(struct snd_sof_dev *sdev)
> > {
> > struct hda_bus *hbus = sof_to_hbus(sdev);
> > - struct hdac_bus *bus = sof_to_bus(sdev);
> > struct hda_codec *codec;
> >
> > - /* disable controller Wake Up event*/
> > - snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0);
> > -
> > list_for_each_codec(codec, hbus)
> > /*
> >* Wake up all jack-detecting codecs regardless whether an
> > event
> > @@ -97,7 +95,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev)
> > codec->jackpoll_interval);
> > }
> > #else
> > -void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {}
> > +void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable) {}
> > void hda_codec_jack_check(struct snd_sof_dev *sdev) {}
> > #endif /* CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC */
> > EXPORT_SYMBOL_NS(hda_codec_jack_wake_enable, SND_SOC_SOF_HDA_AUDIO_CODEC);
> > diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
> > index 2b001151fe37..7d00107cf3b2 100644
> > --- a/sound/soc/sof/intel/hda-dsp.c
> > +++ b/sound/soc/sof/intel/hda-dsp.c
> > @@ -617,7 +617,7 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool
> > runtime_suspend)
> >
> > #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
> > if (runtime_suspend)
> > - hda_codec_jack_wake_enable(sdev);
> > + hda_codec_jack_wake_enable(sdev, true);
> >
> > /* power down all hda link */
> > snd_hdac_ext_bus_link_power_down_all(bus);
> > @@ -683,8 +683,10 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
> > runtime_resume)
> >
> > #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
> > /* check jack status */
> > - if (runtime_resume)
> > + if (runtime_resume) {
> > + hda_codec_jack_wake_enable(sdev, false);
> > hda_codec_jack_check(sdev);
> > + }
> >
> > /* turn off the links that were off before suspend */
> > list_for_each_entry(hlink, &bus->hlink_list, list) {
> > diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> > index 9ec8ae0fd649..a3b6f3e9121c 100644
> > --- a/sound/soc/sof/intel/hda.h
> > +++ b/sound/soc/sof/intel/hda.h
> > @@ -650,7 +650,7 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct
> > device *dev);
> > */
> > void hda_codec_probe_bus(struct snd_sof_dev *sdev,
> >bool hda_codec_use_common_hdmi);
> > -void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev);
> > +void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable);
> > void hda_codec_jack_check(struct snd_sof_dev *sdev);
> >
> > #endif /* CONFIG_SND_SOC_SOF_HDA */
> > --
> > 2.29.2
> >
Re: [PATCH 2/2] ASoC: SOF: Intel: hda: Avoid checking jack on system suspend
On Thu, Dec 31, 2020 at 6:55 PM Takashi Iwai wrote:
>
> On Tue, 29 Dec 2020 14:38:15 +0100,
> Kai-Heng Feng wrote:
> >
> > System takes a very long time to suspend after commit 215a22ed31a1
> > ("ALSA: hda: Refactor codec PM to use direct-complete optimization"):
> > [ 90.065964] PM: suspend entry (s2idle)
> > [ 90.067337] Filesystems sync: 0.001 seconds
> > [ 90.185758] Freezing user space processes ... (elapsed 0.002 seconds)
> > done.
> > [ 90.188713] OOM killer disabled.
> > [ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001
> > seconds) done.
> > [ 90.190024] printk: Suspending console(s) (use no_console_suspend to
> > debug)
> > [ 90.904912] intel_pch_thermal :00:12.0: CPU-PCH is cool [49C],
> > continue to suspend
> > [ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
> > 0x2b8000. -5
> > [ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
> > 0x2b8000. -5
> > [ 329.490933] ACPI: EC: interrupt blocked
> >
> > That commit keeps codec suspended during the system suspend. However,
> > SOF driver's runtime resume, which is for system suspend, calls
> > hda_codec_jack_check() and schedules jackpoll_work. The jackpoll
> > work uses snd_hda_power_up_pm() which tries to resume the codec in
> > system suspend path, hence blocking the suspend process.
> >
> > So only check jack status if it's not in system PM process.
>
> After your previous patch set, the legacy HDA does queue the
> jackpoll_work only if jackpoll_interval is set. I suppose rather the
> same rule would be applied?
It's queued in hda_codec_pm_complete(), which happens at the end of PM process.
This one is queued in the middle of PM suspend, so it's not the same here.
Kai-Heng
>
>
> thanks,
>
> Takashi
>
> >
> > Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
> > optimization")
> > Signed-off-by: Kai-Heng Feng
> > ---
> > sound/soc/sof/intel/hda-dsp.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
> > index 7d00107cf3b2..1c5e05b88a90 100644
> > --- a/sound/soc/sof/intel/hda-dsp.c
> > +++ b/sound/soc/sof/intel/hda-dsp.c
> > @@ -685,7 +685,8 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
> > runtime_resume)
> > /* check jack status */
> > if (runtime_resume) {
> > hda_codec_jack_wake_enable(sdev, false);
> > - hda_codec_jack_check(sdev);
> > + if (sdev->system_suspend_target == SOF_SUSPEND_NONE)
> > + hda_codec_jack_check(sdev);
> > }
> >
> > /* turn off the links that were off before suspend */
> > --
> > 2.29.2
> >
[PATCH] PM: sleep: core: Resume suspended device if direct-complete is disabled
HDA controller can't be runtime-suspended after commit 215a22ed31a1
("ALSA: hda: Refactor codjc PM to use direct-complete optimization"),
which enables direct-complete for HDA codec.
The HDA codec driver doesn't expect direct-complete will be disabled
after it returns a positive value from prepare() callback. So freeze()
is called directly when it's runtime-suspended, breaks the balance of
its internal codec_powered counting.
So if a device is prepared for direct-complete but PM core breaks the
assumption, resume the device to keep PM operations balanced.
Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
optimization")
Signed-off-by: Kai-Heng Feng
---
drivers/base/power/main.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 46793276598d..9c0e25a92ad0 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1849,6 +1849,10 @@ static int device_prepare(struct device *dev,
pm_message_t state)
(ret > 0 || dev->power.no_pm_callbacks) &&
!dev_pm_test_driver_flags(dev, DPM_FLAG_NO_DIRECT_COMPLETE);
spin_unlock_irq(&dev->power.lock);
+
+ if (ret > 0 && !dev->power.direct_complete)
+ pm_runtime_resume(dev);
+
return 0;
}
--
2.29.2
[PATCH] ALSA: hda/realtek: Enable mute and micmute LED on HP EliteBook 850 G7
HP EliteBook 850 G7 uses the same GPIO pins as ALC285_FIXUP_HP_GPIO_LED
to enable mute and micmute LED. So apply the quirk to enable the LEDs.
Signed-off-by: Kai-Heng Feng
---
sound/pci/hda/patch_realtek.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index dde5ba209541..b12e1f083029 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -7959,6 +7959,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x103c, 0x8497, "HP Envy x360",
ALC269_FIXUP_HP_MUTE_LED_MIC3),
SND_PCI_QUIRK(0x103c, 0x84e7, "HP Pavilion 15",
ALC269_FIXUP_HP_MUTE_LED_MIC3),
SND_PCI_QUIRK(0x103c, 0x869d, "HP", ALC236_FIXUP_HP_MUTE_LED),
+ SND_PCI_QUIRK(0x103c, 0x8724, "HP EliteBook 850 G7",
ALC285_FIXUP_HP_GPIO_LED),
SND_PCI_QUIRK(0x103c, 0x8729, "HP", ALC285_FIXUP_HP_GPIO_LED),
SND_PCI_QUIRK(0x103c, 0x8736, "HP", ALC285_FIXUP_HP_GPIO_AMP_INIT),
SND_PCI_QUIRK(0x103c, 0x8760, "HP", ALC285_FIXUP_HP_MUTE_LED),
--
2.29.2
[PATCH] HID: multitouch: Enable multi-input for Synaptics pointstick/touchpad device
Pointstick and its left/right buttons on HP EliteBook 850 G7 need
multi-input quirk to work correctly.
Signed-off-by: Kai-Heng Feng
---
drivers/hid/hid-multitouch.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index d670bcd57bde..0743ef51d3b2 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -2054,6 +2054,10 @@ static const struct hid_device_id mt_devices[] = {
HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
USB_VENDOR_ID_SYNAPTICS, 0xce08) },
+ { .driver_data = MT_CLS_WIN_8_FORCE_MULTI_INPUT,
+ HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
+ USB_VENDOR_ID_SYNAPTICS, 0xce09) },
+
/* TopSeed panels */
{ .driver_data = MT_CLS_TOPSEED,
MT_USB_DEVICE(USB_VENDOR_ID_TOPSEED2,
--
2.29.2
[PATCH 2/2] ASoC: SOF: Intel: hda: Avoid checking jack on system suspend
System takes a very long time to suspend after commit 215a22ed31a1
("ALSA: hda: Refactor codec PM to use direct-complete optimization"):
[ 90.065964] PM: suspend entry (s2idle)
[ 90.067337] Filesystems sync: 0.001 seconds
[ 90.185758] Freezing user space processes ... (elapsed 0.002 seconds) done.
[ 90.188713] OOM killer disabled.
[ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001 seconds)
done.
[ 90.190024] printk: Suspending console(s) (use no_console_suspend to debug)
[ 90.904912] intel_pch_thermal :00:12.0: CPU-PCH is cool [49C], continue
to suspend
[ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 329.490933] ACPI: EC: interrupt blocked
That commit keeps codec suspended during the system suspend. However,
SOF driver's runtime resume, which is for system suspend, calls
hda_codec_jack_check() and schedules jackpoll_work. The jackpoll
work uses snd_hda_power_up_pm() which tries to resume the codec in
system suspend path, hence blocking the suspend process.
So only check jack status if it's not in system PM process.
Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
optimization")
Signed-off-by: Kai-Heng Feng
---
sound/soc/sof/intel/hda-dsp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index 7d00107cf3b2..1c5e05b88a90 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -685,7 +685,8 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
runtime_resume)
/* check jack status */
if (runtime_resume) {
hda_codec_jack_wake_enable(sdev, false);
- hda_codec_jack_check(sdev);
+ if (sdev->system_suspend_target == SOF_SUSPEND_NONE)
+ hda_codec_jack_check(sdev);
}
/* turn off the links that were off before suspend */
--
2.29.2
[PATCH 1/2] ASoC: SOF: Intel: hda: Modify existing helper to disable WAKEEN
Modify hda_codec_jack_wake_enable() to also support disable WAKEEN.
This is a preparation for next patch.
No functional change intended.
Signed-off-by: Kai-Heng Feng
---
sound/soc/sof/intel/hda-codec.c | 16 +++-
sound/soc/sof/intel/hda-dsp.c | 6 --
sound/soc/sof/intel/hda.h | 2 +-
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 6875fa570c2c..bc9ac4abdab5 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -63,16 +63,18 @@ static int hda_codec_load_module(struct hda_codec *codec)
}
/* enable controller wake up event for all codecs with jack connectors */
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable)
{
struct hda_bus *hbus = sof_to_hbus(sdev);
struct hdac_bus *bus = sof_to_bus(sdev);
struct hda_codec *codec;
unsigned int mask = 0;
- list_for_each_codec(codec, hbus)
- if (codec->jacktbl.used)
- mask |= BIT(codec->core.addr);
+ if (enable) {
+ list_for_each_codec(codec, hbus)
+ if (codec->jacktbl.used)
+ mask |= BIT(codec->core.addr);
+ }
snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, mask);
}
@@ -81,12 +83,8 @@ void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev)
void hda_codec_jack_check(struct snd_sof_dev *sdev)
{
struct hda_bus *hbus = sof_to_hbus(sdev);
- struct hdac_bus *bus = sof_to_bus(sdev);
struct hda_codec *codec;
- /* disable controller Wake Up event*/
- snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0);
-
list_for_each_codec(codec, hbus)
/*
* Wake up all jack-detecting codecs regardless whether an event
@@ -97,7 +95,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev)
codec->jackpoll_interval);
}
#else
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {}
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable) {}
void hda_codec_jack_check(struct snd_sof_dev *sdev) {}
#endif /* CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC */
EXPORT_SYMBOL_NS(hda_codec_jack_wake_enable, SND_SOC_SOF_HDA_AUDIO_CODEC);
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index 2b001151fe37..7d00107cf3b2 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -617,7 +617,7 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool
runtime_suspend)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
if (runtime_suspend)
- hda_codec_jack_wake_enable(sdev);
+ hda_codec_jack_wake_enable(sdev, true);
/* power down all hda link */
snd_hdac_ext_bus_link_power_down_all(bus);
@@ -683,8 +683,10 @@ static int hda_resume(struct snd_sof_dev *sdev, bool
runtime_resume)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/* check jack status */
- if (runtime_resume)
+ if (runtime_resume) {
+ hda_codec_jack_wake_enable(sdev, false);
hda_codec_jack_check(sdev);
+ }
/* turn off the links that were off before suspend */
list_for_each_entry(hlink, &bus->hlink_list, list) {
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 9ec8ae0fd649..a3b6f3e9121c 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -650,7 +650,7 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device
*dev);
*/
void hda_codec_probe_bus(struct snd_sof_dev *sdev,
bool hda_codec_use_common_hdmi);
-void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev);
+void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable);
void hda_codec_jack_check(struct snd_sof_dev *sdev);
#endif /* CONFIG_SND_SOC_SOF_HDA */
--
2.29.2
Re: Time to re-enable Runtime PM per default for PCI devcies?
On Sat, Dec 26, 2020 at 11:26 PM Heiner Kallweit wrote:
>
> On 17.11.2020 17:57, Rafael J. Wysocki wrote:
> > On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas wrote:
> >>
> >> [+to Rafael, author of the commit you mentioned,
> >> +cc Mika, Kai Heng, Lukas, linux-pm, linux-kernel]
> >>
> >> On Tue, Nov 17, 2020 at 04:56:09PM +0100, Heiner Kallweit wrote:
> >>> More than 10 yrs ago Runtime PM was disabled per default by bb910a7040
> >>> ("PCI/PM Runtime: Make runtime PM of PCI devices inactive by default").
> >>>
> >>> Reason given: "avoid breakage on systems where ACPI-based wake-up is
> >>> known to fail for some devices"
> >>> Unfortunately the commit message doesn't mention any affected devices
> >>> or systems.
> >
> > Even if it did that, it wouldn't have been a full list almost for sure.
> >
> > We had received multiple problem reports related to that, most likely
> > because the ACPI PM in BIOSes at that time was tailored for
> > system-wide PM transitions only.
> >
>
> To follow up on this discussion:
> We could call pm_runtime_forbid() conditionally, e.g. with the following
> condition. This would enable runtime pm per default for all non-ACPI
> systems, and it uses the BIOS date as an indicator for a hopefully
> not that broken ACPI implementation. However I could understand the
> argument that this looks a little hacky ..
>
> if (IS_ENABLED(CONFIG_ACPI) && dmi_get_bios_year() <= 2016)
dmi_get_bios_year() may not be a good indicator. Last time I used it
caused regression, because the value changed after BIOS update.
For example, my MacBook Pro is manufactured in 2011, but
dmi_get_bios_year() returns 2018 with latest BIOS.
Kai-Heng
>
>
>
> >>> With Runtime PM disabled e.g. the PHY on network devices may remain
> >>> powered up even with no cable plugged in, affecting battery lifetime
> >>> on mobile devices. Currently we have to rely on the respective distro
> >>> or user to enable Runtime PM via sysfs (echo auto > power/control).
> >>> Some devices work around this restriction by calling pm_runtime_allow
> >>> in their probe routine, even though that's not recommended by
> >>> https://www.kernel.org/doc/Documentation/power/pci.txt
> >>>
> >>> Disabling Runtime PM per default seems to be a big hammer, a quirk
> >>> for affected devices / systems may had been better. And we still
> >>> have the option to disable Runtime PM for selected devices via sysfs.
> >>>
> >>> So, to cut a long story short: Wouldn't it be time to remove this
> >>> restriction?
> >>
> >> I don't know the history of this, but maybe Rafael or the others can
> >> shed some light on it.
> >
> > The systems that had those problems 10 years ago would still have
> > them, but I expect there to be more systems where runtime PM can be
> > enabled by default for PCI devices without issues.
> >
>
Re: [PATCH] ALSA: hda: Resume codec for system suspend if LED is controlled by codec
On Sat, Dec 26, 2020 at 3:46 PM Takashi Iwai wrote:
>
> On Fri, 25 Dec 2020 17:47:23 +0100,
> Kai-Heng Feng wrote:
> >
> > Laptop with codec controlled LEDs takes a very long time to suspend
> > after commit 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use
> > direct-complete optimization"):
> > [ 90.065964] PM: suspend entry (s2idle)
> > [ 90.067337] Filesystems sync: 0.001 seconds
> > [ 90.185758] Freezing user space processes ... (elapsed 0.002 seconds)
> > done.
> > [ 90.188713] OOM killer disabled.
> > [ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001
> > seconds) done.
> > [ 90.190024] printk: Suspending console(s) (use no_console_suspend to
> > debug)
> > [ 90.904912] intel_pch_thermal :00:12.0: CPU-PCH is cool [49C],
> > continue to suspend
> > [ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
> > 0x2b8000. -5
> > [ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
> > 0x2b8000. -5
> > [ 329.490933] ACPI: EC: interrupt blocked
> >
> > That commit keeps codec suspended during the system suspend. This
> > doesn't play well with codec controlled mute and micmute LEDs, because
> > LED core will use codec registers to turn off those LEDs.
> >
> > Currently, all users of create_mute_led_cdev() use codec to control
> > LED, so unconditionally runtime resume those codecs before system
> > suspend to solve the problem.
> >
> > Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
> > optimization")
> > Signed-off-by: Kai-Heng Feng
>
> A puzzling point is that it's applied only to the cases with the led
> cdev. Or does it basically apply for the ASoC controller?
> That is, if you run with legacy HDA (snd-intel-dspcfg.dsp_driver=1),
> does the issue appear as well on those machines?
No, the issue doesn't happen with snd-intel-dspcfg.dsp_driver=1. It
only happens when SOF driver is in use.
My analysis was wrong, I will send a new patch to address the root cause.
Kai-Heng
>
>
> thanks,
>
> Takashi
>
> > ---
> > include/sound/hda_codec.h | 1 +
> > sound/pci/hda/hda_codec.c | 7 +++
> > sound/pci/hda/hda_generic.c | 1 +
> > 3 files changed, 9 insertions(+)
> >
> > diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
> > index 2e8d51937acd..b01d76abe008 100644
> > --- a/include/sound/hda_codec.h
> > +++ b/include/sound/hda_codec.h
> > @@ -255,6 +255,7 @@ struct hda_codec {
> > unsigned int relaxed_resume:1; /* don't resume forcibly for jack */
> > unsigned int forced_resume:1; /* forced resume for jack */
> > unsigned int mst_no_extra_pcms:1; /* no backup PCMs for DP-MST */
> > + unsigned int resume_for_sleep:1; /* runtime resume for system sleep
> > */
> >
> > #ifdef CONFIG_PM
> > unsigned long power_on_acct;
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index 687216e74526..b890d9b4339e 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -2983,6 +2983,13 @@ static int hda_codec_runtime_resume(struct device
> > *dev)
> > #ifdef CONFIG_PM_SLEEP
> > static int hda_codec_pm_prepare(struct device *dev)
> > {
> > + struct hda_codec *codec = dev_to_hda_codec(dev);
> > +
> > + if (codec->resume_for_sleep) {
> > + pm_runtime_resume(dev);
> > + return 0;
> > + }
> > +
> > return pm_runtime_suspended(dev);
> > }
> >
> > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> > index 8060cc86dfea..6d259d5bb5bb 100644
> > --- a/sound/pci/hda/hda_generic.c
> > +++ b/sound/pci/hda/hda_generic.c
> > @@ -3913,6 +3913,7 @@ static int create_mute_led_cdev(struct hda_codec
> > *codec,
> > cdev->brightness_set_blocking = callback;
> > cdev->brightness = ledtrig_audio_get(micmute ? LED_AUDIO_MICMUTE :
> > LED_AUDIO_MUTE);
> > cdev->flags = LED_CORE_SUSPENDRESUME;
> > + codec->resume_for_sleep = 1;
> >
> > return devm_led_classdev_register(&codec->core.dev, cdev);
> > }
> > --
> > 2.29.2
> >
[PATCH] ALSA: hda: Resume codec for system suspend if LED is controlled by codec
Laptop with codec controlled LEDs takes a very long time to suspend
after commit 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use
direct-complete optimization"):
[ 90.065964] PM: suspend entry (s2idle)
[ 90.067337] Filesystems sync: 0.001 seconds
[ 90.185758] Freezing user space processes ... (elapsed 0.002 seconds) done.
[ 90.188713] OOM killer disabled.
[ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001 seconds)
done.
[ 90.190024] printk: Suspending console(s) (use no_console_suspend to debug)
[ 90.904912] intel_pch_thermal :00:12.0: CPU-PCH is cool [49C], continue
to suspend
[ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register
0x2b8000. -5
[ 329.490933] ACPI: EC: interrupt blocked
That commit keeps codec suspended during the system suspend. This
doesn't play well with codec controlled mute and micmute LEDs, because
LED core will use codec registers to turn off those LEDs.
Currently, all users of create_mute_led_cdev() use codec to control
LED, so unconditionally runtime resume those codecs before system
suspend to solve the problem.
Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete
optimization")
Signed-off-by: Kai-Heng Feng
---
include/sound/hda_codec.h | 1 +
sound/pci/hda/hda_codec.c | 7 +++
sound/pci/hda/hda_generic.c | 1 +
3 files changed, 9 insertions(+)
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index 2e8d51937acd..b01d76abe008 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -255,6 +255,7 @@ struct hda_codec {
unsigned int relaxed_resume:1; /* don't resume forcibly for jack */
unsigned int forced_resume:1; /* forced resume for jack */
unsigned int mst_no_extra_pcms:1; /* no backup PCMs for DP-MST */
+ unsigned int resume_for_sleep:1; /* runtime resume for system sleep */
#ifdef CONFIG_PM
unsigned long power_on_acct;
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 687216e74526..b890d9b4339e 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2983,6 +2983,13 @@ static int hda_codec_runtime_resume(struct device *dev)
#ifdef CONFIG_PM_SLEEP
static int hda_codec_pm_prepare(struct device *dev)
{
+ struct hda_codec *codec = dev_to_hda_codec(dev);
+
+ if (codec->resume_for_sleep) {
+ pm_runtime_resume(dev);
+ return 0;
+ }
+
return pm_runtime_suspended(dev);
}
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 8060cc86dfea..6d259d5bb5bb 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3913,6 +3913,7 @@ static int create_mute_led_cdev(struct hda_codec *codec,
cdev->brightness_set_blocking = callback;
cdev->brightness = ledtrig_audio_get(micmute ? LED_AUDIO_MICMUTE :
LED_AUDIO_MUTE);
cdev->flags = LED_CORE_SUSPENDRESUME;
+ codec->resume_for_sleep = 1;
return devm_led_classdev_register(&codec->core.dev, cdev);
}
--
2.29.2
Re: [Nouveau] [PATCH v2] ALSA: hda: Continue to probe when codec probe fails
On Tue, Dec 22, 2020 at 1:56 AM Ilia Mirkin wrote:
>
> On Mon, Dec 21, 2020 at 11:33 AM Kai-Heng Feng
> wrote:
> >
> > [+Cc nouveau]
> >
> > On Fri, Dec 18, 2020 at 4:06 PM Takashi Iwai wrote:
> > [snip]
> > > > Quite possibly the system doesn't power up HDA controller when there's
> > > > no external monitor.
> > > > So when it's connected to external monitor, it's still needed for HDMI
> > > > audio.
> > > > Let me ask the user to confirm this.
> > >
> > > Yeah, it's the basic question whether the HD-audio is supposed to work
> > > on this machine at all. If yes, the current approach we take makes
> > > less sense - instead we should rather make the HD-audio controller
> > > working.
> >
> > Yea, confirmed that the Nvidia HDA works when HDMI is connected prior boot.
> >
> > > > > - The second problem is that pci_enable_device() ignores the error
> > > > > returned from pci_set_power_state() if it's -EIO. And the
> > > > > inaccessible access error returns -EIO, although it's rather a fatal
> > > > > problem. So the driver believes as the PCI device gets enabled
> > > > > properly.
> > > >
> > > > This was introduced in 2005, by Alan's 11f3859b1e85 ("[PATCH] PCI: Fix
> > > > regression in pci_enable_device_bars") to fix UHCI controller.
> > > >
> > > > >
> > > > > - The third problem is that HD-audio driver blindly believes the
> > > > > codec_mask read from the register even if it's a read failure as I
> > > > > already showed.
> > > >
> > > > This approach has least regression risk.
> > >
> > > Yes, but it assumes that HD-audio is really non-existent.
> >
> > I really don't know any good approach to address this.
> > On Windows, HDA PCI is "hidden" until HDMI cable is plugged, then the
> > driver will flag the magic bit to make HDA audio appear on the PCI
> > bus.
> > IIRC the current approach is to make nouveau and device link work.
>
> I don't have the full context of this discussion, but the kernel
> force-enables the HDA subfunction nowadays, irrespective of nouveau or
> nvidia or whatever:
That's the problem.
The nvidia HDA controller on the affected system only gets its power
after HDMI cable plugged, so the probe on boot fails.
Kai-Heng
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/quirks.c?h=v5.10#n5267
>
> Cheers,
>
> -ilia
[PATCH v2 2/2] thermal: intel: pch: Fix unexpected shutdown at critical temperature
Like previous patch, the intel_pch_thermal device is not in ACPI
ThermalZone namespace, so a critical trip doesn't mean shutdown.
Override the default .critical callback to prevent surprising thermal
shutdoown.
Signed-off-by: Kai-Heng Feng
---
v2:
- Amend subject.
drivers/thermal/intel/intel_pch_thermal.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/thermal/intel/intel_pch_thermal.c
b/drivers/thermal/intel/intel_pch_thermal.c
index 41723c6c6c0c..527c91f5960b 100644
--- a/drivers/thermal/intel/intel_pch_thermal.c
+++ b/drivers/thermal/intel/intel_pch_thermal.c
@@ -326,10 +326,16 @@ static int pch_get_trip_temp(struct thermal_zone_device
*tzd, int trip, int *tem
return 0;
}
+static void pch_critical(struct thermal_zone_device *tzd)
+{
+ dev_dbg(&tzd->device, "%s: critical temperature reached\n", tzd->type);
+}
+
static struct thermal_zone_device_ops tzd_ops = {
.get_temp = pch_thermal_get_temp,
.get_trip_type = pch_get_trip_type,
.get_trip_temp = pch_get_trip_temp,
+ .critical = pch_critical,
};
enum board_ids {
--
2.29.2
[PATCH v2 1/2] thermal: int340x: Fix unexpected shutdown at critical temperature
We are seeing thermal shutdown on Intel based mobile workstations, the
shutdown happens during the first trip handle in
thermal_zone_device_register():
kernel: thermal thermal_zone15: critical temperature reached (101 C), shutting
down
However, we shouldn't do a thermal shutdown here, since
1) We may want to use a dedicated daemon, Intel's thermald in this case,
to handle thermal shutdown.
2) For ACPI based system, _CRT doesn't mean shutdown unless it's inside
ThermalZone namespace. ACPI Spec, 11.4.4 _CRT (Critical Temperature):
"... If this object it present under a device, the device’s driver
evaluates this object to determine the device’s critical cooling
temperature trip point. This value may then be used by the device’s
driver to program an internal device temperature sensor trip point."
So a "critical trip" here merely means we should take a more aggressive
cooling method.
As int340x device isn't present under ACPI ThermalZone, override the
default .critical callback to prevent surprising thermal shutdown.
Signed-off-by: Kai-Heng Feng
---
v2:
- Amend subject.
- Remove int3400 device.
.../thermal/intel/int340x_thermal/int340x_thermal_zone.c| 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
index 6e479deff76b..d1248ba943a4 100644
--- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
+++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
@@ -146,12 +146,18 @@ static int int340x_thermal_get_trip_hyst(struct
thermal_zone_device *zone,
return 0;
}
+static void int340x_thermal_critical(struct thermal_zone_device *zone)
+{
+ dev_dbg(&zone->device, "%s: critical temperature reached\n",
zone->type);
+}
+
static struct thermal_zone_device_ops int340x_thermal_zone_ops = {
.get_temp = int340x_thermal_get_zone_temp,
.get_trip_temp = int340x_thermal_get_trip_temp,
.get_trip_type = int340x_thermal_get_trip_type,
.set_trip_temp = int340x_thermal_set_trip_temp,
.get_trip_hyst = int340x_thermal_get_trip_hyst,
+ .critical = int340x_thermal_critical,
};
static int int340x_thermal_get_trip_config(acpi_handle handle, char *name,
--
2.29.2
Re: [PATCH v2] ALSA: hda: Continue to probe when codec probe fails
On Tue, Dec 22, 2020 at 12:47 AM Takashi Iwai wrote: [snip] > But what happens if you plug the HDMI cable later and want to use the > HDMI audio? It won't work with your fix, right? No it won't. It's possible to fix from nouveau, but it's at the mercy of Nvidia to fix their proprietary driver, which many users use. Kai-Heng > > > Takashi
Re: [PATCH 1/2] thermal: int340x: Add critical callback to override default shutdown behavior
On Tue, Dec 22, 2020 at 12:55 AM Srinivas Pandruvada
wrote:
>
> On Mon, 2020-12-21 at 21:52 +0800, Kai-Heng Feng wrote:
> > We are seeing thermal shutdown on Intel based mobile workstations,
> > the
> > shutdown happens during the first trip handle in
> > thermal_zone_device_register():
> > kernel: thermal thermal_zone15: critical temperature reached (101 C),
> > shutting down
> >
> > However, we shouldn't do a thermal shutdown here, since
> > 1) We may want to use a dedicated daemon, Intel's thermald in this
> > case,
> > to handle thermal shutdown.
> >
> > 2) For ACPI based system, _CRT doesn't mean shutdown unless it's
> > inside
> > ThermalZone namespace. ACPI Spec, 11.4.4 _CRT (Critical Temperature):
> > "... If this object it present under a device, the device’s driver
> > evaluates this object to determine the device’s critical cooling
> > temperature trip point. This value may then be used by the device’s
> > driver to program an internal device temperature sensor trip point."
> >
> > So a "critical trip" here merely means we should take a more
> > aggressive
> > cooling method.
> >
> > As int340x device isn't present under ACPI ThermalZone, override the
> > default .critical callback to prevent surprising thermal shutdown.
> >
> > Signed-off-by: Kai-Heng Feng
> > ---
> > drivers/thermal/intel/int340x_thermal/int3400_thermal.c | 6
> > ++
> > .../thermal/intel/int340x_thermal/int340x_thermal_zone.c| 6
> > ++
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> > b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> > index 823354a1a91a..9778a6dba939 100644
> > --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> > +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> > @@ -431,9 +431,15 @@ static int int3400_thermal_change_mode(struct
> > thermal_zone_device *thermal,
> > return result;
> > }
> >
> > +static void int3400_thermal_critical(struct thermal_zone_device
> > *thermal)
> > +{
> > + dev_dbg(&thermal->device, "%s: critical temperature
> > reached\n", thermal->type);
> > +}
> > +
> > static struct thermal_zone_device_ops int3400_thermal_ops = {
> > .get_temp = int3400_thermal_get_temp,
> > .change_mode = int3400_thermal_change_mode,
> > + .critical = int3400_thermal_critical,
> > };
>
> You don't need for int3400 device. This is a fake sensor.
Thanks. Let me send a v2.
Kai-Heng
>
> >
> > static struct thermal_zone_params int3400_thermal_params = {
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > index 6e479deff76b..d1248ba943a4 100644
> > --- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > +++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
> > @@ -146,12 +146,18 @@ static int int340x_thermal_get_trip_hyst(struct
> > thermal_zone_device *zone,
> > return 0;
> > }
> >
> > +static void int340x_thermal_critical(struct thermal_zone_device
> > *zone)
> > +{
> > + dev_dbg(&zone->device, "%s: critical temperature reached\n",
> > zone->type);
> > +}
> > +
> > static struct thermal_zone_device_ops int340x_thermal_zone_ops = {
> > .get_temp = int340x_thermal_get_zone_temp,
> > .get_trip_temp = int340x_thermal_get_trip_temp,
> > .get_trip_type = int340x_thermal_get_trip_type,
> > .set_trip_temp = int340x_thermal_set_trip_temp,
> > .get_trip_hyst = int340x_thermal_get_trip_hyst,
> > + .critical = int340x_thermal_critical,
> > };
> >
> > static int int340x_thermal_get_trip_config(acpi_handle handle, char
> > *name,
>
> Thanks,
> Srinivas
>
>
Re: [PATCH v2] ALSA: hda: Continue to probe when codec probe fails
[+Cc nouveau]
On Fri, Dec 18, 2020 at 4:06 PM Takashi Iwai wrote:
[snip]
> > Quite possibly the system doesn't power up HDA controller when there's
> > no external monitor.
> > So when it's connected to external monitor, it's still needed for HDMI
> > audio.
> > Let me ask the user to confirm this.
>
> Yeah, it's the basic question whether the HD-audio is supposed to work
> on this machine at all. If yes, the current approach we take makes
> less sense - instead we should rather make the HD-audio controller
> working.
Yea, confirmed that the Nvidia HDA works when HDMI is connected prior boot.
> > > - The second problem is that pci_enable_device() ignores the error
> > > returned from pci_set_power_state() if it's -EIO. And the
> > > inaccessible access error returns -EIO, although it's rather a fatal
> > > problem. So the driver believes as the PCI device gets enabled
> > > properly.
> >
> > This was introduced in 2005, by Alan's 11f3859b1e85 ("[PATCH] PCI: Fix
> > regression in pci_enable_device_bars") to fix UHCI controller.
> >
> > >
> > > - The third problem is that HD-audio driver blindly believes the
> > > codec_mask read from the register even if it's a read failure as I
> > > already showed.
> >
> > This approach has least regression risk.
>
> Yes, but it assumes that HD-audio is really non-existent.
I really don't know any good approach to address this.
On Windows, HDA PCI is "hidden" until HDMI cable is plugged, then the
driver will flag the magic bit to make HDA audio appear on the PCI
bus.
IIRC the current approach is to make nouveau and device link work.
Kai-Heng
>
>
> thanks,
>
> Takashi
