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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2023-11-20 11:27, Julien Grall wrote:
> Hi Nicola,
>
> On 20/11/2023 08:39, Nicola Vetrini wrote:
> > 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@xxxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxxx/
> > > > >
> > > > > 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@xxxxxxxxxx>
> > > >
> > > > 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?
> No. In series [1], we still need a proper description for SAF-1 as 
> there are still some use in the codebase. So it was correct to have 
> this hunk in this series.
> What I was asking in series [1], it to reword:
>
> +     - Functions and variables used only by asm modules are either 
> marked with
> +       the `asmlinkage` macro or with a SAF-1-safe textual deviation
> +       (see safe.json).
>
>
> to something like:
>
> - Functions and variables used only by asm modules are marked with the 
> `asmlinkage macro``. This may also be marked with the now deprecated 
> SAF-1-safe textual deviation (see safe.json).
> > > 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.
> No need for this time.
>
> Cheers,
Ok, thanks for the patience.

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