diff mbox series

[3/6] pci: add PCI quirk cmdmem fixup for Intel DSA device

Message ID 158560362665.6059.11999047251277108233.stgit@djiang5-desk3.ch.intel.com (mailing list archive)
State Not Applicable
Headers show
Series Add shared workqueue support for idxd driver | expand

Commit Message

Dave Jiang March 30, 2020, 9:27 p.m. UTC 
Since there is no standard way that defines a PCI device that receives
descriptors or commands with synchronous write operations, add quirk to set
cmdmem for the Intel accelerator device that supports it.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/pci/quirks.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Bjorn Helgaas March 31, 2020, 3:59 p.m. UTC | #1 
Take a look and make yours match (applies to other patches in the
series as well):

  $ git log --oneline drivers/pci/quirks.c
  299bd044a6f3 ("PCI: Add ACS quirk for Zhaoxin Root/Downstream Ports")
  0325837c51cb ("PCI: Add ACS quirk for Zhaoxin multi-function devices")
  2880325bda8d ("PCI: Avoid ASMedia XHCI USB PME# from D0 defect")
  b88bf6c3b6ff ("PCI: Add boot interrupt quirk mechanism for Xeon chipsets")
  5e89cd303e3a ("PCI: Mark AMD Navi14 GPU rev 0xc5 ATS as broken")
  7b90dfc4873b ("PCI: Add DMA alias quirk for PLX PEX NTB")
  09298542cd89 ("PCI: Add nr_devfns parameter to pci_add_dma_alias()")

There's no need to mention "PCI" twice.  Also no need for both "quirk"
and "fixup".  This is all in the interest of putting more information
in the small space of the subject line.

On Mon, Mar 30, 2020 at 02:27:06PM -0700, Dave Jiang wrote:
> Since there is no standard way that defines a PCI device that receives
> descriptors or commands with synchronous write operations, add quirk to set
> cmdmem for the Intel accelerator device that supports it.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/pci/quirks.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 29f473ebf20f..ba0572b9b9c8 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5461,3 +5461,14 @@ static void quirk_reset_lenovo_thinkpad_p50_nvgpu(struct pci_dev *pdev)
>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, 0x13b1,
>  			      PCI_CLASS_DISPLAY_VGA, 8,
>  			      quirk_reset_lenovo_thinkpad_p50_nvgpu);
> +
> +/*
> + * Until the PCI Sig defines a standard capaiblity check that indicates a
> + * device has cmdmem with synchronous write capability, we'll add a quirk
> + * for device that supports it.

s/PCI Sig/PCI-SIG/
s/capaiblity/capability/

It's not clear why this would need to be in drivers/pci/quirks.c as
opposed to being in the driver itself.

> + */
> +static void device_cmdmem_fixup(struct pci_dev *pdev)
> +{
> +	pdev->cmdmem = 1;
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0b25, device_cmdmem_fixup);
>
Dave Jiang March 31, 2020, 6:02 p.m. UTC | #2 
On 3/31/2020 8:59 AM, Bjorn Helgaas wrote:
> Take a look and make yours match (applies to other patches in the
> series as well):
>
>    $ git log --oneline drivers/pci/quirks.c
>    299bd044a6f3 ("PCI: Add ACS quirk for Zhaoxin Root/Downstream Ports")
>    0325837c51cb ("PCI: Add ACS quirk for Zhaoxin multi-function devices")
>    2880325bda8d ("PCI: Avoid ASMedia XHCI USB PME# from D0 defect")
>    b88bf6c3b6ff ("PCI: Add boot interrupt quirk mechanism for Xeon chipsets")
>    5e89cd303e3a ("PCI: Mark AMD Navi14 GPU rev 0xc5 ATS as broken")
>    7b90dfc4873b ("PCI: Add DMA alias quirk for PLX PEX NTB")
>    09298542cd89 ("PCI: Add nr_devfns parameter to pci_add_dma_alias()")
>
> There's no need to mention "PCI" twice.  Also no need for both "quirk"
> and "fixup".  This is all in the interest of putting more information
> in the small space of the subject line.
Ok I'll fix up.
>
> On Mon, Mar 30, 2020 at 02:27:06PM -0700, Dave Jiang wrote:
>> Since there is no standard way that defines a PCI device that receives
>> descriptors or commands with synchronous write operations, add quirk to set
>> cmdmem for the Intel accelerator device that supports it.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/pci/quirks.c |   11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 29f473ebf20f..ba0572b9b9c8 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -5461,3 +5461,14 @@ static void quirk_reset_lenovo_thinkpad_p50_nvgpu(struct pci_dev *pdev)
>>   DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, 0x13b1,
>>   			      PCI_CLASS_DISPLAY_VGA, 8,
>>   			      quirk_reset_lenovo_thinkpad_p50_nvgpu);
>> +
>> +/*
>> + * Until the PCI Sig defines a standard capaiblity check that indicates a
>> + * device has cmdmem with synchronous write capability, we'll add a quirk
>> + * for device that supports it.
> s/PCI Sig/PCI-SIG/
> s/capaiblity/capability/
>
> It's not clear why this would need to be in drivers/pci/quirks.c as
> opposed to being in the driver itself.

That would make the driver to set the PCI device struct cap bit instead 
of this being set on discovery right? And if the driver isn't loaded, 
then the cap wouldn't be set. In the future if user space wants to 
discover this information that may be an issue.



>
>> + */
>> +static void device_cmdmem_fixup(struct pci_dev *pdev)
>> +{
>> +	pdev->cmdmem = 1;
>> +}
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0b25, device_cmdmem_fixup);
>>
Christoph Hellwig April 1, 2020, 7:18 a.m. UTC | #3 
On Mon, Mar 30, 2020 at 02:27:06PM -0700, Dave Jiang wrote:
> Since there is no standard way that defines a PCI device that receives
> descriptors or commands with synchronous write operations, add quirk to set
> cmdmem for the Intel accelerator device that supports it.

Why do we need a quirk for this?  Even assuming a flag is needed in
struct pci_dev (and I don't really understand why that is needed to
start with), it could be set in ->probe.
Dan Williams April 2, 2020, 2:20 a.m. UTC | #4 
On Wed, Apr 1, 2020 at 12:19 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Mar 30, 2020 at 02:27:06PM -0700, Dave Jiang wrote:
> > Since there is no standard way that defines a PCI device that receives
> > descriptors or commands with synchronous write operations, add quirk to set
> > cmdmem for the Intel accelerator device that supports it.
>
> Why do we need a quirk for this?  Even assuming a flag is needed in
> struct pci_dev (and I don't really understand why that is needed to
> start with), it could be set in ->probe.

The consideration in my mind is whether this new memory type and
instruction combination warrants a new __attribute__((noderef,
address_space(X))) separate from __iomem. If it stays a single device
concept layered on __iomem then yes, I agree it can all live locally
in the driver. However, when / if this facility becomes wider spread,
as the PCI ECR in question is trending, it might warrant general
annotation.

The enqcmds instruction does not operate on typical x86 mmio
addresses, only these special device portals offer the ability to have
non-posted writes with immediate results in the cpu condition code
flags.
Christoph Hellwig April 2, 2020, 7:39 a.m. UTC | #5 
On Wed, Apr 01, 2020 at 07:20:59PM -0700, Dan Williams wrote:
> On Wed, Apr 1, 2020 at 12:19 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Mon, Mar 30, 2020 at 02:27:06PM -0700, Dave Jiang wrote:
> > > Since there is no standard way that defines a PCI device that receives
> > > descriptors or commands with synchronous write operations, add quirk to set
> > > cmdmem for the Intel accelerator device that supports it.
> >
> > Why do we need a quirk for this?  Even assuming a flag is needed in
> > struct pci_dev (and I don't really understand why that is needed to
> > start with), it could be set in ->probe.
> 
> The consideration in my mind is whether this new memory type and
> instruction combination warrants a new __attribute__((noderef,
> address_space(X))) separate from __iomem. If it stays a single device
> concept layered on __iomem then yes, I agree it can all live locally
> in the driver. However, when / if this facility becomes wider spread,
> as the PCI ECR in question is trending, it might warrant general
> annotation.
> 
> The enqcmds instruction does not operate on typical x86 mmio
> addresses, only these special device portals offer the ability to have
> non-posted writes with immediate results in the cpu condition code
> flags.

But that is not what this series does at all.  And I think it makes
sense to wait until it gains adoption to think about a different address
space.  In this series we could just trivially kill patches 2-4 and make
it much easier to understand.
diff mbox series

Patch

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 29f473ebf20f..ba0572b9b9c8 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5461,3 +5461,14 @@  static void quirk_reset_lenovo_thinkpad_p50_nvgpu(struct pci_dev *pdev)
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, 0x13b1,
 			      PCI_CLASS_DISPLAY_VGA, 8,
 			      quirk_reset_lenovo_thinkpad_p50_nvgpu);
+
+/*
+ * Until the PCI Sig defines a standard capaiblity check that indicates a
+ * device has cmdmem with synchronous write capability, we'll add a quirk
+ * for device that supports it.
+ */
+static void device_cmdmem_fixup(struct pci_dev *pdev)
+{
+	pdev->cmdmem = 1;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0b25, device_cmdmem_fixup);