All of lore.kernel.org
 help / color / mirror / Atom feed
From: Saravana Kannan <saravanak@google.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: Mark Brown <broonie@kernel.org>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Android Kernel Team <kernel-team@android.com>,
	linux-spi <linux-spi@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] spi: Fix spi device unregister flow
Date: Mon, 3 May 2021 11:15:50 -0700	[thread overview]
Message-ID: <CAGETcx_Z9kjzjTaNUBgKNte8UZcT5HJ5tjkmRmFqddy78Nj9Qw@mail.gmail.com> (raw)
In-Reply-To: <20210503175600.GA3864@wunner.de>

On Mon, May 3, 2021 at 10:56 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Mon, May 03, 2021 at 10:21:59AM -0700, Saravana Kannan wrote:
> > On Mon, May 3, 2021 at 3:07 AM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Mon, Apr 26, 2021 at 04:56:38PM -0700, Saravana Kannan wrote:
> > > > When an SPI device is unregistered, the spi->controller->cleanup() is
> > > > called in the device's release callback. That's wrong for a couple of
> > > > reasons:
> > > >
> > > > 1. spi_dev_put() can be called before spi_add_device() is called. And
> > > >    it's spi_add_device() that calls spi_setup(). This will cause clean()
> > > >    to get called without the spi device ever being setup.
> > >
> > > Well, yes, but it's not a big problem in practice so far:
> > >
> > > I've checked all drivers and there are only four which are affected
> > > by this: spi-mpc512x-psc.c spi-pic32.c spi-s3c64xx.c spi-st-ssc4.c
> > >
> > > They all fiddle with the chipselect GPIO in their ->cleanup hook
> > > and the GPIO may not have been requested yet because that happens
> > > during ->setup.
> > >
> > > All the other drivers merely invoke kzalloc() on ->setup and kfree()
> > > on ->cleanup.  The order doesn't matter in this case because
> > > kfree(NULL) is a no-op.
> >
> > That's making a lot of assumptions about drivers not doing certain
> > things in the future or making assumptions about the hardware (chip
> > select or whatever other configuration that might happen). Totally
> > unnecessary and error prone.
>
> I agree, I'm just not happy with the solution presented.
>
> This could be solved by setting a flag in struct spi_device
> once ->setup has returned successfully.
>
>
> > > > 2. There's no guarantee that the controller's driver would be present by
> > > >    the time the spi device's release function gets called.
> > >
> > > How so?  spi_devices are instantiated on ->probe of the controller
> > > via spi_register_controller() and destroyed on ->remove via
> > > spi_unregister_controller().  I don't see how the controller driver
> > > could ever be unavailable, so this point seems moot.
> >
> > Just because put_device() is called on a struct device doesn't mean
> > it's getting destroyed immediately. The refcount needs to reach zero
> > for ->cleanup() to be called eventually. And there's no guarantee that
> > by the time the ref count hits zero that your controller driver is
> > still around. So, it's not a moot point.
>
> In theory, yes, but concretely, how is that going to happen?
>
> We remove all the things that might be holding a ref on the spi_device
> (such as sysfs entries, child devices), so when device_unregister()
> is called from spi_unregister_device(), the expectation is really that
> that's the last reference being dropped.
>
> In theory it would be possible for some other driver to hold a ref,
> but I don't see why it would be doing that.
>
> Perhaps spidev.c makes it possible to keep an spi_device around even
> though the controller has been removed, simply by keeping the device
> file open from user space.  I'm not sure if that's the case but it's
> probably something worth checking.

We can't rule out all the cases and assume refcount would hit zero
when the framework does put_device() on the spi_device. So I don't
think there's even a point in trying to find if this can happen. But
since you asked, creating device links to this device is just one
example of how this could happen.

>
> > > > Fix these issues by simply moving the cleanup from the device release
> > > > callback to the actual spi_unregister_device() function.
> > >
> > > Unfortunately the fix is wrong, it introduces a new problem:
> > >
> > > > @@ -713,6 +717,8 @@ void spi_unregister_device(struct spi_device *spi)
> > > >       if (!spi)
> > > >               return;
> > > >
> > > > +     spi_cleanup(spi);
> > > > +
> > > >       if (spi->dev.of_node) {
> > > >               of_node_clear_flag(spi->dev.of_node, OF_POPULATED);
> > > >               of_node_put(spi->dev.of_node);
> > >
> > > Now you're running ->cleanup before the SPI slave's driver is unbound.
> >
> > By "slave" device, you mean struct spi_device, right?
>
> Yes.
>
>
> > Sorry if I'm mistaken about my understanding of the SPI framework.
> > Please explain how that's happening here. The main place
> > spi_unregister_device() is getting called from is
> > spi_controller_unregister(). If the controller's child/slave
> > spi_device's aren't unbound by then, you've got bigger problems even
> > without my patch?
>
> Without your patch:
>
> spi_unregister_device()
>   device_unregister()
>     device_del()
>       bus_remove_device()
>         device_release_driver() # access to physical SPI device in ->remove()
>     put_device()
>       kobject_put()
>         kref_put()
>           kobject_release()
>             kobject_cleanup()
>               device_release()
>                 spidev_release()
>                   spi->controller->cleanup() # controller_state freed
>
> With your patch:
>
> spi_unregister_device()
>   spi_cleanup()
>     spi->controller->cleanup() # controller_state freed
>   device_unregister()
>     device_del()
>       bus_remove_device()
>         device_release_driver() # access to physical SPI device in ->remove()
>
> As a case in point, an SPI Ethernet driver I'm familiar with,
> drivers/net/ethernet/micrel/ks8851_common.c, performs various
> register accesses on driver unbind in ks8851_net_stop().
> So on driver unbind, the SPI device still needs to be accessible.
>
> However the controller_state may be necessary to access the device,
> so freeing that before unbind is a no-go.
>
> Let me know if this explanation wasn't sufficient.

Ah, make sense. My bad. I saw the of_node_put() in
spi_unregister_device() and glossed over the rest of the code because
I assumed the of_node_put() wouldn't have been done before the device
was released.

So, it looks like the fix is simple. We just need to move
spi_cleanup() to the bottom of spi_unregister_device(). I'll send a
patch for that rather than reverting this and bringing back the other
bugs.

-Saravana

  reply	other threads:[~2021-05-03 18:16 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-26 23:56 [PATCH] spi: Fix spi device unregister flow Saravana Kannan
2021-04-27  6:52 ` Andy Shevchenko
2021-04-27 10:48   ` Mark Brown
2021-04-27 11:42     ` Andy Shevchenko
2021-04-27 11:49       ` Greg Kroah-Hartman
2021-04-27 15:02         ` Saravana Kannan
2021-04-28 16:53 ` Mark Brown
2021-05-03 10:07 ` Lukas Wunner
2021-05-03 10:16   ` Andy Shevchenko
2021-05-03 17:21   ` Saravana Kannan
2021-05-03 17:56     ` Lukas Wunner
2021-05-03 18:15       ` Saravana Kannan [this message]
2021-05-04  9:17         ` Lukas Wunner

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=CAGETcx_Z9kjzjTaNUBgKNte8UZcT5HJ5tjkmRmFqddy78Nj9Qw@mail.gmail.com \
    --to=saravanak@google.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=broonie@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=lukas@wunner.de \
    --cc=m.szyprowski@samsung.com \
    --cc=rafael@kernel.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.