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.
Signed-off-by: Oza Pawandeep <poza@xxxxxxxxxxxxxx>
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.
