linux-kernel - Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver

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]

Date:   Thu, 15 Oct 2020 11:13:47 +0200
From:   Matthias Brugger <matthias.bgg@...il.com>
To:     Neal Liu <neal.liu@...iatek.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        lkml <linux-kernel@...r.kernel.org>,
        wsd_upstream <wsd_upstream@...iatek.com>
Subject: Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver



On 15/10/2020 04:13, Neal Liu wrote:
> On Thu, 2020-10-08 at 10:35 +0800, Neal Liu wrote:
>> On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
>>>
>>> On 27/08/2020 05:06, Neal Liu wrote:
> [...]
> 
>>>> +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
>>>> +{
>>>> +	void __iomem *pd_vio_shift_sta_reg;
>>>> +	void __iomem *pd_vio_shift_sel_reg;
>>>> +	void __iomem *pd_vio_shift_con_reg;
>>>> +	int min_shift_group;
>>>> +	int ret;
>>>> +	u32 val;
>>>> +
>>>> +	pd_vio_shift_sta_reg = ctx->infra_base +
>>>> +			       ctx->data->vio_shift_sta_offset;
>>>> +	pd_vio_shift_sel_reg = ctx->infra_base +
>>>> +			       ctx->data->vio_shift_sel_offset;
>>>> +	pd_vio_shift_con_reg = ctx->infra_base +
>>>> +			       ctx->data->vio_shift_con_offset;
>>>> +
>>>> +	/* Find the minimum shift group which has violation */
>>>> +	val = readl(pd_vio_shift_sta_reg);
>>>> +	if (!val)
>>>> +		return false;
>>>
>>> So bit 0 of selection register (pd_vio_shift_sel_reg) does not represent a
>>> violation group?
>>> I don't know how the HW works, but is seems odd to me. In case that's bit 0
>>> actually doesn't represent anything: how can an interrupt be triggered without
>>> any debug information present (means val == 0)?
>>
>> This check implies HW status has something wrong. It cannot get any
>> debug information for this case.
>> It won't happen in normal scenario. Should we remove this check?
>>
> 
> Sorry, I missed the most common part. Is function is in the while loop:
> while (devapc_sync_vio_dbg(ctx))
> ...
> 
> We keep find the minimum bit in pd_vio_shift_sta_reg to get the
> violation information, (pd_vio_shift_sta_reg might raise multiple bits)
> until all raised bit (shift group) has been handled.
> So I don't think it's necessary to add WARN message in this case.
> Thanks
> 

Correct we would get a warning every time once we have read and cleared all 
violations, but we are only concerned about the IRQ triggered with no violation 
present. But we can skip that check for now, if we see that there could be a HW 
error we can think about it in the future.

Regards,
Matthias