Re: [RFC PATCH 1/1] PCI:delay configuration of SRIOV capability

Re: [RFC PATCH 1/1] PCI:delay configuration of SRIOV capability

[Jesse Barnes]

On Sun, 6 Nov 2011 10:33:57 +0800
Ram Pai <linuxram@us.ibm.com> wrote:

>       The SRIOV capability, namely page size and total_vfs of a device are
>       configured during enumeration phase of the device.
>       This can potentially interfere with the PCI operations of the platform,
>       if the IOV capability of the device is not enabled.
> 
>       The following patch postpones the configuration of the IOV capability of the
>       device to a later point, when the IOV capability is explicitly enabled
>       by the device driver.
> 
>       The patch is tested on x86 and power platform.
> 
>       Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  drivers/pci/iov.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index b0446dd..c1a6f5c 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>  			return rc;
>  	}
>  
> +	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
> +
>  	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
>  	pci_block_user_cfg_access(dev);
>  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
> @@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
>  
>  found:
>  	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
> -	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
>  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
>  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
>  	if (!offset || (total > 1 && !stride))
> @@ -464,7 +465,6 @@ found:
>  		return -EIO;
>  
>  	pgsz &= ~(pgsz - 1);
> -	pci_write_config_dword(dev, pos + PCI_SRIOV_SYS_PGSIZE, pgsz);
>  
>  	nres = 0;
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {

Anyone want to volunteer a tested-by for this one?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [RFC PATCH 1/1] PCI:delay configuration of SRIOV capability

[Don Dutile]

On 12/05/2011 01:17 PM, Jesse Barnes wrote:
> On Sun, 6 Nov 2011 10:33:57 +0800
> Ram Pai<linuxram@us.ibm.com>  wrote:
>
>>        The SRIOV capability, namely page size and total_vfs of a device are
>>        configured during enumeration phase of the device.
>>        This can potentially interfere with the PCI operations of the platform,
>>        if the IOV capability of the device is not enabled.
>>
>>        The following patch postpones the configuration of the IOV capability of the
>>        device to a later point, when the IOV capability is explicitly enabled
>>        by the device driver.
>>
>>        The patch is tested on x86 and power platform.
>>
>>        Signed-off-by: Ram Pai<linuxram@us.ibm.com>
>> ---
>>   drivers/pci/iov.c |    4 ++--
>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index b0446dd..c1a6f5c 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>>   			return rc;
>>   	}
>>
>> +	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
>> +
>>   	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
>>   	pci_block_user_cfg_access(dev);
>>   	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>> @@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
>>
>>   found:
>>   	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
>> -	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
>>   	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET,&offset);
>>   	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE,&stride);
>>   	if (!offset || (total>  1&&  !stride))
>> @@ -464,7 +465,6 @@ found:
>>   		return -EIO;
>>
>>   	pgsz&= ~(pgsz - 1);
>> -	pci_write_config_dword(dev, pos + PCI_SRIOV_SYS_PGSIZE, pgsz);
>>
>>   	nres = 0;
>>   	for (i = 0; i<  PCI_SRIOV_NUM_BARS; i++) {
>
> Anyone want to volunteer a tested-by for this one?
>
> Thanks,

I tested this patch along with Ram's other recent patch for SRIOV:
[RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

With or without this patch, device assignment to a KVM guest
using these patches worked for me.  I used one PF in a quad-function
i350, configuring 2 VFs per i350.  I did 4 assign/deassigns to make
sure multiple enable/disables worked.

So, the patch doesn't correct any error I was seeing,
but it doesn't cause any regression either.

Tested-by: Donald Dutile <ddutile@redhat.com>

Re: [RFC PATCH 1/1] PCI:delay configuration of SRIOV capability

[Ram Pai]

On Mon, Dec 05, 2011 at 04:25:02PM -0500, Don Dutile wrote:
> On 12/05/2011 01:17 PM, Jesse Barnes wrote:
> >On Sun, 6 Nov 2011 10:33:57 +0800
> >Ram Pai<linuxram@us.ibm.com>  wrote:
> >
> >>       The SRIOV capability, namely page size and total_vfs of a device are
> >>       configured during enumeration phase of the device.
> >>       This can potentially interfere with the PCI operations of the platform,
> >>       if the IOV capability of the device is not enabled.
> >>
> >>       The following patch postpones the configuration of the IOV capability of the
> >>       device to a later point, when the IOV capability is explicitly enabled
> >>       by the device driver.
> >>
> >>       The patch is tested on x86 and power platform.
> >>
> >>       Signed-off-by: Ram Pai<linuxram@us.ibm.com>
> >>---
> >>  drivers/pci/iov.c |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>index b0446dd..c1a6f5c 100644
> >>--- a/drivers/pci/iov.c
> >>+++ b/drivers/pci/iov.c
> >>@@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> >>  			return rc;
> >>  	}
> >>
> >>+	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
> >>+
> >>  	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
> >>  	pci_block_user_cfg_access(dev);
> >>  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
> >>@@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
> >>
> >>  found:
> >>  	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
> >>-	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
> >>  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET,&offset);
> >>  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE,&stride);
> >>  	if (!offset || (total>  1&&  !stride))
> >>@@ -464,7 +465,6 @@ found:
> >>  		return -EIO;
> >>
> >>  	pgsz&= ~(pgsz - 1);
> >>-	pci_write_config_dword(dev, pos + PCI_SRIOV_SYS_PGSIZE, pgsz);
> >>
> >>  	nres = 0;
> >>  	for (i = 0; i<  PCI_SRIOV_NUM_BARS; i++) {
> >
> >Anyone want to volunteer a tested-by for this one?
> >
> >Thanks,
> 
> I tested this patch along with Ram's other recent patch for SRIOV:
> [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS
> 
> With or without this patch, device assignment to a KVM guest
> using these patches worked for me.  I used one PF in a quad-function
> i350, configuring 2 VFs per i350.  I did 4 assign/deassigns to make
> sure multiple enable/disables worked.
> 
> So, the patch doesn't correct any error I was seeing,
> but it doesn't cause any regression either.

Thanks Don for your tested-by.

I recollect you had a  issue, which I thought were not related or
addressed by my patches. If I can get a dmesg of your machine, I probably will
be able to see the relation.

RP

Re: [RFC PATCH 1/1] PCI:delay configuration of SRIOV capability

[Don Dutile]

On 12/06/2011 02:51 AM, Ram Pai wrote:
> On Mon, Dec 05, 2011 at 04:25:02PM -0500, Don Dutile wrote:
>> On 12/05/2011 01:17 PM, Jesse Barnes wrote:
>>> On Sun, 6 Nov 2011 10:33:57 +0800
>>> Ram Pai<linuxram@us.ibm.com>   wrote:
>>>
>>>>        The SRIOV capability, namely page size and total_vfs of a device are
>>>>        configured during enumeration phase of the device.
>>>>        This can potentially interfere with the PCI operations of the platform,
>>>>        if the IOV capability of the device is not enabled.
>>>>
>>>>        The following patch postpones the configuration of the IOV capability of the
>>>>        device to a later point, when the IOV capability is explicitly enabled
>>>>        by the device driver.
>>>>
>>>>        The patch is tested on x86 and power platform.
>>>>
>>>>        Signed-off-by: Ram Pai<linuxram@us.ibm.com>
>>>> ---
>>>>   drivers/pci/iov.c |    4 ++--
>>>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>>> index b0446dd..c1a6f5c 100644
>>>> --- a/drivers/pci/iov.c
>>>> +++ b/drivers/pci/iov.c
>>>> @@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>>>>   			return rc;
>>>>   	}
>>>>
>>>> +	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
>>>> +
>>>>   	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
>>>>   	pci_block_user_cfg_access(dev);
>>>>   	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>>>> @@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
>>>>
>>>>   found:
>>>>   	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
>>>> -	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
>>>>   	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET,&offset);
>>>>   	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE,&stride);
>>>>   	if (!offset || (total>   1&&   !stride))
>>>> @@ -464,7 +465,6 @@ found:
>>>>   		return -EIO;
>>>>
>>>>   	pgsz&= ~(pgsz - 1);
>>>> -	pci_write_config_dword(dev, pos + PCI_SRIOV_SYS_PGSIZE, pgsz);
>>>>
>>>>   	nres = 0;
>>>>   	for (i = 0; i<   PCI_SRIOV_NUM_BARS; i++) {
>>>
>>> Anyone want to volunteer a tested-by for this one?
>>>
>>> Thanks,
>>
>> I tested this patch along with Ram's other recent patch for SRIOV:
>> [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS
>>
>> With or without this patch, device assignment to a KVM guest
>> using these patches worked for me.  I used one PF in a quad-function
>> i350, configuring 2 VFs per i350.  I did 4 assign/deassigns to make
>> sure multiple enable/disables worked.
>>
>> So, the patch doesn't correct any error I was seeing,
>> but it doesn't cause any regression either.
>
> Thanks Don for your tested-by.
>
> I recollect you had a  issue, which I thought were not related or
> addressed by my patches. If I can get a dmesg of your machine, I probably will
> be able to see the relation.
>
> RP
>

My issue was more RHEL specific.  Your patches appear to resolve the issues
in a more general manner, and one that saner (to me... delay SRIOV resource allocation
and setup until a driver is actually enabled with it, instead of always on no matter
what the system use of SRIOV is).

- Don

Re: [RFC PATCH 1/1] PCI:delay configuration of SRIOV capability

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 2010 bytes --]

On Sun, 6 Nov 2011 10:33:57 +0800
Ram Pai <linuxram@us.ibm.com> wrote:

>       The SRIOV capability, namely page size and total_vfs of a device are
>       configured during enumeration phase of the device.
>       This can potentially interfere with the PCI operations of the platform,
>       if the IOV capability of the device is not enabled.
> 
>       The following patch postpones the configuration of the IOV capability of the
>       device to a later point, when the IOV capability is explicitly enabled
>       by the device driver.
> 
>       The patch is tested on x86 and power platform.
> 
>       Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  drivers/pci/iov.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index b0446dd..c1a6f5c 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -346,6 +346,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>  			return rc;
>  	}
>  
> +	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
> +
>  	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
>  	pci_block_user_cfg_access(dev);
>  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
> @@ -451,7 +453,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
>  
>  found:
>  	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
> -	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total);
>  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
>  	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
>  	if (!offset || (total > 1 && !stride))
> @@ -464,7 +465,6 @@ found:
>  		return -EIO;
>  
>  	pgsz &= ~(pgsz - 1);
> -	pci_write_config_dword(dev, pos + PCI_SRIOV_SYS_PGSIZE, pgsz);
>  
>  	nres = 0;
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {

Ok applied to linux-next with Don's tested by.  Thanks guys.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

On Sun, 6 Nov 2011 10:33:10 +0800
Ram Pai <linuxram@us.ibm.com> wrote:

>   All the PCI BARs of a device are enabled when the device is enabled using
>   pci_enable_device().  This unnecessarily enables SRIOV BARs of the device.
> 
>   On some platforms, which do not support SRIOV as yet, the pci_enable_device()
>   fails to enable the device if its SRIOV BARs are not allocated resources
>   correctly.
> 
>   The following patch fixes the above problem. The SRIOV BARs are now enabled
>   when IOV capability of the device is enabled in sriov_enable().
> 
>   NOTE: Note, there is subtle change in the pci_enable_device() API.
>   Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>   can fail.
> 
>   The patch has been touch tested on power and x86 platform.
> 
>   Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---

I think this is a good direction, but I'd like to get a tested-by or
two from people using SR-IOV...

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Ram Pai]

On Fri, Nov 11, 2011 at 10:01:43AM -0800, Jesse Barnes wrote:
> On Sun, 6 Nov 2011 10:33:10 +0800
> Ram Pai <linuxram@us.ibm.com> wrote:
> 
> >   All the PCI BARs of a device are enabled when the device is enabled using
> >   pci_enable_device().  This unnecessarily enables SRIOV BARs of the device.
> > 
> >   On some platforms, which do not support SRIOV as yet, the pci_enable_device()
> >   fails to enable the device if its SRIOV BARs are not allocated resources
> >   correctly.
> > 
> >   The following patch fixes the above problem. The SRIOV BARs are now enabled
> >   when IOV capability of the device is enabled in sriov_enable().
> > 
> >   NOTE: Note, there is subtle change in the pci_enable_device() API.
> >   Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
> >   can fail.
> > 
> >   The patch has been touch tested on power and x86 platform.
> > 
> >   Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> 
> I think this is a good direction, but I'd like to get a tested-by or
> two from people using SR-IOV...

I have people within IBM who have tested this patch successfully.
Michael Wang/Nish: Can you provide us a tested-by for this patch?

Prarit: Can you give this patch and the other patch a try?
the other patch is: [RFC PATCH 1/1] PCI:delay configuration of SRIOV capability

RP

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Michael Wang]

On 11/14/2011 12:33 PM, Ram Pai wrote:
> On Fri, Nov 11, 2011 at 10:01:43AM -0800, Jesse Barnes wrote:
>> On Sun, 6 Nov 2011 10:33:10 +0800
>> Ram Pai<linuxram@us.ibm.com>  wrote:
>>
>>>    All the PCI BARs of a device are enabled when the device is enabled using
>>>    pci_enable_device().  This unnecessarily enables SRIOV BARs of the device.
>>>
>>>    On some platforms, which do not support SRIOV as yet, the pci_enable_device()
>>>    fails to enable the device if its SRIOV BARs are not allocated resources
>>>    correctly.
>>>
>>>    The following patch fixes the above problem. The SRIOV BARs are now enabled
>>>    when IOV capability of the device is enabled in sriov_enable().
>>>
>>>    NOTE: Note, there is subtle change in the pci_enable_device() API.
>>>    Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>>>    can fail.
>>>
>>>    The patch has been touch tested on power and x86 platform.
>>>
>>>    Signed-off-by: Ram Pai<linuxram@us.ibm.com>
>>> ---
>> I think this is a good direction, but I'd like to get a tested-by or
>> two from people using SR-IOV...
> I have people within IBM who have tested this patch successfully.
> Michael Wang/Nish: Can you provide us a tested-by for this patch?
Hi

I have tested both patches with Fedora16 KVM kernel on a power machine.
My original issue is that the be2net module failed to be loaded, and the 
patch
fixes it.
After applied the patch, the be2net module can be loaded normally and new
network interfaces appeared.

Tested-by: Michael Wang <wangyun@linux.vnet.ibm.com>

Thanks,
Michael Wang
> Prarit: Can you give this patch and the other patch a try?
> the other patch is: [RFC PATCH 1/1] PCI:delay configuration of SRIOV capability
>
> RP

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On Sun, 6 Nov 2011 10:33:10 +0800
Ram Pai <linuxram@us.ibm.com> wrote:

>   All the PCI BARs of a device are enabled when the device is enabled using
>   pci_enable_device().  This unnecessarily enables SRIOV BARs of the device.
> 
>   On some platforms, which do not support SRIOV as yet, the pci_enable_device()
>   fails to enable the device if its SRIOV BARs are not allocated resources
>   correctly.
> 
>   The following patch fixes the above problem. The SRIOV BARs are now enabled
>   when IOV capability of the device is enabled in sriov_enable().
> 
>   NOTE: Note, there is subtle change in the pci_enable_device() API.
>   Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>   can fail.
> 
>   The patch has been touch tested on power and x86 platform.
> 
>   Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---

Applied to my for-linus branch, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 515 bytes --]

On Mon, Dec 5, 2011 at 10:32 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Sun, 6 Nov 2011 10:33:10 +0800
> Ram Pai <linuxram@us.ibm.com> wrote:
>
>>
>>   NOTE: Note, there is subtle change in the pci_enable_device() API.
>>   Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>>   can fail.
>>
>> ---
>
> Applied to my for-linus branch, thanks.
>

please don't push to linus now.

this one causes regression.

please check attached patch.

Thanks

Yinghai

[-- Attachment #2: fix_sriov_late.patch --]
[-- Type: text/x-patch, Size: 1577 bytes --]

[PATCH] pci: Fix hotplug of Express Module with pci bridges

Found hotplug of one setup does not work with recent change in pci tree.

After checking the bridge conf setup, found bridges get assigned, but not get enabled.

Finally found following commit, simplely ignore bridge resource when enabling pci device.

| commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
| Author: Ram Pai <linuxram@us.ibm.com>
| Date:   Sun Nov 6 10:33:10 2011 +0800
|
|    PCI: defer enablement of SRIOV BARS
|...
|    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
|    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
|    can fail.

Put back bridge resource and ROM resource checking to fix the problem.

That should fix regression like BIOS does not assign correct resource to bridge.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/pci.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
 	if (atomic_add_return(1, &dev->enable_cnt) > 1)
 		return 0;		/* already enabled */
 
-	for (i = 0; i < PCI_ROM_RESOURCE; i++)
+	/* only skip sriov related */
+	for (i = 0; i <= PCI_ROM_RESOURCE; i++)
+		if (dev->resource[i].flags & flags)
+			bars |= (1 << i);
+	for (i = PCI_BRIDGE_RESOURCES; i < DEVICE_COUNT_RESOURCE; i++)
 		if (dev->resource[i].flags & flags)
 			bars |= (1 << i);
 

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Ram Pai]

On Wed, Dec 07, 2011 at 12:22:47AM -0800, Yinghai Lu wrote:
> On Mon, Dec 5, 2011 at 10:32 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Sun, 6 Nov 2011 10:33:10 +0800
> > Ram Pai <linuxram@us.ibm.com> wrote:
> >
> >>
> >>   NOTE: Note, there is subtle change in the pci_enable_device() API.
> >>   Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
> >>   can fail.
> >>
> >> ---
> >
> > Applied to my for-linus branch, thanks.
> >
> 
> please don't push to linus now.
> 
> this one causes regression.
> 
> please check attached patch.
> 
> Thanks
> 
> Yinghai

> [PATCH] pci: Fix hotplug of Express Module with pci bridges
> 
> Found hotplug of one setup does not work with recent change in pci tree.
> 
> After checking the bridge conf setup, found bridges get assigned, but not get enabled.
> 
> Finally found following commit, simplely ignore bridge resource when enabling pci device.
> 
> | commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
> | Author: Ram Pai <linuxram@us.ibm.com>
> | Date:   Sun Nov 6 10:33:10 2011 +0800
> |
> |    PCI: defer enablement of SRIOV BARS
> |...
> |    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
> |    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
> |    can fail.
> 
> Put back bridge resource and ROM resource checking to fix the problem.
> 
> That should fix regression like BIOS does not assign correct resource to bridge.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/pci.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
>  	if (atomic_add_return(1, &dev->enable_cnt) > 1)
>  		return 0;		/* already enabled */
>  
> -	for (i = 0; i < PCI_ROM_RESOURCE; i++)
> +	/* only skip sriov related */
> +	for (i = 0; i <= PCI_ROM_RESOURCE; i++)
> +		if (dev->resource[i].flags & flags)
> +			bars |= (1 << i);
> +	for (i = PCI_BRIDGE_RESOURCES; i < DEVICE_COUNT_RESOURCE; i++)
>  		if (dev->resource[i].flags & flags)
>  			bars |= (1 << i);
>  

Oops. My patch inadvertently dropped ROM and BRIDGE resources.

This patch is right. However would it help if we did something like this
to avoid some code duplication?

	for (i = 0; i < DEVICE_COUNT_RESOURCE;
		i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++)
		if (dev->resource[i].flags & flags)
			bars |= (1 << i);

RP

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Yinghai Lu]

On Wed, Dec 7, 2011 at 1:25 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> On Wed, Dec 07, 2011 at 12:22:47AM -0800, Yinghai Lu wrote:
>> On Mon, Dec 5, 2011 at 10:32 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > On Sun, 6 Nov 2011 10:33:10 +0800
>> > Ram Pai <linuxram@us.ibm.com> wrote:
>> >
>> >>
>> >>   NOTE: Note, there is subtle change in the pci_enable_device() API.
>> >>   Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>> >>   can fail.
>> >>
>> >> ---
>> >
>> > Applied to my for-linus branch, thanks.
>> >
>>
>> please don't push to linus now.
>>
>> this one causes regression.
>>
>> please check attached patch.
>>
>> Thanks
>>
>> Yinghai
>
>> [PATCH] pci: Fix hotplug of Express Module with pci bridges
>>
>> Found hotplug of one setup does not work with recent change in pci tree.
>>
>> After checking the bridge conf setup, found bridges get assigned, but not get enabled.
>>
>> Finally found following commit, simplely ignore bridge resource when enabling pci device.
>>
>> | commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
>> | Author: Ram Pai <linuxram@us.ibm.com>
>> | Date:   Sun Nov 6 10:33:10 2011 +0800
>> |
>> |    PCI: defer enablement of SRIOV BARS
>> |...
>> |    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
>> |    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>> |    can fail.
>>
>> Put back bridge resource and ROM resource checking to fix the problem.
>>
>> That should fix regression like BIOS does not assign correct resource to bridge.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  drivers/pci/pci.c |    6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> Index: linux-2.6/drivers/pci/pci.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/pci/pci.c
>> +++ linux-2.6/drivers/pci/pci.c
>> @@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
>>       if (atomic_add_return(1, &dev->enable_cnt) > 1)
>>               return 0;               /* already enabled */
>>
>> -     for (i = 0; i < PCI_ROM_RESOURCE; i++)
>> +     /* only skip sriov related */
>> +     for (i = 0; i <= PCI_ROM_RESOURCE; i++)
>> +             if (dev->resource[i].flags & flags)
>> +                     bars |= (1 << i);
>> +     for (i = PCI_BRIDGE_RESOURCES; i < DEVICE_COUNT_RESOURCE; i++)
>>               if (dev->resource[i].flags & flags)
>>                       bars |= (1 << i);
>>
>
> Oops. My patch inadvertently dropped ROM and BRIDGE resources.
>
> This patch is right. However would it help if we did something like this
> to avoid some code duplication?
>
>        for (i = 0; i < DEVICE_COUNT_RESOURCE;
>                i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++)
>                if (dev->resource[i].flags & flags)
>                        bars |= (1 << i);

just don't want to make for () too big.

Thanks

Yinghai

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Don Dutile]

On 12/07/2011 04:25 AM, Ram Pai wrote:
> On Wed, Dec 07, 2011 at 12:22:47AM -0800, Yinghai Lu wrote:
>> On Mon, Dec 5, 2011 at 10:32 AM, Jesse Barnes<jbarnes@virtuousgeek.org>  wrote:
>>> On Sun, 6 Nov 2011 10:33:10 +0800
>>> Ram Pai<linuxram@us.ibm.com>  wrote:
>>>
>>>>
>>>>    NOTE: Note, there is subtle change in the pci_enable_device() API.
>>>>    Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>>>>    can fail.
>>>>
>>>> ---
>>>
>>> Applied to my for-linus branch, thanks.
>>>
>>
>> please don't push to linus now.
>>
>> this one causes regression.
>>
>> please check attached patch.
>>
>> Thanks
>>
>> Yinghai
>
>> [PATCH] pci: Fix hotplug of Express Module with pci bridges
>>
>> Found hotplug of one setup does not work with recent change in pci tree.
>>
>> After checking the bridge conf setup, found bridges get assigned, but not get enabled.
>>
>> Finally found following commit, simplely ignore bridge resource when enabling pci device.
>>
>> | commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
>> | Author: Ram Pai<linuxram@us.ibm.com>
>> | Date:   Sun Nov 6 10:33:10 2011 +0800
>> |
>> |    PCI: defer enablement of SRIOV BARS
>> |...
>> |    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
>> |    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>> |    can fail.
>>
>> Put back bridge resource and ROM resource checking to fix the problem.
>>
>> That should fix regression like BIOS does not assign correct resource to bridge.
>>
>> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>>
>> ---
>>   drivers/pci/pci.c |    6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> Index: linux-2.6/drivers/pci/pci.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/pci/pci.c
>> +++ linux-2.6/drivers/pci/pci.c
>> @@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
>>   	if (atomic_add_return(1,&dev->enable_cnt)>  1)
>>   		return 0;		/* already enabled */
>>
>> -	for (i = 0; i<  PCI_ROM_RESOURCE; i++)
>> +	/* only skip sriov related */
>> +	for (i = 0; i<= PCI_ROM_RESOURCE; i++)
>> +		if (dev->resource[i].flags&  flags)
>> +			bars |= (1<<  i);
>> +	for (i = PCI_BRIDGE_RESOURCES; i<  DEVICE_COUNT_RESOURCE; i++)
>>   		if (dev->resource[i].flags&  flags)
>>   			bars |= (1<<  i);
>>
>
> Oops. My patch inadvertently dropped ROM and BRIDGE resources.
>
> This patch is right. However would it help if we did something like this
> to avoid some code duplication?
>
> 	for (i = 0; i<  DEVICE_COUNT_RESOURCE;
> 		i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++)
> 		if (dev->resource[i].flags&  flags)
> 			bars |= (1<<  i);
>

why not something more explicit like:

	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
		if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END))
			continue; /* skip sriov related resources */
                 if (dev->resource[i].flags & flags)
                         bars |= (1 << i);
	}

> RP
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Nishanth Aravamudan]

On 07.12.2011 [15:23:38 -0500], Don Dutile wrote:
> On 12/07/2011 04:25 AM, Ram Pai wrote:
> >On Wed, Dec 07, 2011 at 12:22:47AM -0800, Yinghai Lu wrote:
> >>On Mon, Dec 5, 2011 at 10:32 AM, Jesse Barnes<jbarnes@virtuousgeek.org>  wrote:
> >>>On Sun, 6 Nov 2011 10:33:10 +0800
> >>>Ram Pai<linuxram@us.ibm.com>  wrote:
> >>>
> >>>>
> >>>>   NOTE: Note, there is subtle change in the pci_enable_device() API.
> >>>>   Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
> >>>>   can fail.
> >>>>
> >>>>---
> >>>
> >>>Applied to my for-linus branch, thanks.
> >>>
> >>
> >>please don't push to linus now.
> >>
> >>this one causes regression.
> >>
> >>please check attached patch.
> >>
> >>Thanks
> >>
> >>Yinghai
> >
> >>[PATCH] pci: Fix hotplug of Express Module with pci bridges
> >>
> >>Found hotplug of one setup does not work with recent change in pci tree.
> >>
> >>After checking the bridge conf setup, found bridges get assigned, but not get enabled.
> >>
> >>Finally found following commit, simplely ignore bridge resource when enabling pci device.
> >>
> >>| commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
> >>| Author: Ram Pai<linuxram@us.ibm.com>
> >>| Date:   Sun Nov 6 10:33:10 2011 +0800
> >>|
> >>|    PCI: defer enablement of SRIOV BARS
> >>|...
> >>|    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
> >>|    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
> >>|    can fail.
> >>
> >>Put back bridge resource and ROM resource checking to fix the problem.
> >>
> >>That should fix regression like BIOS does not assign correct resource to bridge.
> >>
> >>Signed-off-by: Yinghai Lu<yinghai@kernel.org>
> >>
> >>---
> >>  drivers/pci/pci.c |    6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >>Index: linux-2.6/drivers/pci/pci.c
> >>===================================================================
> >>--- linux-2.6.orig/drivers/pci/pci.c
> >>+++ linux-2.6/drivers/pci/pci.c
> >>@@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
> >>  	if (atomic_add_return(1,&dev->enable_cnt)>  1)
> >>  		return 0;		/* already enabled */
> >>
> >>-	for (i = 0; i<  PCI_ROM_RESOURCE; i++)
> >>+	/* only skip sriov related */
> >>+	for (i = 0; i<= PCI_ROM_RESOURCE; i++)
> >>+		if (dev->resource[i].flags&  flags)
> >>+			bars |= (1<<  i);
> >>+	for (i = PCI_BRIDGE_RESOURCES; i<  DEVICE_COUNT_RESOURCE; i++)
> >>  		if (dev->resource[i].flags&  flags)
> >>  			bars |= (1<<  i);
> >>
> >
> >Oops. My patch inadvertently dropped ROM and BRIDGE resources.
> >
> >This patch is right. However would it help if we did something like this
> >to avoid some code duplication?
> >
> >	for (i = 0; i<  DEVICE_COUNT_RESOURCE;
> >		i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++)
> >		if (dev->resource[i].flags&  flags)
> >			bars |= (1<<  i);
> >
> 
> why not something more explicit like:
> 
> 	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> 		if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END))
> 			continue; /* skip sriov related resources */
>                 if (dev->resource[i].flags & flags)
>                         bars |= (1 << i);
> 	}

I agree with the more explicit approach, self-commenting and easier to
read. Although, you may want to add a comment as to *why* we're skipping
IOV BARs here.

-Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Don Dutile]

On 12/07/2011 03:35 PM, Nishanth Aravamudan wrote:
> On 07.12.2011 [15:23:38 -0500], Don Dutile wrote:
>> On 12/07/2011 04:25 AM, Ram Pai wrote:
>>> On Wed, Dec 07, 2011 at 12:22:47AM -0800, Yinghai Lu wrote:
>>>> On Mon, Dec 5, 2011 at 10:32 AM, Jesse Barnes<jbarnes@virtuousgeek.org>   wrote:
>>>>> On Sun, 6 Nov 2011 10:33:10 +0800
>>>>> Ram Pai<linuxram@us.ibm.com>   wrote:
>>>>>
>>>>>>
>>>>>>    NOTE: Note, there is subtle change in the pci_enable_device() API.
>>>>>>    Any driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>>>>>>    can fail.
>>>>>>
>>>>>> ---
>>>>>
>>>>> Applied to my for-linus branch, thanks.
>>>>>
>>>>
>>>> please don't push to linus now.
>>>>
>>>> this one causes regression.
>>>>
>>>> please check attached patch.
>>>>
>>>> Thanks
>>>>
>>>> Yinghai
>>>
>>>> [PATCH] pci: Fix hotplug of Express Module with pci bridges
>>>>
>>>> Found hotplug of one setup does not work with recent change in pci tree.
>>>>
>>>> After checking the bridge conf setup, found bridges get assigned, but not get enabled.
>>>>
>>>> Finally found following commit, simplely ignore bridge resource when enabling pci device.
>>>>
>>>> | commit bbef98ab0f019f1b0c25c1acdf1683c68933d41b
>>>> | Author: Ram Pai<linuxram@us.ibm.com>
>>>> | Date:   Sun Nov 6 10:33:10 2011 +0800
>>>> |
>>>> |    PCI: defer enablement of SRIOV BARS
>>>> |...
>>>> |    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any
>>>> |    driver that depends on SRIOV BARS to be enabled in pci_enable_device()
>>>> |    can fail.
>>>>
>>>> Put back bridge resource and ROM resource checking to fix the problem.
>>>>
>>>> That should fix regression like BIOS does not assign correct resource to bridge.
>>>>
>>>> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>>>>
>>>> ---
>>>>   drivers/pci/pci.c |    6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> Index: linux-2.6/drivers/pci/pci.c
>>>> ===================================================================
>>>> --- linux-2.6.orig/drivers/pci/pci.c
>>>> +++ linux-2.6/drivers/pci/pci.c
>>>> @@ -1139,7 +1139,11 @@ static int __pci_enable_device_flags(str
>>>>   	if (atomic_add_return(1,&dev->enable_cnt)>   1)
>>>>   		return 0;		/* already enabled */
>>>>
>>>> -	for (i = 0; i<   PCI_ROM_RESOURCE; i++)
>>>> +	/* only skip sriov related */
>>>> +	for (i = 0; i<= PCI_ROM_RESOURCE; i++)
>>>> +		if (dev->resource[i].flags&   flags)
>>>> +			bars |= (1<<   i);
>>>> +	for (i = PCI_BRIDGE_RESOURCES; i<   DEVICE_COUNT_RESOURCE; i++)
>>>>   		if (dev->resource[i].flags&   flags)
>>>>   			bars |= (1<<   i);
>>>>
>>>
>>> Oops. My patch inadvertently dropped ROM and BRIDGE resources.
>>>
>>> This patch is right. However would it help if we did something like this
>>> to avoid some code duplication?
>>>
>>> 	for (i = 0; i<   DEVICE_COUNT_RESOURCE;
>>> 		i == PCI_ROM_RESOURCE ? PCI_BRIDGE_RESOURCES: i++)
>>> 		if (dev->resource[i].flags&   flags)
>>> 			bars |= (1<<   i);
>>>
>>
>> why not something more explicit like:
>>
>> 	for (i = 0; i<  DEVICE_COUNT_RESOURCE; i++) {
>> 		if ((i>=  PCI_IOV_RESOURCES)&&  (i<= PCI_IOV_RESOURCE_END))
>> 			continue; /* skip sriov related resources */
>>                  if (dev->resource[i].flags&  flags)
>>                          bars |= (1<<  i);
>> 	}
>
> I agree with the more explicit approach, self-commenting and easier to
> read. Although, you may want to add a comment as to *why* we're skipping
> IOV BARs here.
>
+1  for those not living it, yes, good for historical (& noobie) understanding

> -Nish
>

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Yinghai Lu]

On Wed, Dec 7, 2011 at 12:23 PM, Don Dutile <ddutile@redhat.com> wrote:
> why not something more explicit like:
>
>        for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>                if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END))
>                        continue; /* skip sriov related resources */
>
>                if (dev->resource[i].flags & flags)
>                        bars |= (1 << i);
>        }

no, we should not put checking in the loop.

also you will have problem when CONFIG_PCI_IOV is not defined.

Thanks

Yinghai

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Ram Pai]

On Wed, Dec 07, 2011 at 03:11:56PM -0800, Yinghai Lu wrote:
> On Wed, Dec 7, 2011 at 12:23 PM, Don Dutile <ddutile@redhat.com> wrote:
> > why not something more explicit like:
> >
> >        for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> >                if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END))
> >                        continue; /* skip sriov related resources */
> >
> >                if (dev->resource[i].flags & flags)
> >                        bars |= (1 << i);
> >        }

I like this approach too. Offcourse the SRIOV skipping has to be one under #ifdef CONFIG_PCI_IOV.

Yinghai/Jesse, do you want to make a patch on top of the current jesse's to-linus
tree or want to revert my patch and apply your fix? If it is the former, do you want me
to make the patch?

RP

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Jesse Barnes]

On Thu, 8 Dec 2011 10:50:39 +0800
Ram Pai <linuxram@us.ibm.com> wrote:

> On Wed, Dec 07, 2011 at 03:11:56PM -0800, Yinghai Lu wrote:
> > On Wed, Dec 7, 2011 at 12:23 PM, Don Dutile <ddutile@redhat.com> wrote:
> > > why not something more explicit like:
> > >
> > >        for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > >                if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END))
> > >                        continue; /* skip sriov related resources */
> > >
> > >                if (dev->resource[i].flags & flags)
> > >                        bars |= (1 << i);
> > >        }
> 
> I like this approach too. Offcourse the SRIOV skipping has to be one under #ifdef CONFIG_PCI_IOV.
> 
> Yinghai/Jesse, do you want to make a patch on top of the current jesse's to-linus
> tree or want to revert my patch and apply your fix? If it is the former, do you want me
> to make the patch?

I like the above best as well with the comment.

Can you send me a replacement patch?  I'd rather have that than some
breakage in the patches I have in my for-linus branch.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Yinghai Lu]

On Thu, Dec 8, 2011 at 5:53 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 8 Dec 2011 10:50:39 +0800
> Ram Pai <linuxram@us.ibm.com> wrote:
>
>> On Wed, Dec 07, 2011 at 03:11:56PM -0800, Yinghai Lu wrote:
>> > On Wed, Dec 7, 2011 at 12:23 PM, Don Dutile <ddutile@redhat.com> wrote:
>> > > why not something more explicit like:
>> > >
>> > >        for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>> > >                if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END))
>> > >                        continue; /* skip sriov related resources */
>> > >
>> > >                if (dev->resource[i].flags & flags)
>> > >                        bars |= (1 << i);
>> > >        }
>>
>> I like this approach too. Offcourse the SRIOV skipping has to be one under #ifdef CONFIG_PCI_IOV.

I was always told:

#ifdef is ugly. Try to avoid it.

>>
>> Yinghai/Jesse, do you want to make a patch on top of the current jesse's to-linus
>> tree or want to revert my patch and apply your fix? If it is the former, do you want me
>> to make the patch?
>
> I like the above best as well with the comment.
>
> Can you send me a replacement patch?  I'd rather have that than some
> breakage in the patches I have in my for-linus branch.

yes, update your patch.

We should not break the bisecting if possible before it hit linus tree.

Thanks

Yinghai

[PATCH 1/1 v2]PCI: defer enablement of SRIOV BARS

[Ram Pai]

    	PCI: defer enablement of SRIOV BARS

    All the PCI BARs of a device are enabled when the device is enabled using
    pci_enable_device().  This unnecessarily enables SRIOV BARs of the device.

    On some platforms, which do not support SRIOV; as yet, the pci_enable_device()
    fails to enable the device if its SRIOV BARs are not allocated resources
    correctly.

    The following patch fixes the above problem. The SRIOV BARs are now enabled
    when IOV capability of the device is enabled in sriov_enable().

    NOTE: Note, there is subtle change in the pci_enable_device() API.  Any driver
    that depends on SRIOV BARS to be enabled in pci_enable_device() can fail.

    Changelog v2: Fixed a bug reported by Yinghai.The bug lead to ROM and
	BRIDGE BARs getting disabled.  Thanks for code snippets from Yinghai
	and Don Dutile.

   Signed-off-by: Ram Pai <linuxram@us.ibm.com

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9b4e88c..b0446dd 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -282,6 +282,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	struct resource *res;
 	struct pci_dev *pdev;
 	struct pci_sriov *iov = dev->sriov;
+	int bars = 0;
 
 	if (!nr_virtfn)
 		return 0;
@@ -306,6 +307,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 
 	nres = 0;
 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
+		bars |= (1 << (i + PCI_IOV_RESOURCES));
 		res = dev->resource + PCI_IOV_RESOURCES + i;
 		if (res->parent)
 			nres++;
@@ -323,6 +325,11 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 		return -ENOMEM;
 	}
 
+	if (pci_enable_resources(dev, bars)) {
+		dev_err(&dev->dev, "SR-IOV: IOV BARS not allocated\n");
+		return -ENOMEM;
+	}
+
 	if (iov->link != dev->devfn) {
 		pdev = pci_get_slot(dev->bus, iov->link);
 		if (!pdev)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6f45a73..b548bec 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1126,9 +1126,14 @@ static int __pci_enable_device_flags(struct pci_dev *dev,
 	if (atomic_add_return(1, &dev->enable_cnt) > 1)
 		return 0;		/* already enabled */
 
-	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+#ifdef CONFIG_PCI_IOV
+		if ((i >=  PCI_IOV_RESOURCES) && (i <= PCI_IOV_RESOURCE_END))
+			continue; /* skip sriov related resources */
+#endif
 		if (dev->resource[i].flags & flags)
 			bars |= (1 << i);
+	}
 
 	err = do_pci_enable_device(dev, bars);
 	if (err < 0)

Re: [RFC PATCH 1/1]PCI: defer enablement of SRIOV BARS

[Don Dutile]

On 12/07/2011 09:50 PM, Ram Pai wrote:
> On Wed, Dec 07, 2011 at 03:11:56PM -0800, Yinghai Lu wrote:
>> On Wed, Dec 7, 2011 at 12:23 PM, Don Dutile<ddutile@redhat.com>  wrote:
>>> why not something more explicit like:
>>>
>>>         for (i = 0; i<  DEVICE_COUNT_RESOURCE; i++) {
>>>                 if ((i>=  PCI_IOV_RESOURCES)&&  (i<= PCI_IOV_RESOURCE_END))
>>>                         continue; /* skip sriov related resources */
>>>
>>>                 if (dev->resource[i].flags&  flags)
>>>                         bars |= (1<<  i);
>>>         }
>
> I like this approach too. Offcourse the SRIOV skipping has to be one under #ifdef CONFIG_PCI_IOV.
>
+1. Sorry, I forgot that the kernel can be built w/o SRIOV support.


> Yinghai/Jesse, do you want to make a patch on top of the current jesse's to-linus
> tree or want to revert my patch and apply your fix? If it is the former, do you want me
> to make the patch?
>
> RP
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html