Subject: Re: [PATCH v3 00/11] Add arm64/signal initial kselftest support

From: Cristian Marussi <[email-masked]>
Date: Fri, 30 Aug 2019 17:40:42 +0100

Hi

On 13/08/2019 17:22, Dave Martin wrote:
> On Fri, Aug 02, 2019 at 06:02:49PM +0100, Cristian Marussi wrote:
>> Hi
>>
>> this patchset aims to add the initial arch-specific arm64 support to
>> kselftest starting with signals-related test-cases.
>> A common internal test-case layout is proposed which then it is anyway
>> wired-up to the toplevel kselftest Makefile, so that it should be possible
>> at the end to run it on an arm64 target in the usual way with KSFT.
> 
> The tests look like a reasonable base overall and something that we can
> extend later as needed.
> 
> There are various minor things that need attention -- see my comments on
> the individual patches.  Apart for some things that can be factored out,
> I don't think any of it involves redesign.
> 
> 
> A few general comments:
> 
>  * Please wrap all commit messages to <= 75 chars, and follow the other
>    guidelines about commit messages in
>    Documentation/process/submitting-patches.rst).
> 
>  * Remember to run scripts/checkpatch.pl on your patches.  Currently
>    various issues are reported: they should mostly be trivial to fix.
>    checkpatch does report some false positives, but most of the warnings
>    I see look relevant.
> 

Thanks for the review. I addressed latest issues in V4, published now.

I kept tests verbose (outputting to stderr) as of now.
Removed as a whole standalone build/run.

Thanks

Cristian

>  * If you like, you can add an Author: line alongside the copyright
>    notice in new files that you create.  (You'll see this elsewhere in
>    the kernel if you grep.)
> 
> One general stylistic issue (IMHO):
> 
>  * Try to avoid inventing names for things that have no established
>    name (for example "magic0" to mean "magic number 0").
> 
>    The risk is that the reader wastes time grepping for the definition,
>    when really the text should be read at face value.  It's best to use
>    all caps just for #define names, abbreviations, and other things
>    that are customarily capitalised (like "CPU" etc.).  Other words
>    containing underscores may resemble variable / function names, and
>    may cause confusion of there is no actual variable or function with
>    that name.
> 
>    I don't think it's worth heavily reworking the patches for this, but
>    it's something to bear in mind.
> 
> [...]
> 
> Cheers
> ---Dave
> 

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel