From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Lukas Wunner <lukas@wunner.de>, Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci@vger.kernel.org, Hari Vyas <hari.vyas@broadcom.com>,
Ray Jui <ray.jui@broadcom.com>,
Srinath Mannam <srinath.mannam@broadcom.com>,
Guenter Roeck <linux@roeck-us.net>, Jens Axboe <axboe@kernel.dk>,
Konstantin Khlebnikov <khlebnikov@yandex-team.ru>,
Marta Rybczynska <mrybczyn@kalray.eu>,
Pierre-Yves Kerbrat <pkerbrat@kalray.eu>,
linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/6] pci: Set pci_dev->is_added before calling device_add
Date: Sat, 18 Aug 2018 13:41:02 +1000 [thread overview]
Message-ID: <20490639ea65ae09bec4ed33cf07a69b61935959.camel@kernel.crashing.org> (raw)
In-Reply-To: <20180817181501.m4j7jibzsrgljmlj@wunner.de>
On Fri, 2018-08-17 at 20:15 +0200, Lukas Wunner wrote:
>
> The two steps (enumeration and driver attachment) are protected by
> pci_lock_rescan_remove(). Initially, when a pci_dev is newly allocated
> with kzalloc(), is_added is 0. The transition from 0 to 1 happens while
> under pci_lock_rescan_remove(). When that lock is released, is_added is
> always 1, barring a device_attach() error, in which case it will remain
> at 0.
>
> AFAICS, there is no second mutex needed to ensure synchronization of
> is_added, the existing mutex should be sufficient and the only problem
> are RMW races when accessing adjacent flags. Those were correctly addressed
> by Hari's patch. Benjamin seems to be alleging that concurrency issues
> exist beyond the RMW races, I fail to see them, sorry.
Im saying that fixing those issues using atomic bitops is a generally
unsafe practice even if it happens to work in a specific case.
In this one, I argue that the root problem was simply that is_added was
set in the wrong place. The "fix" from Hari touches 9 files, adds
horrible relative includes to an architecture and generally bloats
things while a single 3 liner would have been sufficient.
The current use of is_added is asymetric (it's cleared during
device_attach but set during detach) which is bogus and the entire race
stems from the fact that it is set after device_attach returns.
So setting it early is, imho, the right fix, is much simpler, and fixes
the odd imbalance we have to begin with.
Ben.
next prev parent reply other threads:[~2018-08-18 3:41 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-17 4:48 [RFC PATCH 0/6] pci: Rework is_added race fix and address bridge enable races Benjamin Herrenschmidt
2018-08-17 4:48 ` [RFC PATCH 1/6] Revert "PCI: Fix is_added/is_busmaster race condition" Benjamin Herrenschmidt
2018-08-17 4:57 ` Benjamin Herrenschmidt
2018-08-17 15:44 ` Bjorn Helgaas
2018-08-18 3:24 ` Benjamin Herrenschmidt
2018-08-19 2:24 ` Bjorn Helgaas
2018-08-20 2:10 ` Benjamin Herrenschmidt
2018-08-20 6:25 ` Hari Vyas
2018-08-20 11:09 ` Benjamin Herrenschmidt
2018-08-20 11:43 ` Hari Vyas
2018-08-20 7:17 ` Lukas Wunner
2018-08-20 11:12 ` Benjamin Herrenschmidt
2018-08-17 4:48 ` [RFC PATCH 2/6] pci: Set pci_dev->is_added before calling device_add Benjamin Herrenschmidt
2018-08-17 4:57 ` Benjamin Herrenschmidt
2018-08-17 16:25 ` Bjorn Helgaas
2018-08-17 18:15 ` Lukas Wunner
2018-08-18 3:41 ` Benjamin Herrenschmidt [this message]
2018-08-18 3:28 ` Benjamin Herrenschmidt
2018-08-17 4:48 ` [RFC PATCH 3/6] pci: Remove priv_flags and use dev->error_state for "disconnected" status Benjamin Herrenschmidt
2018-08-17 5:13 ` [RFC PATCH v2 " Benjamin Herrenschmidt
2018-08-17 4:49 ` [RFC PATCH 4/6] pci: Add a mutex to pci_dev to protect device state Benjamin Herrenschmidt
2018-08-17 4:49 ` [RFC PATCH 5/6] pci: Protect the enable/disable state of pci_dev using the state mutex Benjamin Herrenschmidt
2018-08-17 8:09 ` Marta Rybczynska
2018-08-17 8:30 ` Benjamin Herrenschmidt
2018-08-17 9:00 ` Hari Vyas
2018-08-17 9:39 ` Benjamin Herrenschmidt
2018-08-17 10:10 ` Hari Vyas
2018-08-17 10:24 ` Benjamin Herrenschmidt
2018-08-17 4:49 ` [RFC PATCH 6/6] pci: Protect is_busmaster using the state lock Benjamin Herrenschmidt
2018-08-17 5:03 ` [RFC PATCH 0/6] pci: Rework is_added race fix and address bridge enable races Benjamin Herrenschmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20490639ea65ae09bec4ed33cf07a69b61935959.camel@kernel.crashing.org \
--to=benh@kernel.crashing.org \
--cc=axboe@kernel.dk \
--cc=hari.vyas@broadcom.com \
--cc=helgaas@kernel.org \
--cc=khlebnikov@yandex-team.ru \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=lukas@wunner.de \
--cc=mrybczyn@kalray.eu \
--cc=pkerbrat@kalray.eu \
--cc=ray.jui@broadcom.com \
--cc=srinath.mannam@broadcom.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).