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] [day] [month] [year] [list]

Message-ID: <1d9e2055-ac22-334a-f003-911034ef75f4@nvidia.com>
Date:   Fri, 1 Apr 2022 03:25:01 +0530
From:   Ashish Mhetre <amhetre@...dia.com>
To:     Dmitry Osipenko <dmitry.osipenko@...labora.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 4/1/2022 1:19 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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.

I tried using FIELD_PREP() and FIELD_GET() for our use-case but
compilation is failing because these macros require the mask to be 
compile time constant and our mask "mc->soc->ch_intmask" cannot qualify
to be compile time constant.