by Oza Pawandeep

[permalink] [raw]

Subject: Re: [PATCH v11 6/7] PCI: Unify wait for link active into generic pci

On 2018-02-24 21:11, Bjorn Helgaas wrote:
> In subject:
>
> s/pci/PCI/
sure.
>
> On Fri, Feb 23, 2018 at 01:54:03PM +0530, Oza Pawandeep wrote:
>> Clients such as pciehp, dpc are using pcie_wait_link_active, which
>> waits
>> till the link becomes active or inactive.
>
> Use "()" after function names so we have a visual clue that they are
> functions.
>
>> Made generic function and moved it to drivers/pci/pci.c
>>
>> Signed-off-by: Oza Pawandeep <[email protected]>
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c
>> b/drivers/pci/hotplug/pciehp_hpc.c
>> index 18a42f8..de9b0ea 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -231,25 +231,11 @@ bool pciehp_check_link_active(struct controller
>> *ctrl)
>> return ret;
>> }
>>
>> -static void __pcie_wait_link_active(struct controller *ctrl, bool
>> active)
>> +static bool pcie_wait_link_active(struct controller *ctrl)
>> {
>> - int timeout = 1000;
>> -
>> - if (pciehp_check_link_active(ctrl) == active)
>> - return;
>> - while (timeout > 0) {
>> - msleep(10);
>> - timeout -= 10;
>> - if (pciehp_check_link_active(ctrl) == active)
>> - return;
>> - }
>> - ctrl_dbg(ctrl, "Data Link Layer Link Active not %s in 1000 msec\n",
>> - active ? "set" : "cleared");
>> -}
>> + struct pci_dev *pdev = ctrl_dev(ctrl);
>>
>> -static void pcie_wait_link_active(struct controller *ctrl)
>> -{
>> - __pcie_wait_link_active(ctrl, true);
>> + return pci_wait_for_link(pdev, true);
>> }
>>
>> static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index f6a4dd1..f8d44b4 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4176,6 +4176,37 @@ static int pci_pm_reset(struct pci_dev *dev,
>> int probe)
>> return 0;
>> }
>>
>> +/**
>> + * pci_wait_for_link - Wait for link till its active/inactive
>
> s/its/it's/
sure.
>
>> + * @pdev: Bridge device
>> + * @active: waiting for active or inactive ?
>> + *
>> + * Use this to wait till link becomes active or inactive.
>> + */
>> +bool pci_wait_for_link(struct pci_dev *pdev, bool active)
>
> I think this should be "pcie_wait_for_link()". There's no concept of a
> link in conventional PCI.
>
>> +{
>> + int timeout = 1000;
>> + bool ret;
>> + u16 lnk_status;
>> +
>> + for (;;) {
>> + pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
>> + ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
>> + if (ret == active)
>> + return true;
>> + if (timeout <= 0)
>> + break;
>> + timeout -= 10;
>> + }
>> +
>> + dev_printk(KERN_DEBUG, &pdev->dev,
>
> pci_info(). Distros often seem to have logging configured so
> KERN_DEBUG things aren't captured, and this definitely seems worth
> capturing.
>
>> + "Data Link Layer Link Active not %s in 1000 msec\n",
>> + active ? "set" : "cleared");
>> +
>> + return false;
>> +}
>> +EXPORT_SYMBOL(pci_wait_for_link);
>
> I don't think this needs to be exported.
>
sure.
>> void pci_reset_secondary_bus(struct pci_dev *dev)
>> {
>> u16 ctrl;
>> diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
>> index 5c01c63..e15bcda 100644
>> --- a/drivers/pci/pcie/pcie-dpc.c
>> +++ b/drivers/pci/pcie/pcie-dpc.c
>> @@ -120,19 +120,9 @@ static int dpc_wait_rp_inactive(struct dpc_dev
>> *dpc)
>>
>> static void dpc_wait_link_inactive(struct dpc_dev *dpc)
>> {
>> - unsigned long timeout = jiffies + HZ;
>> struct pci_dev *pdev = dpc->dev->port;
>> - struct device *dev = &dpc->dev->device;
>> - u16 lnk_status;
>>
>> - pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
>> - while (lnk_status & PCI_EXP_LNKSTA_DLLLA &&
>> - !time_after(jiffies, timeout)) {
>> - msleep(10);
>> - pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
>> - }
>> - if (lnk_status & PCI_EXP_LNKSTA_DLLLA)
>> - dev_warn(dev, "Link state not disabled for DPC event\n");
>> + pci_wait_for_link(pdev, false);
>> }
>>
>> /**
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 024a1be..cb674c3 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1195,6 +1195,7 @@ int pci_add_ext_cap_save_buffer(struct pci_dev
>> *dev,
>> int pci_request_selected_regions(struct pci_dev *, int, const char
>> *);
>> int pci_request_selected_regions_exclusive(struct pci_dev *, int,
>> const char *);
>> void pci_release_selected_regions(struct pci_dev *, int);
>> +bool pci_wait_for_link(struct pci_dev *pdev, bool active);
>
> I don't think this needs to be available outside the PCI core, so this
> could probably be declared in drivers/pci/pci.h
>

thanks, will move it.

>> /* drivers/pci/bus.c */
>> struct pci_bus *pci_bus_get(struct pci_bus *bus);
>> --
>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
>> Technologies, Inc.,
>> a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
>> Linux Foundation Collaborative Project.
>>

2018-02-27 14:43:03

by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [PATCH v11 3/7] PCI/ERR: add mutex to synchronize recovery

On Tue, Feb 27, 2018 at 10:46:34AM +0530, [email protected] wrote:
> On 2018-02-24 05:15, Bjorn Helgaas wrote:
> > On Fri, Feb 23, 2018 at 01:54:00PM +0530, Oza Pawandeep wrote:
> > > This patch protects pci_do_recovery with mutex.
> >
> > pcie_do_recovery()
> >
> > Please explain why the mutex is necessary. What bad things happen
> > without the mutex?
> >
> > You named (some) of the other things "pcie"; maybe use "pcie" in the
> > mutex name as well so they look the same.
> >
>
> PCIe specification: 6.2.10
> When DPC is triggered due to receipt of an uncorrectable error Message, the
> Requester ID from the Message is recorded in the DPC Error Source ID
> register and that Message is discarded and not forwarded Upstream.
>
> So, having said that, what we think is we dont need Mutex, because in DPC
> enabled system either AER or DPC can be triggered, not both.
> so right now there is no need of guarding pcie_do_recovery() with mutex.
>
> but I was in a thought that; since pcie_do_recovery is supposed to be used
> by error clients,
> from sw architecture point of view, adding mutex takes care of concurrency
> if it exists (in corner cases, faulty hw where both AER and DPC triggered
> etc..)
>
> We can choose to drop this patch, since we dont require mutex.
> Bjorn, please advise.

I'm not trying to convince you that we don't need the mutex. My
point is that if we *do* need it, the changelog needs to say *why*
(and ideally the code will either have a comment or it will be obvious
from the code why it's necessary).

If we don't have a clear indication that it's required, I guess I
would omit it.

> > > Signed-off-by: Oza Pawandeep <[email protected]>
> > >
> > > diff --git a/drivers/pci/pcie/pcie-err.c b/drivers/pci/pcie/pcie-err.c
> > > index fcd5add..f830975 100644
> > > --- a/drivers/pci/pcie/pcie-err.c
> > > +++ b/drivers/pci/pcie/pcie-err.c
> > > @@ -20,6 +20,8 @@
> > > #include <linux/pcieport_if.h>
> > > #include "portdrv.h"
> > >
> > > +static DEFINE_MUTEX(pci_err_recovery_lock);
> > > +
> > > struct aer_broadcast_data {
> > > enum pci_channel_state state;
> > > enum pci_ers_result result;
> > > @@ -283,6 +285,8 @@ void pcie_do_recovery(struct pci_dev *dev, int
> > > severity)
> > > pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
> > > enum pci_channel_state state;
> > >
> > > + mutex_lock(&pci_err_recovery_lock);
> > > +
> > > if (severity == AER_FATAL)
> > > state = pci_channel_io_frozen;
> > > else
> > > @@ -326,9 +330,11 @@ void pcie_do_recovery(struct pci_dev *dev, int
> > > severity)
> > > report_resume);
> > >
> > > dev_info(&dev->dev, "Device recovery successful\n");
> > > + mutex_unlock(&pci_err_recovery_lock);
> > > return;
> > >
> > > failed:
> > > /* TODO: Should kernel panic here? */
> > > dev_info(&dev->dev, "Device recovery failed\n");
> > > + mutex_unlock(&pci_err_recovery_lock);
> > > }
> > > --
> > > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> > > Technologies, Inc.,
> > > a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project.
> > >