linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices
@ 2021-07-07 17:29 Adrian Hunter
  2021-07-07 17:29 ` [PATCH RFC 1/2] " Adrian Hunter
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Adrian Hunter @ 2021-07-07 17:29 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: Greg Kroah-Hartman, Saravana Kannan, Martin K . Petersen,
	James E . J . Bottomley, linux-scsi, Avri Altman, Bean Huo,
	Can Guo, Asutosh Das, Bart Van Assche, linux-pm, linux-kernel

Hi

There is an issue with the SCSI UFS driver when the optional
BOOT well-known LUN fails to probe, which is not a fatal error.
The issue is that the device and its "managed" device link do not
then get deleted.  The device because the device link has a
reference to it.  The device link because it can only be deleted
by device_del(), but device_add() was never called, so device_del()
never will be either.

It is not clear if there is a way to get rid of the device link in
this case, so here is a patch to add a function that does just that.

There is also a patch to use the new function to fix the issue with
the SCSI UFS driver.


Adrian Hunter (2):
      driver core: Add ability to delete device links of unregistered devices
      scsi: ufshcd: Fix device links when BOOT WLUN fails to probe

 Documentation/driver-api/device_link.rst |  7 +++++--
 drivers/base/core.c                      | 22 +++++++++++++++++++---
 drivers/scsi/ufs/ufshcd.c                |  7 +++++++
 include/linux/device.h                   |  1 +
 4 files changed, 32 insertions(+), 5 deletions(-)


Regards
Adrian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH RFC 1/2] driver core: Add ability to delete device links of unregistered devices
  2021-07-07 17:29 [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Adrian Hunter
@ 2021-07-07 17:29 ` Adrian Hunter
  2021-07-07 17:38   ` Greg Kroah-Hartman
  2021-07-07 17:29 ` [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe Adrian Hunter
  2021-07-07 17:40 ` [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Greg Kroah-Hartman
  2 siblings, 1 reply; 17+ messages in thread
From: Adrian Hunter @ 2021-07-07 17:29 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: Greg Kroah-Hartman, Saravana Kannan, Martin K . Petersen,
	James E . J . Bottomley, linux-scsi, Avri Altman, Bean Huo,
	Can Guo, Asutosh Das, Bart Van Assche, linux-pm, linux-kernel

Managed device links are deleted by device_del(). However it is possible to
add a device link to a consumer before device_add(), and then discover an
error prevents the device from being used. In that case normally references
to the device would be dropped and the device would be deleted. However the
device link holds a reference to the device, so the device link and device
remain indefinitely.

Add a function to delete device links of an unregistered device, so that
drivers can get rid of unregistered devices and their device links.

To do that, the devlink_remove_symlinks() function must be amended to cope
with the absence of the consumer's sysfs presence, otherwise
sysfs_remove_link() will generate warnings.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 Documentation/driver-api/device_link.rst |  7 +++++--
 drivers/base/core.c                      | 22 +++++++++++++++++++---
 include/linux/device.h                   |  1 +
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
index ee913ae16371..12c0602822c8 100644
--- a/Documentation/driver-api/device_link.rst
+++ b/Documentation/driver-api/device_link.rst
@@ -75,7 +75,9 @@ driver is compiled as a module, the device link is added on module load and
 orderly deleted on unload.  The same restrictions that apply to device link
 addition (e.g. exclusion of a parallel suspend/resume transition) apply equally
 to deletion.  Device links managed by the driver core are deleted automatically
-by it.
+by it, except if the consumer device is never registered (e.g. because of an
+error), in which case the device links must be explicitly removed using
+:c:func:`device_links_scrap()`.
 
 Several flags may be specified on device link addition, two of which
 have already been mentioned above:  ``DL_FLAG_STATELESS`` to express that no
@@ -317,4 +319,5 @@ State machine
 API
 ===
 
-See device_link_add(), device_link_del() and device_link_remove().
+See device_link_add(), device_link_del(), device_link_remove() and
+device_links_scrap().
diff --git a/drivers/base/core.c b/drivers/base/core.c
index ea5b85354526..2ed32ab50b84 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -562,7 +562,8 @@ static void devlink_remove_symlinks(struct device *dev,
 	struct device *con = link->consumer;
 	char *buf;
 
-	sysfs_remove_link(&link->link_dev.kobj, "consumer");
+	if (device_is_registered(con))
+		sysfs_remove_link(&link->link_dev.kobj, "consumer");
 	sysfs_remove_link(&link->link_dev.kobj, "supplier");
 
 	len = max(strlen(dev_bus_name(sup)) + strlen(dev_name(sup)),
@@ -575,8 +576,10 @@ static void devlink_remove_symlinks(struct device *dev,
 		return;
 	}
 
-	snprintf(buf, len, "supplier:%s:%s", dev_bus_name(sup), dev_name(sup));
-	sysfs_remove_link(&con->kobj, buf);
+	if (device_is_registered(con)) {
+		snprintf(buf, len, "supplier:%s:%s", dev_bus_name(sup), dev_name(sup));
+		sysfs_remove_link(&con->kobj, buf);
+	}
 	snprintf(buf, len, "consumer:%s:%s", dev_bus_name(con), dev_name(con));
 	sysfs_remove_link(&sup->kobj, buf);
 	kfree(buf);
@@ -1538,6 +1541,19 @@ static void device_links_purge(struct device *dev)
 	device_links_write_unlock();
 }
 
+/**
+ * device_links_scrap - Device was never registered, delete any device links.
+ * @dev: Target device.
+ */
+void device_links_scrap(struct device *dev)
+{
+	if (WARN_ON(device_is_registered(dev)))
+		return;
+
+	device_links_purge(dev);
+}
+EXPORT_SYMBOL_GPL(device_links_scrap);
+
 #define FW_DEVLINK_FLAGS_PERMISSIVE	(DL_FLAG_INFERRED | \
 					 DL_FLAG_SYNC_STATE_ONLY)
 #define FW_DEVLINK_FLAGS_ON		(DL_FLAG_INFERRED | \
diff --git a/include/linux/device.h b/include/linux/device.h
index 2a22875238a6..90574f9aa8f2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -967,6 +967,7 @@ struct device_link *device_link_add(struct device *consumer,
 				    struct device *supplier, u32 flags);
 void device_link_del(struct device_link *link);
 void device_link_remove(void *consumer, struct device *supplier);
+void device_links_scrap(struct device *dev);
 void device_links_supplier_sync_state_pause(void);
 void device_links_supplier_sync_state_resume(void);
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-07 17:29 [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Adrian Hunter
  2021-07-07 17:29 ` [PATCH RFC 1/2] " Adrian Hunter
@ 2021-07-07 17:29 ` Adrian Hunter
  2021-07-07 17:39   ` Greg Kroah-Hartman
  2021-07-07 17:40 ` [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Greg Kroah-Hartman
  2 siblings, 1 reply; 17+ messages in thread
From: Adrian Hunter @ 2021-07-07 17:29 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: Greg Kroah-Hartman, Saravana Kannan, Martin K . Petersen,
	James E . J . Bottomley, linux-scsi, Avri Altman, Bean Huo,
	Can Guo, Asutosh Das, Bart Van Assche, linux-pm, linux-kernel

If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
been registered but can still have a device link holding a reference to the
device. The unwanted device link will prevent runtime suspend indefinitely,
and cause some warnings if the supplier is ever deleted (e.g. by unbinding
the UFS host controller). Fix by explicitly deleting the device link when
SCSI destroys the SCSI device.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufshcd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 708b3b62fc4d..483aa74fe2c8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
 		spin_lock_irqsave(hba->host->host_lock, flags);
 		hba->sdev_ufs_device = NULL;
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
+	} else {
+		/*
+		 * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
+		 * will not have been registered but can still have a device
+		 * link holding a reference to the device.
+		 */
+		device_links_scrap(&sdev->sdev_gendev);
 	}
 }
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 1/2] driver core: Add ability to delete device links of unregistered devices
  2021-07-07 17:29 ` [PATCH RFC 1/2] " Adrian Hunter
@ 2021-07-07 17:38   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2021-07-07 17:38 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Rafael J . Wysocki, Saravana Kannan, Martin K . Petersen,
	James E . J . Bottomley, linux-scsi, Avri Altman, Bean Huo,
	Can Guo, Asutosh Das, Bart Van Assche, linux-pm, linux-kernel

On Wed, Jul 07, 2021 at 08:29:47PM +0300, Adrian Hunter wrote:
> +void device_links_scrap(struct device *dev)
> +{
> +	if (WARN_ON(device_is_registered(dev)))

You just rebooted a box if this was hit, never add new WARN_ON() please.

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-07 17:29 ` [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe Adrian Hunter
@ 2021-07-07 17:39   ` Greg Kroah-Hartman
  2021-07-07 17:49     ` Adrian Hunter
  0 siblings, 1 reply; 17+ messages in thread
From: Greg Kroah-Hartman @ 2021-07-07 17:39 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Rafael J . Wysocki, Saravana Kannan, Martin K . Petersen,
	James E . J . Bottomley, linux-scsi, Avri Altman, Bean Huo,
	Can Guo, Asutosh Das, Bart Van Assche, linux-pm, linux-kernel

On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> been registered but can still have a device link holding a reference to the
> device. The unwanted device link will prevent runtime suspend indefinitely,
> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> the UFS host controller). Fix by explicitly deleting the device link when
> SCSI destroys the SCSI device.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 708b3b62fc4d..483aa74fe2c8 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>  		spin_lock_irqsave(hba->host->host_lock, flags);
>  		hba->sdev_ufs_device = NULL;
>  		spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	} else {
> +		/*
> +		 * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> +		 * will not have been registered but can still have a device
> +		 * link holding a reference to the device.
> +		 */
> +		device_links_scrap(&sdev->sdev_gendev);

What created that link?  And why did it do that before probe happened
successfully?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices
  2021-07-07 17:29 [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Adrian Hunter
  2021-07-07 17:29 ` [PATCH RFC 1/2] " Adrian Hunter
  2021-07-07 17:29 ` [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe Adrian Hunter
@ 2021-07-07 17:40 ` Greg Kroah-Hartman
  2021-07-07 17:41   ` Saravana Kannan
  2 siblings, 1 reply; 17+ messages in thread
From: Greg Kroah-Hartman @ 2021-07-07 17:40 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Rafael J . Wysocki, Saravana Kannan, Martin K . Petersen,
	James E . J . Bottomley, linux-scsi, Avri Altman, Bean Huo,
	Can Guo, Asutosh Das, Bart Van Assche, linux-pm, linux-kernel

On Wed, Jul 07, 2021 at 08:29:46PM +0300, Adrian Hunter wrote:
> Hi
> 
> There is an issue with the SCSI UFS driver when the optional
> BOOT well-known LUN fails to probe, which is not a fatal error.
> The issue is that the device and its "managed" device link do not
> then get deleted.  The device because the device link has a
> reference to it.  The device link because it can only be deleted
> by device_del(), but device_add() was never called, so device_del()
> never will be either.

How was a link created for something that never had device_add() called
on it?  Who is doing that?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices
  2021-07-07 17:40 ` [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Greg Kroah-Hartman
@ 2021-07-07 17:41   ` Saravana Kannan
  0 siblings, 0 replies; 17+ messages in thread
From: Saravana Kannan @ 2021-07-07 17:41 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Adrian Hunter, Rafael J . Wysocki, Martin K . Petersen,
	James E . J . Bottomley, linux-scsi, Avri Altman, Bean Huo,
	Can Guo, Asutosh Das, Bart Van Assche, linux-pm, linux-kernel

On Wed, Jul 7, 2021 at 10:40 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jul 07, 2021 at 08:29:46PM +0300, Adrian Hunter wrote:
> > Hi
> >
> > There is an issue with the SCSI UFS driver when the optional
> > BOOT well-known LUN fails to probe, which is not a fatal error.
> > The issue is that the device and its "managed" device link do not
> > then get deleted.  The device because the device link has a
> > reference to it.  The device link because it can only be deleted
> > by device_del(), but device_add() was never called, so device_del()
> > never will be either.
>
> How was a link created for something that never had device_add() called
> on it?  Who is doing that?
>

Greg responded as I was typing :)

Nak for the series. I don't like mixing and matching the control of
managed device links like this.

Can I get more details please. How is the device link added in the first place?

-Saravana

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-07 17:39   ` Greg Kroah-Hartman
@ 2021-07-07 17:49     ` Adrian Hunter
  2021-07-08 12:31       ` Rafael J. Wysocki
  0 siblings, 1 reply; 17+ messages in thread
From: Adrian Hunter @ 2021-07-07 17:49 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J . Wysocki, Saravana Kannan, Martin K . Petersen,
	James E . J . Bottomley, linux-scsi, Avri Altman, Bean Huo,
	Can Guo, Asutosh Das, Bart Van Assche, linux-pm, linux-kernel

On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
>> been registered but can still have a device link holding a reference to the
>> device. The unwanted device link will prevent runtime suspend indefinitely,
>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
>> the UFS host controller). Fix by explicitly deleting the device link when
>> SCSI destroys the SCSI device.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 708b3b62fc4d..483aa74fe2c8 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>>  		spin_lock_irqsave(hba->host->host_lock, flags);
>>  		hba->sdev_ufs_device = NULL;
>>  		spin_unlock_irqrestore(hba->host->host_lock, flags);
>> +	} else {
>> +		/*
>> +		 * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
>> +		 * will not have been registered but can still have a device
>> +		 * link holding a reference to the device.
>> +		 */
>> +		device_links_scrap(&sdev->sdev_gendev);
> 
> What created that link?  And why did it do that before probe happened
> successfully?

The same driver created the link.

The documentation seems to say it is allowed to, if it is the consumer.
From Documentation/driver-api/device_link.rst

  Usage
  =====

  The earliest point in time when device links can be added is after
  :c:func:`device_add()` has been called for the supplier and
  :c:func:`device_initialize()` has been called for the consumer.





^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-07 17:49     ` Adrian Hunter
@ 2021-07-08 12:31       ` Rafael J. Wysocki
  2021-07-08 14:17         ` Adrian Hunter
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2021-07-08 12:31 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Saravana Kannan,
	Martin K . Petersen, James E . J . Bottomley,
	open list:TARGET SUBSYSTEM, Avri Altman, Bean Huo, Can Guo,
	Asutosh Das, Bart Van Assche, Linux PM,
	Linux Kernel Mailing List

On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> >> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> >> been registered but can still have a device link holding a reference to the
> >> device. The unwanted device link will prevent runtime suspend indefinitely,
> >> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> >> the UFS host controller). Fix by explicitly deleting the device link when
> >> SCSI destroys the SCSI device.
> >>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >> ---
> >>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >> index 708b3b62fc4d..483aa74fe2c8 100644
> >> --- a/drivers/scsi/ufs/ufshcd.c
> >> +++ b/drivers/scsi/ufs/ufshcd.c
> >> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> >>              spin_lock_irqsave(hba->host->host_lock, flags);
> >>              hba->sdev_ufs_device = NULL;
> >>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> >> +    } else {
> >> +            /*
> >> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> >> +             * will not have been registered but can still have a device
> >> +             * link holding a reference to the device.
> >> +             */
> >> +            device_links_scrap(&sdev->sdev_gendev);
> >
> > What created that link?  And why did it do that before probe happened
> > successfully?
>
> The same driver created the link.
>
> The documentation seems to say it is allowed to, if it is the consumer.
> From Documentation/driver-api/device_link.rst
>
>   Usage
>   =====
>
>   The earliest point in time when device links can be added is after
>   :c:func:`device_add()` has been called for the supplier and
>   :c:func:`device_initialize()` has been called for the consumer.

Yes, this is allowed, but if you've added device links to a device
object that is not going to be registered after all, you are
responsible for doing the cleanup.

Why can't you call device_link_del() directly on those links?

Or device_link_remove() if you don't want to deal with link pointers?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-08 12:31       ` Rafael J. Wysocki
@ 2021-07-08 14:17         ` Adrian Hunter
  2021-07-08 15:03           ` Rafael J. Wysocki
  2021-07-08 16:45           ` Saravana Kannan
  0 siblings, 2 replies; 17+ messages in thread
From: Adrian Hunter @ 2021-07-08 14:17 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Greg Kroah-Hartman, Saravana Kannan, Martin K . Petersen,
	James E . J . Bottomley, open list:TARGET SUBSYSTEM, Avri Altman,
	Bean Huo, Can Guo, Asutosh Das, Bart Van Assche, Linux PM,
	Linux Kernel Mailing List

On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
>>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
>>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
>>>> been registered but can still have a device link holding a reference to the
>>>> device. The unwanted device link will prevent runtime suspend indefinitely,
>>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
>>>> the UFS host controller). Fix by explicitly deleting the device link when
>>>> SCSI destroys the SCSI device.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>> index 708b3b62fc4d..483aa74fe2c8 100644
>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>>>>              spin_lock_irqsave(hba->host->host_lock, flags);
>>>>              hba->sdev_ufs_device = NULL;
>>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>> +    } else {
>>>> +            /*
>>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
>>>> +             * will not have been registered but can still have a device
>>>> +             * link holding a reference to the device.
>>>> +             */
>>>> +            device_links_scrap(&sdev->sdev_gendev);
>>>
>>> What created that link?  And why did it do that before probe happened
>>> successfully?
>>
>> The same driver created the link.
>>
>> The documentation seems to say it is allowed to, if it is the consumer.
>> From Documentation/driver-api/device_link.rst
>>
>>   Usage
>>   =====
>>
>>   The earliest point in time when device links can be added is after
>>   :c:func:`device_add()` has been called for the supplier and
>>   :c:func:`device_initialize()` has been called for the consumer.
> 
> Yes, this is allowed, but if you've added device links to a device
> object that is not going to be registered after all, you are
> responsible for doing the cleanup.
> 
> Why can't you call device_link_del() directly on those links?
> 
> Or device_link_remove() if you don't want to deal with link pointers?
> 

Those only work for DL_FLAG_STATELESS device links, but we use only
DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-08 14:17         ` Adrian Hunter
@ 2021-07-08 15:03           ` Rafael J. Wysocki
  2021-07-08 15:12             ` Rafael J. Wysocki
  2021-07-08 16:45           ` Saravana Kannan
  1 sibling, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2021-07-08 15:03 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Saravana Kannan,
	Martin K . Petersen, James E . J . Bottomley,
	open list:TARGET SUBSYSTEM, Avri Altman, Bean Huo, Can Guo,
	Asutosh Das, Bart Van Assche, Linux PM,
	Linux Kernel Mailing List

On Thu, Jul 8, 2021 at 4:17 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> >>>> been registered but can still have a device link holding a reference to the
> >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> >>>> the UFS host controller). Fix by explicitly deleting the device link when
> >>>> SCSI destroys the SCSI device.
> >>>>
> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >>>> ---
> >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> >>>>  1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> >>>> --- a/drivers/scsi/ufs/ufshcd.c
> >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> >>>>              hba->sdev_ufs_device = NULL;
> >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> >>>> +    } else {
> >>>> +            /*
> >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> >>>> +             * will not have been registered but can still have a device
> >>>> +             * link holding a reference to the device.
> >>>> +             */
> >>>> +            device_links_scrap(&sdev->sdev_gendev);
> >>>
> >>> What created that link?  And why did it do that before probe happened
> >>> successfully?
> >>
> >> The same driver created the link.
> >>
> >> The documentation seems to say it is allowed to, if it is the consumer.
> >> From Documentation/driver-api/device_link.rst
> >>
> >>   Usage
> >>   =====
> >>
> >>   The earliest point in time when device links can be added is after
> >>   :c:func:`device_add()` has been called for the supplier and
> >>   :c:func:`device_initialize()` has been called for the consumer.
> >
> > Yes, this is allowed, but if you've added device links to a device
> > object that is not going to be registered after all, you are
> > responsible for doing the cleanup.
> >
> > Why can't you call device_link_del() directly on those links?
> >
> > Or device_link_remove() if you don't want to deal with link pointers?
> >
>
> Those only work for DL_FLAG_STATELESS device links, but we use only
> DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.

So I'd probably modify device_link_remove() to check if the consumer
device has been registered and run __device_link_del() directly
instead of device_link_put_kref() if it hasn't.

Or add an argument to it to force the removal.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-08 15:03           ` Rafael J. Wysocki
@ 2021-07-08 15:12             ` Rafael J. Wysocki
  2021-07-08 16:02               ` Adrian Hunter
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2021-07-08 15:12 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Saravana Kannan,
	Martin K . Petersen, James E . J . Bottomley,
	open list:TARGET SUBSYSTEM, Avri Altman, Bean Huo, Can Guo,
	Asutosh Das, Bart Van Assche, Linux PM,
	Linux Kernel Mailing List

On Thu, Jul 8, 2021 at 5:03 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jul 8, 2021 at 4:17 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>
> > >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> > >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> > >>>> been registered but can still have a device link holding a reference to the
> > >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> > >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> > >>>> the UFS host controller). Fix by explicitly deleting the device link when
> > >>>> SCSI destroys the SCSI device.
> > >>>>
> > >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > >>>> ---
> > >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> > >>>>  1 file changed, 7 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> > >>>> --- a/drivers/scsi/ufs/ufshcd.c
> > >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> > >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> > >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> > >>>>              hba->sdev_ufs_device = NULL;
> > >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> > >>>> +    } else {
> > >>>> +            /*
> > >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> > >>>> +             * will not have been registered but can still have a device
> > >>>> +             * link holding a reference to the device.
> > >>>> +             */
> > >>>> +            device_links_scrap(&sdev->sdev_gendev);
> > >>>
> > >>> What created that link?  And why did it do that before probe happened
> > >>> successfully?
> > >>
> > >> The same driver created the link.
> > >>
> > >> The documentation seems to say it is allowed to, if it is the consumer.
> > >> From Documentation/driver-api/device_link.rst
> > >>
> > >>   Usage
> > >>   =====
> > >>
> > >>   The earliest point in time when device links can be added is after
> > >>   :c:func:`device_add()` has been called for the supplier and
> > >>   :c:func:`device_initialize()` has been called for the consumer.
> > >
> > > Yes, this is allowed, but if you've added device links to a device
> > > object that is not going to be registered after all, you are
> > > responsible for doing the cleanup.
> > >
> > > Why can't you call device_link_del() directly on those links?
> > >
> > > Or device_link_remove() if you don't want to deal with link pointers?
> > >
> >
> > Those only work for DL_FLAG_STATELESS device links, but we use only
> > DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
>
> So I'd probably modify device_link_remove() to check if the consumer
> device has been registered and run __device_link_del() directly
> instead of device_link_put_kref() if it hasn't.
>
> Or add an argument to it to force the removal.

Or even modify device_link_put_kref() like this:

 static void device_link_put_kref(struct device_link *link)
 {
          if (link->flags & DL_FLAG_STATELESS)
                  kref_put(&link->kref, __device_link_del);
+        else if (!device_is_registered(link->consumer))
+                __device_link_del(link);
          else
                 WARN(1, "Unable to drop a managed device link reference\n");
 }

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-08 15:12             ` Rafael J. Wysocki
@ 2021-07-08 16:02               ` Adrian Hunter
  0 siblings, 0 replies; 17+ messages in thread
From: Adrian Hunter @ 2021-07-08 16:02 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Greg Kroah-Hartman, Saravana Kannan, Martin K . Petersen,
	James E . J . Bottomley, open list:TARGET SUBSYSTEM, Avri Altman,
	Bean Huo, Can Guo, Asutosh Das, Bart Van Assche, Linux PM,
	Linux Kernel Mailing List

On 8/07/21 6:12 pm, Rafael J. Wysocki wrote:
> On Thu, Jul 8, 2021 at 5:03 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Thu, Jul 8, 2021 at 4:17 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
>>>> On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>
>>>>> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
>>>>>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
>>>>>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
>>>>>>> been registered but can still have a device link holding a reference to the
>>>>>>> device. The unwanted device link will prevent runtime suspend indefinitely,
>>>>>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
>>>>>>> the UFS host controller). Fix by explicitly deleting the device link when
>>>>>>> SCSI destroys the SCSI device.
>>>>>>>
>>>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>>>>> ---
>>>>>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
>>>>>>>  1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>>>> index 708b3b62fc4d..483aa74fe2c8 100644
>>>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>>>>>>>              spin_lock_irqsave(hba->host->host_lock, flags);
>>>>>>>              hba->sdev_ufs_device = NULL;
>>>>>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>>>>> +    } else {
>>>>>>> +            /*
>>>>>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
>>>>>>> +             * will not have been registered but can still have a device
>>>>>>> +             * link holding a reference to the device.
>>>>>>> +             */
>>>>>>> +            device_links_scrap(&sdev->sdev_gendev);
>>>>>>
>>>>>> What created that link?  And why did it do that before probe happened
>>>>>> successfully?
>>>>>
>>>>> The same driver created the link.
>>>>>
>>>>> The documentation seems to say it is allowed to, if it is the consumer.
>>>>> From Documentation/driver-api/device_link.rst
>>>>>
>>>>>   Usage
>>>>>   =====
>>>>>
>>>>>   The earliest point in time when device links can be added is after
>>>>>   :c:func:`device_add()` has been called for the supplier and
>>>>>   :c:func:`device_initialize()` has been called for the consumer.
>>>>
>>>> Yes, this is allowed, but if you've added device links to a device
>>>> object that is not going to be registered after all, you are
>>>> responsible for doing the cleanup.
>>>>
>>>> Why can't you call device_link_del() directly on those links?
>>>>
>>>> Or device_link_remove() if you don't want to deal with link pointers?
>>>>
>>>
>>> Those only work for DL_FLAG_STATELESS device links, but we use only
>>> DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
>>
>> So I'd probably modify device_link_remove() to check if the consumer
>> device has been registered and run __device_link_del() directly
>> instead of device_link_put_kref() if it hasn't.
>>
>> Or add an argument to it to force the removal.
> 
> Or even modify device_link_put_kref() like this:
> 
>  static void device_link_put_kref(struct device_link *link)
>  {
>           if (link->flags & DL_FLAG_STATELESS)
>                   kref_put(&link->kref, __device_link_del);
> +        else if (!device_is_registered(link->consumer))
> +                __device_link_del(link);
>           else
>                  WARN(1, "Unable to drop a managed device link reference\n");
>  }
> 

Thanks! :-) I will do that.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-08 14:17         ` Adrian Hunter
  2021-07-08 15:03           ` Rafael J. Wysocki
@ 2021-07-08 16:45           ` Saravana Kannan
  2021-07-08 16:48             ` Rafael J. Wysocki
  1 sibling, 1 reply; 17+ messages in thread
From: Saravana Kannan @ 2021-07-08 16:45 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Martin K . Petersen,
	James E . J . Bottomley, open list:TARGET SUBSYSTEM, Avri Altman,
	Bean Huo, Can Guo, Asutosh Das, Bart Van Assche, Linux PM,
	Linux Kernel Mailing List

On Thu, Jul 8, 2021 at 7:17 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> >>>> been registered but can still have a device link holding a reference to the
> >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> >>>> the UFS host controller). Fix by explicitly deleting the device link when
> >>>> SCSI destroys the SCSI device.
> >>>>
> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >>>> ---
> >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> >>>>  1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> >>>> --- a/drivers/scsi/ufs/ufshcd.c
> >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> >>>>              hba->sdev_ufs_device = NULL;
> >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> >>>> +    } else {
> >>>> +            /*
> >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> >>>> +             * will not have been registered but can still have a device
> >>>> +             * link holding a reference to the device.
> >>>> +             */
> >>>> +            device_links_scrap(&sdev->sdev_gendev);
> >>>
> >>> What created that link?  And why did it do that before probe happened
> >>> successfully?
> >>
> >> The same driver created the link.
> >>
> >> The documentation seems to say it is allowed to, if it is the consumer.
> >> From Documentation/driver-api/device_link.rst
> >>
> >>   Usage
> >>   =====
> >>
> >>   The earliest point in time when device links can be added is after
> >>   :c:func:`device_add()` has been called for the supplier and
> >>   :c:func:`device_initialize()` has been called for the consumer.
> >
> > Yes, this is allowed, but if you've added device links to a device
> > object that is not going to be registered after all, you are
> > responsible for doing the cleanup.
> >
> > Why can't you call device_link_del() directly on those links?
> >
> > Or device_link_remove() if you don't want to deal with link pointers?
> >
>
> Those only work for DL_FLAG_STATELESS device links, but we use only
> DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.

Is there a reason you can't use DL_FLAG_STATELESS? It doesn't preclude
you from using RPM_ACTIVE as far as I can tell.

-Saravana


-Saravana

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-08 16:45           ` Saravana Kannan
@ 2021-07-08 16:48             ` Rafael J. Wysocki
  2021-07-08 16:57               ` Saravana Kannan
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2021-07-08 16:48 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Adrian Hunter, Rafael J. Wysocki, Greg Kroah-Hartman,
	Martin K . Petersen, James E . J . Bottomley,
	open list:TARGET SUBSYSTEM, Avri Altman, Bean Huo, Can Guo,
	Asutosh Das, Bart Van Assche, Linux PM,
	Linux Kernel Mailing List

On Thu, Jul 8, 2021 at 6:46 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Jul 8, 2021 at 7:17 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>
> > >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> > >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> > >>>> been registered but can still have a device link holding a reference to the
> > >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> > >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> > >>>> the UFS host controller). Fix by explicitly deleting the device link when
> > >>>> SCSI destroys the SCSI device.
> > >>>>
> > >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > >>>> ---
> > >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> > >>>>  1 file changed, 7 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> > >>>> --- a/drivers/scsi/ufs/ufshcd.c
> > >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> > >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> > >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> > >>>>              hba->sdev_ufs_device = NULL;
> > >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> > >>>> +    } else {
> > >>>> +            /*
> > >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> > >>>> +             * will not have been registered but can still have a device
> > >>>> +             * link holding a reference to the device.
> > >>>> +             */
> > >>>> +            device_links_scrap(&sdev->sdev_gendev);
> > >>>
> > >>> What created that link?  And why did it do that before probe happened
> > >>> successfully?
> > >>
> > >> The same driver created the link.
> > >>
> > >> The documentation seems to say it is allowed to, if it is the consumer.
> > >> From Documentation/driver-api/device_link.rst
> > >>
> > >>   Usage
> > >>   =====
> > >>
> > >>   The earliest point in time when device links can be added is after
> > >>   :c:func:`device_add()` has been called for the supplier and
> > >>   :c:func:`device_initialize()` has been called for the consumer.
> > >
> > > Yes, this is allowed, but if you've added device links to a device
> > > object that is not going to be registered after all, you are
> > > responsible for doing the cleanup.
> > >
> > > Why can't you call device_link_del() directly on those links?
> > >
> > > Or device_link_remove() if you don't want to deal with link pointers?
> > >
> >
> > Those only work for DL_FLAG_STATELESS device links, but we use only
> > DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
>
> Is there a reason you can't use DL_FLAG_STATELESS? It doesn't preclude
> you from using RPM_ACTIVE as far as I can tell.

Perhaps he wants the links to be managed if they are used after all.

Anyway, this is a valid use case that is not covered right now.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-08 16:48             ` Rafael J. Wysocki
@ 2021-07-08 16:57               ` Saravana Kannan
  2021-07-08 17:39                 ` Rafael J. Wysocki
  0 siblings, 1 reply; 17+ messages in thread
From: Saravana Kannan @ 2021-07-08 16:57 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Adrian Hunter, Greg Kroah-Hartman, Martin K . Petersen,
	James E . J . Bottomley, open list:TARGET SUBSYSTEM, Avri Altman,
	Bean Huo, Can Guo, Asutosh Das, Bart Van Assche, Linux PM,
	Linux Kernel Mailing List

On Thu, Jul 8, 2021 at 9:49 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jul 8, 2021 at 6:46 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Thu, Jul 8, 2021 at 7:17 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >
> > > On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > > > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > > >>
> > > >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > > >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> > > >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> > > >>>> been registered but can still have a device link holding a reference to the
> > > >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> > > >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> > > >>>> the UFS host controller). Fix by explicitly deleting the device link when
> > > >>>> SCSI destroys the SCSI device.
> > > >>>>
> > > >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > > >>>> ---
> > > >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> > > >>>>  1 file changed, 7 insertions(+)
> > > >>>>
> > > >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> > > >>>> --- a/drivers/scsi/ufs/ufshcd.c
> > > >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> > > >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> > > >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> > > >>>>              hba->sdev_ufs_device = NULL;
> > > >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> > > >>>> +    } else {
> > > >>>> +            /*
> > > >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> > > >>>> +             * will not have been registered but can still have a device
> > > >>>> +             * link holding a reference to the device.
> > > >>>> +             */
> > > >>>> +            device_links_scrap(&sdev->sdev_gendev);
> > > >>>
> > > >>> What created that link?  And why did it do that before probe happened
> > > >>> successfully?
> > > >>
> > > >> The same driver created the link.
> > > >>
> > > >> The documentation seems to say it is allowed to, if it is the consumer.
> > > >> From Documentation/driver-api/device_link.rst
> > > >>
> > > >>   Usage
> > > >>   =====
> > > >>
> > > >>   The earliest point in time when device links can be added is after
> > > >>   :c:func:`device_add()` has been called for the supplier and
> > > >>   :c:func:`device_initialize()` has been called for the consumer.
> > > >
> > > > Yes, this is allowed, but if you've added device links to a device
> > > > object that is not going to be registered after all, you are
> > > > responsible for doing the cleanup.
> > > >
> > > > Why can't you call device_link_del() directly on those links?
> > > >
> > > > Or device_link_remove() if you don't want to deal with link pointers?
> > > >
> > >
> > > Those only work for DL_FLAG_STATELESS device links, but we use only
> > > DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
> >
> > Is there a reason you can't use DL_FLAG_STATELESS? It doesn't preclude
> > you from using RPM_ACTIVE as far as I can tell.
>
> Perhaps he wants the links to be managed if they are used after all.
>
> Anyway, this is a valid use case that is not covered right now.

Maybe. But the suggested patch is certainly risky.

There is no requirement the consumer is registered before the links
are added though. So, randomly deleting a managed link when
device_link_put_kref() is called on a stateless refcount (they are
still the same link still) isn't right. The entity that created the
managed device link might still want it there. Also, if two entities
create a managed link and one of them calls device_link_put_kref()
before the device is registered, we have a UAF problem because managed
links aren't refcounted (more than once).

-Saravana

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe
  2021-07-08 16:57               ` Saravana Kannan
@ 2021-07-08 17:39                 ` Rafael J. Wysocki
  0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2021-07-08 17:39 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rafael J. Wysocki, Adrian Hunter, Greg Kroah-Hartman,
	Martin K . Petersen, James E . J . Bottomley,
	open list:TARGET SUBSYSTEM, Avri Altman, Bean Huo, Can Guo,
	Asutosh Das, Bart Van Assche, Linux PM,
	Linux Kernel Mailing List

On Thu, Jul 8, 2021 at 6:57 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Jul 8, 2021 at 9:49 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Jul 8, 2021 at 6:46 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Jul 8, 2021 at 7:17 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > > >
> > > > On 8/07/21 3:31 pm, Rafael J. Wysocki wrote:
> > > > > On Wed, Jul 7, 2021 at 7:49 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > > > >>
> > > > >> On 7/07/21 8:39 pm, Greg Kroah-Hartman wrote:
> > > > >>> On Wed, Jul 07, 2021 at 08:29:48PM +0300, Adrian Hunter wrote:
> > > > >>>> If a LUN fails to probe (e.g. absent BOOT WLUN), the device will not have
> > > > >>>> been registered but can still have a device link holding a reference to the
> > > > >>>> device. The unwanted device link will prevent runtime suspend indefinitely,
> > > > >>>> and cause some warnings if the supplier is ever deleted (e.g. by unbinding
> > > > >>>> the UFS host controller). Fix by explicitly deleting the device link when
> > > > >>>> SCSI destroys the SCSI device.
> > > > >>>>
> > > > >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > > > >>>> ---
> > > > >>>>  drivers/scsi/ufs/ufshcd.c | 7 +++++++
> > > > >>>>  1 file changed, 7 insertions(+)
> > > > >>>>
> > > > >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > > >>>> index 708b3b62fc4d..483aa74fe2c8 100644
> > > > >>>> --- a/drivers/scsi/ufs/ufshcd.c
> > > > >>>> +++ b/drivers/scsi/ufs/ufshcd.c
> > > > >>>> @@ -5029,6 +5029,13 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> > > > >>>>              spin_lock_irqsave(hba->host->host_lock, flags);
> > > > >>>>              hba->sdev_ufs_device = NULL;
> > > > >>>>              spin_unlock_irqrestore(hba->host->host_lock, flags);
> > > > >>>> +    } else {
> > > > >>>> +            /*
> > > > >>>> +             * If a LUN fails to probe (e.g. absent BOOT WLUN), the device
> > > > >>>> +             * will not have been registered but can still have a device
> > > > >>>> +             * link holding a reference to the device.
> > > > >>>> +             */
> > > > >>>> +            device_links_scrap(&sdev->sdev_gendev);
> > > > >>>
> > > > >>> What created that link?  And why did it do that before probe happened
> > > > >>> successfully?
> > > > >>
> > > > >> The same driver created the link.
> > > > >>
> > > > >> The documentation seems to say it is allowed to, if it is the consumer.
> > > > >> From Documentation/driver-api/device_link.rst
> > > > >>
> > > > >>   Usage
> > > > >>   =====
> > > > >>
> > > > >>   The earliest point in time when device links can be added is after
> > > > >>   :c:func:`device_add()` has been called for the supplier and
> > > > >>   :c:func:`device_initialize()` has been called for the consumer.
> > > > >
> > > > > Yes, this is allowed, but if you've added device links to a device
> > > > > object that is not going to be registered after all, you are
> > > > > responsible for doing the cleanup.
> > > > >
> > > > > Why can't you call device_link_del() directly on those links?
> > > > >
> > > > > Or device_link_remove() if you don't want to deal with link pointers?
> > > > >
> > > >
> > > > Those only work for DL_FLAG_STATELESS device links, but we use only
> > > > DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE flags.
> > >
> > > Is there a reason you can't use DL_FLAG_STATELESS? It doesn't preclude
> > > you from using RPM_ACTIVE as far as I can tell.
> >
> > Perhaps he wants the links to be managed if they are used after all.
> >
> > Anyway, this is a valid use case that is not covered right now.
>
> Maybe. But the suggested patch is certainly risky.
>
> There is no requirement the consumer is registered before the links
> are added though. So, randomly deleting a managed link when
> device_link_put_kref() is called on a stateless refcount (they are
> still the same link still) isn't right.

Device pointers are needed in order to create a device link and it is
quite unlikely that a pointer to an unregistered device will be shared
between two different pieces of code.

> The entity that created the
> managed device link might still want it there.

So the stateless kref is going to be put first.

> Also, if two entities
> create a managed link and one of them calls device_link_put_kref()
> before the device is registered, we have a UAF problem because managed
> links aren't refcounted (more than once).

IMO until a device object is registered, its creator should be allowed
to do the cleanup in the case when it gets released without
registration, including the removal of any device links to it that
have been added so far.

Messing up with a device object created by someone else that may still
go away without registration is a risky business regardless.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-07-08 17:40 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-07 17:29 [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Adrian Hunter
2021-07-07 17:29 ` [PATCH RFC 1/2] " Adrian Hunter
2021-07-07 17:38   ` Greg Kroah-Hartman
2021-07-07 17:29 ` [PATCH RFC 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe Adrian Hunter
2021-07-07 17:39   ` Greg Kroah-Hartman
2021-07-07 17:49     ` Adrian Hunter
2021-07-08 12:31       ` Rafael J. Wysocki
2021-07-08 14:17         ` Adrian Hunter
2021-07-08 15:03           ` Rafael J. Wysocki
2021-07-08 15:12             ` Rafael J. Wysocki
2021-07-08 16:02               ` Adrian Hunter
2021-07-08 16:45           ` Saravana Kannan
2021-07-08 16:48             ` Rafael J. Wysocki
2021-07-08 16:57               ` Saravana Kannan
2021-07-08 17:39                 ` Rafael J. Wysocki
2021-07-07 17:40 ` [PATCH RFC 0/2] driver core: Add ability to delete device links of unregistered devices Greg Kroah-Hartman
2021-07-07 17:41   ` Saravana Kannan

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