[RFC PATCH v2 1/5] iommu/arm-smmu-v3: Add basic debugfs framework

[Qinxin Xia]

On 2026/3/30 18:46:57, Nicolin Chen <nicolinc at nvidia.com> wrote:
> On Sat, Mar 28, 2026 at 06:17:02PM +0800, Qinxin Xia wrote:
>> Add basic debugfs framework for ARM SMMUv3 driver.This creates the
> 
> Needs a space after "."
> 
>> +static int smmu_debugfs_capabilities_show(struct seq_file *seq, void *unused)
>> +{
>> +	struct arm_smmu_device *smmu = seq->private;
>> +
>> +	if (!smmu) {
>> +		seq_puts(seq, "SMMU not available\n");
>> +		return 0;
>> +	}
>> +
>> +	seq_puts(seq, "SMMUv3 Capabilities:\n");
>> +	seq_printf(seq, "  Stage1 Translation: %s\n",
>> +		   smmu->features & ARM_SMMU_FEAT_TRANS_S1 ? "Yes" : "No");
>> +	seq_printf(seq, "  Stage2 Translation: %s\n",
>> +		   smmu->features & ARM_SMMU_FEAT_TRANS_S2 ? "Yes" : "No");
>> +	seq_printf(seq, "  Coherent Walk: %s\n",
>> +		   smmu->features & ARM_SMMU_FEAT_COHERENCY ? "Yes" : "No");
>> +	seq_printf(seq, "  ATS Support: %s\n",
>> +		   smmu->features & ARM_SMMU_FEAT_ATS ? "Yes" : "No");
>> +	seq_printf(seq, "  PRI Support: %s\n",
>> +		   smmu->features & ARM_SMMU_FEAT_PRI ? "Yes" : "No");
>> +	seq_printf(seq, "  Stream Table Size: %d\n", 1 << smmu->sid_bits);
>> +	seq_printf(seq, "  Command Queue Depth: %d\n",
>> +		   1 << smmu->cmdq.q.llq.max_n_shift);
>> +	seq_printf(seq, "  Event Queue Depth: %d\n",
>> +		   1 << smmu->evtq.q.llq.max_n_shift);
> 
> Nit: should we do all sizes or all depths? Any good reason to mix
> them here?
>

Stream Table is a table, not a queue, so 'Size' is more appropriate; 
queues use 'Depth'. But I can change to unify if preferred.

>> +/**
>> + * arm_smmu_debugfs_remove() - Clean up debugfs entries for an SMMU device
>> + * @smmu: SMMU device
>> + *
>> + * This function removes the debugfs directories created by setup.
>> + */
>> +void arm_smmu_debugfs_remove(struct arm_smmu_device *smmu)
>> +{
>> +	struct arm_smmu_debugfs *debugfs;
>> +
>> +	scoped_guard(mutex, &arm_smmu_debugfs_lock) {
> 
> It could be just normal guard().
> 
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 4d00d796f078..cbb3fccc501b 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -4904,6 +4904,15 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>   	/* Check for RMRs and install bypass STEs if any */
>>   	arm_smmu_rmr_install_bypass_ste(smmu);
>>   
>> +#ifdef CONFIG_ARM_SMMU_V3_DEBUGFS
>> +	char name[32];
> 
> This could be moved to the top, as iommu_device_sysfs_add() can use
> it, whether CONFIG_ARM_SMMU_V3_DEBUGFS=y or =n.
> 
>> +	snprintf(name, sizeof(name), "smmu3.%pa", &ioaddr);
> 
> And this could be moved after ioaddr gets a copy from res->start.
> 
>>   
>> +#ifdef CONFIG_ARM_SMMU_V3_DEBUGFS
>> +struct arm_smmu_debugfs {
>> +	struct dentry			*smmu_dir;
> 
> A personal preference: for new structures, maybe drop those tabs?
> 
>>   /* An SMMUv3 instance */
>>   struct arm_smmu_device {
>>   	struct device			*dev;
>> @@ -803,6 +813,11 @@ struct arm_smmu_device {
>>   
>>   	struct rb_root			streams;
>>   	struct mutex			streams_mutex;
>> +
>> +#ifdef CONFIG_ARM_SMMU_V3_DEBUGFS
>> +	/* DebugFS Info */
> 
> Doesn't seem very useful. I'd drop it.
> 
>> +	struct arm_smmu_debugfs		*debugfs;
>> +#endif
>   
> Nicolin
> 
-- 
Thanks,
Qinxin