Re: [PATCH 1/2] PCI/IOV: Add function to allow Function Dependency Link override.

[Bjorn Helgaas]

On Mon, Aug 22, 2016 at 07:49:09AM -0700, David Daney wrote:
> On 08/22/2016 07:36 AM, Bjorn Helgaas wrote:
> >Hi David & Omer,
> >
> >On Fri, Aug 19, 2016 at 03:32:12PM -0700, Omer Khaliq wrote:
> >>From: David Daney 
> >>
> >>Some hardware presents an incorrect SR-IOV Function Dependency Link,
> >>add a function to allow this to be overridden in the PF driver for
> >>such devices.
> >>
> >>Signed-off-by: David Daney 
> >>Signed-off-by: Omer Khaliq 
> >>---
> >>  drivers/pci/iov.c   | 14 ++
> >>  include/linux/pci.h |  1 +
> >>  2 files changed, 15 insertions(+)
> >>
> >>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>index 2194b44..81f0672 100644
> >>--- a/drivers/pci/iov.c
> >>+++ b/drivers/pci/iov.c
> >>@@ -640,6 +640,20 @@ int pci_enable_sriov(struct pci_dev *dev, int 
> >>nr_virtfn)
> >>  EXPORT_SYMBOL_GPL(pci_enable_sriov);
> >>
> >>  /**
> >>+ * pci_sriov_fdl_override - fix incorrect Function Dependency Link
> >>+ * @dev: the PCI device
> >>+ * @fdl: the corrected Function Dependency Link value
> >>+ *
> >>+ * For hardware presenting an incorrect Function Dependency Link in
> >>+ * the SR-IOV Extended Capability, allow a driver to override it.
> >>+ */
> >>+void pci_sriov_fdl_override(struct pci_dev *dev, u8 fdl)
> >>+{
> >>+   dev->sriov->link = fdl;
> >>+}
> >>+EXPORT_SYMBOL_GPL(pci_sriov_fdl_override);
> >
> >We usually use quirks to work around problems in config space.  That's
> >a nice mechanism because we don't have to add new PCI core interfaces
> >and it makes it clear that we're working around a hardware problem.
> >
> >Can you use a quirk here?  We allocate dev->sriov in the
> >pci_init_capabilities() path, so it looks like a pci_fixup_final quirk
> >should work.
> >
> 
> The struct pci_sriov definition is private to drivers/pci, so in
> order to use a quirk to fix this, we would have to put it in
> drivers/pci/quirks.c.  I was trying to keep this very device
> specific code in the driver, which requires an accessor to be able
> to manipulate the dev->sriov->link field.
> 
> If you prefer a quirk in drivers/pci/quirks.c, we can certainly do
> that instead.

Oh, I didn't notice that pci_sriov was declared in drivers/pci/pci.h
instead of linux/pci.h.  I do think I would prefer a quirk, and I
think it's fine to put it in drivers/pci/quirks.c.

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

Re: [PATCH 1/2] PCI/IOV: Add function to allow Function Dependency Link override.

[David Daney]

On 08/22/2016 07:36 AM, Bjorn Helgaas wrote:

Hi David & Omer,

On Fri, Aug 19, 2016 at 03:32:12PM -0700, Omer Khaliq wrote:

From: David Daney 

Some hardware presents an incorrect SR-IOV Function Dependency Link,
add a function to allow this to be overridden in the PF driver for
such devices.

Signed-off-by: David Daney 
Signed-off-by: Omer Khaliq 
---
  drivers/pci/iov.c   | 14 ++
  include/linux/pci.h |  1 +
  2 files changed, 15 insertions(+)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 2194b44..81f0672 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -640,6 +640,20 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
  EXPORT_SYMBOL_GPL(pci_enable_sriov);

  /**
+ * pci_sriov_fdl_override - fix incorrect Function Dependency Link
+ * @dev: the PCI device
+ * @fdl: the corrected Function Dependency Link value
+ *
+ * For hardware presenting an incorrect Function Dependency Link in
+ * the SR-IOV Extended Capability, allow a driver to override it.
+ */
+void pci_sriov_fdl_override(struct pci_dev *dev, u8 fdl)
+{
+   dev->sriov->link = fdl;
+}
+EXPORT_SYMBOL_GPL(pci_sriov_fdl_override);


We usually use quirks to work around problems in config space.  That's
a nice mechanism because we don't have to add new PCI core interfaces
and it makes it clear that we're working around a hardware problem.

Can you use a quirk here?  We allocate dev->sriov in the
pci_init_capabilities() path, so it looks like a pci_fixup_final quirk
should work.



The struct pci_sriov definition is private to drivers/pci, so in order 
to use a quirk to fix this, we would have to put it in 
drivers/pci/quirks.c.  I was trying to keep this very device specific 
code in the driver, which requires an accessor to be able to manipulate 
the dev->sriov->link field.


If you prefer a quirk in drivers/pci/quirks.c, we can certainly do that 
instead.


Thanks for taking the time to look at this,
David Daney





+
+/**
   * pci_disable_sriov - disable the SR-IOV capability
   * @dev: the PCI device
   */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2599a98..da8a5b3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1823,6 +1823,7 @@ int pci_num_vf(struct pci_dev *dev);
  int pci_vfs_assigned(struct pci_dev *dev);
  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
  int pci_sriov_get_totalvfs(struct pci_dev *dev);
+void pci_sriov_fdl_override(struct pci_dev *dev, u8 fdl);
  resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
  #else
  static inline int pci_iov_virtfn_bus(struct pci_dev *dev, int id)
--
1.9.1

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


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

Re: [PATCH 1/2] PCI/IOV: Add function to allow Function Dependency Link override.

[Bjorn Helgaas]

Hi David & Omer,

On Fri, Aug 19, 2016 at 03:32:12PM -0700, Omer Khaliq wrote:
> From: David Daney 
> 
> Some hardware presents an incorrect SR-IOV Function Dependency Link,
> add a function to allow this to be overridden in the PF driver for
> such devices.
> 
> Signed-off-by: David Daney 
> Signed-off-by: Omer Khaliq 
> ---
>  drivers/pci/iov.c   | 14 ++
>  include/linux/pci.h |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 2194b44..81f0672 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -640,6 +640,20 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>  EXPORT_SYMBOL_GPL(pci_enable_sriov);
>  
>  /**
> + * pci_sriov_fdl_override - fix incorrect Function Dependency Link
> + * @dev: the PCI device
> + * @fdl: the corrected Function Dependency Link value
> + *
> + * For hardware presenting an incorrect Function Dependency Link in
> + * the SR-IOV Extended Capability, allow a driver to override it.
> + */
> +void pci_sriov_fdl_override(struct pci_dev *dev, u8 fdl)
> +{
> + dev->sriov->link = fdl;
> +}
> +EXPORT_SYMBOL_GPL(pci_sriov_fdl_override);

We usually use quirks to work around problems in config space.  That's
a nice mechanism because we don't have to add new PCI core interfaces
and it makes it clear that we're working around a hardware problem.

Can you use a quirk here?  We allocate dev->sriov in the
pci_init_capabilities() path, so it looks like a pci_fixup_final quirk
should work.

> +
> +/**
>   * pci_disable_sriov - disable the SR-IOV capability
>   * @dev: the PCI device
>   */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2599a98..da8a5b3 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1823,6 +1823,7 @@ int pci_num_vf(struct pci_dev *dev);
>  int pci_vfs_assigned(struct pci_dev *dev);
>  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
> +void pci_sriov_fdl_override(struct pci_dev *dev, u8 fdl);
>  resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
>  #else
>  static inline int pci_iov_virtfn_bus(struct pci_dev *dev, int id)
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html