Re: [PATCH] PCI: Add guard to avoid mapping a invalid msix base address

[Yijing Wang <wangyijing@huawei.com>]

>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index fd60806..cd401a2 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -699,6 +699,9 @@ static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries)
>>  	pci_read_config_dword(dev, dev->msix_cap + PCI_MSIX_TABLE,
>>  			      &table_offset);
>>  	bir = (u8)(table_offset & PCI_MSIX_TABLE_BIR);
>> +	if (!pci_resource_flags(dev, bir))
>> +		return NULL;
> 
> This definitely seems like a good idea.  We should not just assume that
> space has been assigned for the BAR.  
> 
> However, I don't think we should test the resource flags for zero.
> reset_resource() does set the flags to zero, but I think that is a mistake.
> The PCI core should retain the information about the type and size of the
> BAR, even if it was unable to assign space for it.  So eventually
> reset_resource() should be reworked somehow.
> 
> I would like reset_resource() to set the IORESOURCE_UNSET bit in the flags.
> That would let %pR print it correctly, e.g., "[mem size 0x1000]".  But I
> don't know what other code in setup-bus.c depends on the assumption that
> "flags == 0" means something important.

Agree, "flags == 0" is not clear enough to point the status of the resource.

> 
> In the meantime, I guess you could do something like:
> 
>   flags = pci_resource_flags(dev, bir);
>   if (!flags || (flags & IORESOURCE_UNSET))
>     return NULL;

OK, I will update it, thanks.

Thanks!
Yijing.

> 
>> +
>>  	table_offset &= PCI_MSIX_TABLE_OFFSET;
>>  	phys_addr = pci_resource_start(dev, bir) + table_offset;
>>  
>> -- 
>> 1.7.1
>>
> 
> .
> 


-- 
Thanks!
Yijing