linux-kernel - Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bf8bee92-0d0e-453b-5ff1-1829a4406fd1@collabora.com>
Date:   Thu, 31 Mar 2022 22:49:38 +0300
From:   Dmitry Osipenko <dmitry.osipenko@...labora.com>
To:     Ashish Mhetre <amhetre@...dia.com>,
        Dmitry Osipenko <digetx@...il.com>,
        krzysztof.kozlowski@...onical.com, robh+dt@...nel.org,
        thierry.reding@...il.com, jonathanh@...dia.com,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-tegra@...r.kernel.org
Cc:     vdumpa@...dia.com, Snikam@...dia.com
Subject: Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186
 onward

On 3/30/22 14:22, Ashish Mhetre wrote:
...
>>> If we are to remove this callback then how to handle unknown interrupt
>>> channel error?
>>
>> Create a common helper function that returns ID of the raised channel or
>> errorno if not bits are set.
>>
> So something like this:
> 
> int status_to_channel(const struct tegra_mc *mc, u32 status,
>               unsigned int *mc_channel)
> {
>     if ((status & mc->soc->ch_intmask) == 0)
>         return -EINVAL;
> 
>     *mc_channel = __ffs((status & mc->soc->ch_intmask) >>
>                  mc->soc->status_reg_chan_shift);
> 
>     return 0;
> }
> 
> Correct?

Yes

>>> Also we want to handle interrupts on one channel at a time and then
>>> clear it from status register. There can be interrupts on multiple
>>> channel. So multiple bits from status will be set. Hence it will be
>>> hard to parameterize shift such that it gives appropriate channel.
>>> So I think current approach is fine. Please correct me if I am wrong
>>> somewhere.
>>
>> You may do the following:
>>
>> 1. find the first channel bit set in the status reg
>> 2. handle that channel
>> 3. clear only the handled status bit, don't clear the other bits
>> 4. return from interrupt
>>
>> If there are other bits set, then interrupt handler will fire again and
>> next channel will be handled.
> 
> For clearing status bit after handling, we can retrieve channel bit by
> something like this:
> 
> ch_bit = BIT(*mc_channel) << mc->soc->status_reg_chan_shift;
> 
> Correct?

Yes

Perhaps using FIELD_PREP() and alike helpers could make it look nice in
the code.