| 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
| ||
|
Message-ID: <fb9d4791-73e2-491a-8117-e8e5f811fe7a@collabora.com>
Date: Fri, 8 Mar 2024 14:02:36 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Jianjun Wang (王建军) <Jianjun.Wang@...iatek.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
"wenst@...omium.org" <wenst@...omium.org>,
"kernel@...labora.com" <kernel@...labora.com>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
"lpieralisi@...nel.org" <lpieralisi@...nel.org>,
Ryder Lee <Ryder.Lee@...iatek.com>,
"nfraprado@...labora.com" <nfraprado@...labora.com>
Subject: Re: [PATCH v2] PCI: mediatek-gen3: Assert MAC reset only if PHY reset
also present
Il 08/03/24 10:44, Jianjun Wang (王建军) ha scritto:
> On Wed, 2024-03-06 at 09:50 +0100, AngeloGioacchino Del Regno wrote:
>> Il 01/03/24 03:48, Jianjun Wang (王建军) ha scritto:
>>> Hi Angelo,
>>>
>>> Thanks for your patch.
>>>
>>> On Thu, 2024-02-29 at 10:24 +0100, AngeloGioacchino Del Regno
>>> wrote:
>>>> Some SoCs have two PCI-Express controllers: in the case of
>>>> MT8195,
>>>> one of them is using a dedicated PHY, but the other uses a combo
>>>> PHY
>>>> that is shared with USB and in that case the PHY cannot be reset
>>>> from the PCIe driver, or USB functionality will be unable to
>>>> resume.
>>>>
>>>> Resetting the PCIe MAC without also resetting the PHY will result
>>>> in
>>>> a full system lockup at PCIe resume time and the only option to
>>>> resume operation is to hard reboot the system (with a PMIC cut-
>>>> off).
>>>>
>>>> To resolve this issue, check if we've got both a PHY and a MAC
>>>> reset
>>>> and, if not, never assert resets at PM suspend time: in that
>>>> case,
>>>> the link is still getting powered down as both the clocks and the
>>>> power domains will go down anyway.
>>>>
>>>> Fixes: d537dc125f07 ("PCI: mediatek-gen3: Add system PM support")
>>>> Signed-off-by: AngeloGioacchino Del Regno <
>>>> angelogioacchino.delregno@...labora.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Rebased over next-20240229
>>>>
>>>> drivers/pci/controller/pcie-mediatek-gen3.c | 25
>>>> ++++++++++++++-----
>>>> --
>>>> 1 file changed, 17 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
>>>> b/drivers/pci/controller/pcie-mediatek-gen3.c
>>>> index 975b3024fb08..99b5d7a49be1 100644
>>>> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
>>>> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
>>>> @@ -874,17 +874,26 @@ static int mtk_pcie_power_up(struct
>>>> mtk_gen3_pcie *pcie)
>>>> return err;
>>>> }
>>>>
>>>> -static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie)
>>>> +static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie, bool
>>>> is_suspend)
>>>> {
>>>> + bool suspend_reset_supported = pcie->mac_reset && pcie-
>>>>> phy_reset;
>>>>
>>>> +
>>>> clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
>>>>
>>>> pm_runtime_put_sync(pcie->dev);
>>>> pm_runtime_disable(pcie->dev);
>>>> - reset_control_assert(pcie->mac_reset);
>>>> +
>>>> + /*
>>>> + * Assert MAC reset only if we also got a PHY reset, otherwise
>>>> + * the system will lockup at PM resume time.
>>>> + */
>>>> + if (is_suspend && suspend_reset_supported)
>>>> + reset_control_assert(pcie->mac_reset);
>>>>
>>>> phy_power_off(pcie->phy);
>>>> phy_exit(pcie->phy);
>>>> - reset_control_assert(pcie->phy_reset);
>>>> + if (is_suspend && suspend_reset_supported)
>>>> + reset_control_assert(pcie->phy_reset);
>>>> }
>>>>
>>>> static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
>>>> @@ -920,7 +929,7 @@ static int mtk_pcie_setup(struct
>>>> mtk_gen3_pcie
>>>> *pcie)
>>>> return 0;
>>>>
>>>> err_setup:
>>>> - mtk_pcie_power_down(pcie);
>>>> + mtk_pcie_power_down(pcie, false);
>>>>
>>>> return err;
>>>> }
>>>> @@ -951,7 +960,7 @@ static int mtk_pcie_probe(struct
>>>> platform_device
>>>> *pdev)
>>>> err = pci_host_probe(host);
>>>> if (err) {
>>>> mtk_pcie_irq_teardown(pcie);
>>>> - mtk_pcie_power_down(pcie);
>>>> + mtk_pcie_power_down(pcie, false);
>>>> return err;
>>>> }
>>>>
>>>> @@ -969,7 +978,7 @@ static void mtk_pcie_remove(struct
>>>> platform_device *pdev)
>>>> pci_unlock_rescan_remove();
>>>>
>>>> mtk_pcie_irq_teardown(pcie);
>>>> - mtk_pcie_power_down(pcie);
>>>> + mtk_pcie_power_down(pcie, false);
>>>
>>> Is there any reason not to reset the MAC and PHY when probe fails
>>> and
>>> driver removing? Some SoCs may not have MTCMOS to cut off their
>>> power,
>>> we need to assert the reset signal to save power in that case.
>>>
>>
>> Sorry for the late reply - yes, there is a reason.
>>
>> On platforms needing this quirk, resetting at .remove() time will
>> hang the
>> machine if the module is reinserted later (hence, .probe() called at
>> a later
>> time).
>
> Does this only happen when the PCIe MAC is reset without resetting the
> PHY? Is it related to the reset framework?
>
Happens only when MAC is reset without resetting the PHY after calling
phy_power_off(); phy_exit();
It's not a reset framework issue, that's for sure.
Angelo
> Thanks.
>>
>> Regards,
>> Angelo
>>
>>> Thanks.
>>>
>>>> }
>>>>
>>>> static void mtk_pcie_irq_save(struct mtk_gen3_pcie *pcie)
>>>> @@ -1044,7 +1053,7 @@ static int mtk_pcie_suspend_noirq(struct
>>>> device
>>>> *dev)
>>>> dev_dbg(pcie->dev, "entered L2 states successfully");
>>>>
>>>> mtk_pcie_irq_save(pcie);
>>>> - mtk_pcie_power_down(pcie);
>>>> + mtk_pcie_power_down(pcie, true);
>>>>
>>>> return 0;
>>>> }
>>>> @@ -1060,7 +1069,7 @@ static int mtk_pcie_resume_noirq(struct
>>>> device
>>>> *dev)
>>>>
>>>> err = mtk_pcie_startup_port(pcie);
>>>> if (err) {
>>>> - mtk_pcie_power_down(pcie);
>>>> + mtk_pcie_power_down(pcie, false);
>>>> return err;
>>>> }
>>>>
>>
>>
>>
--
AngeloGioacchino Del Regno
Senior Software Engineer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718
Powered by blists - more mailing lists