[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 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? > 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. >>> #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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |