Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v9 4/5] iommu/arm-smmu: Add the device_link between
 masters and smmu
To:     Tomasz Figa <tfiga@chromium.org>
Cc:     Vivek Gautam <vivek.gautam@codeaurora.org>,
        Joerg Roedel <joro@8bytes.org>,
        Rob Herring <robh+dt@kernel.org>,
        "open list:IOMMU DRIVERS" <iommu@lists.linux-foundation.org>,
        devicetree@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Will Deacon <will.deacon@arm.com>,
        Rob Clark <robdclark@gmail.com>,
        Sricharan R <sricharan@codeaurora.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Archit Taneja <architt@codeaurora.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>
References: <20180313085534.11650-1-vivek.gautam@codeaurora.org>
 <20180313085534.11650-5-vivek.gautam@codeaurora.org>
 <8b427ea2-5c13-4712-13d1-e4c1aed0779e@arm.com>
 <CAAFQd5AqERQMLsJNLAsVXox79kZ+ZtpCUuMSutn70FGK-3Q7vw@mail.gmail.com>
From:   Robin Murphy <robin.murphy@arm.com>
Message-ID: <2cab6e92-c44c-edeb-bf50-946cec5cccac@arm.com>
Date:   Thu, 15 Mar 2018 10:44:32 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <CAAFQd5AqERQMLsJNLAsVXox79kZ+ZtpCUuMSutn70FGK-3Q7vw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 15/03/18 06:18, Tomasz Figa wrote:
> On Thu, Mar 15, 2018 at 2:50 AM, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 13/03/18 08:55, Vivek Gautam wrote:
>>>
>>> From: Sricharan R <sricharan@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.
>>>
>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
>>> ---
>>>    drivers/iommu/arm-smmu.c | 29 +++++++++++++++++++++++++++++
>>>    1 file changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index 56a04ae80bf3..64953ff2281f 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -1460,10 +1460,31 @@ static int arm_smmu_add_device(struct device *dev)
>>>          iommu_device_link(&smmu->iommu, dev);
>>>    +     if (pm_runtime_enabled(smmu->dev)) {
>>> +               struct device_link *link;
>>> +
>>> +               /*
>>> +                * 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) {
>>
>>
>> FWIW, given that we don't really care about link itself, I'd be quite happy
>> to simplify that lot down to:
>>
>>          if (pm_runtime_enabled(smmu_dev) &&
>>              !device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME)) {
>>
>>> +                       dev_warn(smmu->dev,
>>> +                                "Unable to add link to the consumer
>>> %s\n",
>>> +                                dev_name(dev));
>>
>>
>> (side note: since device_link_add() already prints a message on success,
>> maybe it could print its own failure message too?)
> 
> I think we care whether adding the link succeeded. If it fails to be
> added, we might end up with a complete system lockup on a system with
> power domains.

Well, yeah, that was implicit - the point is that we *only* care about 
whether it succeeded or not. Thus we may as well just check for NULL 
directly instead of assigning the value as if we were actually going to 
do anything with it.

Robin.