Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1653331
| From | Scott Branden <scott.branden@broadcom.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [RFC PATCH v2] pci: Concurrency issue in NVMe Init through PCIe switch |
| Date | 2017-05-30 17:50 +0200 |
| Message-ID | <tMRzc-KJ-9@gated-at.bofh.it> (permalink) |
| References | <tMLk5-5mP-15@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
Hi Srinath,
On 17-05-30 02:08 AM, Srinath Mannam wrote:
> We found a concurrency issue in NVMe Init when we initialize
> multiple NVMe connected over PCIe switch.
>
> Setup details:
> - SMP system with 8 ARMv8 cores running Linux kernel(4.11).
> - Two NVMe cards are connected to PCIe RC through bridge as shown
> in the below figure.
>
> [RC]
> |
> [BRIDGE]
> |
> -----------
> | |
> [NVMe] [NVMe]
>
> Issue description:
> After PCIe enumeration completed NVMe driver probe function called
> for both the devices from two CPUS simultaneously.
> From nvme_probe, pci_enable_device_mem called for both the EPs. This
> function called pci_enable_bridge enable recursively until RC.
>
> Inside pci_enable_bridge function, at two places concurrency issue is
> observed.
>
> Place 1:
> CPU 0:
> 1. Done Atomic increment dev->enable_cnt
> in pci_enable_device_flags
> 2. Inside pci_enable_resources
> 3. Completed pci_read_config_word(dev, PCI_COMMAND, &cmd)
> 4. Ready to set PCI_COMMAND_MEMORY (0x2) in
> pci_write_config_word(dev, PCI_COMMAND, cmd)
> CPU 1:
> 1. Check pci_is_enabled in function pci_enable_bridge
> and it is true
> 2. Check (!dev->is_busmaster) also true
> 3. Gone into pci_set_master
> 4. Completed pci_read_config_word(dev, PCI_COMMAND, &old_cmd)
> 5. Ready to set PCI_COMMAND_MASTER (0x4) in
> pci_write_config_word(dev, PCI_COMMAND, cmd)
>
> By the time of last point for both the CPUs are read value 0 and
> ready to write 2 and 4.
> After last point final value in PCI_COMMAND register is 4 instead of 6.
>
> Place 2:
> CPU 0:
> 1. Done Atomic increment dev->enable_cnt in
> pci_enable_device_flags
>
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> ---
> Changes since v1:
> - Used mutex to syncronize pci_enable_bridge
>
> drivers/pci/pci.c | 4 ++++
> drivers/pci/probe.c | 1 +
> include/linux/pci.h | 1 +
> 3 files changed, 6 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b01bd5b..5bff3e7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1347,7 +1347,9 @@ static void pci_enable_bridge(struct pci_dev *dev)
> {
> struct pci_dev *bridge;
> int retval;
> + struct mutex *lock = &dev->bridge_lock;
>
> + mutex_lock(lock);
> bridge = pci_upstream_bridge(dev);
> if (bridge)
> pci_enable_bridge(bridge);
> @@ -1355,6 +1357,7 @@ static void pci_enable_bridge(struct pci_dev *dev)
> if (pci_is_enabled(dev)) {
> if (!dev->is_busmaster)
> pci_set_master(dev);
> + mutex_unlock(lock);
> return;
> }
>
> @@ -1363,6 +1366,7 @@ static void pci_enable_bridge(struct pci_dev *dev)
> dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n",
> retval);
> pci_set_master(dev);
> + mutex_unlock(lock);
> }
Looking at above function I think it should be restructured so that
mute_unlock only needs to be called in one place.
How about below to make things more clear?
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 563901c..82c232e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1347,22 +1347,29 @@ static void pci_enable_bridge(struct pci_dev *dev)
{
struct pci_dev *bridge;
int retval;
+ struct mutex *lock = &dev->bridge_lock;
+
+ /*
+ * Add comment here explaining what needs concurrency protection
+ */
+ mutex_lock(lock);
bridge = pci_upstream_bridge(dev);
if (bridge)
pci_enable_bridge(bridge);
- if (pci_is_enabled(dev)) {
- if (!dev->is_busmaster)
- pci_set_master(dev);
- return;
+ if (!pci_is_enabled(dev)) {
+ retval = pci_enable_device(dev);
+ if (retval)
+ dev_err(&dev->dev,
+ "Error enabling bridge (%d), continuing\n",
+ retval);
}
- retval = pci_enable_device(dev);
- if (retval)
- dev_err(&dev->dev, "Error enabling bridge (%d),
continuing\n",
- retval);
- pci_set_master(dev);
+ if (!dev->is_busmaster)
+ pci_set_master(dev);
+
+ mutex_unlock(lock);
}
>
> static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 19c8950..1c25d1c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -880,6 +880,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> child->dev.parent = child->bridge;
> pci_set_bus_of_node(child);
> pci_set_bus_speed(child);
> + mutex_init(&bridge->bridge_lock);
>
> /* Set up default resource pointers and names.. */
> for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33c2b0b..7e88f41 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -266,6 +266,7 @@ struct pci_dev {
> void *sysdata; /* hook for sys-specific extension */
> struct proc_dir_entry *procent; /* device entry in /proc/bus/pci */
> struct pci_slot *slot; /* Physical slot this device is in */
> + struct mutex bridge_lock;
>
> unsigned int devfn; /* encoded device & function index */
> unsigned short vendor;
Regards,
Scott
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[RFC PATCH v2] pci: Concurrency issue in NVMe Init through PCIe switch Srinath Mannam <srinath.mannam@broadcom.com> - 2017-05-30 11:10 +0200
Re: [RFC PATCH v2] pci: Concurrency issue in NVMe Init through PCIe switch Scott Branden <scott.branden@broadcom.com> - 2017-05-30 17:50 +0200
Re: [RFC PATCH v2] pci: Concurrency issue in NVMe Init through PCIe switch Ray Jui <ray.jui@broadcom.com> - 2017-05-30 19:10 +0200
Re: [RFC PATCH v2] pci: Concurrency issue in NVMe Init through PCIe switch Scott Branden <scott.branden@broadcom.com> - 2017-05-30 20:20 +0200
Re: [RFC PATCH v2] pci: Concurrency issue in NVMe Init through PCIe switch Ray Jui <ray.jui@broadcom.com> - 2017-05-30 20:30 +0200
csiph-web