[PATCH] PCI: quirk for preventing bus reset on TI C667X

[PATCH] PCI: quirk for preventing bus reset on TI C667X

[Antti Järvinen]

TI C667X does not support bus/hot reset.
See https://e2e.ti.com/support/processors/f/791/t/954382

Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>
---
 drivers/pci/quirks.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..c8fcf24c5bd0 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3578,6 +3578,12 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
  */
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
 
+/*
+ * Some TI keystone C667X devices do no support bus/hot reset.
+ * https://e2e.ti.com/support/processors/f/791/t/954382
+ */
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
+
 static void quirk_no_pm_reset(struct pci_dev *dev)
 {
 	/*
-- 
2.17.1

Re: [PATCH] PCI: quirk for preventing bus reset on TI C667X

[Bjorn Helgaas]

[+cc Alex, Murali, Kishon]

On Tue, Jan 12, 2021 at 03:36:43PM +0000, Antti Järvinen wrote:
> TI C667X does not support bus/hot reset.
> See https://e2e.ti.com/support/processors/f/791/t/954382

You can cite the URL as the source, but the URL will eventually become
stale, so let's include the relevant details here directly.  

From the forum, it looks like the device doesn't respond after a
reset (config accesses return ~0).  It seems somewhat surprising that
something as basic as a reset would be completely broken.  I wonder if
we're not doing the reset correctly.

It looks like we would probably be trying a Secondary Bus Reset using
the bridge leading to the C667X.  Can you confirm?  Wonder if you
could try doing what pci_reset_secondary_bus() does by hand:

  # BRIDGE=...                              # PCI address, e.g., 00:1c.0
  # C667X=...
  # setpci -s$C667X VENDOR_ID.w
  # setpci -s$BRIDGE BRIDGE_CONTROL.w       # prints "val"
  # setpci -s$BRIDGE BRIDGE_CONTROL.w=      # val | 0x40 (set SBR)
  # sleep 1
  # setpci -s$BRIDGE BRIDGE_CONTROL.w=      # val (clear SBR)
  # sleep 1
  # setpci -s$C667X VENDOR_ID.w=0
  # setpci -s$C667X VENDOR_ID.w

If we use this quirk and avoid the reset, I assume that means
assigning the device to VMs with VFIO will leak state between VMs?

> Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>
> ---
>  drivers/pci/quirks.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 653660e3ba9e..c8fcf24c5bd0 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3578,6 +3578,12 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
>   */
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
>  
> +/*
> + * Some TI keystone C667X devices do no support bus/hot reset.
> + * https://e2e.ti.com/support/processors/f/791/t/954382
> + */
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
> +
>  static void quirk_no_pm_reset(struct pci_dev *dev)
>  {
>  	/*
> -- 
> 2.17.1
> 

Re: [PATCH] PCI: quirk for preventing bus reset on TI C667X

[Antti Järvinen]

On 22.1.2021 1.55, Bjorn Helgaas wrote:> 

> It looks like we would probably be trying a Secondary Bus Reset using
> the bridge leading to the C667X.  Can you confirm? 

Yes, this is my understanding too.

> Wonder if you
> could try doing what pci_reset_secondary_bus() does by hand:
> 

I tried this by hand. It looks that result is same as through VFIO.

# cat sbr.sh
BRIDGE=10:00.0
C667X=11:00.0

setpci -s$C667X VENDOR_ID.w

VAL=$(setpci -s$BRIDGE BRIDGE_CONTROL.w)
echo $VAL
setpci -s$BRIDGE BRIDGE_CONTROL.w=$(($VAL | 0x40))
sleep 1
setpci -s$BRIDGE BRIDGE_CONTROL.w=$VAL
sleep 1
setpci -s$C667X VENDOR_ID.w=0
setpci -s$C667X VENDOR_ID.w


# ./sbr.sh
104c
0003
ffff


>   # BRIDGE=...                              # PCI address, e.g., 00:1c.0
>   # C667X=...
>   # setpci -s$C667X VENDOR_ID.w
>   # setpci -s$BRIDGE BRIDGE_CONTROL.w       # prints "val"
>   # setpci -s$BRIDGE BRIDGE_CONTROL.w=      # val | 0x40 (set SBR)
>   # sleep 1
>   # setpci -s$BRIDGE BRIDGE_CONTROL.w=      # val (clear SBR)
>   # sleep 1
>   # setpci -s$C667X VENDOR_ID.w=0
>   # setpci -s$C667X VENDOR_ID.w
> 
> If we use this quirk and avoid the reset, I assume that means
> assigning the device to VMs with VFIO will leak state between VMs?

I think this is true.

Re: [PATCH] PCI: quirk for preventing bus reset on TI C667X

[Bjorn Helgaas]

On Tue, Jan 26, 2021 at 01:22:18PM +0200, Antti Järvinen wrote:
> On 22.1.2021 1.55, Bjorn Helgaas wrote:> 
> 
> > It looks like we would probably be trying a Secondary Bus Reset using
> > the bridge leading to the C667X.  Can you confirm? 
> 
> Yes, this is my understanding too.
> 
> > Wonder if you
> > could try doing what pci_reset_secondary_bus() does by hand:
> > 
> 
> I tried this by hand. It looks that result is same as through VFIO.
> 
> # cat sbr.sh
> BRIDGE=10:00.0
> C667X=11:00.0
> 
> setpci -s$C667X VENDOR_ID.w
> 
> VAL=$(setpci -s$BRIDGE BRIDGE_CONTROL.w)
> echo $VAL
> setpci -s$BRIDGE BRIDGE_CONTROL.w=$(($VAL | 0x40))
> sleep 1
> setpci -s$BRIDGE BRIDGE_CONTROL.w=$VAL
> sleep 1
> setpci -s$C667X VENDOR_ID.w=0
> setpci -s$C667X VENDOR_ID.w
> 
> 
> # ./sbr.sh
> 104c
> 0003
> ffff
> 
> 
> >   # BRIDGE=...                              # PCI address, e.g., 00:1c.0
> >   # C667X=...
> >   # setpci -s$C667X VENDOR_ID.w
> >   # setpci -s$BRIDGE BRIDGE_CONTROL.w       # prints "val"
> >   # setpci -s$BRIDGE BRIDGE_CONTROL.w=      # val | 0x40 (set SBR)
> >   # sleep 1
> >   # setpci -s$BRIDGE BRIDGE_CONTROL.w=      # val (clear SBR)
> >   # sleep 1
> >   # setpci -s$C667X VENDOR_ID.w=0
> >   # setpci -s$C667X VENDOR_ID.w
> > 
> > If we use this quirk and avoid the reset, I assume that means
> > assigning the device to VMs with VFIO will leak state between VMs?
> 
> I think this is true.

Alex, is there some warning about situations like this where a device
may leak state between VMs?

There's nothing in quirk_no_bus_reset() itself where we set
PCI_DEV_FLAGS_NO_BUS_RESET, and nothing in pci_parent_bus_reset() or
pci_dev_reset_slot_function() where we test it, but I didn't chase
into VFIO.

Seems important enough that we might want to mention it at least once
and maybe even every time we try to reset the device.  I hope the leak
isn't completely silent.

Bjorn

Re: [PATCH] PCI: quirk for preventing bus reset on TI C667X

[Bjorn Helgaas]

On Thu, Jan 21, 2021 at 05:55:47PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 12, 2021 at 03:36:43PM +0000, Antti Järvinen wrote:
> > TI C667X does not support bus/hot reset.
> > See https://e2e.ti.com/support/processors/f/791/t/954382
> 
> You can cite the URL as the source, but the URL will eventually become
> stale, so let's include the relevant details here directly.  

Thanks for trying the experiment below.  I'll look for a repost that
includes details from the URL directly in the commit log.

> From the forum, it looks like the device doesn't respond after a
> reset (config accesses return ~0).  It seems somewhat surprising that
> something as basic as a reset would be completely broken.  I wonder if
> we're not doing the reset correctly.
> 
> It looks like we would probably be trying a Secondary Bus Reset using
> the bridge leading to the C667X.  Can you confirm?  Wonder if you
> could try doing what pci_reset_secondary_bus() does by hand:
> 
>   # BRIDGE=...                              # PCI address, e.g., 00:1c.0
>   # C667X=...
>   # setpci -s$C667X VENDOR_ID.w
>   # setpci -s$BRIDGE BRIDGE_CONTROL.w       # prints "val"
>   # setpci -s$BRIDGE BRIDGE_CONTROL.w=      # val | 0x40 (set SBR)
>   # sleep 1
>   # setpci -s$BRIDGE BRIDGE_CONTROL.w=      # val (clear SBR)
>   # sleep 1
>   # setpci -s$C667X VENDOR_ID.w=0
>   # setpci -s$C667X VENDOR_ID.w
> 
> If we use this quirk and avoid the reset, I assume that means
> assigning the device to VMs with VFIO will leak state between VMs?
> 
> > Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>
> > ---
> >  drivers/pci/quirks.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 653660e3ba9e..c8fcf24c5bd0 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3578,6 +3578,12 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
> >   */
> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
> >  
> > +/*
> > + * Some TI keystone C667X devices do no support bus/hot reset.
> > + * https://e2e.ti.com/support/processors/f/791/t/954382
> > + */
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
> > +
> >  static void quirk_no_pm_reset(struct pci_dev *dev)
> >  {
> >  	/*
> > -- 
> > 2.17.1
> > 

[PATCH v2] PCI: quirk for preventing bus reset on TI C667X

[Antti Järvinen]

Some TI keystone C667X devices do no support bus/hot reset. Its PCIESS
automatically disables LTSSM when secondary bus reset is received and
device stops working. Prevent bus reset by adding quirk_no_bus_reset to
the device. With this change device can be assigned to VMs with VFIO,
but it will leak state between VMs.

Reference https://e2e.ti.com/support/processors/f/791/t/954382

Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>
---
 drivers/pci/quirks.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..d9201ad1ca39 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3578,6 +3578,16 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
  */
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
 
+/*
+ * Some TI keystone C667X devices do no support bus/hot reset.
+ * Its PCIESS automatically disables LTSSM when secondary bus reset is
+ * received and device stops working. Prevent bus reset by adding
+ * quirk_no_bus_reset to the device. With this change device can be
+ * assigned to VMs with VFIO, but it will leak state between VMs.
+ * Reference https://e2e.ti.com/support/processors/f/791/t/954382
+ */
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
+
 static void quirk_no_pm_reset(struct pci_dev *dev)
 {
 	/*
-- 
2.17.1

Re: [PATCH v2] PCI: quirk for preventing bus reset on TI C667X

[Krzysztof Wilczyński]

Hi Antti,

A few nitpicks, so feel free to ignore these, of course.

If possible, capitalise the subject line.  Also, perhaps "Add quirk to
prevent bus (...)" might read better.

> Some TI keystone C667X devices do no support bus/hot reset. Its PCIESS
[...]

It would be KeyStone in the above sentence.

[...]
> With this change device can be assigned to VMs with VFIO, but it will
> leak state between VMs.

Following-up on Bojrn's question about the state leak, see:
  https://lore.kernel.org/linux-pci/20210129234946.GA124037@bjorn-Precision-5520/

Would there be anything else that has to be done?

Krzysztof

[PATCH v3] PCI: Add quirk for preventing bus reset on TI C667X

[Antti Järvinen]

Some TI KeyStone C667X devices do no support bus/hot reset. Its PCIESS
automatically disables LTSSM when secondary bus reset is received and
device stops working. Prevent bus reset by adding quirk_no_bus_reset to
the device. With this change device can be assigned to VMs with VFIO,
but it will leak state between VMs.

Reference: https://e2e.ti.com/support/processors/f/791/t/954382
Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>
---
 drivers/pci/quirks.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..d9201ad1ca39 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3578,6 +3578,16 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
  */
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
 
+/*
+ * Some TI keystone C667X devices do no support bus/hot reset.
+ * Its PCIESS automatically disables LTSSM when secondary bus reset is
+ * received and device stops working. Prevent bus reset by adding
+ * quirk_no_bus_reset to the device. With this change device can be
+ * assigned to VMs with VFIO, but it will leak state between VMs.
+ * Reference https://e2e.ti.com/support/processors/f/791/t/954382
+ */
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
+
 static void quirk_no_pm_reset(struct pci_dev *dev)
 {
 	/*
-- 
2.17.1

Re: [PATCH v3] PCI: Add quirk for preventing bus reset on TI C667X

[Bjorn Helgaas]

On Mon, Mar 08, 2021 at 02:21:30PM +0000, Antti Järvinen wrote:
> Some TI KeyStone C667X devices do no support bus/hot reset. Its PCIESS
> automatically disables LTSSM when secondary bus reset is received and
> device stops working. Prevent bus reset by adding quirk_no_bus_reset to
> the device. With this change device can be assigned to VMs with VFIO,
> but it will leak state between VMs.

s/do no/do/not/ (also in the comment below)

Does the user get any indication of this leaking state?  I looked
through drivers/vfio and drivers/pci, but I haven't found anything
yet.

We *could* log something in quirk_no_bus_reset(), but that would just
be noise for people who don't pass the device through to a VM.  So
maybe it would be nicer if we logged something when we actually *do*
pass it through to a VM.

> Reference: https://e2e.ti.com/support/processors/f/791/t/954382
> Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>
> ---
>  drivers/pci/quirks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 653660e3ba9e..d9201ad1ca39 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3578,6 +3578,16 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
>   */
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
>  
> +/*
> + * Some TI keystone C667X devices do no support bus/hot reset.
> + * Its PCIESS automatically disables LTSSM when secondary bus reset is
> + * received and device stops working. Prevent bus reset by adding
> + * quirk_no_bus_reset to the device. With this change device can be
> + * assigned to VMs with VFIO, but it will leak state between VMs.
> + * Reference https://e2e.ti.com/support/processors/f/791/t/954382
> + */
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
> +
>  static void quirk_no_pm_reset(struct pci_dev *dev)
>  {
>  	/*
> -- 
> 2.17.1
> 

[PATCH v4] PCI: Add quirk for preventing bus reset on TI C667X

[Antti Järvinen]

Some TI KeyStone C667X devices do not support bus/hot reset. Its PCIESS
automatically disables LTSSM when secondary bus reset is received and
device stops working. Prevent bus reset by adding quirk_no_bus_reset to
the device. With this change device can be assigned to VMs with VFIO,
but it will leak state between VMs.

Reference: https://e2e.ti.com/support/processors/f/791/t/954382
Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>
---
 drivers/pci/quirks.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..d9201ad1ca39 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3578,6 +3578,16 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
  */
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
 
+/*
+ * Some TI keystone C667X devices do no support bus/hot reset.
+ * Its PCIESS automatically disables LTSSM when secondary bus reset is
+ * received and device stops working. Prevent bus reset by adding
+ * quirk_no_bus_reset to the device. With this change device can be
+ * assigned to VMs with VFIO, but it will leak state between VMs.
+ * Reference https://e2e.ti.com/support/processors/f/791/t/954382
+ */
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
+
 static void quirk_no_pm_reset(struct pci_dev *dev)
 {
 	/*
-- 
2.17.1

Re: [PATCH v4] PCI: Add quirk for preventing bus reset on TI C667X

[Kishon Vijay Abraham I]

On 15/03/21 3:56 pm, Antti Järvinen wrote:
> Some TI KeyStone C667X devices do not support bus/hot reset. Its PCIESS
> automatically disables LTSSM when secondary bus reset is received and
> device stops working. Prevent bus reset by adding quirk_no_bus_reset to
> the device. With this change device can be assigned to VMs with VFIO,
> but it will leak state between VMs.
> 
> Reference: https://e2e.ti.com/support/processors/f/791/t/954382
> Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>

Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/quirks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 653660e3ba9e..d9201ad1ca39 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3578,6 +3578,16 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
>   */
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
>  
> +/*
> + * Some TI keystone C667X devices do no support bus/hot reset.
> + * Its PCIESS automatically disables LTSSM when secondary bus reset is
> + * received and device stops working. Prevent bus reset by adding
> + * quirk_no_bus_reset to the device. With this change device can be
> + * assigned to VMs with VFIO, but it will leak state between VMs.
> + * Reference https://e2e.ti.com/support/processors/f/791/t/954382
> + */
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
> +
>  static void quirk_no_pm_reset(struct pci_dev *dev)
>  {
>  	/*
> 

Re: [PATCH v4] PCI: Add quirk for preventing bus reset on TI C667X

[Bjorn Helgaas]

On Mon, Mar 15, 2021 at 10:26:06AM +0000, Antti Järvinen wrote:
> Some TI KeyStone C667X devices do not support bus/hot reset. Its PCIESS
> automatically disables LTSSM when secondary bus reset is received and
> device stops working. Prevent bus reset by adding quirk_no_bus_reset to
> the device. With this change device can be assigned to VMs with VFIO,
> but it will leak state between VMs.
> 
> Reference: https://e2e.ti.com/support/processors/f/791/t/954382
> Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>

Applied to pci/virtualization for v5.14 with subject

  PCI: Mark TI C667X to avoid bus reset

Thanks!

> ---
>  drivers/pci/quirks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 653660e3ba9e..d9201ad1ca39 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3578,6 +3578,16 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
>   */
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
>  
> +/*
> + * Some TI keystone C667X devices do no support bus/hot reset.
> + * Its PCIESS automatically disables LTSSM when secondary bus reset is
> + * received and device stops working. Prevent bus reset by adding
> + * quirk_no_bus_reset to the device. With this change device can be
> + * assigned to VMs with VFIO, but it will leak state between VMs.
> + * Reference https://e2e.ti.com/support/processors/f/791/t/954382
> + */
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
> +
>  static void quirk_no_pm_reset(struct pci_dev *dev)
>  {
>  	/*
> -- 
> 2.17.1
> 

Re: [PATCH v3] PCI: Add quirk for preventing bus reset on TI C667X

[Antti Järvinen]

On 12.3.2021 23.09, Bjorn Helgaas wrote:
> On Mon, Mar 08, 2021 at 02:21:30PM +0000, Antti Järvinen wrote:
>> Some TI KeyStone C667X devices do no support bus/hot reset. Its PCIESS
>> automatically disables LTSSM when secondary bus reset is received and
>> device stops working. Prevent bus reset by adding quirk_no_bus_reset to
>> the device. With this change device can be assigned to VMs with VFIO,
>> but it will leak state between VMs.
> 
> s/do no/do/not/ (also in the comment below)
> 

Should be fixed in v4 patch.
 
> Does the user get any indication of this leaking state?  I looked
> through drivers/vfio and drivers/pci, but I haven't found anything
> yet.
> 

I haven't seen any indication too. 

Overall I think all devices having this quirk will leak state, as they
don't get resetted.


> We *could* log something in quirk_no_bus_reset(), but that would just
> be noise for people who don't pass the device through to a VM.  So
> maybe it would be nicer if we logged something when we actually *do*
> pass it through to a VM.
> 
>> Reference: https://e2e.ti.com/support/processors/f/791/t/954382
>> Signed-off-by: Antti Järvinen <antti.jarvinen@gmail.com>
>> ---
>>  drivers/pci/quirks.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 653660e3ba9e..d9201ad1ca39 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3578,6 +3578,16 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0034, quirk_no_bus_reset);
>>   */
>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset);
>>  
>> +/*
>> + * Some TI keystone C667X devices do no support bus/hot reset.
>> + * Its PCIESS automatically disables LTSSM when secondary bus reset is
>> + * received and device stops working. Prevent bus reset by adding
>> + * quirk_no_bus_reset to the device. With this change device can be
>> + * assigned to VMs with VFIO, but it will leak state between VMs.
>> + * Reference https://e2e.ti.com/support/processors/f/791/t/954382
>> + */
>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TI, 0xb005, quirk_no_bus_reset);
>> +
>>  static void quirk_no_pm_reset(struct pci_dev *dev)
>>  {
>>  	/*
>> -- 
>> 2.17.1
>>

Re: [PATCH v2] PCI: quirk for preventing bus reset on TI C667X

[Antti Järvinen]

On 7.3.2021 2.22, Krzysztof Wilczyński wrote:
> Hi Antti,
> 
> A few nitpicks, so feel free to ignore these, of course.
> 
> If possible, capitalise the subject line.  Also, perhaps "Add quirk to
> prevent bus (...)" might read better.
> 
>> Some TI keystone C667X devices do no support bus/hot reset. Its PCIESS
> [...]
> 
> It would be KeyStone in the above sentence.
> 
> [...]
>> With this change device can be assigned to VMs with VFIO, but it will
>> leak state between VMs.
> 
> Following-up on Bojrn's question about the state leak, see:
>   https://lore.kernel.org/linux-pci/20210129234946.GA124037@bjorn-Precision-5520/
> 
> Would there be anything else that has to be done?
>

To my understanding this is all that needs to be done. At least on other similar
case, adding quirk was the only change https://lore.kernel.org/patchwork/patch/1086083/