* [PATCH V2 0/2] driver core: Add ability to delete device links of unregistered devices @ 2021-07-09 6:43 Adrian Hunter 2021-07-09 6:43 ` [PATCH V2 1/2] " Adrian Hunter 2021-07-09 6:43 ` [PATCH V2 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe Adrian Hunter 0 siblings, 2 replies; 7+ messages in thread From: Adrian Hunter @ 2021-07-09 6:43 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. These V2 patches fix the issue by amending device link removal to accept removal of a link with an unregistered consumer device, as suggested by Rafael. Changes in V2: Take approach suggested by Rafael 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 drivers/base/core.c | 11 ++++++++--- drivers/scsi/ufs/ufshcd.c | 23 +++++++++++++++++++++-- 2 files changed, 29 insertions(+), 5 deletions(-) Regards Adrian ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH V2 1/2] driver core: Add ability to delete device links of unregistered devices 2021-07-09 6:43 [PATCH V2 0/2] driver core: Add ability to delete device links of unregistered devices Adrian Hunter @ 2021-07-09 6:43 ` Adrian Hunter 2021-07-09 9:55 ` Greg Kroah-Hartman 2021-07-09 11:28 ` Rafael J. Wysocki 2021-07-09 6:43 ` [PATCH V2 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe Adrian Hunter 1 sibling, 2 replies; 7+ messages in thread From: Adrian Hunter @ 2021-07-09 6:43 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. Amend device link removal to accept removal of a link with an unregistered consumer device. To make that work nicely, 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 a warning. Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Fixes: b294ff3e34490 ("scsi: ufs: core: Enable power management for wlun") Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/base/core.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/base/core.c b/drivers/base/core.c index ea5b85354526..24bacdb315c6 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); @@ -885,6 +888,8 @@ 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->kref); else WARN(1, "Unable to drop a managed device link reference\n"); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH V2 1/2] driver core: Add ability to delete device links of unregistered devices 2021-07-09 6:43 ` [PATCH V2 1/2] " Adrian Hunter @ 2021-07-09 9:55 ` Greg Kroah-Hartman 2021-07-09 11:19 ` Rafael J. Wysocki 2021-07-09 11:28 ` Rafael J. Wysocki 1 sibling, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2021-07-09 9:55 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 Fri, Jul 09, 2021 at 09:43:40AM +0300, Adrian Hunter wrote: > 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. Why are you not just manually removing the link you just created? You manually added it, you know something failed so you need to clean up, so why not clean this up too? thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2 1/2] driver core: Add ability to delete device links of unregistered devices 2021-07-09 9:55 ` Greg Kroah-Hartman @ 2021-07-09 11:19 ` Rafael J. Wysocki 0 siblings, 0 replies; 7+ messages in thread From: Rafael J. Wysocki @ 2021-07-09 11:19 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Adrian Hunter, 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 Fri, Jul 9, 2021 at 11:55 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Fri, Jul 09, 2021 at 09:43:40AM +0300, Adrian Hunter wrote: > > 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. > > Why are you not just manually removing the link you just created? You > manually added it, you know something failed so you need to clean up, so > why not clean this up too? His changes are needed to do just that. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2 1/2] driver core: Add ability to delete device links of unregistered devices 2021-07-09 6:43 ` [PATCH V2 1/2] " Adrian Hunter 2021-07-09 9:55 ` Greg Kroah-Hartman @ 2021-07-09 11:28 ` Rafael J. Wysocki 2021-07-09 13:18 ` Adrian Hunter 1 sibling, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2021-07-09 11:28 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 Fri, Jul 9, 2021 at 8:43 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > 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. > > Amend device link removal to accept removal of a link with an > unregistered consumer device. > > To make that work nicely, 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 a warning. > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Fixes: b294ff3e34490 ("scsi: ufs: core: Enable power management for wlun") > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/base/core.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index ea5b85354526..24bacdb315c6 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"); I think that this is needed regardless of the changes in device_link_put_kref(), because if somebody decides to delete a stateless device link before registering the consumer device, sysfs_remove_link() will still complain, won't it? > 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); > + } And here too, if I'm not mistaken. So in that case it would be better to put the above changes into a separate patch and add a Fixes tag to it. > snprintf(buf, len, "consumer:%s:%s", dev_bus_name(con), dev_name(con)); > sysfs_remove_link(&sup->kobj, buf); > kfree(buf); > @@ -885,6 +888,8 @@ 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->kref); > else > WARN(1, "Unable to drop a managed device link reference\n"); > } > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2 1/2] driver core: Add ability to delete device links of unregistered devices 2021-07-09 11:28 ` Rafael J. Wysocki @ 2021-07-09 13:18 ` Adrian Hunter 0 siblings, 0 replies; 7+ messages in thread From: Adrian Hunter @ 2021-07-09 13:18 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 9/07/21 2:28 pm, Rafael J. Wysocki wrote: > On Fri, Jul 9, 2021 at 8:43 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> 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. >> >> Amend device link removal to accept removal of a link with an >> unregistered consumer device. >> >> To make that work nicely, 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 a warning. >> >> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Fixes: b294ff3e34490 ("scsi: ufs: core: Enable power management for wlun") >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> drivers/base/core.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> index ea5b85354526..24bacdb315c6 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"); > > I think that this is needed regardless of the changes in > device_link_put_kref(), because if somebody decides to delete a > stateless device link before registering the consumer device, > sysfs_remove_link() will still complain, won't it? I would think so. > >> 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); >> + } > > And here too, if I'm not mistaken. > > So in that case it would be better to put the above changes into a > separate patch and add a Fixes tag to it. Yes, that makes sense. I'll send a V3 > >> snprintf(buf, len, "consumer:%s:%s", dev_bus_name(con), dev_name(con)); >> sysfs_remove_link(&sup->kobj, buf); >> kfree(buf); >> @@ -885,6 +888,8 @@ 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->kref); >> else >> WARN(1, "Unable to drop a managed device link reference\n"); >> } >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH V2 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe 2021-07-09 6:43 [PATCH V2 0/2] driver core: Add ability to delete device links of unregistered devices Adrian Hunter 2021-07-09 6:43 ` [PATCH V2 1/2] " Adrian Hunter @ 2021-07-09 6:43 ` Adrian Hunter 1 sibling, 0 replies; 7+ messages in thread From: Adrian Hunter @ 2021-07-09 6:43 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. Fixes: b294ff3e34490 ("scsi: ufs: core: Enable power management for wlun") Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/scsi/ufs/ufshcd.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 708b3b62fc4d..9864a8ee0263 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -5020,15 +5020,34 @@ static int ufshcd_slave_configure(struct scsi_device *sdev) static void ufshcd_slave_destroy(struct scsi_device *sdev) { struct ufs_hba *hba; + unsigned long flags; hba = shost_priv(sdev->host); /* Drop the reference as it won't be needed anymore */ if (ufshcd_scsi_to_upiu_lun(sdev->lun) == UFS_UPIU_UFS_DEVICE_WLUN) { - unsigned long flags; - spin_lock_irqsave(hba->host->host_lock, flags); hba->sdev_ufs_device = NULL; spin_unlock_irqrestore(hba->host->host_lock, flags); + } else if (hba->sdev_ufs_device) { + struct device *supplier = NULL; + + /* Ensure UFS Device WLUN exists and does not disappear */ + spin_lock_irqsave(hba->host->host_lock, flags); + if (hba->sdev_ufs_device) { + supplier = &hba->sdev_ufs_device->sdev_gendev; + get_device(supplier); + } + spin_unlock_irqrestore(hba->host->host_lock, flags); + + if (supplier) { + /* + * 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_link_remove(&sdev->sdev_gendev, supplier); + put_device(supplier); + } } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-09 13:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-09 6:43 [PATCH V2 0/2] driver core: Add ability to delete device links of unregistered devices Adrian Hunter 2021-07-09 6:43 ` [PATCH V2 1/2] " Adrian Hunter 2021-07-09 9:55 ` Greg Kroah-Hartman 2021-07-09 11:19 ` Rafael J. Wysocki 2021-07-09 11:28 ` Rafael J. Wysocki 2021-07-09 13:18 ` Adrian Hunter 2021-07-09 6:43 ` [PATCH V2 2/2] scsi: ufshcd: Fix device links when BOOT WLUN fails to probe Adrian Hunter
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).