* [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 related [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
* [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 related [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 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
* 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
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 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.