On 2024-03-26 16:13, Jan Beulich wrote:
On 26.03.2024 15:30, Nicola Vetrini wrote:
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.
And use of msi_disable() in another expression would then likely not do
what's expected?
Actually I just noticed that some of these macros are never used
(msi_disable being one of them), as far as I can tell.
I'm in favour of a consistent style to be converted in.
This also applies below.
I'm all for consistency; I just don't know what you want to be
consistent
with, here.
I would propose adding parentheses around assignments to control, so
that all macros are consistently parenthesized.
#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.
Right, hence how I started my earlier reply. Question is - do we want
to
go just half the way here, or would we better tidy things all in one
go?
In the latter case I could see about getting to that (whether to take
your patch as basis or instead do it from scratch isn't quite clear to
me at this point).
Jan
How about I revise this patch with parentheses added where needed, as
suggested earlier, and then you can submit a further cleanup patch to
remove e.g. the open coding?
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
