linux-kernel - Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <kyu4bpuqvmc3iyqekmqvbpxqpbbxbq7df725dcpiu3dnvcztyy@yyqwm2uqjobj>
Date: Wed, 16 Jul 2025 10:58:01 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Johan Hovold <johan@...nel.org>, Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Subject: Re: [PATCH 2/2] PCI: qcom: Move qcom_pcie_icc_opp_update() to
 notifier callback

On Tue, Jul 15, 2025 at 12:45:36PM GMT, Konrad Dybcio wrote:
> On 7/15/25 12:36 PM, Manivannan Sadhasivam wrote:
> > On Tue, Jul 15, 2025 at 11:54:48AM GMT, Konrad Dybcio wrote:
> >> On 7/14/25 8:01 PM, Manivannan Sadhasivam wrote:
> >>> It allows us to group all the settings that need to be done when a PCI
> >>> device is attached to the bus in a single place.
> >>>
> >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> >>> ---
> >>>  drivers/pci/controller/dwc/pcie-qcom.c | 3 +--
> >>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >>> index b4993642ed90915299e825e47d282b8175a78346..b364977d78a2c659f65f0f12ce4274601d20eaa6 100644
> >>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >>> @@ -1616,8 +1616,6 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> >>>  		pci_lock_rescan_remove();
> >>>  		pci_rescan_bus(pp->bridge->bus);
> >>>  		pci_unlock_rescan_remove();
> >>> -
> >>> -		qcom_pcie_icc_opp_update(pcie);
> >>>  	} else {
> >>>  		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
> >>>  			      status);
> >>> @@ -1765,6 +1763,7 @@ static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
> >>>  	switch (action) {
> >>>  	case BUS_NOTIFY_BIND_DRIVER:
> >>>  		qcom_pcie_enable_aspm(pdev);
> >>> +		qcom_pcie_icc_opp_update(pcie);
> >>
> >> So I assume that we're not exactly going to do much with the device if
> >> there isn't a driver for it, but I have concerns that since the link
> >> would already be established(?), the icc vote may be too low, especially
> >> if the user uses something funky like UIO
> >>
> > 
> > Hmm, that's a good point. Not enabling ASPM wouldn't have much consequence, but
> > not updating OPP would be.
> > 
> > Let me think of other ways to call these two APIs during the device addition. If
> > there are no sane ways, I'll drop *this* patch.
> 
> Would it be too naive to assume BUS_NOTIFY_ADD_DEVICE is a good fit?

BUS_NOTIFY_ADD_DEVICE is not currently a good fit as ASPM link state
initialization happen after all the devices are enumerated for the slot. This is
something to be fixed in the PCI core and would allow us to use
BUS_NOTIFY_ADD_DEVICE.

I talked to Bjorn H and we both agreed that this needs to be revisited. But I'm
just worrried that until this happens, we cannot upstream the ASPM fix and not
even backport it to 6.16/16.

So maybe we need to resort to this patch as an interim fix if everyone agrees.

> Do
> ASPM setting need to be reapplied after the PCIe device is reset? (well
> I would assume there are probably multiple levels of "reset" :/)
> 

I'm assuming that you are referring to link down reset here. PCI core takes care
of saving both the endpoint as well as Root Port config space when that happens
and restores them afterwards.

- Mani

-- 
மணிவண்ணன் சதாசிவம்