Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Robin Murphy]

On 10/02/17 15:33, Joerg Roedel wrote:
> On Fri, Feb 10, 2017 at 02:35:39PM +0000, Robin Murphy wrote:
>> On 09/02/17 11:32, Joerg Roedel wrote:
>>> +	ret = iommu_device_sysfs_add(&iommu->iommu, iommu->dev, NULL,
>>> +				     "msm-smmu.%pa", &ioaddr);
>>> +	if (ret) {
>>> +		pr_err("Could not add msm-smmu at %pa to sysfs\n", &ioaddr);
>>> +		goto fail;
>>> +	}
>>
>> Nit: there's a bit of inconsistency with printing errors between the
>> various drivers (for both _sysfs_add and _register). I reckon if we want
>> error messages we may as well just fold them into the helper functions.
> 
> Yeah, this could be unified too. For now I looked how verbose the
> driver was that I was going to change and added messages to be
> consistent inside the drivers.
> 
>>
>>> +
>>> +	iommu_device_set_ops(&iommu->iommu, &msm_iommu_ops);
>>> +	iommu_device_set_fwnode(&iommu->iommu, &pdev->dev.of_node->fwnode);
>>> +
>>> +	ret = iommu_device_register(&iommu->iommu);
>>> +	if (ret) {
>>> +		pr_err("Could not register msm-smmu at %pa\n", &ioaddr);
>>> +		goto fail;
>>> +	}
>>
>> I think there's a corresponding unregister missing for
>> msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
>> back). I know it's not strictly a problem at the moment, but I do now
>> have IOMMU-drivers-as-modules working on top of the probe deferral
>> series... ;)
> 
> Well, that there was an iommu_register_instance() without any
> unregistration interface at all makes me believe that unregistering
> iommus is not really implemented yet.
> 
> And in fact, the remove functions for msm and arm-smmu seem to only
> disable the hardware, but are not removing the corresponding data
> structures.

For the ARM SMMUs at least, the SMMU-specific data is (well, should be)
all devm_* managed, thus freed automatically by the driver core after
remove() returns. It is true that there's an implicit expectation that
the SMMU won't be removed until all domains, groups and masters have
been explicitly torn down by the relevant detach()/remove()/free()
calls, although I guess the sysfs links might actually help enforce that.

> So I think we are fine from that side.

Sure, I mostly just wanted not to lose sight of the future possibility
of unloadable IOMMU drivers (admittedly I've not even tried that yet,
only loading them post-boot).

Robin.

> 
> 
> 	Joerg
>