linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexander Duyck <alexander.duyck@gmail.com>
To: bvanassche@acm.org
Cc: alexander.h.duyck@linux.intel.com,
	Greg KH <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	len.brown@intel.com, rafael@kernel.org, linux-pm@vger.kernel.org,
	jiangshanlai@gmail.com, pavel@ucw.cz, zwisler@kernel.org,
	Tejun Heo <tj@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [driver-core PATCH v4 4/6] driver core: Probe devices asynchronously instead of the driver
Date: Thu, 18 Oct 2018 19:20:12 -0700	[thread overview]
Message-ID: <CAKgT0Ucf-v3FNTA0z=3nPR-xS=0NYeqDjRfDW-KjDE5Ed-n1_g@mail.gmail.com> (raw)
In-Reply-To: <1539893636.81977.29.camel@acm.org>

On Thu, Oct 18, 2018 at 1:15 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On Thu, 2018-10-18 at 12:38 -0700, Alexander Duyck wrote:
> > Basically if somebody loads a driver the dev->driver becomes set. If a
> > driver is removed it will clear dev->driver and set driver_data to
> > 0/NULL. That is what I am using as a mutex to track it in conjunction
> > with the device mutex. Basically if somebody attempts to attach a driver
> > before we get there we just exit and don't attempt to load this driver.
>
> I don't think that the above matches your code. __device_attach() does not
> set the dev->driver pointer before scheduling an asynchronous probe. Only
> dev->driver_data gets set before the asynchonous probe is scheduled. Since
> driver_detach() only iterates over devices that are in the per-driver klist
> it will skip all devices for which an asynchronous probe has been scheduled
> but __device_attach_async_helper() has not yet been called. My conclusion
> remains that this patch does not prevent a driver pointer to become invalid
> concurrently with __device_attach_async_helper() dereferencing the same
> driver pointer.
>
> Bart.

I see what you are talking about now. Actually I think this was an
existing issue before my patch even came into play. Basically the code
as it currently stands is device specific in terms of the attach and
release code.

I wonder if we shouldn't have the async_synchronize_full call in
__device_release_driver moved down and into driver_detach before we
even start the for loop. Assuming the driver is no longer associated
with the bus that should flush out all devices so that we can then
pull them out of the devices list at least. I may look at adding an
additional bitflag to the device struct to indicate that it has a
driver attach pending. Then for things like races between any attach
and detach calls the logic becomes pretty straight forward. Attach
will set the bit and provide driver data, detach will clear the bit
and the driver data. If a driver loads in between it should clear the
bit as well.

I'll work on it over the next couple days and hopefully have something
ready for testing/review early next week.

Thanks.

- Alex

  reply	other threads:[~2018-10-19  2:20 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-15 15:09 [driver-core PATCH v4 0/6] Add NUMA aware async_schedule calls Alexander Duyck
2018-10-15 15:09 ` [driver-core PATCH v4 1/6] workqueue: Provide queue_work_node to queue work near a given NUMA node Alexander Duyck
2018-10-15 15:09 ` [driver-core PATCH v4 2/6] async: Add support for queueing on specific " Alexander Duyck
2018-10-15 15:09 ` [driver-core PATCH v4 3/6] device core: Consolidate locking and unlocking of parent and device Alexander Duyck
2018-10-18  7:46   ` Rafael J. Wysocki
2018-10-18 17:53   ` Bart Van Assche
2018-10-15 15:09 ` [driver-core PATCH v4 4/6] driver core: Probe devices asynchronously instead of the driver Alexander Duyck
2018-10-18 18:11   ` Bart Van Assche
2018-10-18 19:38     ` Alexander Duyck
2018-10-18 20:13       ` Bart Van Assche
2018-10-19  2:20         ` Alexander Duyck [this message]
2018-10-19  2:31           ` Bart Van Assche
2018-10-19 22:35             ` Alexander Duyck
2018-10-15 15:09 ` [driver-core PATCH v4 5/6] driver core: Attach devices on CPU local to device node Alexander Duyck
2018-10-15 15:09 ` [driver-core PATCH v4 6/6] PM core: Use new async_schedule_dev command Alexander Duyck

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='CAKgT0Ucf-v3FNTA0z=3nPR-xS=0NYeqDjRfDW-KjDE5Ed-n1_g@mail.gmail.com' \
    --to=alexander.duyck@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=alexander.h.duyck@linux.intel.com \
    --cc=bvanassche@acm.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jiangshanlai@gmail.com \
    --cc=len.brown@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=rafael@kernel.org \
    --cc=tj@kernel.org \
    --cc=zwisler@kernel.org \
    --subject='Re: [driver-core PATCH v4 4/6] driver core: Probe devices asynchronously instead of the driver' \
    /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

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).