| Message ID | c924aa0d5b3b6adbb24cc638f739173cbc41862c.1711118582.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address some violations of MISRA C Rule 20.7 | expand |
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? > #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? There also look to be cases where MASK_EXTR() / MASK_INSR() would want using, in favor of using open-coded numbers. > +#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
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
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
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?
>>>>> +#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 see why. The bodies of the macros are open-coded in multiple places in msi.c. I can't really tell whether that's intentional or not, but this is an opportunity to do a more general cleanup I guess, beyond the scope of this patch. >>> 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?
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h index 997ccb87be0c..e24d46d95a02 100644 --- 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) #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)) +#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE #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); +#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 + +#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) /* * MSI Defined Data Structures
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. No functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/x86/include/asm/msi.h | 47 +++++++++++++++++----------------- 1 file changed, 24 insertions(+), 23 deletions(-)