Re: [PATCH v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu
Hi Robin, On 2/2/2018 5:01 PM, Robin Murphy wrote: > On 02/02/18 05:40, Sricharan R wrote: >> Hi Robin/Vivek, >> >> On 2/1/2018 2:23 PM, Vivek Gautam wrote: >>> Hi, >>> >>> >>> On 1/31/2018 6:39 PM, Robin Murphy wrote: >>>> On 19/01/18 11:43, Vivek Gautam wrote: >>>>> From: Sricharan R <sricha...@codeaurora.org> >>>>> >>>>> Finally add the device link between the master device and >>>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>>> master needs it. This is done from add_device callback which gets >>>>> called once when the master is added to the smmu. >>>> >>>> Don't we need to balance this with a device_link_del() in .remove_device >>>> (like exynos-iommu does)? >>> >>> Right. Will add device_link_del() call. Thanks for pointing out. >> >> The reason for not adding device_link_del from .remove_device was, the >> core device_del >> which calls the .remove_device from notifier, calls device_links_purge >> before that. >> That does the same thing as device_link_del. So by the time .remove_device >> is called, >> device_links for that device is already cleaned up. Vivek, you may want to >> check once that >> calling device_link_del from .remove_device has no effect, just to confirm >> once more. > > There is at least one path in which .remove_device is not called via the > notifier from device_del(), which is in the cleanup path of iommu_bus_init(). > AFAICS any links created by .add_device during that process would be left > dangling, because the device(s) would be live but otherwise disassociated > from the IOMMU afterwards. > > From a maintenance perspective it's easier to have the call in its logical > place even if it does nothing 99% of the time; that way we shouldn't have to > keep an eye out for subtle changes in the power management code or driver > core that might invalidate the device_del() reasoning above, and the power > management guys shouldn't have to comprehend the internals of the IOMMU API > to make sense of the unbalanced call if they ever want to change their API. Ha, for a moment was thinking that with probe deferral add/remove_iommu_group in iommu_bus_init is dummy. But that may not be true for all Archs. Surely agree for the maintainability reason as well. Thanks. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 4/6] iommu/arm-smmu: Add the device_link between masters and smmu
Hi Robin/Vivek,
On 2/1/2018 2:23 PM, Vivek Gautam wrote:
> Hi,
>
>
> On 1/31/2018 6:39 PM, Robin Murphy wrote:
>> On 19/01/18 11:43, Vivek Gautam wrote:
>>> From: Sricharan R <sricha...@codeaurora.org>
>>>
>>> Finally add the device link between the master device and
>>> smmu, so that the smmu gets runtime enabled/disabled only when the
>>> master needs it. This is done from add_device callback which gets
>>> called once when the master is added to the smmu.
>>
>> Don't we need to balance this with a device_link_del() in .remove_device
>> (like exynos-iommu does)?
>
> Right. Will add device_link_del() call. Thanks for pointing out.
The reason for not adding device_link_del from .remove_device was, the core
device_del
which calls the .remove_device from notifier, calls device_links_purge before
that.
That does the same thing as device_link_del. So by the time .remove_device is
called,
device_links for that device is already cleaned up. Vivek, you may want to
check once that
calling device_link_del from .remove_device has no effect, just to confirm
once more.
Regards,
Sricharan
>
> regards
> Vivek
>
>>
>> Robin.
>>
>>> Signed-off-by: Sricharan R <sricha...@codeaurora.org>
>>> ---
>>> drivers/iommu/arm-smmu.c | 11 +++
>>> 1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index 95478bfb182c..33bbcfedb896 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev)
>>> struct arm_smmu_device *smmu;
>>> struct arm_smmu_master_cfg *cfg;
>>> struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>> + struct device_link *link;
>>> int i, ret;
>>> if (using_legacy_binding) {
>>> @@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device *dev)
>>> pm_runtime_put_sync(smmu->dev);
>>> + /*
>>> + * Establish the link between smmu and master, so that the
>>> + * smmu gets runtime enabled/disabled as per the master's
>>> + * needs.
>>> + */
>>> + link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>>> + if (!link)
>>> + dev_warn(smmu->dev, "Unable to create device link between %s and
>>> %s\n",
>>> + dev_name(smmu->dev), dev_name(dev));
>>> +
>>> return 0;
>>> out_cfg_free:
>>>
>
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
