Re: [XEN PATCH v3] xen: replace occurrences of SAF-1-safe with asmlinkage attribute

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2023-11-17 20:15, Julien Grall wrote:
> Hi Nicola,
> 
> On 16/11/2023 09:15, Nicola Vetrini wrote:
>> On 2023-11-16 10:08, Nicola Vetrini wrote:
>>> The comment-based justifications for MISRA C:2012 Rule 8.4 are 
>>> replaced
>>> by the asmlinkage pseudo-attribute, for the sake of uniformity.
>>> 
>>> Add missing 'xen/compiler.h' #include-s where needed.
>>> 
>>> The text in docs/misra/deviations.rst and docs/misra/safe.json
>>> is modified to reflect this change.
>>> 
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>> This patch should be applied after patch 2 of this series.
>>> The request made by Julien to update the wording is
>>> contained in the present patch.
>>> https://lore.kernel.org/all/9ad7f6210c15f520297aac00e8af0e64@bugseng.com/
>>> 
>>> Concerns about efi_multiboot2 will be dealt with separately.
>>> 
>>> Changes in v2:
>>> - Edit safe.json.
>>> - Remove mention of SAF-1-safe in deviations.rst.
>>> Changes in v3:
>>> - Sorted #include-s and rebased against
>>> 7ad0c774e474 ("x86/boot: tidy #include-s")
>>> ---
>>>  docs/misra/deviations.rst   |  5 ++---
>>>  docs/misra/safe.json        |  2 +-
>>>  xen/arch/arm/cpuerrata.c    |  7 +++----
>>>  xen/arch/arm/setup.c        |  5 ++---
>>>  xen/arch/arm/smpboot.c      |  3 +--
>>>  xen/arch/arm/traps.c        | 21 +++++++--------------
>>>  xen/arch/x86/boot/cmdline.c |  5 +++--
>>>  xen/arch/x86/boot/reloc.c   |  6 +++---
>>>  xen/arch/x86/extable.c      |  3 +--
>>>  xen/arch/x86/setup.c        |  3 +--
>>>  xen/arch/x86/traps.c        | 27 +++++++++------------------
>>>  xen/common/efi/boot.c       |  5 ++---
>>>  12 files changed, 35 insertions(+), 57 deletions(-)
>>> 
>> 
>> In hindsight I should have added an
>> 
>> Acked-by: Julien Grall <jgrall@amazon.com>
>> 
>> given that the comment has been addressed in my opinion.
> 
> I am a bit confused how you considered it was addressed. I see no 
> update in safe.json when I clearly asked for some (I wouldn't have 
> bothered to comment in v2 otherwise and just gave an ack).
> 

I did update safe.json like so:

-            "text": "Functions and variables used only by asm modules 
do not need to have a visible declaration prior to their definition."
+            "text": "Not used anymore."

but given what you wrote below, maybe you wanted this in the series [1], 
right?

> To be explicit, I requested to:
>   1. update the description in [2] to clarify that SAF-1 is deprecated.
>   2. This patch is rebased on top and therefore remove completely the 
> mention of SAF-1.
> 
> I am well-aware that the end result is technically the same. But 
> patches are meant to be self-contained so if we revert the latest, then 
> the meaning is still the same.
> 
> This patch is unlikely to be removed and this is now the nth time I 
> asked it the same (maybe it was not clear enough?). So I am going to 
> content with the current proposal because this is not worth to go 
> further. But I will at least express my discontent how this is handled.
> 

I misunderstood your previous comments then. When you commented on v2 I 
surmised that you were ok with this patch condensing all the shuffling. 
Clearly this was not the case, but I also want to point out that. Given 
that [2] hasn't been committed yet either, then I can do what you asked.

[2] 
https://lore.kernel.org/all/cover.1698829473.git.nicola.vetrini@bugseng.com/

> TBH, there are far too many MISRA patches on the ML spread across 
> multiple threads. Some are based on top of the others. This makes 
> extremely difficult to follow and know what is addressed or not. Can we 
> at least try to condense some of work in similar area in the same 
> series? For instance, this patch could have been included in the other 
> series [1].
> 
> Lastly, right now, I have 300 emails (31 threads) with MISRA in the 
> title in my inbox. It is a little unclear what has been 
> committed/review or require input. I am concerned to miss key series 
> (the patch to compile in docs/ was nearly missed).
> 
> Do we track anywhere which series are still inflights? Can we consider 
> to pause or at least slow down the rate of new MISRA patches until the 
> backlog is cleared? (Adding more patches is not really helping).
> 

I do have a folder with all my in-flight patches. Please note that this 
big backlog has partly been the result of patches being acked, but not 
committed. I'll double check with my colleagues Stefano's list in his 
reply, to see if there are any differences.

> Cheers,
> 
> [1] 
> https://lore.kernel.org/all/a1b5c3b273145c35535fed3647bf850d9ae5db7f.1698829473.git.nicola.vetrini@bugseng.com/
> 
> 
> I pointed out that the patch in
> 
>> 

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)