lkml.org 
[lkml]   [2018]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 5/6] pci: Protect the enable/disable state of pci_dev using the state mutex


----- On 17 Aug, 2018, at 06:49, Benjamin Herrenschmidt benh@kernel.crashing.org wrote:

> This protects enable/disable operations using the state mutex to
> avoid races with, for example, concurrent enables on a bridge.
>
> The bus hierarchy is walked first before taking the lock to
> avoid lock nesting (though it would be ok if we ensured that
> we always nest them bottom-up, it is better to just avoid the
> issue alltogether, especially as we might find cases where
> we want to take it top-down later).
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>


>
> static void pci_enable_bridge(struct pci_dev *dev)
> {
> struct pci_dev *bridge;
> - int retval;
> + int retval, enabled;
>
> bridge = pci_upstream_bridge(dev);
> if (bridge)
> pci_enable_bridge(bridge);
>
> - if (pci_is_enabled(dev)) {
> - if (!dev->is_busmaster)
> - pci_set_master(dev);
> + /* Already enabled ? */
> + pci_dev_state_lock(dev);
> + enabled = pci_is_enabled(dev);
> + if (enabled && !dev->is_busmaster)
> + pci_set_master(dev);
> + pci_dev_state_unlock(dev);
> + if (enabled)
> return;
> - }
>

This looks complicated too me especially with the double locking. What do you
think about a function doing that check that used an unlocked version of
pcie_set_master?

Like:

dev_state_lock(dev);
enabled = pci_is_enabled(dev);
if (enabled && !dev->is_busmaster)
pci_set_master_unlocked();
pci_dev_state_unlock(dev);

BTW If I remember correctly the code today can set bus mastering multiple
times without checking if already done.

Marta

\
 
 \ /
  Last update: 2018-08-17 10:18    [W:0.165 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site