From patchwork Tue Mar 7 18:41:33 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Robin Murphy X-Patchwork-Id: 736326 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vd59W2VfYz9rvt for ; Wed, 8 Mar 2017 05:42:07 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="THDhpcxD"; dkim-atps=neutral DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=4Kf6PSlYxxTMX3vaLPpPbvaxKTWdGRg7I07AaUJUvOA=; b=THDhpcxDmTdQt+ pYeVWpCHssL4VcJa8r2mbZD5E0tnpOe1EpLHkUWQd3sUY0V2CrtFrWVrAF393wP5LgnqMHRWwA9eW OFLGF7rUspK+Nr11+oAdQN5wloTFx4YJuJr2+64jZJWHLTEM4PlmnH8eV6bZqgfZSOiGI9kQEdaJS JAgfp/sWz854m6lf3I+h4Eo9M61SwpKgKjAf2D1Fxu+wDP/vLxvCbFqGNGQGiUgIWbL8slhgtkh8X WAeL6gaHORzSPlC4RgAugcBNjoks+fGF9lgv+qcsO+ptIIEDe6DRSajfuxdBz3nD4MEeexWDbx827 IobBIvJWiQ55E2G58ZkQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1clK3h-0008KP-0u; Tue, 07 Mar 2017 18:42:05 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1clK3b-00087i-2B for linux-arm-kernel@lists.infradead.org; Tue, 07 Mar 2017 18:42:02 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 81E70687; Tue, 7 Mar 2017 10:41:36 -0800 (PST) Received: from [10.1.210.40] (e110467-lin.cambridge.arm.com [10.1.210.40]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 505AF3F220; Tue, 7 Mar 2017 10:41:35 -0800 (PST) Subject: Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg To: Robert Richter References: <20170306135833.21455-1-rrichter@cavium.com> <20170307140607.GY16822@rric.localdomain> From: Robin Murphy Message-ID: Date: Tue, 7 Mar 2017 18:41:33 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170307140607.GY16822@rric.localdomain> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170307_104159_184862_66A044ED X-CRM114-Status: GOOD ( 28.04 ) X-Spam-Score: -6.9 (------) X-Spam-Report: SpamAssassin version 3.4.1 on bombadil.infradead.org summary: Content analysis details: (-6.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [217.140.101.70 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Joerg Roedel , Will Deacon , iommu@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org On 07/03/17 14:06, Robert Richter wrote: > On 06.03.17 18:22:08, Robin Murphy wrote: >> On 06/03/17 13:58, Robert Richter wrote: >>> The ARM SMMU detection especially depends from system firmware. For >>> better diagnostic, log the detected type in dmesg. >> >> This paragraph especially depends from grammar. I think. > > Thanks for the mail on you. :) > >> >>> The smmu type's name is now stored in struct arm_smmu_type and ACPI >>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA() >>> macro to ARM_SMMU_TYPE() for better readability. >>> >>> Signed-off-by: Robert Richter >>> --- >>> drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------ >>> 1 file changed, 30 insertions(+), 31 deletions(-) >> >> That seems a relatively invasive diffstat for the sake of printing a >> string once at boot time to what I can only assume is a small audience >> of firmware developers who find "cat >> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI >> equivalent) too hard ;) > > Reading firmware data is not really a solution as you don't know what > the driver is doing with it. The actual background of this patch is to > be sure a certain workaround was enabled in the kernel. ARM's cpu > errata framework does this nicely. In case of smmus we just have the > internal model implementation type which is not visible in the logs. > Right now, there is no way to figure that out without knowing fw > specifics and kernel sources. Ah, now it starts to become clear. In that case, if we want to confirm the presence of specific workarounds, we should actually _confirm the presence of specific workarounds_. I'd have no complaint with e.g. this: -----8<----- /* ID2 */ ----->8----- and the equivalent for other things, if need be. If you just print "hey, this is SMMU-x", the user is in fact no better off, since they would then still have to go and look at the source for whatever kernel they're running to find out which particular workarounds for SMMU-x bugs that particular kernel implements. Robin. > The change is big but most of it is a reasonable rework anyway. I > didn't want to split that into a series of patches. But I could do > that. > >> Assuming there is a really good reason somewhere to justify this, I >> still wonder if a simple self-contained "smmu->model to string" function >> wouldn't do, if we really want to do this? Maybe it's not quite that >> simple if the generic case needs to key off smmu->version as well, but >> still. Arguably, just searching the of_match_table by model/version and >> printing the corresponding DT compatible would do the job (given an >> MMU-400 model to disambiguate those). > > Whatever you prefer. To me a dynamic search makes things more complex > for no benefit. And providing DT names in an ACPI context is confusing > either. > >> Either way it ought to be replacing the "SMMUv%d with:" message in >> arm_smmu_device_cfg_probe() - this driver is noisy enough already >> without starting to repeat itself. >> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index abf6496843a6..5c793b3d3173 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -366,6 +366,7 @@ struct arm_smmu_device { >>> u32 options; >>> enum arm_smmu_arch_version version; >>> enum arm_smmu_implementation model; >>> + const char *name; >> >> If we are going to add a pointer to static implementation data, it may >> as well be a "const arm_smmu_type *type" pointer to subsume version and >> model as well. > > The name still may change but not the particular string. Both work for > me. > >> >>> >>> u32 num_context_banks; >>> u32 num_s2_context_banks; >>> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) >>> return 0; >>> } >>> >>> -struct arm_smmu_match_data { >>> +struct arm_smmu_type { >>> enum arm_smmu_arch_version version; >>> enum arm_smmu_implementation model; >>> + const char *name; >>> }; >>> >>> -#define ARM_SMMU_MATCH_DATA(name, ver, imp) \ >>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp } >>> +#define ARM_SMMU_TYPE(var, ver, imp, _name) \ >>> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name } >>> >>> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU); >>> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU); >>> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU); >>> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500); >>> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2); >>> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1"); >>> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2"); >>> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401"); >> >> Strictly, I think you mean "ARM® CoreLink™ MMU-401". Printing the name >> of a driver-internal structure looks like someone left a debugging hack in. >> >>> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500"); >> >> And similarly. Of course, I'm not actually suggesting that using the >> full marketing names of things is a great idea, but then again if we do >> go naming specific IPs, and anyone gets sniffy about using the names >> "properly", then guess what's going to get removed again? You'll always >> find me firmly in the "easier not to go there" camp. > > A change of naming is unrelated to this patch. I leave this up to you. > > Thank you for review. > > -Robert > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index f7411109670f..9e50a092632c 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) atomic_add_return(smmu->num_context_banks, &cavium_smmu_context_count); smmu->cavium_id_base -= smmu->num_context_banks; + dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n", + smmu->cavium_id_base); }