Re: [PATCH 2/4] x86/msi: fix Misra Rule 20.7 in msi.h

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Nov 19, 2024 at 03:35:34PM +0000, Andrew Cooper wrote:
> On 19/11/2024 2:39 pm, Roger Pau Monné wrote:
> > On Tue, Nov 19, 2024 at 02:21:35PM +0000, Andrew Cooper wrote:
> >> On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> >>> ---
> >>>  xen/arch/x86/include/asm/msi.h | 35 ++++++++++++++--------------------
> >>>  1 file changed, 14 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/include/asm/msi.h 
> >>> b/xen/arch/x86/include/asm/msi.h
> >>> index 748bc3cd6d8b..49a576383288 100644
> >>> --- a/xen/arch/x86/include/asm/msi.h
> >>> +++ b/xen/arch/x86/include/asm/msi.h
> >>> @@ -147,33 +147,26 @@ int msi_free_irq(struct msi_desc *entry);
> >>>   */
> >>>  #define NR_HP_RESERVED_VECTORS   20
> >>>  
> >>> -#define msi_control_reg(base)            (base + PCI_MSI_FLAGS)
> >>> -#define msi_lower_address_reg(base)      (base + PCI_MSI_ADDRESS_LO)
> >>> -#define msi_upper_address_reg(base)      (base + PCI_MSI_ADDRESS_HI)
> >>> +#define msi_control_reg(base)            ((base) + PCI_MSI_FLAGS)
> >>> +#define msi_lower_address_reg(base)      ((base) + PCI_MSI_ADDRESS_LO)
> >>> +#define msi_upper_address_reg(base)      ((base) + PCI_MSI_ADDRESS_HI)
> >>>  #define msi_data_reg(base, is64bit)      \
> >>> - ( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
> >>> + ((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))
> >>>  #define msi_mask_bits_reg(base, is64bit) \
> >>> - ( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
> >>> + ((base) + PCI_MSI_MASK_BIT - ((is64bit) ? 0 : 4))
> >>>  #define msi_pending_bits_reg(base, is64bit) \
> >>>   ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
> >>> -#define msi_disable(control)             control &= ~PCI_MSI_FLAGS_ENABLE
> >>>  #define multi_msi_capable(control) \
> >>> - (1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> >>> + (1U << MASK_EXTR(control, PCI_MSI_FLAGS_QMASK))
> >>>  #define multi_msi_enable(control, num) \
> >>> - control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> >>> -#define is_64bit_address(control)        (!!(control & 
> >>> PCI_MSI_FLAGS_64BIT))
> >>> -#define is_mask_bit_support(control)     (!!(control & 
> >>> PCI_MSI_FLAGS_MASKBIT))
> >>> -#define msi_enable(control, num) multi_msi_enable(control, num); \
> >>> - control |= PCI_MSI_FLAGS_ENABLE
> >>> -
> >>> -#define msix_control_reg(base)           (base + PCI_MSIX_FLAGS)
> >>> -#define msix_table_offset_reg(base)      (base + PCI_MSIX_TABLE)
> >>> -#define msix_pba_offset_reg(base)        (base + PCI_MSIX_PBA)
> >>> -#define msix_enable(control)             control |= PCI_MSIX_FLAGS_ENABLE
> >>> -#define msix_disable(control)            control &= 
> >>> ~PCI_MSIX_FLAGS_ENABLE
> >>> -#define msix_table_size(control)         ((control & 
> >>> PCI_MSIX_FLAGS_QSIZE)+1)
> >>> -#define msix_unmask(address)             (address & 
> >>> ~PCI_MSIX_VECTOR_BITMASK)
> >>> -#define msix_mask(address)               (address | 
> >>> PCI_MSIX_VECTOR_BITMASK)
> >>> + ((control) |= MASK_INSR(fls(num) - 1, PCI_MSI_FLAGS_QSIZE))
> >>> +#define is_64bit_address(control)        !!((control) & 
> >>> PCI_MSI_FLAGS_64BIT)
> >>> +#define is_mask_bit_support(control)     !!((control) & 
> >>> PCI_MSI_FLAGS_MASKBIT)
> >> You need to retain the outermost brackets for other MISRA reasons.
> > I was borderline on dropping those braces, as I was expecting Misra to
> > require them.
> >
> >> I'm happy to fix up on commit, even splitting the patch (seeing as I've
> >> already done the split in order to review the rest).
> > Fine, by split I think you mean the pruning of unused macros vs the
> > fixing of the parentheses?
> 
> Split pruning out into another patch, fold the bracket fix into this one.

If you want I can do that myself, as I will likely need to resend #3
to drop the unneeded __maybe_unused attribute.

Thanks, Roger.