| Message ID | 20210716114408.17320-1-adrian.hunter@intel.com |
|---|---|
| Headers | show |
| Series | driver core: Add ability to delete device links of unregistered devices | expand |
> 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) { Was just checked in the outer clause? Thanks, Avri > + 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
On 17/07/21 9:02 pm, Avri Altman wrote: >> 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) { > Was just checked in the outer clause? Yes, but need to re-check with the spinlock locked. > > Thanks, > Avri > >> + 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 >
> On 17/07/21 9:02 pm, Avri Altman wrote: > >> 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) { > > Was just checked in the outer clause? > > Yes, but need to re-check with the spinlock locked. OK. Looks good to me. Thanks, Avri > > > > > Thanks, > > Avri > > > >> + 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 > >
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. Since V2, these 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 V4: driver core: Prevent warning when removing a device link from unregistered consumer Add stable tag and Rafael's Reviewed-by driver core: Add ability to delete device links of unregistered devices Amend comment "discover an error" -> "discovering an error" Merge with next patch scsi: ufshcd: Fix device links when BOOT WLUN fails to probe Merge with previous patch Add Rafael's Reviewed-by Changes in V3: driver core: Prevent warning when removing a device link from unregistered consumer New patch split from "driver core: Add ability to delete device links of unregistered devices" except first chunk from that patch dropped as unnecessary driver core: Add ability to delete device links of unregistered devices Move warning fix to separate patch. Changes in V2: Take approach suggested by Rafael Adrian Hunter (2): driver core: Prevent warning when removing a device link from unregistered consumer scsi: ufshcd: Fix device links when BOOT WLUN fails to probe drivers/base/core.c | 8 ++++++-- drivers/scsi/ufs/ufshcd.c | 23 +++++++++++++++++++++-- 2 files changed, 27 insertions(+), 4 deletions(-) Regards Adrian