From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH v9 4/5] iommu/arm-smmu: Add the device_link between masters and smmu Date: Thu, 15 Mar 2018 15:18:08 +0900 Message-ID: References: <20180313085534.11650-1-vivek.gautam@codeaurora.org> <20180313085534.11650-5-vivek.gautam@codeaurora.org> <8b427ea2-5c13-4712-13d1-e4c1aed0779e@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <8b427ea2-5c13-4712-13d1-e4c1aed0779e-5wv7dgnIgG8@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Robin Murphy Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-msm , Will Deacon , Linux Kernel Mailing List , "open list:IOMMU DRIVERS" , Rob Herring List-Id: devicetree@vger.kernel.org On Thu, Mar 15, 2018 at 2:50 AM, Robin Murphy wrote: > On 13/03/18 08:55, Vivek Gautam wrote: >> >> From: Sricharan R >> >> 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 >> Signed-off-by: Vivek Gautam >> Reviewed-by: Tomasz Figa >> --- >> 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. Best regards, Tomasz