From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 552DFC10F0B for ; Wed, 27 Jan 2021 00:35:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 26DAB2054F for ; Wed, 27 Jan 2021 00:35:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404356AbhA0AcE (ORCPT ); Tue, 26 Jan 2021 19:32:04 -0500 Received: from foss.arm.com ([217.140.110.172]:49436 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2387609AbhAZRZj (ORCPT ); Tue, 26 Jan 2021 12:25:39 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 53A0331B; Tue, 26 Jan 2021 09:24:52 -0800 (PST) Received: from [10.57.43.46] (unknown [10.57.43.46]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 207543F66E; Tue, 26 Jan 2021 09:24:51 -0800 (PST) Subject: Re: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions To: Shameerali Kolothum Thodi Cc: "linux-kernel@vger.kernel.org" , "iommu@lists.linux-foundation.org" , "jean-philippe@linaro.org" , "will@kernel.org" , "linuxarm@openeuler.org" , "Zengtao (B)" References: <20210126130629.8928-1-shameerali.kolothum.thodi@huawei.com> <20210126135039.000039a0@arm.com> <8654e506fa26443f8f4413ec8fd96bf7@huawei.com> From: Robin Murphy Message-ID: <5828b2f9-e1d3-fd96-ebf3-2a38c903c9c3@arm.com> Date: Tue, 26 Jan 2021 17:24:45 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: <8654e506fa26443f8f4413ec8fd96bf7@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2021-01-26 16:40, Shameerali Kolothum Thodi wrote: > Hi Robin, > >> -----Original Message----- >> From: Robin Murphy [mailto:robin.murphy@arm.com] >> Sent: 26 January 2021 13:51 >> To: Shameerali Kolothum Thodi >> Cc: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org; >> jean-philippe@linaro.org; will@kernel.org; linuxarm@openeuler.org; Zengtao >> (B) >> Subject: Re: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions >> >> On Tue, 26 Jan 2021 13:06:29 +0000 >> Shameer Kolothum wrote: >> >>> The device iommu probe/attach might have failed leaving dev->iommu to >>> NULL and device drivers may still invoke these functions resulting a >>> crash in iommu vendor driver code. Hence make sure we check that. >>> >>> Signed-off-by: Shameer Kolothum >>> --- >>> drivers/iommu/iommu.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index >>> ffeebda8d6de..cb68153c5cc0 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -2867,7 +2867,7 @@ bool iommu_dev_has_feature(struct device *dev, >>> enum iommu_dev_features feat) { >>> const struct iommu_ops *ops = dev->bus->iommu_ops; >>> >>> - if (ops && ops->dev_has_feat) >>> + if (dev->iommu && ops && ops->dev_has_feat) >>> return ops->dev_has_feat(dev, feat); >> >> Might make sense to make these more self-contained, e.g.: >> >> if (dev->iommu && dev->iommu->ops->foo) >> dev->iommu->ops->foo() > > Right. Does that mean adding ops to "struct dev_iommu" or retrieve ops like > below, > > if (dev->iommu && dev->iommu->iommu_dev->ops->foo) > dev->iommu->iommu_dev->ops->foo() > > Sorry, not clear to me. Bleh, I was thinking that dev->iommu pointed directly to a struct iommu_device there, sorry. There are too many things and not enough distinct names for the things. But yeah, basically that if the device's "I am associated with an IOMMU" data is set, then by construction it must lead to a set of ops which are definitely valid. Conceptually it's cleaner than combining two different data sources (the per-device info plus the bus ops which may or may not be relevant to a given device), even if cosmetically we have to juggle through practically every possible permutation of the names "iommu" and "device" to get there :/ Robin.