| Message ID | 20221228084028.46528-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | Qcom: LLCC/EDAC: Fix base address used for LLCC banks | expand |
On Wed, Dec 28, 2022 at 06:55:47PM +0100, Borislav Petkov wrote: > On Wed, Dec 28, 2022 at 10:17:11PM +0530, Manivannan Sadhasivam wrote: > > Well, some maintainers prefer to pick the independent patches through their > > tree. That's why I moved those patches to the start of the series. > > Once some maintainers experience a crazy dependency hell between trees, > they would find routing it all through a single tree a lot easier the > next time. > > > If you are fine with all patches going through qcom tree, I do not > > have any issue :) > > I'm reviewing. > Ok! I'll wait for your reviews on the rest of the EDAC patches before doing the respin. Thanks, Mani > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Jan 02, 2023 at 11:00:45PM +0530, Manivannan Sadhasivam wrote: > On Wed, Dec 28, 2022 at 06:55:47PM +0100, Borislav Petkov wrote: > > On Wed, Dec 28, 2022 at 10:17:11PM +0530, Manivannan Sadhasivam wrote: > > > Well, some maintainers prefer to pick the independent patches through their > > > tree. That's why I moved those patches to the start of the series. > > > > Once some maintainers experience a crazy dependency hell between trees, > > they would find routing it all through a single tree a lot easier the > > next time. > > > > > If you are fine with all patches going through qcom tree, I do not > > > have any issue :) > > > > I'm reviewing. > > > > Ok! I'll wait for your reviews on the rest of the EDAC patches before doing the > respin. > Ping! Thanks, Mani > Thanks, > Mani > > > -- > > Regards/Gruss, > > Boris. > > > > https://people.kernel.org/tglx/notes-about-netiquette > > -- > மணிவண்ணன் சதாசிவம்
On Wed, Dec 28, 2022 at 02:10:26PM +0530, Manivannan Sadhasivam wrote: > The Qualcomm LLCC/EDAC drivers were using a fixed register stride for > accessing the (Control and Status Registers) CSRs of each LLCC bank. > This stride 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. If this patch fixes a crash, then it should be Cc: <stable@kernel.org> If there are prerequisites to it, they should be CC:stable too. So looking at the urgent stuff: patches 1, 3, I'm thinking I can take them through the EDAC tree and send them to Linus now, after you've addressed the review comments. This one can go through some other tree, I presume, but since it fixes a crash it should go in now too... > For fixing this issue, let's obtain the base address of each LLCC bank from > devicetree and get rid of the fixed stride. This also means, we no longer Please use passive voice in your commit message: no "we" or "I", etc, and describe your changes in imperative mood. Personal pronouns are ambiguous in text, especially with so many parties/companies/etc developing the kernel so let's avoid them please. > need to rely on reg-names property and get the base addresses using index. > > First index is LLCC bank 0 and last index is LLCC broadcast. If the SoC > supports more than one bank, then those needs to be defined in devicetree s/needs/need/ > for index from 1..N-1. > > Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com> > Tested-by: Luca Weiss <luca.weiss@fairphone.com> > Tested-by: Steev Klimaszewski <steev@kali.org> # Thinkpad X13s > Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> With the above addressed, for the EDAC bits: Reviewed-by: Borislav Petkov (AMD) <bp@alien8.de> Thx.
On Sat, Jan 14, 2023 at 02:27:50PM +0100, Borislav Petkov wrote: > On Wed, Dec 28, 2022 at 02:10:26PM +0530, Manivannan Sadhasivam wrote: > > The Qualcomm LLCC/EDAC drivers were using a fixed register stride for > > accessing the (Control and Status Registers) CSRs of each LLCC bank. > > This stride 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. > > If this patch fixes a crash, then it should be > > Cc: <stable@kernel.org> > > If there are prerequisites to it, they should be CC:stable too. > That's what I did in previous revision but then Krzysztof reported that backporting would break old DTs. See discussion on v2: https://lore.kernel.org/lkml/20221212123311.146261-1-manivannan.sadhasivam@linaro.org/ Thanks, Mani > So looking at the urgent stuff: patches 1, 3, I'm thinking I can take them > through the EDAC tree and send them to Linus now, after you've addressed the > review comments. > > This one can go through some other tree, I presume, but since it fixes a crash > it should go in now too... > > > For fixing this issue, let's obtain the base address of each LLCC bank from > > devicetree and get rid of the fixed stride. This also means, we no longer > > Please use passive voice in your commit message: no "we" or "I", etc, > and describe your changes in imperative mood. > > Personal pronouns are ambiguous in text, especially with so many > parties/companies/etc developing the kernel so let's avoid them please. > > > need to rely on reg-names property and get the base addresses using index. > > > > First index is LLCC bank 0 and last index is LLCC broadcast. If the SoC > > supports more than one bank, then those needs to be defined in devicetree > > s/needs/need/ > > > for index from 1..N-1. > > > > Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com> > > Tested-by: Luca Weiss <luca.weiss@fairphone.com> > > Tested-by: Steev Klimaszewski <steev@kali.org> # Thinkpad X13s > > Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8540p-ride > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > With the above addressed, for the EDAC bits: > > Reviewed-by: Borislav Petkov (AMD) <bp@alien8.de> > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
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 has been tested on SM8250, SM8450, SM6350, SC8280XP, SA8540P, and SDM845. Merging strategy ---------------- Patches 1/17, 2/17 and 3/17 can be merged independently to EDAC tree. Rest of the patches should be merged to qcom tree due to LLCC dependency. Thanks, Mani Changes in v5: * Reduced the size of llcc0 to 0x45000 on SDM845 due to overlapping with BWMON * Added a patch to disable creation of EDAC platform device on SDM845 * Rebase on top of v6.2-rc1 * Moved the EDAC specific patches to the start so that they can be applied independently of LLCC patches Changes in v4: * Added a patch that fixes the use-after-free bug in qcom_edac driver Changes in v3: * Brought back reg-names property for compatibility (Krzysztof) * Removed Fixes tag and stable list as backporting the drivers/binding/dts patches alone would break (Krzysztof) * Fixed the uninitialized variable issue (Kbot) * Added a patch to make use of driver supplied polling interval (Luca) * Added a patch for module autoloading (Andrew) * Didn't collect Review tags from Sai as the dts patches were changed. Changes in v2: * Removed reg-names property and used index of reg property to parse LLCC bank base address (Bjorn) * Collected Ack from Sai for binding * Added a new patch for polling mode (Luca) * Renamed subject of patches targeting SC7180 and SM6350 Manivannan Sadhasivam (17): EDAC/device: Make use of poll_msec value in edac_device_ctl_info struct EDAC/qcom: Add platform_device_id table for module autoloading EDAC/qcom: Do not pass llcc_driv_data as edac_device_ctl_info's pvt_info dt-bindings: arm: msm: Update the maintainers for LLCC dt-bindings: arm: msm: Fix register regions used for LLCC banks arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks arm64: dts: qcom: sc7180: Fix the base addresses of LLCC banks arm64: dts: qcom: sc7280: Fix the base addresses of LLCC banks arm64: dts: qcom: sc8280xp: Fix the base addresses of LLCC banks arm64: dts: qcom: sm8150: Fix the base addresses of LLCC banks arm64: dts: qcom: sm8250: Fix the base addresses of LLCC banks arm64: dts: qcom: sm8350: Fix the base addresses of LLCC banks arm64: dts: qcom: sm8450: Fix the base addresses of LLCC banks arm64: dts: qcom: sm6350: Fix the base addresses of LLCC banks qcom: llcc/edac: Fix the base address used for accessing LLCC banks qcom: llcc/edac: Support polling mode for ECC handling soc: qcom: llcc: Do not create EDAC platform device on SDM845 .../bindings/arm/msm/qcom,llcc.yaml | 128 ++++++++++++++++-- arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +- arch/arm64/boot/dts/qcom/sc7280.dtsi | 5 +- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 10 +- arch/arm64/boot/dts/qcom/sdm845.dtsi | 7 +- arch/arm64/boot/dts/qcom/sm6350.dtsi | 2 +- arch/arm64/boot/dts/qcom/sm8150.dtsi | 7 +- arch/arm64/boot/dts/qcom/sm8250.dtsi | 7 +- arch/arm64/boot/dts/qcom/sm8350.dtsi | 7 +- arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +- drivers/edac/edac_device.c | 2 +- drivers/edac/qcom_edac.c | 63 +++++---- drivers/soc/qcom/llcc-qcom.c | 80 ++++++----- include/linux/soc/qcom/llcc-qcom.h | 6 +- 14 files changed, 244 insertions(+), 89 deletions(-)