Re: [PATCH 1/7] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Robin Murphy <robin.murphy@arm.com>]

On 2023-10-06 13:30, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 01:05:03PM +0100, Robin Murphy wrote:
>> On 2023-10-05 19:28, Jason Gunthorpe wrote:
>>> Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
>>> s2cr configuration, instead pass it in as a parameter. It always returns
>>> zero so make it return void.
>>
>> It doesn't follow that a function named arm_smmu_domain_<operation>() should
>> not operate on an arm_smmu_domain... I think this is the point to rename it
>> to something like arm_smmu_master_install_s2crs() to reflect that what it's
>> actually doing by now is a lot less than it did 10 years ago.
> 
> heh, yes, I did not notice
>   
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> index d6d1a2a55cc069..7f33363719f4ac 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> @@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
>>>    	mutex_unlock(&smmu->stream_map_mutex);
>>>    }
>>> -static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>>> -				      struct arm_smmu_master_cfg *cfg,
>>> -				      struct iommu_fwspec *fwspec)
>>> +static void arm_smmu_domain_add_master(struct arm_smmu_device *smmu,
>>
>> We already have the SMMU device in cfg->smmu, no need to pass it twice.
> 
> Sure, but the caller also already has it on its stack..

Stack? Caller? Not in my build ;)

00000000000040b8 <arm_smmu_attach_dev>:
...
         smmu = cfg->smmu;
     410c:       f94002d3        ldr     x19, [x22]
...
         smmu_domain->smmu = smmu;
     4268:       f90002f3        str     x19, [x23]
...
         struct arm_smmu_s2cr *s2cr = smmu->s2crs;
     44e0:       f9403a79        ldr     x25, [x19, #112]
...
         ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
     4544:       5280001c        mov     w28, #0x0
...

Even if the compiler didn't inline the heck out of an easy inlining 
candidate, this is hardly a critical hot path where an extra load would 
make any difference, so the only thing this code needs optimising for is 
readability, or Donald Knuth will be sad :(

Thanks,
Robin.