[PATCH v2 0/3] driver core: sync state fixups

[PATCH v2 0/3] driver core: sync state fixups

[Saravana Kannan]

Patch 1/3 fixes a bug where sync_state() might not be called when it
should be. Patches 2/3 and 3/3 are just minor fix ups that I'm grouping
together. Not much to say here.

-Saravana

v1->v2:
- Fix compilation issue in 3/3 (forgot to commit --amend in v1)

Saravana Kannan (3):
  driver core: Call sync_state() even if supplier has no consumers
  driver core: Add dev_has_sync_state()
  driver core: Skip unnecessary work when device doesn't have
    sync_state()

 drivers/base/core.c    | 27 ++++++++++++++++++++-------
 include/linux/device.h | 11 +++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 1/3] driver core: Call sync_state() even if supplier has no consumers

[Saravana Kannan]

The initial patch that added sync_state() support didn't handle the case
where a supplier has no consumers. This was because when a device is
successfully bound with a driver, only its suppliers were checked to see
if they are eligible to get a sync_state(). This is not sufficient for
devices that have no consumers but still need to do device state clean
up. So fix this.

Fixes: fc5a251d0fd7ca90 (driver core: Add sync_state driver/bus callback)
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 42a672456432..3306d5ae92a6 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -745,25 +745,31 @@ static void __device_links_queue_sync_state(struct device *dev,
 /**
  * device_links_flush_sync_list - Call sync_state() on a list of devices
  * @list: List of devices to call sync_state() on
+ * @dont_lock_dev: Device for which lock is already held by the caller
  *
  * Calls sync_state() on all the devices that have been queued for it. This
- * function is used in conjunction with __device_links_queue_sync_state().
+ * function is used in conjunction with __device_links_queue_sync_state(). The
+ * @dont_lock_dev parameter is useful when this function is called from a
+ * context where a device lock is already held.
  */
-static void device_links_flush_sync_list(struct list_head *list)
+static void device_links_flush_sync_list(struct list_head *list,
+					 struct device *dont_lock_dev)
 {
 	struct device *dev, *tmp;
 
 	list_for_each_entry_safe(dev, tmp, list, links.defer_sync) {
 		list_del_init(&dev->links.defer_sync);
 
-		device_lock(dev);
+		if (dev != dont_lock_dev)
+			device_lock(dev);
 
 		if (dev->bus->sync_state)
 			dev->bus->sync_state(dev);
 		else if (dev->driver && dev->driver->sync_state)
 			dev->driver->sync_state(dev);
 
-		device_unlock(dev);
+		if (dev != dont_lock_dev)
+			device_unlock(dev);
 
 		put_device(dev);
 	}
@@ -801,7 +807,7 @@ void device_links_supplier_sync_state_resume(void)
 out:
 	device_links_write_unlock();
 
-	device_links_flush_sync_list(&sync_list);
+	device_links_flush_sync_list(&sync_list, NULL);
 }
 
 static int sync_state_resume_initcall(void)
@@ -865,6 +871,11 @@ void device_links_driver_bound(struct device *dev)
 			driver_deferred_probe_add(link->consumer);
 	}
 
+	if (defer_sync_state_count)
+		__device_links_supplier_defer_sync(dev);
+	else
+		__device_links_queue_sync_state(dev, &sync_list);
+
 	list_for_each_entry(link, &dev->links.suppliers, c_node) {
 		if (!(link->flags & DL_FLAG_MANAGED))
 			continue;
@@ -883,7 +894,7 @@ void device_links_driver_bound(struct device *dev)
 
 	device_links_write_unlock();
 
-	device_links_flush_sync_list(&sync_list);
+	device_links_flush_sync_list(&sync_list, dev);
 }
 
 static void device_link_drop_managed(struct device_link *link)
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 2/3] driver core: Add dev_has_sync_state()

[Saravana Kannan]

Add an API to check if a device has sync_state support in its driver or
bus.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 include/linux/device.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 0cd7c647c16c..fa04dfd22bbc 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -798,6 +798,17 @@ static inline struct device_node *dev_of_node(struct device *dev)
 	return dev->of_node;
 }
 
+static inline bool dev_has_sync_state(struct device *dev)
+{
+	if (!dev)
+		return false;
+	if (dev->driver && dev->driver->sync_state)
+		return true;
+	if (dev->bus && dev->bus->sync_state)
+		return true;
+	return false;
+}
+
 /*
  * High level routines for use by the bus drivers
  */
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 3/3] driver core: Skip unnecessary work when device doesn't have sync_state()

[Saravana Kannan]

A bunch of busy work is done for devices that don't have sync_state()
support. Stop doing the busy work.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3306d5ae92a6..dbb0f9130f42 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -718,6 +718,8 @@ static void __device_links_queue_sync_state(struct device *dev,
 {
 	struct device_link *link;
 
+	if (!dev_has_sync_state(dev))
+		return;
 	if (dev->state_synced)
 		return;
 
@@ -819,7 +821,7 @@ late_initcall(sync_state_resume_initcall);
 
 static void __device_links_supplier_defer_sync(struct device *sup)
 {
-	if (list_empty(&sup->links.defer_sync))
+	if (list_empty(&sup->links.defer_sync) && dev_has_sync_state(sup))
 		list_add_tail(&sup->links.defer_sync, &deferred_sync);
 }
 
-- 
2.25.0.265.gbab2e86ba0-goog

Re: [PATCH v2 1/3] driver core: Call sync_state() even if supplier has no consumers

[Greg Kroah-Hartman]

On Fri, Feb 21, 2020 at 12:05:08AM -0800, Saravana Kannan wrote:
> The initial patch that added sync_state() support didn't handle the case
> where a supplier has no consumers. This was because when a device is
> successfully bound with a driver, only its suppliers were checked to see
> if they are eligible to get a sync_state(). This is not sufficient for
> devices that have no consumers but still need to do device state clean
> up. So fix this.
> 
> Fixes: fc5a251d0fd7ca90 (driver core: Add sync_state driver/bus callback)

Should be:
Fixes: fc5a251d0fd7 ("driver core: Add sync_state driver/bus callback")

> Signed-off-by: Saravana Kannan <saravanak@google.com>

So this needs to go to 5.5 also, right?

thanks,

greg k-h

Re: [PATCH v2 1/3] driver core: Call sync_state() even if supplier has no consumers

[Saravana Kannan]

On Fri, Feb 21, 2020 at 1:25 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Feb 21, 2020 at 12:05:08AM -0800, Saravana Kannan wrote:
> > The initial patch that added sync_state() support didn't handle the case
> > where a supplier has no consumers. This was because when a device is
> > successfully bound with a driver, only its suppliers were checked to see
> > if they are eligible to get a sync_state(). This is not sufficient for
> > devices that have no consumers but still need to do device state clean
> > up. So fix this.
> >
> > Fixes: fc5a251d0fd7ca90 (driver core: Add sync_state driver/bus callback)
>
> Should be:
> Fixes: fc5a251d0fd7 ("driver core: Add sync_state driver/bus callback")

Sorry, late night sleepy patches are never good!
Btw I thought the sha should be only 12 characters but then saw
another instance where you used 16 chars. What's the right one?

>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> So this needs to go to 5.5 also, right?

Did you mean 5.4? Yes.

-Saravana

Re: [PATCH v2 1/3] driver core: Call sync_state() even if supplier has no consumers

[Greg Kroah-Hartman]

On Fri, Feb 21, 2020 at 01:48:49AM -0800, Saravana Kannan wrote:
> On Fri, Feb 21, 2020 at 1:25 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Feb 21, 2020 at 12:05:08AM -0800, Saravana Kannan wrote:
> > > The initial patch that added sync_state() support didn't handle the case
> > > where a supplier has no consumers. This was because when a device is
> > > successfully bound with a driver, only its suppliers were checked to see
> > > if they are eligible to get a sync_state(). This is not sufficient for
> > > devices that have no consumers but still need to do device state clean
> > > up. So fix this.
> > >
> > > Fixes: fc5a251d0fd7ca90 (driver core: Add sync_state driver/bus callback)
> >
> > Should be:
> > Fixes: fc5a251d0fd7 ("driver core: Add sync_state driver/bus callback")
> 
> Sorry, late night sleepy patches are never good!
> Btw I thought the sha should be only 12 characters but then saw
> another instance where you used 16 chars. What's the right one?

I used 16 chars?  12 should be all that is needed.

> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> >
> > So this needs to go to 5.5 also, right?
> 
> Did you mean 5.4? Yes.

fc5a251d0fd7 is only in 5.5, not 5.4 from what I can see, right?

thanks,

greg k-h

Re: [PATCH v2 1/3] driver core: Call sync_state() even if supplier has no consumers

[Saravana Kannan]

On Fri, Feb 21, 2020 at 2:32 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Feb 21, 2020 at 01:48:49AM -0800, Saravana Kannan wrote:
> > On Fri, Feb 21, 2020 at 1:25 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Feb 21, 2020 at 12:05:08AM -0800, Saravana Kannan wrote:
> > > > The initial patch that added sync_state() support didn't handle the case
> > > > where a supplier has no consumers. This was because when a device is
> > > > successfully bound with a driver, only its suppliers were checked to see
> > > > if they are eligible to get a sync_state(). This is not sufficient for
> > > > devices that have no consumers but still need to do device state clean
> > > > up. So fix this.
> > > >
> > > > Fixes: fc5a251d0fd7ca90 (driver core: Add sync_state driver/bus callback)
> > >
> > > Should be:
> > > Fixes: fc5a251d0fd7 ("driver core: Add sync_state driver/bus callback")
> >
> > Sorry, late night sleepy patches are never good!
> > Btw I thought the sha should be only 12 characters but then saw
> > another instance where you used 16 chars. What's the right one?
>
> I used 16 chars?  12 should be all that is needed.
>
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > >
> > > So this needs to go to 5.5 also, right?
> >
> > Did you mean 5.4? Yes.
>
> fc5a251d0fd7 is only in 5.5, not 5.4 from what I can see, right?

Ah, right!


-Saravana

Re: [PATCH v2 0/3] driver core: sync state fixups

[Greg Kroah-Hartman]

On Fri, Feb 21, 2020 at 12:05:07AM -0800, Saravana Kannan wrote:
> Patch 1/3 fixes a bug where sync_state() might not be called when it
> should be. Patches 2/3 and 3/3 are just minor fix ups that I'm grouping
> together. Not much to say here.
> 
> -Saravana
> 
> v1->v2:
> - Fix compilation issue in 3/3 (forgot to commit --amend in v1)
> 
> Saravana Kannan (3):
>   driver core: Call sync_state() even if supplier has no consumers
>   driver core: Add dev_has_sync_state()
>   driver core: Skip unnecessary work when device doesn't have
>     sync_state()
> 
>  drivers/base/core.c    | 27 ++++++++++++++++++++-------
>  include/linux/device.h | 11 +++++++++++
>  2 files changed, 31 insertions(+), 7 deletions(-)
> 
> -- 
> 2.25.0.265.gbab2e86ba0-goog
> 


All now queued up, sorry for the delay.

greg k-h

Re: [PATCH v2 3/3] driver core: Skip unnecessary work when device doesn't have sync_state()

[Davide Caratti]

On Fri, 2020-02-21 at 00:05 -0800, Saravana Kannan wrote:
> A bunch of busy work is done for devices that don't have sync_state()
> support. Stop doing the busy work.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

hello Greg,

this patch and patch 2/3 of the same series proved to fix systematic
crashes (NULL pointer dereference in device_links_flush_sync_list() while
loading mac80211_hwsim.ko, see [1]) on Fedora 31, that are triggered by
NetworkManager-ci [2]. May I ask to queue these two patches for the next
5.5 stable?

thank you in advance (and thanks to Vladimir for reporting)!

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1816765
[2] https://github.com/NetworkManager/NetworkManager-ci
 
-- 
davide  

Re: [PATCH v2 3/3] driver core: Skip unnecessary work when device doesn't have sync_state()

[Saravana Kannan]

On Tue, Mar 24, 2020 at 1:03 PM Davide Caratti <dcaratti@redhat.com> wrote:
>
> On Fri, 2020-02-21 at 00:05 -0800, Saravana Kannan wrote:
> > A bunch of busy work is done for devices that don't have sync_state()
> > support. Stop doing the busy work.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
>
> hello Greg,
>
> this patch and patch 2/3 of the same series proved to fix systematic
> crashes (NULL pointer dereference in device_links_flush_sync_list() while
> loading mac80211_hwsim.ko, see [1]) on Fedora 31, that are triggered by
> NetworkManager-ci [2]. May I ask to queue these two patches for the next
> 5.5 stable?
>
> thank you in advance (and thanks to Vladimir for reporting)!

This was reported internally on some backports I did of 1/3. But I
forgot 1/3 was cherry-picked onto stable releases too. Otherwise, I'd
have reported this upstream too.

Thanks for reporting this and sorry about the breakage. 2/3 and 3/3
weren't meant to be fixes for 1/3, but they just happened to fix 1/3.
Problems of testing a series I suppose.

Thanks,
Saravana

Re: [PATCH v2 3/3] driver core: Skip unnecessary work when device doesn't have sync_state()

[Greg Kroah-Hartman]

On Tue, Mar 24, 2020 at 09:03:28PM +0100, Davide Caratti wrote:
> On Fri, 2020-02-21 at 00:05 -0800, Saravana Kannan wrote:
> > A bunch of busy work is done for devices that don't have sync_state()
> > support. Stop doing the busy work.
> > 
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> 
> hello Greg,
> 
> this patch and patch 2/3 of the same series proved to fix systematic
> crashes (NULL pointer dereference in device_links_flush_sync_list() while
> loading mac80211_hwsim.ko, see [1]) on Fedora 31, that are triggered by
> NetworkManager-ci [2]. May I ask to queue these two patches for the next
> 5.5 stable?

What are the git commit ids of these patches in Linus's tree that you
want backported?

thanks,

greg k-h

Re: [PATCH v2 3/3] driver core: Skip unnecessary work when device doesn't have sync_state()

[Davide Caratti]

On Wed, 2020-03-25 at 08:44 +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 24, 2020 at 09:03:28PM +0100, Davide Caratti wrote:
> > On Fri, 2020-02-21 at 00:05 -0800, Saravana Kannan wrote:
> > > A bunch of busy work is done for devices that don't have sync_state()
> > > support. Stop doing the busy work.
> > > 
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/base/core.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > 
> > hello Greg,
> > 
> > this patch and patch 2/3 of the same series proved to fix systematic
> > crashes (NULL pointer dereference in device_links_flush_sync_list() while
> > loading mac80211_hwsim.ko, see [1]) on Fedora 31, that are triggered by
> > NetworkManager-ci [2]. May I ask to queue these two patches for the next
> > 5.5 stable?
> 
> What are the git commit ids of these patches in Linus's tree that you
> want backported?

right, I should have mentioned them also here: 

ac338acf514e "(driver core: Add dev_has_sync_state())" <-- patch 2/3 
77036165d8bc "(driver core: Skip unnecessary work when device doesn't have sync_state())" <-- patch 3/3

like Saravana mentioned, the problem is probably introduced by patch
1/3 of the series, 

77036165d8bc "(driver core: Skip unnecessary work when device doesn't have sync_state())"

that's already in stable 5.5.

thanks!
-- 
davide

Re: [PATCH v2 3/3] driver core: Skip unnecessary work when device doesn't have sync_state()

[Greg Kroah-Hartman]

On Wed, Mar 25, 2020 at 10:11:19AM +0100, Davide Caratti wrote:
> On Wed, 2020-03-25 at 08:44 +0100, Greg Kroah-Hartman wrote:
> > On Tue, Mar 24, 2020 at 09:03:28PM +0100, Davide Caratti wrote:
> > > On Fri, 2020-02-21 at 00:05 -0800, Saravana Kannan wrote:
> > > > A bunch of busy work is done for devices that don't have sync_state()
> > > > support. Stop doing the busy work.
> > > > 
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/base/core.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > 
> > > hello Greg,
> > > 
> > > this patch and patch 2/3 of the same series proved to fix systematic
> > > crashes (NULL pointer dereference in device_links_flush_sync_list() while
> > > loading mac80211_hwsim.ko, see [1]) on Fedora 31, that are triggered by
> > > NetworkManager-ci [2]. May I ask to queue these two patches for the next
> > > 5.5 stable?
> > 
> > What are the git commit ids of these patches in Linus's tree that you
> > want backported?
> 
> right, I should have mentioned them also here: 
> 
> ac338acf514e "(driver core: Add dev_has_sync_state())" <-- patch 2/3 
> 77036165d8bc "(driver core: Skip unnecessary work when device doesn't have sync_state())" <-- patch 3/3
> 
> like Saravana mentioned, the problem is probably introduced by patch
> 1/3 of the series, 
> 
> 77036165d8bc "(driver core: Skip unnecessary work when device doesn't have sync_state())"
> 
> that's already in stable 5.5.

Now queued up, thanks!

greg k-h