From patchwork Thu Feb 22 18:12:46 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Serge Semin X-Patchwork-Id: 13567988 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org 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 smtp.lore.kernel.org (Postfix) with ESMTPS id BCEB9C54791 for ; Thu, 22 Feb 2024 19:16:28 +0000 (UTC) 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:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=kd8vFMLobnR/HvemO16aJVyaNvcxoYlVVsXL1O73wfk=; b=bcMRfIjv/MoGuA ui63FWMWnwJhGtOcGt1mDpw08yWxD597HI4ZeXTzsVLrS10PgGvOBr6SU9QkAYlmg1q3VnBPQGDwL gG4WjPhAOtKU3RhChy5+jlbErNkW8OvNbCNpRkHep7ZjGickSDxlTyK/HKwn+cL2c7K9eGRMr5WTg l2Oim7M/rtlKTpxiFxKltdV17t2RhY+2F0iqHwo0a19Zn4cKgbrjCqaQGAgxGFjMqIgTmyvigif28 dYc7lTfBqhfuBOgSPS40WUjRwY6RLrw5CD2JXnT2M1D/fT487vg/MJ2aEkA8ehD0t+8PZcA3/N9/9 dp07jow7Qyz6oyQLNMFw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rdEYN-00000006I2g-1vGW; Thu, 22 Feb 2024 19:16:19 +0000 Received: from mail-lf1-x12c.google.com ([2a00:1450:4864:20::12c]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rdDaW-00000005x3X-3OuQ for linux-arm-kernel@lists.infradead.org; Thu, 22 Feb 2024 18:14:41 +0000 Received: by mail-lf1-x12c.google.com with SMTP id 2adb3069b0e04-5101cd91017so76567e87.2 for ; Thu, 22 Feb 2024 10:14:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708625663; x=1709230463; darn=lists.infradead.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=/qyJghl0xqwmscdyWmOAn7eBB6UolwNf9WzGJ00ORnM=; b=PqwquHHZTsJyTSSE5upDsMj/8MT0BI2PL0bMLkrQon6LhxhmUmULxWHWwImebKnwKH p7bD5dYvAq8PF/PxZhV1qqYIswFXw5jIhRfOW6omtbMI4b1HtLLr9leRhLmFV+awtnXq M0ptiAYRb/LKpxV5p3NNgwnBnzJFVJHh1U5CbDvka10HthiBlJuODCu7RVoERT2V6MNK 12tryWQ7k3iwUzQGl2ZzFAhKPY0jApdH8B5DB79sbz8deBNNRkcqRunLo/JuyIArIg3a 92TOfARKs9y0/sxX3YSWh5J/FCpUG9HG3GaYpUfBNgNtmoIxKPlMJgDjyiZ/L8+ethTd v7wQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708625663; x=1709230463; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=/qyJghl0xqwmscdyWmOAn7eBB6UolwNf9WzGJ00ORnM=; b=XKrITJccyojSNrgsN/V6xkw440mi28aR3ie6ygdA5mmXwijjG+Mapmy2mRD8auw6Ll P+mbHmpc9f+3t87NfyLus4LHEWmVQ9bXD1hK5pvZICLj6gdH0Ie1X4E6+X9/uWT9mo9w GUgEgTqwRp2YToYRsWYJOnCKvbBZ5tFHdw6N9V0fLECJ3Leb4e5v8cpa8oaTCWlXNtQg eQadqt2uxz9XDdg1AhMvKqyFFqy2bwPvKzAWHGak2LgrPgQOv0TEbnID6hC1XPOT3cHS l4ywIP7ZGOzCEXi2VuxQ+3makBKY2jy0JDCFh4dyNmQkJD3JfULaT+KaU8iu9cQkqKpk Fmsg== X-Forwarded-Encrypted: i=1; AJvYcCVkyS7xxQBx2RDfFodLpUWYZq28emv2vM9+pd9z68ZGZz1rDWGXdz02kiu+nalHDJCl+hVQDuO38y9NsrXpAzPuzbvKx3xusPTU9oJhnaniln4G/A4= X-Gm-Message-State: AOJu0YyU53KnX3NX82mkkMMm0Ez+6MgXTadeDmSe4QSs1u86fM5PtDkX zt0oqZe4Qj3XT9e8v+QytZ53XE6aBOXc4WnU7a29kJ7zMWkg9O5T X-Google-Smtp-Source: AGHT+IFijz6QFs1d/1Wsr4KVrtHFrWPHYvhKrodu21VdqHALqVlmpK2g3OEicBVmuVI9kJVbBEFJpw== X-Received: by 2002:a05:6512:3fc:b0:512:ceae:93d9 with SMTP id n28-20020a05651203fc00b00512ceae93d9mr3374265lfq.28.1708625662700; Thu, 22 Feb 2024 10:14:22 -0800 (PST) Received: from localhost ([178.176.56.174]) by smtp.gmail.com with ESMTPSA id g12-20020ac24d8c000000b005126ab0e3fasm2152711lfe.124.2024.02.22.10.14.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Feb 2024 10:14:22 -0800 (PST) From: Serge Semin To: Michal Simek , Alexander Stein , Borislav Petkov , Tony Luck , James Morse , Mauro Carvalho Chehab , Robert Richter , Dinh Nguyen Cc: Serge Semin , Punnaiah Choudary Kalluri , Arnd Bergmann , Greg Kroah-Hartman , linux-arm-kernel@lists.infradead.org, linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org, Sherry Sun , Borislav Petkov Subject: [PATCH v5 01/20] EDAC/synopsys: Fix ECC status data and IRQ disable race condition Date: Thu, 22 Feb 2024 21:12:46 +0300 Message-ID: <20240222181324.28242-2-fancer.lancer@gmail.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240222181324.28242-1-fancer.lancer@gmail.com> References: <20240222181324.28242-1-fancer.lancer@gmail.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240222_101432_559771_41D3F70F X-CRM114-Status: GOOD ( 23.02 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org The race condition around the ECCCLR register access happens in the IRQ disable method called in the device remove() procedure and in the ECC IRQ handler: 1. Enable IRQ: a. ECCCLR = EN_CE | EN_UE 2. Disable IRQ: a. ECCCLR = 0 3. IRQ handler: a. ECCCLR = CLR_CE | CLR_CE_CNT | CLR_CE | CLR_CE_CNT b. ECCCLR = 0 c. ECCCLR = EN_CE | EN_UE So if the IRQ disabling procedure is called concurrently with the IRQ handler method the IRQ might be actually left enabled due to the statement 3c. The root cause of the problem is that ECCCLR register (which since v3.10a has been called as ECCCTL) has intermixed ECC status data clear flags and the IRQ enable/disable flags. Thus the IRQ disabling (clear EN flags) and handling (write 1 to clear ECC status data) procedures must be serialised around the ECCCTL register modification to prevent the race. So fix the problem described above by adding the spin-lock around the ECCCLR modifications and preventing the IRQ-handler from modifying the IRQs enable flags (there is no point in disabling the IRQ and then re-enabling it again within a single IRQ handler call, see the statements 3a/3b and 3c above). Fixes: f7824ded4149 ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR") Signed-off-by: Serge Semin --- Cc: Sherry Sun Changelog v4: - This is a new patch detached from [PATCH v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure - Rename lock to reglock (Borislav) --- drivers/edac/synopsys_edac.c | 50 ++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c index 709babce43ba..0168b05e3ca1 100644 --- a/drivers/edac/synopsys_edac.c +++ b/drivers/edac/synopsys_edac.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -299,6 +300,7 @@ struct synps_ecc_status { /** * struct synps_edac_priv - DDR memory controller private instance data. * @baseaddr: Base address of the DDR controller. + * @reglock: Concurrent CSRs access lock. * @message: Buffer for framing the event specific info. * @stat: ECC status information. * @p_data: Platform data. @@ -313,6 +315,7 @@ struct synps_ecc_status { */ struct synps_edac_priv { void __iomem *baseaddr; + spinlock_t reglock; char message[SYNPS_EDAC_MSG_SIZE]; struct synps_ecc_status stat; const struct synps_platform_data *p_data; @@ -408,7 +411,8 @@ static int zynq_get_error_info(struct synps_edac_priv *priv) static int zynqmp_get_error_info(struct synps_edac_priv *priv) { struct synps_ecc_status *p; - u32 regval, clearval = 0; + u32 regval, clearval; + unsigned long flags; void __iomem *base; base = priv->baseaddr; @@ -452,10 +456,14 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv) p->ueinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK); p->ueinfo.data = readl(base + ECC_UESYND0_OFST); out: - clearval = ECC_CTRL_CLR_CE_ERR | ECC_CTRL_CLR_CE_ERRCNT; - clearval |= ECC_CTRL_CLR_UE_ERR | ECC_CTRL_CLR_UE_ERRCNT; + spin_lock_irqsave(&priv->reglock, flags); + + clearval = readl(base + ECC_CLR_OFST) | + ECC_CTRL_CLR_CE_ERR | ECC_CTRL_CLR_CE_ERRCNT | + ECC_CTRL_CLR_UE_ERR | ECC_CTRL_CLR_UE_ERRCNT; writel(clearval, base + ECC_CLR_OFST); - writel(0x0, base + ECC_CLR_OFST); + + spin_unlock_irqrestore(&priv->reglock, flags); return 0; } @@ -515,24 +523,41 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p) static void enable_intr(struct synps_edac_priv *priv) { + unsigned long flags; + /* Enable UE/CE Interrupts */ - if (priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR) - writel(DDR_UE_MASK | DDR_CE_MASK, - priv->baseaddr + ECC_CLR_OFST); - else + if (!(priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR)) { writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK, priv->baseaddr + DDR_QOS_IRQ_EN_OFST); + return; + } + + spin_lock_irqsave(&priv->reglock, flags); + + writel(DDR_UE_MASK | DDR_CE_MASK, + priv->baseaddr + ECC_CLR_OFST); + + spin_unlock_irqrestore(&priv->reglock, flags); } static void disable_intr(struct synps_edac_priv *priv) { + unsigned long flags; + /* Disable UE/CE Interrupts */ - if (priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR) - writel(0x0, priv->baseaddr + ECC_CLR_OFST); - else + if (!(priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR)) { writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK, priv->baseaddr + DDR_QOS_IRQ_DB_OFST); + + return; + } + + spin_lock_irqsave(&priv->reglock, flags); + + writel(0, priv->baseaddr + ECC_CLR_OFST); + + spin_unlock_irqrestore(&priv->reglock, flags); } /** @@ -576,8 +601,6 @@ static irqreturn_t intr_handler(int irq, void *dev_id) /* v3.0 of the controller does not have this register */ if (!(priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR)) writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST); - else - enable_intr(priv); return IRQ_HANDLED; } @@ -1359,6 +1382,7 @@ static int mc_probe(struct platform_device *pdev) priv = mci->pvt_info; priv->baseaddr = baseaddr; priv->p_data = p_data; + spin_lock_init(&priv->reglock); mc_init(mci, pdev);