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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 18/11/2023 02:47, Stefano Stabellini wrote:
> On Fri, 17 Nov 2023, Julien Grall wrote:
> > 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).
> >
> > To be explicit, I requested to:
> >    1. update the description in [1] 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.
> Just to be extra clearm, you are not happy with it, but you would
> tolerate the patch to be committed as is, right?
Correct.

> > 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 cleared out the ones I was tracking and were acked. I hope this helps.
> As far as I can tell these are the ones currently under discussion:
>
> - [XEN PATCH v5 0/2] use the documentation for MISRA C:2012 Dir 4.1
> - first 4 patches of [XEN PATCH][for-4.19 v4 0/8] address violations of MISRA 
> C:2012 Rule 10.1
> - [XEN PATCH][for-4.19 v2 0/2] use the macro ISOLATE_LOW_BIT where appropriate
> - [XEN PATCH v2] domain: add ASSERT to help static analysis tools
> - [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3
> - [XEN PATCH v2] automation/eclair: add deviations for MISRA C:2012 Rule 8.3
> - this patch
> - [XEN PATCH 0/5] xen: address some violations of MISRA C:2012 Rule 8.2

Thanks! I will remove the rest from my inbox.

Cheers,

--
Julien Grall