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=-1.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 5B658C5ACAE for ; Thu, 12 Sep 2019 08:50:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2E7AF2084F for ; Thu, 12 Sep 2019 08:50:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1568278229; bh=XCfuILquDaYAwWjsyBacc2YJXnwSbKHCz8MTJJkUcUU=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=OFV3Au2H4wa3DG3L5t6muC7GFS+76LpMLSYQbKiiIrOEysNdmGv/Vgbn73SEbD2uX up1MpMLOObDMfoaQ/kaZirzvzqS9rZYLu5VCiwrK/MuBoU92evH8HsXLtZg8c+DSav xvR+iNwhteULdL2Ek1zgg5awKkYOBAnogT1oZ5hc= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730487AbfILIu2 convert rfc822-to-8bit (ORCPT ); Thu, 12 Sep 2019 04:50:28 -0400 Received: from foss.arm.com ([217.140.110.172]:59200 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726159AbfILIu2 (ORCPT ); Thu, 12 Sep 2019 04:50:28 -0400 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 266951000; Thu, 12 Sep 2019 01:50:27 -0700 (PDT) Received: from big-swifty.misterjones.org (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 04DE63F71F; Thu, 12 Sep 2019 01:50:23 -0700 (PDT) Date: Thu, 12 Sep 2019 09:50:22 +0100 Message-ID: <865zlxsxtd.wl-maz@kernel.org> From: Marc Zyngier To: "Shenhar, Talel" Cc: , , , , , , , , , , , , James Morse Subject: Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver In-Reply-To: <3205f7ae-5568-c064-23ac-ea726246173b@amazon.com> References: <1568142310-17622-1-git-send-email-talel@amazon.com> <1568142310-17622-3-git-send-email-talel@amazon.com> <86d0g6syva.wl-maz@kernel.org> <3205f7ae-5568-c064-23ac-ea726246173b@amazon.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 12 Sep 2019 07:50:03 +0100, "Shenhar, Talel" wrote: > > Hi Marc, > > > On 9/11/2019 5:15 PM, Marc Zyngier wrote: > > [+James] > > > > Hi Talel, > > > > On Tue, 10 Sep 2019 20:05:09 +0100, > > Talel Shenhar wrote: > > > >> + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); > > Do you actually need the implied barriers? I'd expect that relaxed > > accesses should be enough. > > You are correct. Barriers are not needed, In v1 this driver indeed > used _relaxed versions. > > Due to request coming from Arnd in v1 patch series I removed it. As > this is not data path I had no strong objection for removing it. Independently from whether this has any material impact on performance (this obviously isn't a hot path, unless it can be directly generated by userspace or a guest), I believe it is important to use the right type of accessor, if only because code gets copied around... Others would probably argue that this is the very reason why we should always use the "safe" option... > > > > >> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) > >> + return IRQ_NONE; > >> + > >> + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); > >> + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > >> + > >> + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); > >> + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); > >> + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); > >> + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); > >> + > >> + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", > >> + addr, request_id, bresp); > > What is this information? How do we make use of it? Given that this is > > asynchronous, how do we correlate it to the offending software? > > Indeed this information arriving from the HW is asynchronous. > > There is no direct method to get the offending software. > > There are all kinds of hacks we do to find the offending software once > we find this error. most of the time its a new patch introduced but > some of the time is just digging. OK, so that the moment, this is more of a debug tool than anything else, right? > > The whole think looks to me like a poor man's EDAC handling, and I'd > > expect to be plugged in that subsystem instead. Any reason why this > > isn't the case? It would certainly make the handling uniform for the > > user. > > This logic was not plugged into EDAC as there is no "Correctable" > error here. its just error event. Not all errors are EDAC in the sense > of Error Detection And *Correction*. There are no correctable errors > for this driver. I'd argue the opposite! Because you obviously don't let a read-only register being written to, the error has been corrected, and you signal the correction status. > So plugging it  under EDAC seems like abusing the EDAC system. > > Now that I've emphasize the reason for not putting this under EDAC, > what do you think? should this "only uncorrectable event" driver > should be part of EDAC? My choice would be to plug it into the EDAC subsystem, and report all interrupts as "Corrected" events. Optionally, and only if you are debugging something that requires it, report the error as "Uncorrectable", in which case the EDAC subsystem should trigger a panic. At least you'd get the infrastructure, logging and tooling that the EDAC subsystem offers (parsing the kernel log doesn't really count). Thanks, M. -- Jazz is not dead, it just smells funny. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver Date: Thu, 12 Sep 2019 09:50:22 +0100 Message-ID: <865zlxsxtd.wl-maz@kernel.org> References: <1568142310-17622-1-git-send-email-talel@amazon.com> <1568142310-17622-3-git-send-email-talel@amazon.com> <86d0g6syva.wl-maz@kernel.org> <3205f7ae-5568-c064-23ac-ea726246173b@amazon.com> Mime-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <3205f7ae-5568-c064-23ac-ea726246173b@amazon.com> Sender: linux-kernel-owner@vger.kernel.org To: "Shenhar, Talel" Cc: robh+dt@kernel.org, tglx@linutronix.de, jason@lakedaemon.net, mark.rutland@arm.com, nicolas.ferre@microchip.com, mchehab+samsung@kernel.org, shawn.lin@rock-chips.com, gregkh@linuxfoundation.org, dwmw@amazon.co.uk, benh@kernel.crashing.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, James Morse List-Id: devicetree@vger.kernel.org On Thu, 12 Sep 2019 07:50:03 +0100, "Shenhar, Talel" wrote: > > Hi Marc, > > > On 9/11/2019 5:15 PM, Marc Zyngier wrote: > > [+James] > > > > Hi Talel, > > > > On Tue, 10 Sep 2019 20:05:09 +0100, > > Talel Shenhar wrote: > > > >> + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); > > Do you actually need the implied barriers? I'd expect that relaxed > > accesses should be enough. > > You are correct. Barriers are not needed, In v1 this driver indeed > used _relaxed versions. > > Due to request coming from Arnd in v1 patch series I removed it. As > this is not data path I had no strong objection for removing it. Independently from whether this has any material impact on performance (this obviously isn't a hot path, unless it can be directly generated by userspace or a guest), I believe it is important to use the right type of accessor, if only because code gets copied around... Others would probably argue that this is the very reason why we should always use the "safe" option... > > > > >> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) > >> + return IRQ_NONE; > >> + > >> + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); > >> + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > >> + > >> + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); > >> + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); > >> + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); > >> + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); > >> + > >> + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", > >> + addr, request_id, bresp); > > What is this information? How do we make use of it? Given that this is > > asynchronous, how do we correlate it to the offending software? > > Indeed this information arriving from the HW is asynchronous. > > There is no direct method to get the offending software. > > There are all kinds of hacks we do to find the offending software once > we find this error. most of the time its a new patch introduced but > some of the time is just digging. OK, so that the moment, this is more of a debug tool than anything else, right? > > The whole think looks to me like a poor man's EDAC handling, and I'd > > expect to be plugged in that subsystem instead. Any reason why this > > isn't the case? It would certainly make the handling uniform for the > > user. > > This logic was not plugged into EDAC as there is no "Correctable" > error here. its just error event. Not all errors are EDAC in the sense > of Error Detection And *Correction*. There are no correctable errors > for this driver. I'd argue the opposite! Because you obviously don't let a read-only register being written to, the error has been corrected, and you signal the correction status. > So plugging it  under EDAC seems like abusing the EDAC system. > > Now that I've emphasize the reason for not putting this under EDAC, > what do you think? should this "only uncorrectable event" driver > should be part of EDAC? My choice would be to plug it into the EDAC subsystem, and report all interrupts as "Corrected" events. Optionally, and only if you are debugging something that requires it, report the error as "Uncorrectable", in which case the EDAC subsystem should trigger a panic. At least you'd get the infrastructure, logging and tooling that the EDAC subsystem offers (parsing the kernel log doesn't really count). Thanks, M. -- Jazz is not dead, it just smells funny.