Messages in this thread Patch in this message | | | Date | Thu, 28 Sep 2023 17:02:38 +0800 | Subject | Re: [PATH v3 2/3] iommu/vt-d: debugfs: Create/remove debugfs file per {device, pasid} | From | "Liu, Jingqi" <> |
| |
On 9/28/2023 9:58 AM, Baolu Lu wrote: > On 9/27/23 11:15 PM, Jingqi Liu wrote: ...... >> + >> +/* Create a debugfs directory for each device. */ >> +void intel_iommu_debugfs_create_dev(struct device *dev) >> +{ >> + struct dentry *dev_dir; >> + >> + dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug); >> + if (!dev_dir) { >> + dev_dir = debugfs_create_dir(dev_name(dev), intel_iommu_debug); >> + if (IS_ERR(dev_dir)) >> + pr_info("%s: Failed to create debugfs directory.\n", >> + dev_name(dev)); >> + } else >> + dput(dev_dir); >> +} > > Above could simply be like this: > > void intel_iommu_debugfs_create_dev(struct device *dev) > { > struct device_domain_info *info = dev_iommu_priv_get(dev); > > info->debugfs_entry = debugfs_create_dir(dev_name(dev), > intel_iommu_debug); > } > > Isn't it? Thanks. Good point. If add an "info->debugfs_dentry" to the device "info" to save the dentry of device debugfs directory, there's no need to lookup the dentry by debugfs_lookup(). Just simply get it from the device "info".
> >> + >> +void intel_iommu_debugfs_remove_dev(struct device *dev) >> +{ >> + struct dentry *dev_dir, *sub_dir, *dentry; >> + struct list_head *plist; >> + >> + dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug); >> + if (!dev_dir) >> + return; >> + >> + list_for_each(plist, &(dev_dir->d_subdirs)) { >> + sub_dir = list_entry(plist, struct dentry, d_child); >> + if(sub_dir) { >> + dentry = debugfs_lookup("domain_translation_struct", >> + sub_dir); >> + if (!dentry) >> + continue; >> + >> + if (dentry->d_inode->i_private) >> + kfree(dentry->d_inode->i_private); >> + >> + dput(dentry); >> + } >> + } >> + >> + debugfs_remove_recursive(dev_dir); >> + dput(dev_dir); >> +} > > And this could simply be like this: > > void intel_iommu_debugfs_remove_dev(struct device *dev) > { > struct device_domain_info *info = dev_iommu_priv_get(dev); > > debugfs_remove(info->debugfs_entry); > } > Yes. Just get the debugfs dentry of device simply for removing. This helper should be called before the "info" is freed in intel_iommu_release_device(). Like this:
+ intel_iommu_debugfs_remove_dev(dev); kfree(info);
>> + >> +/* >> + * Create a debugfs directory per pair of {device, pasid}, >> + * then create the corresponding debugfs file in this directory >> + * for user to dump its page table. e.g. >> + * >> /sys/kernel/debug/iommu/intel/0000:00:01.0/0/domain_translation_struct >> + */ >> +void intel_iommu_debugfs_create_dev_pasid(struct iommu_domain *domain, >> + struct device *dev, u32 pasid) >> +{ >> + struct dentry *dev_dir, *pasid_dir; >> + struct show_domain_info *sinfo; >> + char dir_name[10]; >> + >> + /* >> + * The debugfs only dumps the page tables whose mappings are >> created >> + * and destroyed by the iommu_map/unmap() interfaces. Check the >> + * mapping type of the domain before creating debugfs directory. >> + */ >> + if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING)) >> + return; >> + >> + dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug); >> + if (!dev_dir) >> + return; >> + >> + sprintf(dir_name, "%x", pasid); >> + pasid_dir = debugfs_create_dir(dir_name, dev_dir); >> + if (IS_ERR(pasid_dir)) >> + goto dput_out; >> + >> + sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL); >> + if (!sinfo) >> + goto dput_out; >> + >> + sinfo->dev = dev; >> + sinfo->pasid = pasid; >> + debugfs_create_file("domain_translation_struct", 0444, >> + pasid_dir, sinfo, >> + &domain_translation_struct_fops); >> +dput_out: >> + dput(dev_dir); >> +} > > And here, > > void intel_iommu_debugfs_create_dev_pasid(struct iommu_domain *domain, > struct device *dev, u32 pasid) > { > sprintf(dir_name, "%x", pasid); > dev_pasid->debugfs_entry = debugfs_create_dir(dir_name, > info->debugfs_entry); > > debugfs_create_file("domain_translation_struct", 0444, > dev_pasid->debugfs_entry, dev_pasid, > &domain_translation_struct_fops); > } > Thanks. You mean to add 'debugfs_entry' in below structure. struct dev_pasid_info *dev_pasid; This structure is also allocated per pair of {dev, pasid}. The debugfs dentry of {dev, pasid} can be simply obtained from 'dev_pasid_info'.
So the 'dev_pasid_info' can be passed as a parameter of this helper, right ? Like this: void intel_iommu_debugfs_create_dev_pasid(struct iommu_domain *domain, struct dev_pasid_info *dev_pasid) ; >> + >> +/* >> + * Remove the debugfs directory and file corresponding to each pair of >> + * {device, pasid}. >> + */ >> +void intel_iommu_debugfs_remove_dev_pasid(struct device *dev, u32 >> pasid) >> +{ >> + struct dentry *dev_dir, *pasid_dir, *dentry; >> + char dir_name[10]; >> + >> + dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug); >> + if (!dev_dir) >> + return; >> + >> + sprintf(dir_name, "%x", pasid); >> + pasid_dir = debugfs_lookup(dir_name, dev_dir); >> + if (!pasid_dir) >> + goto dput_dev; >> + >> + dentry = debugfs_lookup("domain_translation_struct", pasid_dir); >> + if (!dentry) >> + goto dput_pasid; >> + >> + if (dentry->d_inode->i_private) >> + kfree(dentry->d_inode->i_private); >> + >> + debugfs_remove_recursive(pasid_dir); >> + >> + dput(dentry); >> +dput_pasid: >> + dput(pasid_dir); >> +dput_dev: >> + dput(dev_dir); >> +} > > The same thing here: > > debugfs_remove(dev_pasid->debugfs_entry); > Yes. Just get the debugfs dentry from "dev_pasid" instead of 'debugfs_lookup()'. And this helper should be called before the "struct dev_pasid_info" is freed in intel_iommu_remove_dev_pasid(). Like this:
+ intel_iommu_debugfs_remove_dev_pasid(dev, pasid); kfree(dev_pasid);
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >> index dd8ff358867d..af9c989035a2 100644 >> --- a/drivers/iommu/intel/iommu.c >> +++ b/drivers/iommu/intel/iommu.c >> @@ -2488,6 +2488,13 @@ static int dmar_domain_attach_device(struct >> dmar_domain *domain, >> iommu_enable_pci_caps(info); >> + /* >> + * Create a debugfs directory specified by RID_PASID >> + * in the debugfs device directory. >> + */ >> + intel_iommu_debugfs_create_dev_pasid(&info->domain->domain, >> + dev, IOMMU_NO_PASID); > > The function name is self-explained. So no need to add comments. Ditto > to all other places. > Indeed. I'll delete all related comments.
Thanks, Jingqi
| |