Re: [PATCH 1/1] iommu/arm-smmu: Add SID information to context fault log

[Vivek Gautam <vivek.gautam@codeaurora.org>]

Hi Marc,


On 4/15/2019 2:24 PM, Marc Gonzalez wrote:
> Trimming CC list for very minor suggestions (feel free to ignore)

Thanks for the review. Please find my responses inline below.

>
> On 15/04/2019 10:07, Vivek Gautam wrote:
>
>> Extract the SID and add the information to context fault log.
>> This is specially useful in a distributed smmu architecture
>> where multiple masters are connected to smmu. SID information
>> helps to quickly identify the faulting master device.
> I find it slightly clearer to keep acronyms capitalized, such as SMMU.

Sure, will change it.
>
>
>> @@ -575,7 +575,10 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>   	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
>> +	void __iomem *gr1_base = ARM_SMMU_GR1(smmu);
>>   	void __iomem *cb_base;
>> +	u32 cbfrsynra;
>> +	u16 sid;
>>   
>>   	cb_base = ARM_SMMU_CB(smmu, cfg->cbndx);
>>   	fsr = readl_relaxed(cb_base + ARM_SMMU_CB_FSR);
>> @@ -586,9 +589,16 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>>   	fsynr = readl_relaxed(cb_base + ARM_SMMU_CB_FSYNR0);
>>   	iova = readq_relaxed(cb_base + ARM_SMMU_CB_FAR);
>>   
>> +	cbfrsynra = readl_relaxed(gr1_base +
>> +				  ARM_SMMU_GR1_CBFRSYNRA(cfg->cbndx));
> I find it slightly clearer to write
>
> 	void __iomem *reg;
> 	...
> 	reg = ARM_SMMU_GR1(smmu) + ARM_SMMU_GR1_CBFRSYNRA(cfg->cbndx);
> 	cbfrsynra = readl_relaxed(reg);

I would actually argue here, to go with the flow of how this function 
declares the variables, for the simple case of symmetry.
Let me know what do you think.

Thanks & regards
Vivek


>
> Regards.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel