linux-kernel - Re: [PATCH v3 1/5] soc: mediatek: pwrap: Use readx_poll

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: <9f197c69-fe04-b636-afb7-8474763c8a3a@collabora.com>
Date:   Tue, 17 May 2022 11:41:28 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Matthias Brugger <matthias.bgg@...il.com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        nfraprado@...labora.com, rex-bc.chen@...iatek.com,
        zhiyong.tao@...iatek.com
Subject: Re: [PATCH v3 1/5] soc: mediatek: pwrap: Use readx_poll_timeout()
 instead of custom function

Il 17/05/22 11:25, Matthias Brugger ha scritto:
> 
> 
> On 16/05/2022 14:46, AngeloGioacchino Del Regno wrote:
>> Function pwrap_wait_for_state() is a function that polls an address
>> through a helper function, but this is the very same operation that
>> the readx_poll_timeout macro means to do.
>> Convert all instances of calling pwrap_wait_for_state() to instead
>> use the read_poll_timeout macro.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
>> Tested-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
>> ---
>>   drivers/soc/mediatek/mtk-pmic-wrap.c | 60 +++++++++++++++-------------
>>   1 file changed, 33 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c 
>> b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> index bf39a64f3ecc..54a5300ab72b 100644
>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> @@ -13,6 +13,9 @@
>>   #include <linux/regmap.h>
>>   #include <linux/reset.h>
>> +#define PWRAP_POLL_DELAY_US    10
>> +#define PWRAP_POLL_TIMEOUT_US    10000
>> +
>>   #define PWRAP_MT8135_BRIDGE_IORD_ARB_EN        0x4
>>   #define PWRAP_MT8135_BRIDGE_WACS3_EN        0x10
>>   #define PWRAP_MT8135_BRIDGE_INIT_DONE3        0x14
>> @@ -1241,27 +1244,14 @@ static bool pwrap_is_fsm_idle_and_sync_idle(struct 
>> pmic_wrapper *wrp)
>>           (val & PWRAP_STATE_SYNC_IDLE0);
>>   }
>> -static int pwrap_wait_for_state(struct pmic_wrapper *wrp,
>> -        bool (*fp)(struct pmic_wrapper *))
>> -{
>> -    unsigned long timeout;
>> -
>> -    timeout = jiffies + usecs_to_jiffies(10000);
>> -
>> -    do {
>> -        if (time_after(jiffies, timeout))
>> -            return fp(wrp) ? 0 : -ETIMEDOUT;
>> -        if (fp(wrp))
>> -            return 0;
>> -    } while (1);
>> -}
>> -
>>   static int pwrap_read16(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
>>   {
>> +    bool tmp;
>>       int ret;
>>       u32 val;
>> -    ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle);
>> +    ret = readx_poll_timeout(pwrap_is_fsm_idle, wrp, tmp, tmp,
> 
> hm, if we make the cond (tmp > 0) that would help to understand the code. At least 
> I had to think about it for a moment. But I leave it to you if you think it's worth 
> the effort.
> 

I would prefer size over readability in this case... if we do (tmp > 0), it would
be incorrect to keep tmp as a `bool`, we would have to set it as an integer var,
which is unnecessarily bigger (that's the reason why I wrote it like so!).

Another way to increase human readability would be to do (tmp == true), but it
looks a bit weird to me, doesn't it?
If you disagree about that looking weird, though, I can go with that one, perhaps!

Cheers,
Angelo