linux-kernel - Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1a48feae-f55f-4df8-b165-84c1cb2f6658@collabora.com>
Date: Tue, 7 Jan 2025 12:44:43 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Jianjun Wang (王建军) <Jianjun.Wang@...iatek.com>,
 "manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>,
 "robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
 "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
 "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
 "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
 "lpieralisi@...nel.org" <lpieralisi@...nel.org>
Cc: "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
 "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
 Ryder Lee <Ryder.Lee@...iatek.com>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 Xavier Chang (張獻文) <Xavier.Chang@...iatek.com>
Subject: Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

Il 07/01/25 03:18, Jianjun Wang (王建军) ha scritto:
> On Fri, 2025-01-03 at 10:16 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> Il 03/01/25 07:00, Jianjun Wang ha scritto:
>>> Disable ASPM L0s support because it does not significantly save
>>> power
>>> but impacts performance.
>>>
>>
>> That may be a good idea but, without numbers to support your
>> statement, it's a bit
>> difficult to say.
>>
>> How much power does ASPM L0s save on MediaTek SoCs, in microwatts?
>> How is the performance impacted, and on which specific device(s) on
>> the PCIe bus?
> 
> It's hard to tell the exact number because it is difficult to measure,
> and the number of entries into the L0s state may vary even in the same
> test scenario.
> 
> However, we have encountered some compatibility issues when connected
> with some PCIe EPs, and disabling the L0s can fix it. I think disabling
> L0s might be the better way, since we usually use L1ss for power-saving
> when the link is idle.
> 

To actually decide, we should know what's actually broken, then.

Is the MediaTek controller broken, or is the device broken?
So, is it a MTK quirk, or a device quirk?

If the problem is actually device-related, then this should be handled as
a device-specific quirk, as not just MediaTek platforms would be affected
by compatibility issues.

If the MediaTek PCIe controller is at fault, instead, I agree about just
disabling L0s at the controller level - but then this shall be mentioned
in the commit message, and should have a Fixes tag as well.

Cheers,
Angelo

> Thanks.
> 
>>
>> Cheers,
>> Angelo
>>
>>> Signed-off-by: Jianjun Wang <jianjun.wang@...iatek.com>
>>> ---
>>>    drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
>>> b/drivers/pci/controller/pcie-mediatek-gen3.c
>>> index ed3c0614486c..4bd3b39eebe2 100644
>>> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
>>> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
>>> @@ -84,6 +84,9 @@
>>>    #define PCIE_MSI_SET_ENABLE_REG             0x190
>>>    #define PCIE_MSI_SET_ENABLE         GENMASK(PCIE_MSI_SET_NUM - 1,
>>> 0)
>>>
>>> +#define PCIE_LOW_POWER_CTRL_REG              0x194
>>> +#define PCIE_FORCE_DIS_L0S           BIT(8)
>>> +
>>>    #define PCIE_PIPE4_PIE8_REG         0x338
>>>    #define PCIE_K_FINETUNE_MAX         GENMASK(5, 0)
>>>    #define PCIE_K_FINETUNE_ERR         GENMASK(7, 6)
>>> @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct
>>> mtk_gen3_pcie *pcie)
>>>        val &= ~PCIE_INTX_ENABLE;
>>>        writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
>>>
>>> +     /*
>>> +      * Disable L0s support because it does not significantly save
>>> power
>>> +      * but impacts performance.
>>> +      */
>>> +     val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG);
>>> +     val |= PCIE_FORCE_DIS_L0S;
>>> +     writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
>>> +
>>>        /* Disable DVFSRC voltage request */
>>>        val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
>>>        val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
>>
>>