Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

["Jan Beulich" <jbeulich@novell.com>]

>>> Grant Grundler <grundler@parisc-linux.org> 08.11.08 09:28 >>>
>On Fri, Nov 07, 2008 at 08:53:49AM +0000, Jan Beulich wrote:
>> @@ -389,9 +364,8 @@ static int msi_capability_init(struct pc
>>  		pci_read_config_dword(dev,
>>  			msi_mask_bits_reg(pos, entry->msi_attrib.is_64),
>>  			&maskbits);
>> -		temp = (1 << multi_msi_capable(control));
>> -		temp = ((temp - 1) & ~temp);
>> -		maskbits |= temp;
>> +		temp = 1U << (multi_msi_capable(control) - 1);
>> +		maskbits |= (temp << 1) - 1;
>
>Isn't this the new code the same as:
>    maskbits |= (1U << multi_msi_capable(control)) - 1;

No.

>So the "& ~temp" got dropped...which looks correct to me.
>"& ~temp" isn't needed given only one bit could be set in temp. 
>
>In any case, I'm feeling a bit dense since I'm not seeing the problem.
>If multi_msi_capable(control) is 5, how is the shift of a
>signed int ("1") undefined?

multi_msi_capable() already shifts 1 by the value read out of the control
word, so if the bit field read is 5, we'd shift 1 left by 32 here.

>> @@ -23,7 +23,7 @@
>>  #define multi_msi_capable(control) \
>>  	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>>  #define multi_msi_enable(control, num) \
>> -	control |= (((num >> 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>> +	control |= ((ilog2(num) << 4) & PCI_MSI_FLAGS_QSIZE)
>
>I've no clue what the issue is here.
>Can you point at a section in the PCI sec that defines this?

What this apparently tries to do is set the number of vectors the device
is allowed to actually use (i.e. the field named 'Multiple Message Enable' in
section 6.8.1.3 of the 3.0 copy I'm looking at). Actually, as I now realize
this probably rather ought to be ilog2(num - 1) + 1, since the spec
requires the requested value to be rounded up, not down.

Jan