On 2020-08-12 20:00, Daejun Park wrote: > On 2020-08-06 02:11, Daejun Park wrote: >>> +static int ufshpb_create_sysfs(struct ufs_hba *hba, struct ufshpb_lu *hpb) >>> +{ >>> + int ret; >>> + >>> + ufshpb_stat_init(hpb); >>> + >>> + kobject_init(&hpb->kobj, &ufshpb_ktype); >>> + mutex_init(&hpb->sysfs_lock); >>> + >>> + ret = kobject_add(&hpb->kobj, kobject_get(&hba->dev->kobj), >>> + "ufshpb_lu%d", hpb->lun); >>> + >>> + if (ret) >>> + return ret; >>> + >>> + ret = sysfs_create_group(&hpb->kobj, &ufshpb_sysfs_group); >>> + >>> + if (ret) { >>> + dev_err(hba->dev, "ufshpb_lu%d create file error\n", hpb->lun); >>> + return ret; >>> + } >>> + >>> + dev_info(hba->dev, "ufshpb_lu%d sysfs adds uevent", hpb->lun); >>> + kobject_uevent(&hpb->kobj, KOBJ_ADD); >>> + >>> + return 0; >>> +} >> >> Please attach these sysfs attributes to /sys/class/scsi_device/*/device >> instead of creating a new kobject. Consider using the following >> scsi_host_template member to define LUN sysfs attributes: > > I am not rejecting your comment. But I added kobject for distinguishing > between other attributes and attributes related to HPB feature. > If you think it's pointless, I'll fix it. Hi Daejun, I see two reasons to add these sysfs attributes under /sys/class/scsi_device/*/device: - This makes the behavior of the UFS driver similar to that of other Linux SCSI LLD drivers. - This makes it easier for people who want to write udev rules that read from these attributes. Since ufshpb_lu%d is attached to the UFS controller it is not clear to me which attributes will appear first in sysfs - the SCSI device attributes or the ufshpb_lu%d attributes. If there are only SCSI device attributes there is no such ambiguity and hence authors of udev rules won't have to worry about this race condition. >>> +void ufshpb_remove(struct ufs_hba *hba) >>> +{ >>> + struct ufshpb_lu *hpb, *n_hpb; >>> + struct ufsf_feature_info *ufsf; >>> + struct scsi_device *sdev; >>> + >>> + ufsf = &hba->ufsf; >>> + >>> + list_for_each_entry_safe(hpb, n_hpb, &lh_hpb_lu, list_hpb_lu) { >>> + ufshpb_set_state(hpb, HPB_FAILED); >>> + >>> + sdev = hpb->sdev_ufs_lu; >>> + sdev->hostdata = NULL; >>> + >>> + ufshpb_destroy_region_tbl(hpb); >>> + >>> + list_del_init(&hpb->list_hpb_lu); >>> + ufshpb_remove_sysfs(hpb); >>> + >>> + kfree(hpb); >>> + } >>> + >>> + dev_info(hba->dev, "ufshpb: remove success\n"); >>> +} >> >> Should the code in the body of the above loop perhaps be called from inside >> ufshcd_slave_destroy()? > > Moving other stuffs in the loop is good idea, but removing attributes is problem. > To avoid adding new kobject, I will try to use sysfs_merge_group() > for adding attributes. To delete merged attributes, sysfs_unmerge_group() > should be called. But sysfs_remove_groups() is called before calling ufshcd_slave_destroy(). Hmm ... I don't see why the sdev_groups host template attribute can't be used? Please don't use sysfs_merge_group() and sysfs_unmerge_group() because that would create a race condition against udev rules if these functions are called after the device core has emitted a KOBJ_ADD event. Thanks, Bart.