Prune unused macros and adjust the remaining ones to parenthesize macro
arguments.
No functional change intended.
Singed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
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)
+
+#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_table_size(control) (((control) & PCI_MSIX_FLAGS_QSIZE) + 1)
/*
* MSI Defined Data Structures
--
2.46.0
On 19/11/2024 10:34 am, Roger Pau Monne wrote: > Prune unused macros and adjust the remaining ones to parenthesize macro > arguments. > > No functional change intended. > > Singed-off-by: Roger Pau Monné <roger.pau@citrix.com> It's a little early for carol season, isn't it? It would help to identify which macros are being dropped, because the diff is far from simple to read. AFAICT, its: msi_disable() msi_enable() msix_enable() msix_disable() msix_unmask() msix_mask() Splitting this change does make a marginal improvement in the diff, and a substantial improvement in `git diff --color-word`'s ability to review this change. You've also introduced uses of MASK_EXTR() and MASK_INSR(), which at least ought to be noted in the commit message. Technically I think it's a bugfix for multi_msi_enable(), because I think it now won't overflow the 3-bit field if an overly large num is passed in. Bloat-o-meter reports: add/remove: 0/0 grow/shrink: 3/1 up/down: 15/-61 (-46) Function old new delta set_iommu_interrupt_handler 366 373 +7 write_msi_msg 348 352 +4 init_msi 574 578 +4 pci_enable_msi 1084 1023 -61 Taking the first example, that's caused by swapping this: > iommu->msi.msi.mpos = ( ((!!(control & 0x80)) == 1) ? > iommu->msi.msi_attrib.pos+16 : iommu->msi.msi_attrib.pos+16 -4); for this: > iommu->msi.msi.mpos = ((iommu->msi.msi_attrib.pos) + 16 - > (((!!((control) & 0x80))) ? 0 : 4)); and code generation changing from a CMOV to straight-line arithmetic. In write_msi_msg(), we actually drop a conditional branch and replace it with straight-line arithmetic. init_msi() gets a substantial restructuring, but it looks like two branches are dropped. pci_enable_msi() has the biggest change, but doesn't obviously reduce the number of branches. There is clearly less register setup around existing branches, so my best guess is that the new macro forms are more amenable to common-sub-expression-elimination. Either way, it's all minor. Staring at the diff for long enough, I'm pretty sure it's all good. > --- > 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'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). ~Andrew
On Tue, Nov 19, 2024 at 02:21:35PM +0000, Andrew Cooper wrote:
> On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> > Prune unused macros and adjust the remaining ones to parenthesize macro
> > arguments.
> >
> > No functional change intended.
> >
> > Singed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> It's a little early for carol season, isn't it?
>
> It would help to identify which macros are being dropped, because the
> diff is far from simple to read.
>
> AFAICT, its:
>
> msi_disable()
> msi_enable()
> msix_enable()
> msix_disable()
> msix_unmask()
> msix_mask()
>
> Splitting this change does make a marginal improvement in the diff, and
> a substantial improvement in `git diff --color-word`'s ability to review
> this change.
Hm, yes, it would likely be easier to parse, I just went on a spree to
clean it up.
> You've also introduced uses of MASK_EXTR() and MASK_INSR(), which at
> least ought to be noted in the commit message. Technically I think it's
> a bugfix for multi_msi_enable(), because I think it now won't overflow
> the 3-bit field if an overly large num is passed in.
Hm, I've become used to MASK_{EXTR,INSR}(), so the change felt natural
since I was already adjusting the code.
>
> Bloat-o-meter reports:
>
> add/remove: 0/0 grow/shrink: 3/1 up/down: 15/-61 (-46)
> Function old new delta
> set_iommu_interrupt_handler 366 373 +7
> write_msi_msg 348 352 +4
> init_msi 574 578 +4
> pci_enable_msi 1084 1023 -61
>
>
> Taking the first example, that's caused by swapping this:
>
> > iommu->msi.msi.mpos = ( ((!!(control & 0x80)) == 1) ?
> > iommu->msi.msi_attrib.pos+16 : iommu->msi.msi_attrib.pos+16 -4);
>
> for this:
>
> > iommu->msi.msi.mpos = ((iommu->msi.msi_attrib.pos) + 16 -
> > (((!!((control) & 0x80))) ? 0 : 4));
>
> and code generation changing from a CMOV to straight-line arithmetic.
>
> In write_msi_msg(), we actually drop a conditional branch and replace it
> with straight-line arithmetic.
>
> init_msi() gets a substantial restructuring, but it looks like two
> branches are dropped.
>
> pci_enable_msi() has the biggest change, but doesn't obviously reduce
> the number of branches. There is clearly less register setup around
> existing branches, so my best guess is that the new macro forms are more
> amenable to common-sub-expression-elimination.
>
>
> Either way, it's all minor. Staring at the diff for long enough, I'm
> pretty sure it's all good.
Thanks.
> > ---
> > 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?
Thanks, Roger.
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:
>>> Prune unused macros and adjust the remaining ones to parenthesize macro
>>> arguments.
>>>
>>> No functional change intended.
>>>
>>> Singed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> It's a little early for carol season, isn't it?
>>
>> It would help to identify which macros are being dropped, because the
>> diff is far from simple to read.
>>
>> AFAICT, its:
>>
>> msi_disable()
>> msi_enable()
>> msix_enable()
>> msix_disable()
>> msix_unmask()
>> msix_mask()
>>
>> Splitting this change does make a marginal improvement in the diff, and
>> a substantial improvement in `git diff --color-word`'s ability to review
>> this change.
> Hm, yes, it would likely be easier to parse, I just went on a spree to
> clean it up.
>
>> You've also introduced uses of MASK_EXTR() and MASK_INSR(), which at
>> least ought to be noted in the commit message. Technically I think it's
>> a bugfix for multi_msi_enable(), because I think it now won't overflow
>> the 3-bit field if an overly large num is passed in.
> Hm, I've become used to MASK_{EXTR,INSR}(), so the change felt natural
> since I was already adjusting the code.
>
>> Bloat-o-meter reports:
>>
>> add/remove: 0/0 grow/shrink: 3/1 up/down: 15/-61 (-46)
>> Function old new delta
>> set_iommu_interrupt_handler 366 373 +7
>> write_msi_msg 348 352 +4
>> init_msi 574 578 +4
>> pci_enable_msi 1084 1023 -61
>>
>>
>> Taking the first example, that's caused by swapping this:
>>
>>> iommu->msi.msi.mpos = ( ((!!(control & 0x80)) == 1) ?
>>> iommu->msi.msi_attrib.pos+16 : iommu->msi.msi_attrib.pos+16 -4);
>> for this:
>>
>>> iommu->msi.msi.mpos = ((iommu->msi.msi_attrib.pos) + 16 -
>>> (((!!((control) & 0x80))) ? 0 : 4));
>> and code generation changing from a CMOV to straight-line arithmetic.
>>
>> In write_msi_msg(), we actually drop a conditional branch and replace it
>> with straight-line arithmetic.
>>
>> init_msi() gets a substantial restructuring, but it looks like two
>> branches are dropped.
>>
>> pci_enable_msi() has the biggest change, but doesn't obviously reduce
>> the number of branches. There is clearly less register setup around
>> existing branches, so my best guess is that the new macro forms are more
>> amenable to common-sub-expression-elimination.
>>
>>
>> Either way, it's all minor. Staring at the diff for long enough, I'm
>> pretty sure it's all good.
> Thanks.
>
>>> ---
>>> 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.
~Andrew
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.
© 2016 - 2024 Red Hat, Inc.