diff mbox series

PCI: quirk for preventing bus reset on TI C667X

Message ID 20210112153643.17930-1-antti.jarvinen@gmail.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI: quirk for preventing bus reset on TI C667X | expand

Commit Message

Antti Järvinen Jan. 12, 2021, 3:36 p.m. UTC
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(+)

Comments

Bjorn Helgaas Jan. 21, 2021, 11:55 p.m. UTC | #1
[+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
>
Antti Järvinen Jan. 26, 2021, 11:22 a.m. UTC | #2
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.
Bjorn Helgaas Jan. 29, 2021, 11:49 p.m. UTC | #3
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
Bjorn Helgaas Feb. 17, 2021, 9:18 p.m. UTC | #4
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
> >
diff mbox series

Patch

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)
 {
 	/*