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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

>>>>> +#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)