Re: [PATCH v9 3/3] PCI: Don't extend device's size when using default alignment for all devices

[Yongji Xie <elohimes@gmail.com>]

On Thu, Mar 23, 2017 at 04:55:58PM -0500, Bjorn Helgaas wrote:
>
> On Wed, Feb 15, 2017 at 02:45:06PM +0800, Yongji Xie wrote:
>> Currently we reassign the alignment by extending resources' size in
>> pci_reassigndev_resource_alignment(). This could potentially break
>> some drivers when the driver uses the size to locate register
>> whose length is related to the size. Some examples as below:
>
> I understand the motivation to stop changing the resource size.  That
> makes good sense.
>
>> - misc\Hpilo.c:
>
> This isn't Windows; please use regular Linux forward slashes here.
>

Will fix it.

>> off = pci_resource_len(pdev, bar) - 0x2000;
>>
>> - net\ethernet\chelsio\cxgb4\cxgb4_uld.h:
>> (pci_resource_len((pdev), 2) - roundup_pow_of_two((vres)->ocq.size))
>>
>> - infiniband\hw\nes\Nes_hw.c:
>> num_pds = pci_resource_len(nesdev->pcidev, BAR_1) >> PAGE_SHIFT;
>>
>> This risk could be easily prevented before because we only had one way
>> (kernel parameter resource_alignment) to touch those codes. And even
>> some users may be happy to see the extended size.
>>
>> But now we define a default alignment for all devices which would also
>> touch those codes. It would be hard to prevent the risk in this case. So
>> this patch tries to use START_ALIGNMENT to identify the resource's
>> alignment without extending the size when the alignment reassigning is
>> caused by the default alignment.
>
> But I don't understand the new START_ALIGNMENT case.
>

The START_ALIGNMENT case will be used when we have default alignment
for PCI devices, which can identify the resource's alignment without
extending the size. If we don't use START_ALIGNMENT for default alignment,
resources' size would be extended by default, that means the system's 
default action could potentially break drivers, it looks unreasonable.

>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>> drivers/pci/pci.c |   34 ++++++++++++++++++++++++----------
>> 1 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 2622e9b..0017e5e 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4958,7 +4958,8 @@ void pci_ignore_hotplug(struct pci_dev *dev)
>>  * RETURNS: Resource alignment if it is specified.
>>  *          Zero if it is not specified.
>
> Since there's function doc, please update it to reflect the new
> "resize" return parameter.
>

Will do it.

> What does "resize" mean?  I can't figure it out from your code.

"resize" means we will update the resource'alignment by extending the 
resource's size. This would happen when we use kernel parameter to identify 
resource's alignment. If the resource's alignment is identified by the default 
alignment function(macro) rather than kernel parameter, "resize" should be 
false, which means we update the resource'alignment without extending the 
resource's size.

Thanks,
Yongji