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@oss.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);
break;
}
--
2.45.2On 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@oss.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
Konrad
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@oss.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.
- Mani
--
மணிவண்ணன் சதாசிவம்
On 7/15/2025 4:06 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@oss.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.
>
How about using enable_device in host bridge, without pci_enable_device
call the endpoints can't start the transfers. May be we can use that.
- Krishna Chaitanya.
> - Mani
>
On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote:
>
>
> On 7/15/2025 4:06 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@oss.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.
> >
> How about using enable_device in host bridge, without pci_enable_device
> call the endpoints can't start the transfers. May be we can use that.
>
Q: Who is going to call pci_enable_device()?
A: The PCI client driver
This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier.
- Mani
--
மணிவண்ணன் சதாசிவம்
On Wed, Jul 16, 2025 at 12:16:42PM +0530, Manivannan Sadhasivam wrote: > On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote: > > How about using enable_device in host bridge, without pci_enable_device > > call the endpoints can't start the transfers. May be we can use that. > > Q: Who is going to call pci_enable_device()? > A: The PCI client driver > > This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier. It seems to me that enable_device() may be a better fit if we're only going to enable ASPM for devices with a driver (or when enabled through sysfs). PCI core will already have placed the device in D0, and this avoids dealing with notifiers. Johan
On Mon, Jul 21, 2025 at 11:02:01AM GMT, Johan Hovold wrote: > On Wed, Jul 16, 2025 at 12:16:42PM +0530, Manivannan Sadhasivam wrote: > > On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote: > > > > How about using enable_device in host bridge, without pci_enable_device > > > call the endpoints can't start the transfers. May be we can use that. > > > > Q: Who is going to call pci_enable_device()? > > A: The PCI client driver > > > > This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier. > > It seems to me that enable_device() may be a better fit if we're only > going to enable ASPM for devices with a driver (or when enabled through > sysfs). > > PCI core will already have placed the device in D0, and this avoids > dealing with notifiers. > I'm planning to drop this series in favor of this patch (with one yet-to-be-submitted patch for pcie-qcom on top): https://lore.kernel.org/linux-pci/20250720190140.2639200-1-david.e.box@linux.intel.com/ This patch is more elegant compared to this series and also avoids the issue we are discussing. - Mani -- மணிவண்ணன் சதாசிவம்
On 7/16/2025 12:16 PM, Manivannan Sadhasivam wrote:
> On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 7/15/2025 4:06 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@oss.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.
>>>
>> How about using enable_device in host bridge, without pci_enable_device
>> call the endpoints can't start the transfers. May be we can use that.
>>
>
> Q: Who is going to call pci_enable_device()?
> A: The PCI client driver
>
> This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier.
>
userspace can enable device using sysfs[1] without attaching
any kernel drivers.
[1]
https://elixir.bootlin.com/linux/v6.16-rc6/source/drivers/pci/pci-sysfs.c#L315
- Krishna Chaitanya.
> - Mani
>
On Wed, Jul 16, 2025 at 12:23:54PM GMT, Krishna Chaitanya Chundru wrote:
>
>
> On 7/16/2025 12:16 PM, Manivannan Sadhasivam wrote:
> > On Wed, Jul 16, 2025 at 10:24:23AM GMT, Krishna Chaitanya Chundru wrote:
> > >
> > >
> > > On 7/15/2025 4:06 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@oss.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.
> > > >
> > > How about using enable_device in host bridge, without pci_enable_device
> > > call the endpoints can't start the transfers. May be we can use that.
> > >
> >
> > Q: Who is going to call pci_enable_device()?
> > A: The PCI client driver
> >
> > This is same as relying on BUS_NOTIFY_BIND_DRIVER notifier.
> >
> userspace can enable device using sysfs[1] without attaching
> any kernel drivers.
>
But that's not a common usecase. Even so, we cannot insist users to write to the
sysfs knob to let ASPM/OPP work without a driver.
- Mani
--
மணிவண்ணன் சதாசிவம்
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@oss.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? Do
ASPM setting need to be reapplied after the PCIe device is reset? (well
I would assume there are probably multiple levels of "reset" :/)
Konrad
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@oss.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
--
மணிவண்ணன் சதாசிவம்
On 7/16/25 7:28 AM, Manivannan Sadhasivam wrote: > 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@oss.qualcomm.com> >>>>> --- [...] >>> 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. I'm not opposed if there's going to be an improved solution next cycle. Having ASPM 99.9% of the time is much better than not having it at all > >> 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. Nice, thanks for confirming Konrad
On Wed, Jul 16, 2025 at 12:33:48PM +0200, Konrad Dybcio wrote: > On 7/16/25 7:28 AM, Manivannan Sadhasivam wrote: > > 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@oss.qualcomm.com> > >>>>> --- > > [...] > > >>> 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. > > I'm not opposed if there's going to be an improved solution next cycle. > Having ASPM 99.9% of the time is much better than not having it at all This has been broken since 6.15 (not 6.13 as the commit message of patch 1/2 suggests) so there's no rush to get it sorted in 6.16. The current approach also works for everything but devices using pwrctrl (read: anything but ath11k/ath12k). It seems like adding an enable_device() callback can be used as minimal, backportable fix for the ath11k/ath12k regression on Qualcomm platforms, while working on something more general (e.g. also handling the OPPs) if that turns out to be more invasive. Johan
On Mon, Jul 14, 2025 at 11:31:05PM +0530, 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.
This commit message should be amended so that it makes sense on its own
(e.g. without Subject).
> @@ -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);
I guess you should also drop the now redundant
qcom_pcie_icc_opp_update() call from probe()?
> break;
> }
Johan
On Tue, Jul 15, 2025 at 09:51:40AM GMT, Johan Hovold wrote:
> On Mon, Jul 14, 2025 at 11:31:05PM +0530, 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.
>
> This commit message should be amended so that it makes sense on its own
> (e.g. without Subject).
>
> > @@ -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);
>
> I guess you should also drop the now redundant
> qcom_pcie_icc_opp_update() call from probe()?
>
Oops. This got sneaked in. I removed it locally but eventually lost the change
while rebasing. Will remove it in next version. This API just bails out if the
link is not up. So no reason to call it here also now.
- Mani
--
மணிவண்ணன் சதாசிவம்
© 2016 - 2025 Red Hat, Inc.