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=-4.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,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 9C1E0C432BE for ; Fri, 27 Aug 2021 16:27:48 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6454E60E99 for ; Fri, 27 Aug 2021 16:27:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 6454E60E99 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=nHq1HyIWHlySu70cYRFi8k12vL/nLnwBdtBRMRBXhR0=; b=38XcW/kwMp7t4f jN36RGdsROGv+o5va0BRxXuRAdInHP7YS0y8TI0OFzMB+6k8feI0KN+NhIVVtFWy6E02I1ojFA6KB VmMH5MFXvhA4q+pQzLu5yNriFbX7KRA1vmNBqed/+6NGuWp4svOlN+RKdcl/6lppd/LvfVpKYnjAw imggK0/nPzdr5ALp7te2WPOROO21hKFyIO9FZeZkhRgEn/7o9mnPzIEXxOL2kP4lNoQkNYEhYVAEs 7XuUNl6M2j35NPUq51s/VgaENJBTs+oZ+G0C0fBAaK/YGsM3TAMF+3MbCcn5GEOKOjoZCq3JW8SAc Mut5xIht528aQGiUZBpQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mJefC-00Ck6U-NF; Fri, 27 Aug 2021 16:25:06 +0000 Received: from mail-pl1-x62f.google.com ([2607:f8b0:4864:20::62f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mJef8-00Ck4W-DO for linux-arm-kernel@lists.infradead.org; Fri, 27 Aug 2021 16:25:04 +0000 Received: by mail-pl1-x62f.google.com with SMTP id m4so4287694pll.0 for ; Fri, 27 Aug 2021 09:25:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=olUZYI52FUW5gVkrVAMR5duictYeHKMMzQr6nXakedQ=; b=aNaHRKsiD+SM+Lu9zvZtUAsPhN72aJZEO5ggEUbd+9i2E2zx+tYCvMVsH57GRkwHdf TEaqxgEcVqpk2OQ9H7VyX1+9BLBU1szPxLHX49NuTsQbgdwMV28fkCyIXIQW5Xd+QD3R WQ8/mhACDBZ1RuZyFTGtQQEZWkuERfp+C3Hd1pWoNSnZkvtdCCCdX43x97hoWme9ZdEO 45oFzDbvZXtq0iWx7LSdO55FY2oYiiyByKOWYg3IoUUbV30sBk3aaucbioyl2Cd8PO6k bo+KZEykhRVe2BTh/XS6P2KQyoHNyAxEkUHVXaf1/Vmq1vtUC/5rzUkbwFKRZBjsF7bZ 26nA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=olUZYI52FUW5gVkrVAMR5duictYeHKMMzQr6nXakedQ=; b=K1o6Lb1++wN134YBeJYWnLViqPGOf2/BFZrYGubST7T5ouvUFPW3mbcDoahANFhUZt NEgQX0H/fTfiTpFjd1XjVe6ms4zBDMappBHsWuYEL0sP3YQIQc4SEmhh6wcLv1C1jeJV KA2m08gYm/aHUN7XHi3eWxhD7tZyHJe7bGObCJZDAhkSJtqgonPgOUSPeQJoRRxUZPV2 vs/2H4ef3FRIlSyphGBNGR8pza5e+kOgFOMyJIw+LfhL7PWoZ7fiUWgiFtBLEufnfaHU aoPAj7JAQUPIfqRHtnbiUM80wU+swvVom4KQue76SX0vw1idB1cgc5yN6191C408/LbW +Swg== X-Gm-Message-State: AOAM530FU9b9FTkV0jL0UfnbjyxbHMKRGmpC6PkTzgCwpc+ola3VGRrN gd61nW2q9lvDQi5R/LUBAjTbvP/lIuil6tb3tZoFeg== X-Google-Smtp-Source: ABdhPJy9Fys2MKe+4NKONrWKg51ffmdsBaOmd1A6Uev8PSTHXgB2mLpKO7Fg2ktC2vZ1DnSMGf/U9PYBzR/IUsq76cs= X-Received: by 2002:a17:90a:708c:: with SMTP id g12mr24462608pjk.13.1630081500678; Fri, 27 Aug 2021 09:25:00 -0700 (PDT) MIME-Version: 1.0 References: <20210803113134.2262882-1-iwona.winiarska@intel.com> <20210803113134.2262882-8-iwona.winiarska@intel.com> In-Reply-To: From: Dan Williams Date: Fri, 27 Aug 2021 09:24:49 -0700 Message-ID: Subject: Re: [PATCH v2 07/15] peci: Add peci-aspeed controller driver To: "Winiarska, Iwona" Cc: "corbet@lwn.net" , "jae.hyun.yoo@linux.intel.com" , "d.mueller@elsoft.ch" , "linux-hwmon@vger.kernel.org" , "andrew@aj.id.au" , "Luck, Tony" , "Lutomirski, Andy" , "andriy.shevchenko@linux.intel.com" , "mchehab@kernel.org" , "jdelvare@suse.com" , "linux-kernel@vger.kernel.org" , "olof@lixom.net" , "mingo@redhat.com" , "rdunlap@infradead.org" , "devicetree@vger.kernel.org" , "tglx@linutronix.de" , "linux-aspeed@lists.ozlabs.org" , "arnd@arndb.de" , "linux-doc@vger.kernel.org" , "linux@roeck-us.net" , "zweiss@equinix.com" , "robh+dt@kernel.org" , "openbmc@lists.ozlabs.org" , "gregkh@linuxfoundation.org" , "joel@jms.id.au" , "yazen.ghannam@amd.com" , "linux-arm-kernel@lists.infradead.org" , "pierre-louis.bossart@linux.intel.com" , "x86@kernel.org" , "bp@alien8.de" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210827_092502_610646_AA51A37A X-CRM114-Status: GOOD ( 45.42 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Aug 26, 2021 at 4:55 PM Winiarska, Iwona wrote: > > On Wed, 2021-08-25 at 18:35 -0700, Dan Williams wrote: > > On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska > > wrote: > > > > > > From: Jae Hyun Yoo > > > > > > ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical > > > interface (a.k.a PECI wire). > > > > Maybe a one sentence blurb here and in the Kconfig reminding people > > why they should care if they have a PECI driver or not? > > Ok, I'll expand it a bit. [..] > > > +static int aspeed_peci_xfer(struct peci_controller *controller, > > > + u8 addr, struct peci_request *req) > > > +{ > > > + struct aspeed_peci *priv = dev_get_drvdata(controller->dev.parent); > > > + unsigned long flags, timeout = msecs_to_jiffies(priv- > > > >cmd_timeout_ms); > > > + u32 peci_head; > > > + int ret; > > > + > > > + if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX || > > > + req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX) > > > + return -EINVAL; > > > + > > > + /* Check command sts and bus idle state */ > > > + ret = aspeed_peci_check_idle(priv); > > > + if (ret) > > > + return ret; /* -ETIMEDOUT */ > > > + > > > + spin_lock_irqsave(&priv->lock, flags); > > > + reinit_completion(&priv->xfer_complete); > > > + > > > + peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) | > > > + FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) | > > > + FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len); > > > + > > > + writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH); > > > + > > > + memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf, > > > min_t(u8, req->tx.len, 16)); > > > + if (req->tx.len > 16) > > > + memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req->tx.buf + > > > 16, > > > + req->tx.len - 16); > > > + > > > + dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head); > > > + print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req- > > > >tx.len); > > > > On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of > > reading through this buffer, but skip emitting it. Are you sure you > > want to pay that overhead for every transaction? > > I can remove it or I can add something like: > > #if IS_ENABLED(CONFIG_PECI_DEBUG) > #define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__) > #else > #define peci_debug(...) do { } while (0) > #endif It's the hex dump I'm worried about, not the debug statements as much. I think the choices are remove the print_hex_dump_bytes(), put it behind an IS_ENABLED(CONFIG_DYNAMIC_DEBUG) to ensure the overhead is skipped in the CONFIG_DYNAMIC_DEBUG=n case, or live with the overhead if this is not a fast path / infrequently used. > > (and similar peci_trace with trace_printk for usage in IRQ handlers and such). > > What do you think? In general, no, don't wrap the base level print routines with driver-specific ones. Also, trace_printk() is only for debug builds. Note that trace points are built to be even less overhead than dev_dbg(), so there's no overhead concern with disabled tracepoints, they literally translate to nops when disabled. > > > > > > + > > > + priv->status = 0; > > > + writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD); > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + > > > + ret = wait_for_completion_interruptible_timeout(&priv- > > > >xfer_complete, timeout); > > > > spin_lock_irqsave() says "I don't know if interrupts are disabled > > already, so I'll save the state, whatever it is, and restore later" > > > > wait_for_completion_interruptible_timeout() says "I know I am in a > > sleepable context where interrupts are enabled" > > > > So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()? > > You're right - I'll fix it. > > > > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (ret == 0) { > > > + dev_dbg(priv->dev, "Timeout waiting for a response!\n"); > > > + return -ETIMEDOUT; > > > + } > > > + > > > + spin_lock_irqsave(&priv->lock, flags); > > > + > > > + writel(0, priv->base + ASPEED_PECI_CMD); > > > + > > > + if (priv->status != ASPEED_PECI_INT_CMD_DONE) { > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + dev_dbg(priv->dev, "No valid response!\n"); > > > + return -EIO; > > > + } > > > + > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + > > > + memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0, > > > min_t(u8, req->rx.len, 16)); > > > + if (req->rx.len > 16) > > > + memcpy_fromio(req->rx.buf + 16, priv->base + > > > ASPEED_PECI_RD_DATA4, > > > + req->rx.len - 16); > > > + > > > + print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req- > > > >rx.len); > > > + > > > + return 0; > > > +} > > > + > > > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg) > > > +{ > > > + struct aspeed_peci *priv = arg; > > > + u32 status; > > > + > > > + spin_lock(&priv->lock); > > > + status = readl(priv->base + ASPEED_PECI_INT_STS); > > > + writel(status, priv->base + ASPEED_PECI_INT_STS); > > > + priv->status |= (status & ASPEED_PECI_INT_MASK); > > > + > > > + /* > > > + * In most cases, interrupt bits will be set one by one but also > > > note > > > + * that multiple interrupt bits could be set at the same time. > > > + */ > > > + if (status & ASPEED_PECI_INT_BUS_TIMEOUT) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_BUS_TIMEOUT\n"); > > > + > > > + if (status & ASPEED_PECI_INT_BUS_CONTENTION) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_BUS_CONTENTION\n"); > > > + > > > + if (status & ASPEED_PECI_INT_WR_FCS_BAD) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_WR_FCS_BAD\n"); > > > + > > > + if (status & ASPEED_PECI_INT_WR_FCS_ABORT) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_WR_FCS_ABORT\n"); > > > > Are you sure these would not be better as tracepoints? If you're > > debugging an interrupt related failure, the ratelimiting might get in > > your way when you really need to know when one of these error > > interrupts fire relative to another event. > > Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ status > would probably be too much. Tracepoints become ABI once someone ships tooling that depends on them being there. These don't look attractive for a tool, and they don't look difficult to maintain if the interrupt handler needs to be reworked. I.e. it would be trivial to keep a dead tracepoint around if worse came to worse to keep a tool from failing to load. > I was thinking about something like trace_printk hidden under a > "CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future > improvement? Again trace_printk() is only for private builds. > > > > > > + > > > + /* > > > + * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE > > > bit > > > + * set even in an error case. > > > + */ > > > + if (status & ASPEED_PECI_INT_CMD_DONE) > > > + complete(&priv->xfer_complete); > > > > Hmm, no need to check if there was a sequencing error, like a command > > was never submitted? > > It's handled by checking if HW is idle in xfer before a command is sent, where > we just expect a single interrupt per command. I'm asking how do you determine if this status was spurious, or there was a sequencing error in the driver? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel