Message-ID: <20221213052802.GB4862@thinkpad>
Date:   Tue, 13 Dec 2022 10:58:02 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Andrew Halaney <ahalaney@...hat.com>
Cc:     andersson@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, bp@...en8.de,
        tony.luck@...el.com, quic_saipraka@...cinc.com,
        konrad.dybcio@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, james.morse@....com,
        mchehab@...nel.org, rric@...nel.org, linux-edac@...r.kernel.org,
        quic_ppareek@...cinc.com, luca.weiss@...rphone.com
Subject: Re: [PATCH v2 00/13] Qcom: LLCC/EDAC: Fix base address used for LLCC
 banks

On Mon, Dec 12, 2022 at 01:23:40PM -0600, Andrew Halaney wrote:
> On Mon, Dec 12, 2022 at 06:02:58PM +0530, Manivannan Sadhasivam wrote:
> > The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> > accessing the (Control and Status Regsiters) CSRs of each LLCC bank.
> > This offset only works for some SoCs like SDM845 for which driver support
> > was initially added.
> >
> > But the later SoCs use different register stride that vary between the
> > banks with holes in-between. So it is not possible to use a single register
> > stride for accessing the CSRs of each bank. By doing so could result in a
> > crash with the current drivers. So far this crash is not reported since
> > EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the
> > driver extensively by triggering the EDAC IRQ (that's where each bank
> > CSRs are accessed).
> >
> > For fixing this issue, let's obtain the base address of each LLCC bank from
> > devicetree and get rid of the fixed stride.
> >
> > This series affects multiple platforms but I have only tested this on
> > SM8250 and SM8450. Testing on other platforms is welcomed.
> >
> 
> Tested-by: Andrew Halaney <ahalaney@...hat.com> # sa8540p-ride
> 

Thanks!

> I took this for a quick spin on the qdrive3 I've got access to without
> any issue:
> 
>     [root@...alhost ~]# modprobe qcom_edac
>     [root@...alhost ~]# dmesg | grep -i edac
>     [    0.620723] EDAC MC: Ver: 3.0.0
>     [    1.165417] ghes_edac: GHES probing device list is empty
>     [  594.688103] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (INTERRUPT)
>     [root@...alhost ~]# cat /proc/interrupts | grep ecc
>     174:          0          0          0          0          0          0          0          0     GICv3 614 Level     llcc_ecc
>     [root@...alhost ~]#
> 
> Potentially stupid question, but are users expected to manually load the
> driver as I did? I don't see how it would be loaded automatically in the
> current state, but thought it was funny that I needed to modprobe
> myself.
> 
> Please let me know if you want me to do any more further testing!
> 

Well, I always ended up using the driver as a built-in. I do make it module for
build test but never really used it as a module, so didn't catch this issue.

This is due to the module alias not exported by the qcom_edac driver. Below
diff allows kernel to autoload it:

diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
index f7afb5375293..13919d01c22d 100644
--- a/drivers/edac/qcom_edac.c
+++ b/drivers/edac/qcom_edac.c
@@ -419,3 +419,4 @@ module_platform_driver(qcom_llcc_edac_driver);
 
 MODULE_DESCRIPTION("QCOM EDAC driver");
 MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom_llcc_edac");

Please test and let me know. I will add this as a new patch in next version.

Thanks,
Mani

> Thanks,
> Andrew
> 

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists