[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN PATCH 09/11] x86/msi: address violation of MISRA C Rule 20.7 and coding style



On 2024-03-26 11:05, Jan Beulich wrote:
On 22.03.2024 17:01, Nicola Vetrini wrote:
MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

While at it, the style of these macros has been somewhat uniformed.

Hmm, yes, but they then still leave more to be desired. I guess I can see
though why you don't want to e.g. ...

--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -147,33 +147,34 @@ 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_data_reg(base, is64bit)    \
-       ( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
-#define msi_mask_bits_reg(base, is64bit) \
-       ( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
+#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)
+#define msi_mask_bits_reg(base, is64bit)                \
+    (((is64bit) == 1) ? (base) + PCI_MSI_MASK_BIT       \
+                      : (base) + PCI_MSI_MASK_BIT - 4)

... drop the bogus == 1 in these two, making them similar to ...

 #define msi_pending_bits_reg(base, is64bit) \
-       ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
-#define msi_disable(control)           control &= ~PCI_MSI_FLAGS_ENABLE
+    ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))

... this.

+#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE

Doesn't this need an outer pair of parentheses, too?


Not necessarily. I'm in favour of a consistent style to be converted in. This also applies below.

 #define multi_msi_capable(control) \
-       (1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
+    (1 << (((control) & PCI_MSI_FLAGS_QMASK) >> 1))
 #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))
+    (control) |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);

And this, together with dropping the bogus semicolon?


I'll drop the semicolon.

There also look to be cases where MASK_EXTR() / MASK_INSR() would want using,
in favor of using open-coded numbers.


Yes, perhaps. However, the risk that I make some mistakes in doing so are quite high, though.

+#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) |= PCI_MSI_FLAGS_ENABLE

This again is suspiciously missing outer parentheses; really here, with
the earlier statement, it look like do { ... } while ( 0 ) or ({ ... })
are wanted.

+#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

Outer parentheses missing for these two again?

Jan

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.