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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

> > > > > +#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?
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)