Messages in this thread | | | Date | Wed, 23 Dec 2020 09:31:49 +0800 | From | Can Guo <> | Subject | Re: [PATCH v5 1/7] scsi: ufs: Add "wb_on" sysfs node to control WB on/off |
| |
On 2020-12-23 06:11, Bean Huo wrote: > On Tue, 2020-12-22 at 21:57 +0100, Bean Huo wrote: >> > > May this operation race with UFS shutdown flow? >> > > >> > > To be more clear, ufshcd_wb_ctrl() here may be executed after >> > > host >> > > clock >> > > is disabled by shutdown flow? >> > > >> > > If yes, we need to avoid it. >> > >> > I have the same doubt - can user still access sysfs nodes after >> > system >> > starts to run shutdown routines? If yes, then we need to remove all >> > UFS >> > sysfs nodes in ufshcd_shutdown(). >> > >> >> No, we shouldn't do in this way, user space complains this. I think >> the nodes in the sysfs can be shileded write, but the nodes shouldn't >> be flash of its presence frequently. >> >> Thanks, >> Bean >> >> >> > Thanks, >> > >> > Can Guo. > > > Hi Can > Got your point, you don't want user space to interrupt UFS by sysyfs if > UFS is in power down mode. if this is true, insteading of removing all > sysfs node in ufshcd_shutdown, maybe just add this checkup before > accessing UFS device descriptors/flag/attributes/LU: > > for example, for the device descriptor: > > > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs- > sysfs.c > index b3bf7fca00e5..881fe1c24a9f 100644 > --- a/drivers/scsi/ufs/ufs-sysfs.c > +++ b/drivers/scsi/ufs/ufs-sysfs.c > @@ -262,6 +262,9 @@ static ssize_t ufs_sysfs_read_desc_param(struct > ufs_hba *hba, > u8 desc_buf[8] = {0}; > int ret; > > + if (!ufshcd_is_ufs_dev_active(hba) || > !ufshcd_is_link_active(hba)) > + return -EACCES; > + > > Bean
First of all, this check is not helping at all, during ufshcd_shutdown(), both the link and dev can be active for a moment, so this check is not helping the race condition. And assume you come up with a better check, you want to add the check everywhere? You must have noticed the fix to the func ufshcd_clk_gate_enable_store() from Jaegeuk Kim. So, don't expect any luck from user space, so long the sysfs nodes are available, users can grasp them and even stress them just for testing purposes. I don't see why removing the sysfs nodes during ufshcd_shutdown() is a concern to customer - we should do whatever is needed to protect LLD contexts from user space intervene. What do you think?
Can Guo.
Thanks,
Can Guo.
| |