linux-kernel - Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description

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]

Date:   Tue, 14 Feb 2023 05:47:08 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Pali Rohár <pali@...nel.org>
CC:     Michael Ellerman <mpe@...erman.id.au>,
        Nicholas Piggin <npiggin@...il.com>,
        Scott Wood <oss@...error.net>, Sinan Akman <sinan@...teme.com>,
        Martin Kennedy <hurricos@...il.com>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine
 description



Le 13/02/2023 à 21:11, Pali Rohár a écrit :
> On Monday 13 February 2023 19:58:15 Christophe Leroy wrote:
>> Le 09/02/2023 à 01:15, Pali Rohár a écrit :
>>>>
>>>> This patch moves all p2020 boards from mpc85xx_rdb.c and mpc85xx_ds.c
>>>> files into new p2020.c file, and plus it copies all helper functions
>>>> which p2020 boards requires. This patch does not introduce any new code
>>>> or functional change. It should be really plain copy/move.
>>
>> Yes after looking into it in more details, it is exactly that. You
>> copied all helper functions but this is not said in the commit message.
>> I think it should be said, and more important it should be explained why.
>> Because this is exactly what I was not understanding, why I couldn't see
>> all moved functions: just because many of them were not moved but copied.
>>
>> In the two first pages you made some function static, and then you
>> duplicated it. Why ? Why not keep it global and just use it from one
>> place to the other ?
>>
>> Because after patch 3 we have:
>>
>> arch/powerpc/platforms/85xx/mpc85xx_rdb.c:static void __init
>> mpc85xx_rdb_pic_init(void)
>> arch/powerpc/platforms/85xx/p2020.c:static void __init
>> mpc85xx_rdb_pic_init(void)
>>
>> arch/powerpc/platforms/85xx/mpc85xx_ds.c:static void __init
>> mpc85xx_ds_pic_init(void)
>> arch/powerpc/platforms/85xx/p2020.c:static void __init
>> mpc85xx_ds_pic_init(void)
>>
>> Why not just drop patches 1 and 2 and keep those two functions and all
>> the other common functions like mpc85xx_8259_cascade()
>> mpc85xx_ds_uli_init() and a lot more  in a separate common file ?
>>
>> Christophe
> 
> After applying all patches there is no mpc85xx_rdb_pic_init() /
> mpc85xx_ds_pic_init() function in p2020.c file. There is
> p2020_pic_init() in p2020.c but it is slightly different than previous
> two functions.

Ok, fair enough, but then please explain in the commit message that you 
copy the functions and then they will be re-written in following 
patches. That way we know exactly what we are reviewing.

> 
> Maybe it could be possible to create one function mpc85xx_pic_init() as
> unification of previous 3 functions, but such change would be needed to
> test on lot of mpc85xx boards, which would be affected by such change.
> And I do not have them for testing. I have only P2020.

No, if the function are different it's better each platform has its own. 
My comment was for functions that are exactly the same.

> 
> So I wrote *_pic_init() function which is p2020 specific, like already
> existing ds and rdb specific functions in their own source files.