Re: [PATCH 2/2] drivers: edac: Add EDAC support for Kryo CPU caches

Sai Prakash Ranjan
2020-01-24T14:52:18Z

Hi James,

On 2020-01-16 00:19, James Morse wrote:
> Hi guys,
>
> (CC: +Tyler)
>
> On 13/01/2020 05:44, Sai Prakash Ranjan wrote:
>> On 2019-12-30 17:20, Borislav Petkov wrote:
>>> On Thu, Dec 05, 2019 at 09:53:18AM +0000, Sai Prakash Ranjan wrote:
>>>> Kryo{3,4}XX CPU cores implement RAS extensions to support
>>>> Error Correcting Code(ECC). Currently all Kryo{3,4}XX CPU
>>>> cores (gold/silver a.k.a big/LITTLE) support ECC via RAS.
>>>
>>> via RAS what? ARM64_RAS_EXTN?
>>>
>>> In any case, this needs James to look at and especially if there's
>>> some
>>> ARM-generic functionality in there which should be shared, of course.
>
>> Yes it is ARM64_RAS_EXTN and I have been hoping if James can provide
>> the feedback,
>> it has been some time now since I posted this out.
>
> Sorry, I was out of the office for most of November/December, and I'm
> slowly catching up...
>
>
>>>> +
>>>> +config EDAC_QCOM_KRYO_POLL
>>>> +    depends on EDAC_QCOM_KRYO
>>>> +    bool "Poll on Kryo ECC registers"
>>>> +    help
>>>> +      This option chooses whether or not you want to poll on the
>>>> Kryo ECC
>>>> +      registers. When this is enabled, the polling rate can be set
>>>> as a
>>>> +      module parameter. By default, it will call the polling
>>>> function every
>>>> +      second.
>>>
>>> Why is this a separate option and why should people use that?
>>>
>>> Can the polling/irq be switched automatically?
>
>> No it cannot be switched automatically. It is used in case some SoCs
>> do not support an irq
>> based mechanism for EDAC.
>> But I am contradicting myself because I am telling that atleast one
>> interrupt should be
>> specified in bindings,
>> so it is best if I drop this polling option for now.
>
> For now, sure. But I think this will come back for systems with
> embarrassing amounts of
> RAM that would rather scrub the errors than take a flood of IRQs. I'd
> like this to be
> controllable from user-space.
>

Ok so we should have an option to switch between polling and irq.

>
>>>> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
>>>> index d77200c9680b..29edcfa6ec0e 100644
>>>> --- a/drivers/edac/Makefile
>>>> +++ b/drivers/edac/Makefile
>>>> @@ -85,5 +85,6 @@ obj-$(CONFIG_EDAC_SYNOPSYS)        +=
>>>> synopsys_edac.o
>>>>  obj-$(CONFIG_EDAC_XGENE)        += xgene_edac.o
>>>>  obj-$(CONFIG_EDAC_TI)            += ti_edac.o
>>>>  obj-$(CONFIG_EDAC_QCOM)            += qcom_edac.o
>>>> +obj-$(CONFIG_EDAC_QCOM_KRYO)        += qcom_kryo_edac.o
>>>
>>> What is the difference between this new driver and the qcom_edac one?
>>> Can
>>> functionality be shared?
>
> High-level story time:
> Until the 'v8.2' revision of the 'v8' Arm-architecture (the 64bit
> one), arm didn't
> describe how RAS should work. Partners implemented what they needed,
> and we ended up with
> this collection of drivers because they were all different.
>
> v8.2 fixed all this, the good news is once its done, we should never
> need another edac
> driver. (at least, not for SoCs built for v8.2). The downside is there
> is quite a lot in
> there, and we need to cover ACPI machines as well as DT.
>

That is true but the qcom_edac one which is merged is for LLC(system
cache) which is a QCOM IP.

>> qcom_edac driver is for QCOM system cache(last level cache), it should
>> be renamed to
>> qcom_llcc_edac.c.
>> This new driver is for QCOM Kryo CPU core caches(L1,L2,L3).
>>
>> Functionality cannot be shared as these two are different IP blocks
>> and best kept separate.
>
> The qcom_edac will be Qualcomm's pre-v8.2 support.
> This series is about the v8.2 support which all looks totally
> different to Linux.
>

As said before qcom_edac is for LLC which is not available on all SoCs.
QCOM's pre v8.2 support is not upstreamed.

>
>>>> + * ARM Cortex-A55, Cortex-A75, Cortex-A76 TRM Chapter B3.3
>>>
>>> Chapter? Where? URL?
>>>
>>
>> I chose this because these TRMs are openly available and if you search
>> for these above
>> terms like
>> "Cortex-A76 TRM Chapter B3.3" in google, then the first search result
>> will be the TRM pdf,
>> otherwise
>> I would have to specify the long URL for the pdf and we do not know
>> how long that URL link
>> will be active.
>
> These are SoC/CPU specific. Using these we can't solve the whole
> problem.
>
> The architecture all those should fit into is here:
> https://static.docs.arm.com/ddi0587/cb/2019_07_05_DD_0587_C_b.pdf
> (or https://developer.arm.com/docs/ and look for 'RAS')
>
> ... and the arm-arm.
>

Thanks for the link.

>
>>>> +static void dump_syndrome_reg(int error_type, int level,
>>>> +                  u64 errxstatus, u64 errxmisc,
>>>> +                  struct edac_device_ctl_info *edev_ctl)
>>>> +{
>>>> +    char msg[KRYO_EDAC_MSG_MAX];
>>>> +    const char *error_msg;
>>>> +    int cpu;
>>>> +
>>>> +    cpu = raw_smp_processor_id();
>>>
>>> Why raw_?
>>>
>>
>> Because we will be calling smp_processor_id in preemptible context and
>> if we enable
>> CONFIG_DEBUG_PREEMPT,
>> we would get a nice backtrace.
>>
>> [    3.747468] BUG: using smp_processor_id() in preemptible [00000000]
>> code: swapper/0/1
>> [    3.755527] caller is qcom_kryo_edac_probe+0x138/0x2b8
>> [    3.760819] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G
>> S
>> 5.4.0-rc7-next-20191113-00009-g8666855d6a5b-dirty #107
>> [    3.772323] Hardware name: Qualcomm Technologies, Inc. SM8150 MTP
>> (DT)
>> [    3.779030] Call trace:
>> [    3.781556]  dump_backtrace+0x0/0x158
>> [    3.785331]  show_stack+0x14/0x20
>> [    3.788741]  dump_stack+0xb0/0xf4
>> [    3.792164]  debug_smp_processor_id+0xd8/0xe0
>> [    3.796639]  qcom_kryo_edac_probe+0x138/0x2b8
>> [    3.801116]  platform_drv_probe+0x50/0xa8
>> [    3.805236]  really_probe+0x108/0x360
>> [    3.808999]  driver_probe_device+0x58/0x100
>> [    3.813304]  device_driver_attach+0x6c/0x78
>> [    3.817606]  __driver_attach+0xb0/0xf0
>> [    3.821459]  bus_for_each_dev+0x68/0xc8
>> [    3.825407]  driver_attach+0x20/0x28
>> [    3.829083]  bus_add_driver+0x160/0x1f0
>> [    3.833030]  driver_register+0x60/0x110
>> [    3.836976]  __platform_driver_register+0x40/0x48
>> [    3.841813]  qcom_kryo_edac_driver_init+0x18/0x20
>> [    3.846645]  do_one_initcall+0x58/0x1a0
>> [    3.850596]  kernel_init_freeable+0x19c/0x240
>> [    3.855075]  kernel_init+0x10/0x108
>> [    3.858665]  ret_from_fork+0x10/0x1c
>
> and raw_ stops the backtrace? You are still preemptible. The problem
> still exists, you've
> just suppressed the warning.
>
> At any time in dump_syndrome_reg(), you could get an interrupt and
> another task gets
> scheduled. Later your thread is started on another cpu... but not the
> one whose cpu number
> you read from smp_processor_id(). Whatever you needed it for, might
> have the wrong value.
>

Ok will correct this.

>
>>>> +static int kryo_l1_l2_setup_irq(struct platform_device *pdev,
>>>> +                struct edac_device_ctl_info *edev_ctl)
>>>> +{
>>>> +    int cpu, errirq, faultirq, ret;
>>>> +
>>>> +    edac_dev = devm_alloc_percpu(&pdev->dev, *edac_dev);
>>>> +    if (!edac_dev)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    for_each_possible_cpu(cpu) {
>>>> +        preempt_disable();
>>>> +        per_cpu(edac_dev, cpu) = edev_ctl;
>>>> +        preempt_enable();
>>>> +    }
>>>
>>> That sillyness doesn't belong here, if at all.
>
>> Sorry but I do not understand the sillyness here. Could you please
>> explain?
>
> preempt_disable() prevents another task being scheduled instead of
> you, avoiding the risk
> that you get scheduled on another cpu. In this case it doesn't matter
> which cpu you are
> running on as you aren't accessing _this_ cpu's edac_dev, you are
> accessing each one in a
> loop.
>

Thanks for the explanation James, now I get the sillyness.

Thanks,
Sai