From: Kuppuswamy Sathyanarayanan <[email protected]>
Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
other VFs") calculates and caches the cfg_size for VF0 device before
initializing the pcie_cap of the device which results in using incorrect
cfg_size for all VF devices > 0. So set pcie_cap of the device before
calculating the cfg_size of VF0 device.
Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
other VFs")
Cc: Ashok Raj <[email protected]>
Suggested-by: Mike Campin <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
Changes since v1:
* Fixed a typo in commit message.
drivers/pci/iov.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 3aa115ed3a65..2869011c0e35 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
virtfn->device = iov->vf_device;
virtfn->is_virtfn = 1;
virtfn->physfn = pci_dev_get(dev);
+ virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP);
if (id == 0)
pci_read_vf_config_common(virtfn);
--
2.21.0
This looks to basically be a duplicate of what Alex Williamson posted
8 days ago: https://lore.kernel.org/linux-pci/[email protected]/
Alex says Hao Zheng had a similar patch as well.
On Wed, Jun 12, 2019 at 12:08 PM
<[email protected]> wrote:
>
> From: Kuppuswamy Sathyanarayanan <[email protected]>
>
> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> other VFs") calculates and caches the cfg_size for VF0 device before
> initializing the pcie_cap of the device which results in using incorrect
> cfg_size for all VF devices > 0. So set pcie_cap of the device before
> calculating the cfg_size of VF0 device.
>
> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> other VFs")
> Cc: Ashok Raj <[email protected]>
> Suggested-by: Mike Campin <[email protected]>
> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> ---
>
> Changes since v1:
> * Fixed a typo in commit message.
>
> drivers/pci/iov.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 3aa115ed3a65..2869011c0e35 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> virtfn->device = iov->vf_device;
> virtfn->is_virtfn = 1;
> virtfn->physfn = pci_dev_get(dev);
> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP);
>
> if (id == 0)
> pci_read_vf_config_common(virtfn);
> --
> 2.21.0
>
On Wed, 12 Jun 2019 10:06:47 -0700
[email protected] wrote:
> From: Kuppuswamy Sathyanarayanan <[email protected]>
>
> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> other VFs") calculates and caches the cfg_size for VF0 device before
> initializing the pcie_cap of the device which results in using incorrect
> cfg_size for all VF devices > 0. So set pcie_cap of the device before
> calculating the cfg_size of VF0 device.
>
> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> other VFs")
> Cc: Ashok Raj <[email protected]>
> Suggested-by: Mike Campin <[email protected]>
> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> ---
>
> Changes since v1:
> * Fixed a typo in commit message.
>
> drivers/pci/iov.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 3aa115ed3a65..2869011c0e35 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> virtfn->device = iov->vf_device;
> virtfn->is_virtfn = 1;
> virtfn->physfn = pci_dev_get(dev);
> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP);
>
> if (id == 0)
> pci_read_vf_config_common(virtfn);
Why not re-order until after we've setup pcie_cap?
https://lore.kernel.org/linux-pci/[email protected]/T/#
Thanks,
Alex
On 6/12/19 11:19 AM, Alex Williamson wrote:
> On Wed, 12 Jun 2019 10:06:47 -0700
> [email protected] wrote:
>
>> From: Kuppuswamy Sathyanarayanan <[email protected]>
>>
>> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
>> other VFs") calculates and caches the cfg_size for VF0 device before
>> initializing the pcie_cap of the device which results in using incorrect
>> cfg_size for all VF devices > 0. So set pcie_cap of the device before
>> calculating the cfg_size of VF0 device.
>>
>> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
>> other VFs")
>> Cc: Ashok Raj <[email protected]>
>> Suggested-by: Mike Campin <[email protected]>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
>> ---
>>
>> Changes since v1:
>> * Fixed a typo in commit message.
>>
>> drivers/pci/iov.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index 3aa115ed3a65..2869011c0e35 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
>> virtfn->device = iov->vf_device;
>> virtfn->is_virtfn = 1;
>> virtfn->physfn = pci_dev_get(dev);
>> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP);
>>
>> if (id == 0)
>> pci_read_vf_config_common(virtfn);
> Why not re-order until after we've setup pcie_cap?
>
> https://lore.kernel.org/linux-pci/[email protected]/T/#
pci_read_vf_config_common() also caches values for properties like
class, hdr_type, susbsystem_vendor/device. These values are read/used in
pci_setup_device(). So if we can use cached values in
pci_setup_device(), we don't have to read them from registers twice for
each device.
>
> Thanks,
> Alex
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
On Wed, 12 Jun 2019 11:41:36 -0700
sathyanarayanan kuppuswamy <[email protected]>
wrote:
> On 6/12/19 11:19 AM, Alex Williamson wrote:
> > On Wed, 12 Jun 2019 10:06:47 -0700
> > [email protected] wrote:
> >
> >> From: Kuppuswamy Sathyanarayanan <[email protected]>
> >>
> >> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> >> other VFs") calculates and caches the cfg_size for VF0 device before
> >> initializing the pcie_cap of the device which results in using incorrect
> >> cfg_size for all VF devices > 0. So set pcie_cap of the device before
> >> calculating the cfg_size of VF0 device.
> >>
> >> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> >> other VFs")
> >> Cc: Ashok Raj <[email protected]>
> >> Suggested-by: Mike Campin <[email protected]>
> >> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> >> ---
> >>
> >> Changes since v1:
> >> * Fixed a typo in commit message.
> >>
> >> drivers/pci/iov.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >> index 3aa115ed3a65..2869011c0e35 100644
> >> --- a/drivers/pci/iov.c
> >> +++ b/drivers/pci/iov.c
> >> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> >> virtfn->device = iov->vf_device;
> >> virtfn->is_virtfn = 1;
> >> virtfn->physfn = pci_dev_get(dev);
> >> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP);
> >>
> >> if (id == 0)
> >> pci_read_vf_config_common(virtfn);
> > Why not re-order until after we've setup pcie_cap?
> >
> > https://lore.kernel.org/linux-pci/[email protected]/T/#
>
> pci_read_vf_config_common() also caches values for properties like
> class, hdr_type, susbsystem_vendor/device. These values are read/used in
> pci_setup_device(). So if we can use cached values in
> pci_setup_device(), we don't have to read them from registers twice for
> each device.
Sorry, I missed that dependency, a bit too subtle. It's still pretty
ugly that pci_setup_device()->set_pcie_port_type() is the canonical
location for setting pcie_cap and now we need to kludge it earlier.
What about the question in the self follow-up to my patch in the link
above, can we simply assume 4K config space on a VF? Thanks,
Alex
On Wed, Jun 12, 2019 at 12:58:17PM -0600, Alex Williamson wrote:
> On Wed, 12 Jun 2019 11:41:36 -0700
> sathyanarayanan kuppuswamy <[email protected]>
> wrote:
>
> > On 6/12/19 11:19 AM, Alex Williamson wrote:
> > > On Wed, 12 Jun 2019 10:06:47 -0700
> > > [email protected] wrote:
> > >
> > >> From: Kuppuswamy Sathyanarayanan <[email protected]>
> > >>
> > >> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> > >> other VFs") calculates and caches the cfg_size for VF0 device before
> > >> initializing the pcie_cap of the device which results in using incorrect
> > >> cfg_size for all VF devices > 0. So set pcie_cap of the device before
> > >> calculating the cfg_size of VF0 device.
> > >>
> > >> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> > >> other VFs")
> > >> Cc: Ashok Raj <[email protected]>
> > >> Suggested-by: Mike Campin <[email protected]>
> > >> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> > >> ---
> > >>
> > >> Changes since v1:
> > >> * Fixed a typo in commit message.
> > >>
> > >> drivers/pci/iov.c | 1 +
> > >> 1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > >> index 3aa115ed3a65..2869011c0e35 100644
> > >> --- a/drivers/pci/iov.c
> > >> +++ b/drivers/pci/iov.c
> > >> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> > >> virtfn->device = iov->vf_device;
> > >> virtfn->is_virtfn = 1;
> > >> virtfn->physfn = pci_dev_get(dev);
> > >> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP);
> > >>
> > >> if (id == 0)
> > >> pci_read_vf_config_common(virtfn);
> > > Why not re-order until after we've setup pcie_cap?
> > >
> > > https://lore.kernel.org/linux-pci/[email protected]/T/#
> >
> > pci_read_vf_config_common() also caches values for properties like
> > class, hdr_type, susbsystem_vendor/device. These values are read/used in
> > pci_setup_device(). So if we can use cached values in
> > pci_setup_device(), we don't have to read them from registers twice for
> > each device.
>
> Sorry, I missed that dependency, a bit too subtle. It's still pretty
> ugly that pci_setup_device()->set_pcie_port_type() is the canonical
> location for setting pcie_cap and now we need to kludge it earlier.
> What about the question in the self follow-up to my patch in the link
> above, can we simply assume 4K config space on a VF? Thanks,
There should be no issue simply reading them once? I don't know
what that exact optimization saves, unless some broken VFs didn't
actually expose all the capabilities in config space and this happens
to workaround the problem.
+ Karim
Cheers,
Ashok
On Wed, 12 Jun 2019 12:03:03 -0700
"Raj, Ashok" <[email protected]> wrote:
> On Wed, Jun 12, 2019 at 12:58:17PM -0600, Alex Williamson wrote:
> > On Wed, 12 Jun 2019 11:41:36 -0700
> > sathyanarayanan kuppuswamy <[email protected]>
> > wrote:
> >
> > > On 6/12/19 11:19 AM, Alex Williamson wrote:
> > > > On Wed, 12 Jun 2019 10:06:47 -0700
> > > > [email protected] wrote:
> > > >
> > > >> From: Kuppuswamy Sathyanarayanan <[email protected]>
> > > >>
> > > >> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> > > >> other VFs") calculates and caches the cfg_size for VF0 device before
> > > >> initializing the pcie_cap of the device which results in using incorrect
> > > >> cfg_size for all VF devices > 0. So set pcie_cap of the device before
> > > >> calculating the cfg_size of VF0 device.
> > > >>
> > > >> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> > > >> other VFs")
> > > >> Cc: Ashok Raj <[email protected]>
> > > >> Suggested-by: Mike Campin <[email protected]>
> > > >> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> > > >> ---
> > > >>
> > > >> Changes since v1:
> > > >> * Fixed a typo in commit message.
> > > >>
> > > >> drivers/pci/iov.c | 1 +
> > > >> 1 file changed, 1 insertion(+)
> > > >>
> > > >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > >> index 3aa115ed3a65..2869011c0e35 100644
> > > >> --- a/drivers/pci/iov.c
> > > >> +++ b/drivers/pci/iov.c
> > > >> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> > > >> virtfn->device = iov->vf_device;
> > > >> virtfn->is_virtfn = 1;
> > > >> virtfn->physfn = pci_dev_get(dev);
> > > >> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP);
> > > >>
> > > >> if (id == 0)
> > > >> pci_read_vf_config_common(virtfn);
> > > > Why not re-order until after we've setup pcie_cap?
> > > >
> > > > https://lore.kernel.org/linux-pci/[email protected]/T/#
> > >
> > > pci_read_vf_config_common() also caches values for properties like
> > > class, hdr_type, susbsystem_vendor/device. These values are read/used in
> > > pci_setup_device(). So if we can use cached values in
> > > pci_setup_device(), we don't have to read them from registers twice for
> > > each device.
> >
> > Sorry, I missed that dependency, a bit too subtle. It's still pretty
> > ugly that pci_setup_device()->set_pcie_port_type() is the canonical
> > location for setting pcie_cap and now we need to kludge it earlier.
> > What about the question in the self follow-up to my patch in the link
> > above, can we simply assume 4K config space on a VF? Thanks,
>
> There should be no issue simply reading them once? I don't know
> what that exact optimization saves, unless some broken VFs didn't
> actually expose all the capabilities in config space and this happens
> to workaround the problem.
AIUI the original commit only saves us a few config space reads for VFs
after the first where we test that extended config space is available
and not simply an alias of standard config space. Thanks,
Alex
On Wed, 2019-06-12 at 12:03 -0700, Raj, Ashok wrote:
> On Wed, Jun 12, 2019 at 12:58:17PM -0600, Alex Williamson wrote:
> >
> > On Wed, 12 Jun 2019 11:41:36 -0700
> > sathyanarayanan kuppuswamy <[email protected]>
> > wrote:
> >
> > >
> > > On 6/12/19 11:19 AM, Alex Williamson wrote:
> > > >
> > > > On Wed, 12 Jun 2019 10:06:47 -0700
> > > > [email protected] wrote:
> > > >
> > > > >
> > > > > From: Kuppuswamy Sathyanarayanan <[email protected]>
> > > > >
> > > > > Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> > > > > other VFs") calculates and caches the cfg_size for VF0 device before
> > > > > initializing the pcie_cap of the device which results in using incorrect
> > > > > cfg_size for all VF devices > 0. So set pcie_cap of the device before
> > > > > calculating the cfg_size of VF0 device.
> > > > >
> > > > > Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
> > > > > other VFs")
> > > > > Cc: Ashok Raj <[email protected]>
> > > > > Suggested-by: Mike Campin <[email protected]>
> > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> > > > > ---
> > > > >
> > > > > Changes since v1:
> > > > > * Fixed a typo in commit message.
> > > > >
> > > > > drivers/pci/iov.c | 1 +
> > > > > 1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > > > index 3aa115ed3a65..2869011c0e35 100644
> > > > > --- a/drivers/pci/iov.c
> > > > > +++ b/drivers/pci/iov.c
> > > > > @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> > > > > virtfn->device = iov->vf_device;
> > > > > virtfn->is_virtfn = 1;
> > > > > virtfn->physfn = pci_dev_get(dev);
> > > > > + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP);
> > > > >
> > > > > if (id == 0)
> > > > > pci_read_vf_config_common(virtfn);
> > > > Why not re-order until after we've setup pcie_cap?
> > > >
> > > > https://lore.kernel.org/linux-pci/[email protected]/T/#
> > >
> > > pci_read_vf_config_common() also caches values for properties like
> > > class, hdr_type, susbsystem_vendor/device. These values are read/used in
> > > pci_setup_device(). So if we can use cached values in
> > > pci_setup_device(), we don't have to read them from registers twice for
> > > each device.
> >
> > Sorry, I missed that dependency, a bit too subtle. It's still pretty
> > ugly that pci_setup_device()->set_pcie_port_type() is the canonical
> > location for setting pcie_cap and now we need to kludge it earlier.
> > What about the question in the self follow-up to my patch in the link
> > above, can we simply assume 4K config space on a VF? Thanks,
>
> There should be no issue simply reading them once? I don't know
> what that exact optimization saves, unless some broken VFs didn't
> actually expose all the capabilities in config space and this happens
> to workaround the problem.
The original patch was to save time when you have hundreds of VFs in the systemÂ
and doing this for each one of them is just a waste of time.
>
> + Karim
>
> Cheers,
> Ashok
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879