[PATCH v3 3/3] Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"

[Marc Zyngier]

This reverts commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7.

Now that we can properly reset the uPD72020x without a hard PCI reset,
let's get rid of the existing quirks.

Tested-by: Domenico Andreoli <domenico.andre...@linux.com>
Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/pci-quirks.c | 20 
 drivers/usb/host/pci-quirks.h |  1 -
 drivers/usb/host/xhci-pci.c   |  7 ---
 3 files changed, 28 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 67ad4bb6919a..3625a5c1a41b 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1268,23 +1268,3 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
-
-bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
-{
-   /*
-* Our dear uPD72020{1,2} friend only partially resets when
-* asked to via the XHCI interface, and may end up doing DMA
-* at the wrong addresses, as it keeps the top 32bit of some
-* addresses from its previous programming under obscure
-* circumstances.
-* Give it a good wack at probe time. Unfortunately, this
-* needs to happen before we've had a chance to discover any
-* quirk, or the system will be in a rather bad state.
-*/
-   if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   (pdev->device == 0x0014 || pdev->device == 0x0015))
-   return true;
-
-   return false;
-}
-EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index 4ca0d9b7e463..63c633077d9e 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -16,7 +16,6 @@ void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
 void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
 void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
 void sb800_prefetch(struct device *dev, int on);
-bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
 bool usb_amd_pt_check_port(struct device *device, int port);
 #else
 struct pci_dev;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index e0a0a12871e2..6372edf339d9 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -288,13 +288,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const 
struct pci_device_id *id)
 
driver = (struct hc_driver *)id->driver_data;
 
-   /* For some HW implementation, a XHCI reset is just not enough... */
-   if (usb_xhci_needs_pci_reset(dev)) {
-   dev_info(>dev, "Resetting\n");
-   if (pci_reset_function_locked(dev))
-   dev_warn(>dev, "Reset failed");
-   }
-
/* Prevent runtime suspending between USB-2 and USB-3 initialization */
pm_runtime_get_noresume(>dev);
 
-- 
2.14.2

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

[PATCH v3 2/3] xhci: Add quirk to zero 64bit registers on Renesas PCIe controllers

[Marc Zyngier]

Some Renesas controllers get into a weird state if they are reset while
programmed with 64bit addresses (they will preserve the top half of the
address in internal, non visible registers).

You end up with half the address coming from the kernel, and the other
half coming from the firmware.

Also, changing the programming leads to extra accesses even if the
controller is supposed to be halted. The controller ends up with a fatal
fault, and is then ripe for being properly reset. On the flip side,
this is completely unsafe if the defvice isn't behind an IOMMU, so
we have to make sure that this is the case. Can you say "broken"?

This is an alternative method to the one introduced in 8466489ef5ba
("xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"),
which will subsequently be removed.

Tested-by: Domenico Andreoli <domenico.andre...@linux.com>
Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/xhci-pci.c |  8 --
 drivers/usb/host/xhci.c | 65 +
 drivers/usb/host/xhci.h |  1 +
 3 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 85ffda85f8ab..e0a0a12871e2 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -196,11 +196,15 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
xhci->quirks |= XHCI_BROKEN_STREAMS;
}
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0014)
+   pdev->device == 0x0014) {
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
+   xhci->quirks |= XHCI_ZERO_64B_REGS;
+   }
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0015)
+   pdev->device == 0x0015) {
xhci->quirks |= XHCI_RESET_ON_RESUME;
+   xhci->quirks |= XHCI_ZERO_64B_REGS;
+   }
if (pdev->vendor == PCI_VENDOR_ID_VIA)
xhci->quirks |= XHCI_RESET_ON_RESUME;
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 8dba26d3de07..4d9437279681 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -209,6 +209,68 @@ int xhci_reset(struct xhci_hcd *xhci)
return ret;
 }
 
+static void xhci_zero_64b_regs(struct xhci_hcd *xhci)
+{
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
+   int err, i;
+   u64 val;
+
+   /*
+* Some Renesas controllers get into a weird state if they are
+* reset while programmed with 64bit addresses (they will preserve
+* the top half of the address in internal, non visible
+* registers). You end up with half the address coming from the
+* kernel, and the other half coming from the firmware. Also,
+* changing the programming leads to extra accesses even if the
+* controller is supposed to be halted. The controller ends up with
+* a fatal fault, and is then ripe for being properly reset.
+*
+* Special care is taken to only apply this if the device is behind
+* an iommu. Doing anything when there is no iommu is definitely
+* unsafe...
+*/
+   if (!(xhci->quirks & XHCI_ZERO_64B_REGS) || !dev->iommu_group)
+   return;
+
+   xhci_info(xhci, "Zeroing 64bit base registers, expecting fault\n");
+
+   /* Clear HSEIE so that faults do not get signaled */
+   val = readl(>op_regs->command);
+   val &= ~CMD_HSEIE;
+   writel(val, >op_regs->command);
+
+   /* Clear HSE (aka FATAL) */
+   val = readl(>op_regs->status);
+   val |= STS_FATAL;
+   writel(val, >op_regs->status);
+
+   /* Now zero the registers, and brace for impact */
+   val = xhci_read_64(xhci, >op_regs->dcbaa_ptr);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >op_regs->dcbaa_ptr);
+   val = xhci_read_64(xhci, >op_regs->cmd_ring);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >op_regs->cmd_ring);
+
+   for (i = 0; i < HCS_MAX_INTRS(xhci->hcs_params1); i++) {
+   struct xhci_intr_reg __iomem *ir;
+
+   ir = >run_regs->ir_set[i];
+   val = xhci_read_64(xhci, >erst_base);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >erst_base);
+   val= xhci_read_64(xhci, >erst_dequeue);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >erst_dequeue);
+   }
+
+   /* Wait for the fault to appear. It will be cleared on reset */
+   err = xhci_handshake(>op_regs->status,
+STS_FATAL, STS_FATAL,
+XHCI_MAX

[PATCH v3 1/3] xhci: Allow more than 32 quirks

[Marc Zyngier]

We now have 32 different quirks, and the field that holds them
is full. Let's bump it up to the next stage so that we can handle
some more... The type is now an unsigned long long, which is 64bit
on most architectures.

We take this opportunity to change the quirks from using (1 << x)
to BIT_ULL(x).

Tested-by: Domenico Andreoli <domenico.andre...@linux.com>
Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/xhci.c |  6 ++---
 drivers/usb/host/xhci.h | 66 -
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 711da3306b14..8dba26d3de07 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -33,8 +33,8 @@ static int link_quirk;
 module_param(link_quirk, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(link_quirk, "Don't clear the chain bit on a link TRB");
 
-static unsigned int quirks;
-module_param(quirks, uint, S_IRUGO);
+static unsigned long long quirks;
+module_param(quirks, ullong, S_IRUGO);
 MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default");
 
 /* TODO: copied from ehci-hcd.c - can this be refactored? */
@@ -4963,7 +4963,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t 
get_quirks)
return retval;
xhci_dbg(xhci, "Called HCD init\n");
 
-   xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%08x\n",
+   xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%016llx\n",
  xhci->hcc_params, xhci->hci_version, xhci->quirks);
 
return 0;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 6dfc4867dbcf..e322afd3f08b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1787,12 +1787,12 @@ struct xhci_hcd {
 #define XHCI_STATE_DYING   (1 << 0)
 #define XHCI_STATE_HALTED  (1 << 1)
 #define XHCI_STATE_REMOVING(1 << 2)
-   unsigned intquirks;
-#defineXHCI_LINK_TRB_QUIRK (1 << 0)
-#define XHCI_RESET_EP_QUIRK(1 << 1)
-#define XHCI_NEC_HOST  (1 << 2)
-#define XHCI_AMD_PLL_FIX   (1 << 3)
-#define XHCI_SPURIOUS_SUCCESS  (1 << 4)
+   unsigned long long  quirks;
+#defineXHCI_LINK_TRB_QUIRK BIT_ULL(0)
+#define XHCI_RESET_EP_QUIRKBIT_ULL(1)
+#define XHCI_NEC_HOST  BIT_ULL(2)
+#define XHCI_AMD_PLL_FIX   BIT_ULL(3)
+#define XHCI_SPURIOUS_SUCCESS  BIT_ULL(4)
 /*
  * Certain Intel host controllers have a limit to the number of endpoint
  * contexts they can handle.  Ideally, they would signal that they can't handle
@@ -1802,35 +1802,35 @@ struct xhci_hcd {
  * commands, reset device commands, disable slot commands, and address device
  * commands.
  */
-#define XHCI_EP_LIMIT_QUIRK(1 << 5)
-#define XHCI_BROKEN_MSI(1 << 6)
-#define XHCI_RESET_ON_RESUME   (1 << 7)
-#defineXHCI_SW_BW_CHECKING (1 << 8)
-#define XHCI_AMD_0x96_HOST (1 << 9)
-#define XHCI_TRUST_TX_LENGTH   (1 << 10)
-#define XHCI_LPM_SUPPORT   (1 << 11)
-#define XHCI_INTEL_HOST(1 << 12)
-#define XHCI_SPURIOUS_REBOOT   (1 << 13)
-#define XHCI_COMP_MODE_QUIRK   (1 << 14)
-#define XHCI_AVOID_BEI (1 << 15)
-#define XHCI_PLAT  (1 << 16)
-#define XHCI_SLOW_SUSPEND  (1 << 17)
-#define XHCI_SPURIOUS_WAKEUP   (1 << 18)
+#define XHCI_EP_LIMIT_QUIRKBIT_ULL(5)
+#define XHCI_BROKEN_MSIBIT_ULL(6)
+#define XHCI_RESET_ON_RESUME   BIT_ULL(7)
+#defineXHCI_SW_BW_CHECKING BIT_ULL(8)
+#define XHCI_AMD_0x96_HOST BIT_ULL(9)
+#define XHCI_TRUST_TX_LENGTH   BIT_ULL(10)
+#define XHCI_LPM_SUPPORT   BIT_ULL(11)
+#define XHCI_INTEL_HOSTBIT_ULL(12)
+#define XHCI_SPURIOUS_REBOOT   BIT_ULL(13)
+#define XHCI_COMP_MODE_QUIRK   BIT_ULL(14)
+#define XHCI_AVOID_BEI BIT_ULL(15)
+#define XHCI_PLAT  BIT_ULL(16)
+#define XHCI_SLOW_SUSPEND  BIT_ULL(17)
+#define XHCI_SPURIOUS_WAKEUP   BIT_ULL(18)
 /* For controllers with a broken beyond repair streams implementation */
-#define XHCI_BROKEN_STREAMS(1 << 19)
-#define XHCI_PME_STUCK_QUIRK   (1 << 20)
-#define XHCI_MTK_HOST  (1 << 21)
-#define XHCI_SSIC_PORT_UNUSED  (1 << 22)
-#define XHCI_NO_64BIT_SUPPORT  (1 << 23)
-#define XHCI_MISSING_CAS   (1 << 24)
+#define XHCI_BROKEN_STREAMSBIT_ULL(19)
+#define XHCI_PME_STUCK_QUIRK   BIT_ULL(20)
+#define XHCI_MTK_HOST  BIT_ULL(21)
+#define XHCI_SSIC_PORT_UNUSED  BIT_ULL(22)
+#define XHCI_NO_64BIT_SUPPORT  BIT_ULL(23)
+#define XHCI_MISSING_CAS   BIT_ULL(24)
 /* For controller with a broken Port Disable implementation */
-#define XHCI_BROKEN_PORT_PED   (1 << 25)
-#define XHCI_LIMIT_ENDPOINT_INTE

[PATCH v3 0/3] Revised Renesas uPD72020x workaround for 32bit DMA issue

[Marc Zyngier]

Back around the 4.13 timeframe, we tried to address a rather bad issue
with the Renesas uPD72020x USB3 controller family. They have trouble
with the programming of the base addresses which tend to stick on XHCI
reset. This makes transitionning from 64bit to 32bit addresses
completely unsafe. This was observed on a fairly popular arm64
platform (AMD Opteron 1100, which has all of its memory above 4GB).

The fix was to perform a PCI reset of the device, but we have had
multiple reports that this generated regressions (the controller not
being usable after the patch was applied).

This series reverts the problematic patch, and tries to address it in
a more constrained way. If the controller is behind an IOMMU (and only
in that case), we zero its base addresses before reseting it. In the
affected configuration, this has the effect of putting the device in a
state where the XHCI reset will be effective.

It must be noted that in the absence of an IOMMU (and maybe even in
its presence), this device is completely unsafe, and may silently
corrupt memory.

* From v1:
  - Fixed stupid hunk misplacement, now properly moved back to patch 2
  - Converted all quirks to BIT_ULL()

* From v2:
  - Moved zeroing code to its own function
  - Also perform the zeroing on hibernate

Marc Zyngier (3):
  xhci: Allow more than 32 quirks
  xhci: Add quirk to zero 64bit registers on Renesas PCIe controllers
  Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA
issue"

 drivers/usb/host/pci-quirks.c | 20 
 drivers/usb/host/pci-quirks.h |  1 -
 drivers/usb/host/xhci-pci.c   | 15 -
 drivers/usb/host/xhci.c   | 71 +--
 drivers/usb/host/xhci.h   | 67 
 5 files changed, 108 insertions(+), 66 deletions(-)

-- 
2.14.2

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

Re: [PATCH v2 2/3] xhci: Add quirk to zero 64bit registers on Renesas PCIe controllers

[Marc Zyngier]

On 21/05/18 09:23, Mathias Nyman wrote:
> On 18.05.2018 19:29, Marc Zyngier wrote:
>> Some Renesas controllers get into a weird state if they are reset while
>> programmed with 64bit addresses (they will preserve the top half of the
>> address in internal, non visible registers).
>>
>> You end up with half the address coming from the kernel, and the other
>> half coming from the firmware.
> 
> Should those registers be zeroed in resume from hibernate where we also
> reset the host controller?

That's a very good point. Given that this controller also has the
XHCI_RESET_ON_RESUME quick, it is always treated as coming out of
hibernate (how this thing ended up in production is beyond me).

I'll place another stick of dynamite at that level too.

>> Also, changing the programming leads to extra accesses even if the
>> controller is supposed to be halted. The controller ends up with a fatal
>> fault, and is then ripe for being properly reset. On the flip side,
>> this is completely unsafe if the defvice isn't behind an IOMMU, so
>> we have to make sure that this is the case. Can you say "broken"?
>>
>> This is an alternative method to the one introduced in 8466489ef5ba
>> ("xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"),
>> which will subsequently be removed.
>>
>> Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
>> ---
>>   drivers/usb/host/xhci-pci.c |  8 --
>>   drivers/usb/host/xhci.c | 59 
>> +
>>   drivers/usb/host/xhci.h |  1 +
>>   3 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 85ffda85f8ab..e0a0a12871e2 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -196,11 +196,15 @@ static void xhci_pci_quirks(struct device *dev, struct 
>> xhci_hcd *xhci)
>>  xhci->quirks |= XHCI_BROKEN_STREAMS;
>>  }
>>  if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>> -pdev->device == 0x0014)
>> +pdev->device == 0x0014) {
>>  xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>> +xhci->quirks |= XHCI_ZERO_64B_REGS;
>> +}
>>  if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>> -pdev->device == 0x0015)
>> +pdev->device == 0x0015) {
>>  xhci->quirks |= XHCI_RESET_ON_RESUME;
>> +xhci->quirks |= XHCI_ZERO_64B_REGS;
>> +}
>>  if (pdev->vendor == PCI_VENDOR_ID_VIA)
>>  xhci->quirks |= XHCI_RESET_ON_RESUME;
>>   
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 8dba26d3de07..07272d1ce32a 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4921,6 +4921,65 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
>> xhci_get_quirks_t get_quirks)
>>  if (retval)
>>  return retval;
>>   
>> +/*
>> + * Some Renesas controllers get into a weird state if they are
>> + * reset while programmed with 64bit addresses (they will preserve
>> + * the top half of the address in internal, non visible
>> + * registers). You end up with half the address coming from the
>> + * kernel, and the other half coming from the firmware. Also,
>> + * changing the programming leads to extra accesses even if the
>> + * controller is supposed to be halted. The controller ends up with
>> + * a fatal fault, and is then ripe for being properly reset.
>> + *
>> + * Special care is taken to only apply this if the device is behind
>> + * an iommu. Doing anything when there is no iommu is definitely
>> + * unsafe...
>> + */
>> +if ((xhci->quirks & XHCI_ZERO_64B_REGS) && dev->iommu_group) {
>> +u64 val;
>> +int i;
>> +
>> +xhci_info(xhci,
>> +  "Zeroing 64bit base registers, expecting fault\n");
>> +
>> +/* Clear HSEIE so that faults do not get signaled */
>> +val = readl(>op_regs->command);
>> +val &= ~CMD_HSEIE;
>> +writel(val, >op_regs->command);
>> +
>> +/* Clear HSE (aka FATAL) */
>> +val = readl(>op_regs->status);
>> +val |= STS_FATAL;
>> +writel(val, >op_regs->status);
>> +
>> +/* Now zero the

[PATCH v2 2/3] xhci: Add quirk to zero 64bit registers on Renesas PCIe controllers

[Marc Zyngier]

Some Renesas controllers get into a weird state if they are reset while
programmed with 64bit addresses (they will preserve the top half of the
address in internal, non visible registers).

You end up with half the address coming from the kernel, and the other
half coming from the firmware.

Also, changing the programming leads to extra accesses even if the
controller is supposed to be halted. The controller ends up with a fatal
fault, and is then ripe for being properly reset. On the flip side,
this is completely unsafe if the defvice isn't behind an IOMMU, so
we have to make sure that this is the case. Can you say "broken"?

This is an alternative method to the one introduced in 8466489ef5ba
("xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"),
which will subsequently be removed.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/xhci-pci.c |  8 --
 drivers/usb/host/xhci.c | 59 +
 drivers/usb/host/xhci.h |  1 +
 3 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 85ffda85f8ab..e0a0a12871e2 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -196,11 +196,15 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
xhci->quirks |= XHCI_BROKEN_STREAMS;
}
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0014)
+   pdev->device == 0x0014) {
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
+   xhci->quirks |= XHCI_ZERO_64B_REGS;
+   }
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0015)
+   pdev->device == 0x0015) {
xhci->quirks |= XHCI_RESET_ON_RESUME;
+   xhci->quirks |= XHCI_ZERO_64B_REGS;
+   }
if (pdev->vendor == PCI_VENDOR_ID_VIA)
xhci->quirks |= XHCI_RESET_ON_RESUME;
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 8dba26d3de07..07272d1ce32a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4921,6 +4921,65 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
xhci_get_quirks_t get_quirks)
if (retval)
return retval;
 
+   /*
+* Some Renesas controllers get into a weird state if they are
+* reset while programmed with 64bit addresses (they will preserve
+* the top half of the address in internal, non visible
+* registers). You end up with half the address coming from the
+* kernel, and the other half coming from the firmware. Also,
+* changing the programming leads to extra accesses even if the
+* controller is supposed to be halted. The controller ends up with
+* a fatal fault, and is then ripe for being properly reset.
+*
+* Special care is taken to only apply this if the device is behind
+* an iommu. Doing anything when there is no iommu is definitely
+* unsafe...
+*/
+   if ((xhci->quirks & XHCI_ZERO_64B_REGS) && dev->iommu_group) {
+   u64 val;
+   int i;
+
+   xhci_info(xhci,
+ "Zeroing 64bit base registers, expecting fault\n");
+
+   /* Clear HSEIE so that faults do not get signaled */
+   val = readl(>op_regs->command);
+   val &= ~CMD_HSEIE;
+   writel(val, >op_regs->command);
+
+   /* Clear HSE (aka FATAL) */
+   val = readl(>op_regs->status);
+   val |= STS_FATAL;
+   writel(val, >op_regs->status);
+
+   /* Now zero the registers, and brace for impact */
+   val = xhci_read_64(xhci, >op_regs->dcbaa_ptr);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >op_regs->dcbaa_ptr);
+   val = xhci_read_64(xhci, >op_regs->cmd_ring);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >op_regs->cmd_ring);
+
+   for (i = 0; i < HCS_MAX_INTRS(xhci->hcs_params1); i++) {
+   struct xhci_intr_reg __iomem *ir;
+
+   ir = >run_regs->ir_set[i];
+   val = xhci_read_64(xhci, >erst_base);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >erst_base);
+   val= xhci_read_64(xhci, >erst_dequeue);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >erst_dequeue);
+   }
+
+   /* Wait for the fault to appear. It will be cleared on reset */
+   retval 

[PATCH v2 3/3] Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"

[Marc Zyngier]

This reverts commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7.

Now that we can properly reset the uPD72020x without a hard PCI reset,
let's get rid of the existing quirks.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/pci-quirks.c | 20 
 drivers/usb/host/pci-quirks.h |  1 -
 drivers/usb/host/xhci-pci.c   |  7 ---
 3 files changed, 28 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 67ad4bb6919a..3625a5c1a41b 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1268,23 +1268,3 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
-
-bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
-{
-   /*
-* Our dear uPD72020{1,2} friend only partially resets when
-* asked to via the XHCI interface, and may end up doing DMA
-* at the wrong addresses, as it keeps the top 32bit of some
-* addresses from its previous programming under obscure
-* circumstances.
-* Give it a good wack at probe time. Unfortunately, this
-* needs to happen before we've had a chance to discover any
-* quirk, or the system will be in a rather bad state.
-*/
-   if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   (pdev->device == 0x0014 || pdev->device == 0x0015))
-   return true;
-
-   return false;
-}
-EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index 4ca0d9b7e463..63c633077d9e 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -16,7 +16,6 @@ void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
 void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
 void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
 void sb800_prefetch(struct device *dev, int on);
-bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
 bool usb_amd_pt_check_port(struct device *device, int port);
 #else
 struct pci_dev;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index e0a0a12871e2..6372edf339d9 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -288,13 +288,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const 
struct pci_device_id *id)
 
driver = (struct hc_driver *)id->driver_data;
 
-   /* For some HW implementation, a XHCI reset is just not enough... */
-   if (usb_xhci_needs_pci_reset(dev)) {
-   dev_info(>dev, "Resetting\n");
-   if (pci_reset_function_locked(dev))
-   dev_warn(>dev, "Reset failed");
-   }
-
/* Prevent runtime suspending between USB-2 and USB-3 initialization */
pm_runtime_get_noresume(>dev);
 
-- 
2.14.2

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

[PATCH v2 1/3] xhci: Allow more than 32 quirks

[Marc Zyngier]

We now have 32 different quirks, and the field that holds them
is full. Let's bump it up to the next stage so that we can handle
some more... The type is now an unsigned long long, which is 64bit
on most architectures.

We take this opportunity to change the quirks from using (1 << x)
to BIT_ULL(x).

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/xhci.c |  6 ++---
 drivers/usb/host/xhci.h | 66 -
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 711da3306b14..8dba26d3de07 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -33,8 +33,8 @@ static int link_quirk;
 module_param(link_quirk, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(link_quirk, "Don't clear the chain bit on a link TRB");
 
-static unsigned int quirks;
-module_param(quirks, uint, S_IRUGO);
+static unsigned long long quirks;
+module_param(quirks, ullong, S_IRUGO);
 MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default");
 
 /* TODO: copied from ehci-hcd.c - can this be refactored? */
@@ -4963,7 +4963,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t 
get_quirks)
return retval;
xhci_dbg(xhci, "Called HCD init\n");
 
-   xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%08x\n",
+   xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%016llx\n",
  xhci->hcc_params, xhci->hci_version, xhci->quirks);
 
return 0;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 6dfc4867dbcf..e322afd3f08b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1787,12 +1787,12 @@ struct xhci_hcd {
 #define XHCI_STATE_DYING   (1 << 0)
 #define XHCI_STATE_HALTED  (1 << 1)
 #define XHCI_STATE_REMOVING(1 << 2)
-   unsigned intquirks;
-#defineXHCI_LINK_TRB_QUIRK (1 << 0)
-#define XHCI_RESET_EP_QUIRK(1 << 1)
-#define XHCI_NEC_HOST  (1 << 2)
-#define XHCI_AMD_PLL_FIX   (1 << 3)
-#define XHCI_SPURIOUS_SUCCESS  (1 << 4)
+   unsigned long long  quirks;
+#defineXHCI_LINK_TRB_QUIRK BIT_ULL(0)
+#define XHCI_RESET_EP_QUIRKBIT_ULL(1)
+#define XHCI_NEC_HOST  BIT_ULL(2)
+#define XHCI_AMD_PLL_FIX   BIT_ULL(3)
+#define XHCI_SPURIOUS_SUCCESS  BIT_ULL(4)
 /*
  * Certain Intel host controllers have a limit to the number of endpoint
  * contexts they can handle.  Ideally, they would signal that they can't handle
@@ -1802,35 +1802,35 @@ struct xhci_hcd {
  * commands, reset device commands, disable slot commands, and address device
  * commands.
  */
-#define XHCI_EP_LIMIT_QUIRK(1 << 5)
-#define XHCI_BROKEN_MSI(1 << 6)
-#define XHCI_RESET_ON_RESUME   (1 << 7)
-#defineXHCI_SW_BW_CHECKING (1 << 8)
-#define XHCI_AMD_0x96_HOST (1 << 9)
-#define XHCI_TRUST_TX_LENGTH   (1 << 10)
-#define XHCI_LPM_SUPPORT   (1 << 11)
-#define XHCI_INTEL_HOST(1 << 12)
-#define XHCI_SPURIOUS_REBOOT   (1 << 13)
-#define XHCI_COMP_MODE_QUIRK   (1 << 14)
-#define XHCI_AVOID_BEI (1 << 15)
-#define XHCI_PLAT  (1 << 16)
-#define XHCI_SLOW_SUSPEND  (1 << 17)
-#define XHCI_SPURIOUS_WAKEUP   (1 << 18)
+#define XHCI_EP_LIMIT_QUIRKBIT_ULL(5)
+#define XHCI_BROKEN_MSIBIT_ULL(6)
+#define XHCI_RESET_ON_RESUME   BIT_ULL(7)
+#defineXHCI_SW_BW_CHECKING BIT_ULL(8)
+#define XHCI_AMD_0x96_HOST BIT_ULL(9)
+#define XHCI_TRUST_TX_LENGTH   BIT_ULL(10)
+#define XHCI_LPM_SUPPORT   BIT_ULL(11)
+#define XHCI_INTEL_HOSTBIT_ULL(12)
+#define XHCI_SPURIOUS_REBOOT   BIT_ULL(13)
+#define XHCI_COMP_MODE_QUIRK   BIT_ULL(14)
+#define XHCI_AVOID_BEI BIT_ULL(15)
+#define XHCI_PLAT  BIT_ULL(16)
+#define XHCI_SLOW_SUSPEND  BIT_ULL(17)
+#define XHCI_SPURIOUS_WAKEUP   BIT_ULL(18)
 /* For controllers with a broken beyond repair streams implementation */
-#define XHCI_BROKEN_STREAMS(1 << 19)
-#define XHCI_PME_STUCK_QUIRK   (1 << 20)
-#define XHCI_MTK_HOST  (1 << 21)
-#define XHCI_SSIC_PORT_UNUSED  (1 << 22)
-#define XHCI_NO_64BIT_SUPPORT  (1 << 23)
-#define XHCI_MISSING_CAS   (1 << 24)
+#define XHCI_BROKEN_STREAMSBIT_ULL(19)
+#define XHCI_PME_STUCK_QUIRK   BIT_ULL(20)
+#define XHCI_MTK_HOST  BIT_ULL(21)
+#define XHCI_SSIC_PORT_UNUSED  BIT_ULL(22)
+#define XHCI_NO_64BIT_SUPPORT  BIT_ULL(23)
+#define XHCI_MISSING_CAS   BIT_ULL(24)
 /* For controller with a broken Port Disable implementation */
-#define XHCI_BROKEN_PORT_PED   (1 << 25)
-#define XHCI_LIMIT_ENDPOINT_INTERVAL_7 (1 << 26)
-#define XHCI_U2_DISABLE_WA

[PATCH v2 0/3] Revised Renesas uPD72020x workaround for 32bit DMA issue

[Marc Zyngier]

Back around the 4.13 timeframe, we tried to address a rather bad issue
with the Renesas uPD72020x USB3 controller family. They have trouble
with the programming of the base addresses which tend to stick on XHCI
reset. This makes transitionning from 64bit to 32bit addresses
completely unsafe. This was observed on a fairly popular arm64
platform (AMD Opteron 1100, which has all of its memory above 4GB).

The fix was to perform a PCI reset of the device, but we have had
multiple reports that this generated regressions (the controller not
being usable after the patch was applied).

This series reverts the problematic patch, and tries to address it in
a more constrained way. If the controller is behind an IOMMU (and only
in that case), we zero its base addresses before reseting it. In the
affected configuration, this has the effect of putting the device in a
state where the XHCI reset will be effective.

It must be noted that in the absence of an IOMMU (and maybe even in
its presence), this device is completely unsafe, and may silently
corrupt memory.

* From v1:
  - Fixed stupid hunk misplacement, now properly moved back to patch 2
  - Converted all quirks to BIT_ULL()

Marc Zyngier (3):
  xhci: Allow more than 32 quirks
  xhci: Add quirk to zero 64bit registers on Renesas PCIe controllers
  Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA
issue"

 drivers/usb/host/pci-quirks.c | 20 -
 drivers/usb/host/pci-quirks.h |  1 -
 drivers/usb/host/xhci-pci.c   | 15 --
 drivers/usb/host/xhci.c   | 65 +++--
 drivers/usb/host/xhci.h   | 67 ++-
 5 files changed, 102 insertions(+), 66 deletions(-)

-- 
2.14.2

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

Re: [PATCH 3/3] Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"

[Marc Zyngier]

On 17/05/18 14:29, Greg KH wrote:
> On Thu, May 17, 2018 at 01:58:36PM +0100, Marc Zyngier wrote:
>> This reverts commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7.
>>
>> Now that we can properly reset the uPD72020x without a hard PCI reset,
>> let's get rid of the existing quirks.
>>
>> Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
>> ---
>>  drivers/usb/host/pci-quirks.c | 20 
>>  drivers/usb/host/pci-quirks.h |  1 -
>>  drivers/usb/host/xhci-pci.c   |  7 ---
>>  drivers/usb/host/xhci.c   | 21 -
>>  drivers/usb/host/xhci.h   |  2 +-
>>  5 files changed, 13 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
>> index 67ad4bb6919a..3625a5c1a41b 100644
>> --- a/drivers/usb/host/pci-quirks.c
>> +++ b/drivers/usb/host/pci-quirks.c
>> @@ -1268,23 +1268,3 @@ static void quirk_usb_early_handoff(struct pci_dev 
>> *pdev)
>>  }
>>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
>>  PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
>> -
>> -bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
>> -{
>> -/*
>> - * Our dear uPD72020{1,2} friend only partially resets when
>> - * asked to via the XHCI interface, and may end up doing DMA
>> - * at the wrong addresses, as it keeps the top 32bit of some
>> - * addresses from its previous programming under obscure
>> - * circumstances.
>> - * Give it a good wack at probe time. Unfortunately, this
>> - * needs to happen before we've had a chance to discover any
>> - * quirk, or the system will be in a rather bad state.
>> - */
>> -if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>> -(pdev->device == 0x0014 || pdev->device == 0x0015))
>> -return true;
>> -
>> -return false;
>> -}
>> -EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
>> diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
>> index 4ca0d9b7e463..63c633077d9e 100644
>> --- a/drivers/usb/host/pci-quirks.h
>> +++ b/drivers/usb/host/pci-quirks.h
>> @@ -16,7 +16,6 @@ void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
>>  void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
>>  void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
>>  void sb800_prefetch(struct device *dev, int on);
>> -bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
>>  bool usb_amd_pt_check_port(struct device *device, int port);
>>  #else
>>  struct pci_dev;
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index e0a0a12871e2..6372edf339d9 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -288,13 +288,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const 
>> struct pci_device_id *id)
>>  
>>  driver = (struct hc_driver *)id->driver_data;
>>  
>> -/* For some HW implementation, a XHCI reset is just not enough... */
>> -if (usb_xhci_needs_pci_reset(dev)) {
>> -dev_info(>dev, "Resetting\n");
>> -if (pci_reset_function_locked(dev))
>> -dev_warn(>dev, "Reset failed");
>> -}
>> -
>>  /* Prevent runtime suspending between USB-2 and USB-3 initialization */
>>  pm_runtime_get_noresume(>dev);
>>  
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index cd6b48c43007..07272d1ce32a 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4923,16 +4923,19 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
>> xhci_get_quirks_t get_quirks)
>>  
>>  /*
>>   * Some Renesas controllers get into a weird state if they are
>> - * reset while programmed with 64bit addresses (they will
>> - * preserve the top half of the address in internal, non
>> - * visible registers). You end up with half the address coming
>> - * from the kernel, and the other half coming from the
>> - * firmware. Also, changing the programming leads to extra
>> - * accesses even if the controller is supposed to be
>> - * halted. The controller ends up with a fatal fault, and is
>> - * then ripe for being properly reset.
>> + * reset while programmed with 64bit addresses (they will preserve
>> + * the top half of the address in internal, non visible
>> + * registers). You end up with half the address coming from the
&

Re: [PATCH 1/3] xhci: Allow more than 32 quirks

[Marc Zyngier]

On 17/05/18 14:28, Greg KH wrote:
> On Thu, May 17, 2018 at 01:58:34PM +0100, Marc Zyngier wrote:
>> We now have 32 different quirks, and the field that holds them
>> is full. Let's bump it up to the next stage so that we can handle
>> some more... The type is now an unsigned long long, which is 64bit
>> on most architectures.
>>
>> Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
>> ---
>>  drivers/usb/host/xhci.c | 6 +++---
>>  drivers/usb/host/xhci.h | 2 +-
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 711da3306b14..8dba26d3de07 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -33,8 +33,8 @@ static int link_quirk;
>>  module_param(link_quirk, int, S_IRUGO | S_IWUSR);
>>  MODULE_PARM_DESC(link_quirk, "Don't clear the chain bit on a link TRB");
>>  
>> -static unsigned int quirks;
>> -module_param(quirks, uint, S_IRUGO);
>> +static unsigned long long quirks;
>> +module_param(quirks, ullong, S_IRUGO);
>>  MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default");
>>  
>>  /* TODO: copied from ehci-hcd.c - can this be refactored? */
>> @@ -4963,7 +4963,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
>> xhci_get_quirks_t get_quirks)
>>  return retval;
>>  xhci_dbg(xhci, "Called HCD init\n");
>>  
>> -xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%08x\n",
>> +xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%016llx\n",
>>xhci->hcc_params, xhci->hci_version, xhci->quirks);
>>  
>>  return 0;
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 6dfc4867dbcf..42848dfc3445 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1787,7 +1787,7 @@ struct xhci_hcd {
>>  #define XHCI_STATE_DYING(1 << 0)
>>  #define XHCI_STATE_HALTED   (1 << 1)
>>  #define XHCI_STATE_REMOVING (1 << 2)
>> -unsigned intquirks;
>> +unsigned long long  quirks;
> 
> u64?

Sure. That's just slightly odd with the "unsigned long long" imposed by
the module_param above.

> 
>>  #define XHCI_LINK_TRB_QUIRK (1 << 0)
>>  #define XHCI_RESET_EP_QUIRK (1 << 1)
>>  #define XHCI_NEC_HOST   (1 << 2)
> 
> And these all can use BIT(), right?
If you're happy with the churn, I'll repaint them.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] xhci: Add quirk to zero 64bit registers on Renesas PCIe controllers

[Marc Zyngier]

Some Renesas controllers get into a weird state if they are reset while
programmed with 64bit addresses (they will preserve the top half of the
address in internal, non visible registers).

You end up with half the address coming from the kernel, and the other
half coming from the firmware.

Also, changing the programming leads to extra accesses even if the
controller is supposed to be halted. The controller ends up with a fatal
fault, and is then ripe for being properly reset. On the flip side,
this is completely unsafe if the defvice isn't behind an IOMMU, so
we have to make sure that this is the case. Can you say "broken"?

This is an alternative method to the one introduced in 8466489ef5ba
("xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"),
which will subsequently be removed.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/xhci-pci.c |  8 +--
 drivers/usb/host/xhci.c | 56 +
 drivers/usb/host/xhci.h |  1 +
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 85ffda85f8ab..e0a0a12871e2 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -196,11 +196,15 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
xhci->quirks |= XHCI_BROKEN_STREAMS;
}
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0014)
+   pdev->device == 0x0014) {
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
+   xhci->quirks |= XHCI_ZERO_64B_REGS;
+   }
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0015)
+   pdev->device == 0x0015) {
xhci->quirks |= XHCI_RESET_ON_RESUME;
+   xhci->quirks |= XHCI_ZERO_64B_REGS;
+   }
if (pdev->vendor == PCI_VENDOR_ID_VIA)
xhci->quirks |= XHCI_RESET_ON_RESUME;
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 8dba26d3de07..cd6b48c43007 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4921,6 +4921,62 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
xhci_get_quirks_t get_quirks)
if (retval)
return retval;
 
+   /*
+* Some Renesas controllers get into a weird state if they are
+* reset while programmed with 64bit addresses (they will
+* preserve the top half of the address in internal, non
+* visible registers). You end up with half the address coming
+* from the kernel, and the other half coming from the
+* firmware. Also, changing the programming leads to extra
+* accesses even if the controller is supposed to be
+* halted. The controller ends up with a fatal fault, and is
+* then ripe for being properly reset.
+*/
+   if (xhci->quirks & XHCI_ZERO_64B_REGS) {
+   u64 val;
+   int i;
+
+   xhci_info(xhci,
+ "Zeroing 64bit base registers, expecting fault\n");
+
+   /* Clear HSEIE so that faults do not get signaled */
+   val = readl(>op_regs->command);
+   val &= ~CMD_HSEIE;
+   writel(val, >op_regs->command);
+
+   /* Clear HSE (aka FATAL) */
+   val = readl(>op_regs->status);
+   val |= STS_FATAL;
+   writel(val, >op_regs->status);
+
+   /* Now zero the registers, and brace for impact */
+   val = xhci_read_64(xhci, >op_regs->dcbaa_ptr);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >op_regs->dcbaa_ptr);
+   val = xhci_read_64(xhci, >op_regs->cmd_ring);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >op_regs->cmd_ring);
+
+   for (i = 0; i < HCS_MAX_INTRS(xhci->hcs_params1); i++) {
+   struct xhci_intr_reg __iomem *ir;
+
+   ir = >run_regs->ir_set[i];
+   val = xhci_read_64(xhci, >erst_base);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >erst_base);
+   val= xhci_read_64(xhci, >erst_dequeue);
+   if (upper_32_bits(val))
+   xhci_write_64(xhci, 0, >erst_dequeue);
+   }
+
+   /* Wait for the fault to appear. It will be cleared on reset */
+   retval = xhci_handshake(>op_regs->status,
+   STS_FATAL, STS_FATAL,
+   XHCI_MAX_HALT_USEC);
+   if (!retval)
+   xhc

[PATCH 0/3] Revised Renesas uPD72020x workaround for 32bit DMA issue

[Marc Zyngier]

Back around the 4.13 timeframe, we tried to address a rather bad issue
with the Renesas uPD72020x USB3 controller family, that have a
horrible issue with the programming of the base addresses which tend
to stick on XHCI reset. This makes transitionning from 64bit to 32bit
addresses completely unsafe. This was observed on a fairly popular
arm64 platform (AMD Opteron 1100, which has all of its memory above
4GB).

The fixe was to perform a PCI reset of the device, but we have
multiple reports that this generated regressions (the controller not
being usable after the patch was applied).

This series reverts the problematic patch, and tries to address it in
a more constrained way. If the controller is behind an IOMMU (and only
in that case), we zero its base addresses before reseting it. In the
affected configuration, this has the effect of putting the device in a
state where the XHCI reset will be effective.

It must be noted that in the absence of an IOMMU (and maybe even in
its presence), this device is completely unsafe, and may silently
corrupt memory.

Marc Zyngier (3):
  xhci: Allow more than 32 quirks
  xhci: Add quirk to zero 64bit registers on Renesas PCIe controllers
  Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA
issue"

 drivers/usb/host/pci-quirks.c | 20 -
 drivers/usb/host/pci-quirks.h |  1 -
 drivers/usb/host/xhci-pci.c   | 15 --
 drivers/usb/host/xhci.c   | 65 +--
 drivers/usb/host/xhci.h   |  3 +-
 5 files changed, 70 insertions(+), 34 deletions(-)

-- 
2.14.2

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

[PATCH 3/3] Revert "xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"

[Marc Zyngier]

This reverts commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7.

Now that we can properly reset the uPD72020x without a hard PCI reset,
let's get rid of the existing quirks.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/pci-quirks.c | 20 
 drivers/usb/host/pci-quirks.h |  1 -
 drivers/usb/host/xhci-pci.c   |  7 ---
 drivers/usb/host/xhci.c   | 21 -
 drivers/usb/host/xhci.h   |  2 +-
 5 files changed, 13 insertions(+), 38 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 67ad4bb6919a..3625a5c1a41b 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1268,23 +1268,3 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
-
-bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
-{
-   /*
-* Our dear uPD72020{1,2} friend only partially resets when
-* asked to via the XHCI interface, and may end up doing DMA
-* at the wrong addresses, as it keeps the top 32bit of some
-* addresses from its previous programming under obscure
-* circumstances.
-* Give it a good wack at probe time. Unfortunately, this
-* needs to happen before we've had a chance to discover any
-* quirk, or the system will be in a rather bad state.
-*/
-   if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   (pdev->device == 0x0014 || pdev->device == 0x0015))
-   return true;
-
-   return false;
-}
-EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index 4ca0d9b7e463..63c633077d9e 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -16,7 +16,6 @@ void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
 void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
 void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
 void sb800_prefetch(struct device *dev, int on);
-bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
 bool usb_amd_pt_check_port(struct device *device, int port);
 #else
 struct pci_dev;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index e0a0a12871e2..6372edf339d9 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -288,13 +288,6 @@ static int xhci_pci_probe(struct pci_dev *dev, const 
struct pci_device_id *id)
 
driver = (struct hc_driver *)id->driver_data;
 
-   /* For some HW implementation, a XHCI reset is just not enough... */
-   if (usb_xhci_needs_pci_reset(dev)) {
-   dev_info(>dev, "Resetting\n");
-   if (pci_reset_function_locked(dev))
-   dev_warn(>dev, "Reset failed");
-   }
-
/* Prevent runtime suspending between USB-2 and USB-3 initialization */
pm_runtime_get_noresume(>dev);
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index cd6b48c43007..07272d1ce32a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4923,16 +4923,19 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
xhci_get_quirks_t get_quirks)
 
/*
 * Some Renesas controllers get into a weird state if they are
-* reset while programmed with 64bit addresses (they will
-* preserve the top half of the address in internal, non
-* visible registers). You end up with half the address coming
-* from the kernel, and the other half coming from the
-* firmware. Also, changing the programming leads to extra
-* accesses even if the controller is supposed to be
-* halted. The controller ends up with a fatal fault, and is
-* then ripe for being properly reset.
+* reset while programmed with 64bit addresses (they will preserve
+* the top half of the address in internal, non visible
+* registers). You end up with half the address coming from the
+* kernel, and the other half coming from the firmware. Also,
+* changing the programming leads to extra accesses even if the
+* controller is supposed to be halted. The controller ends up with
+* a fatal fault, and is then ripe for being properly reset.
+*
+* Special care is taken to only apply this if the device is behind
+* an iommu. Doing anything when there is no iommu is definitely
+* unsafe...
 */
-   if (xhci->quirks & XHCI_ZERO_64B_REGS) {
+   if ((xhci->quirks & XHCI_ZERO_64B_REGS) && dev->iommu_group) {
u64 val;
int i;
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 5ea0b52b49cc..758864767fd4 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1831,7

[PATCH 1/3] xhci: Allow more than 32 quirks

[Marc Zyngier]

We now have 32 different quirks, and the field that holds them
is full. Let's bump it up to the next stage so that we can handle
some more... The type is now an unsigned long long, which is 64bit
on most architectures.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/xhci.c | 6 +++---
 drivers/usb/host/xhci.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 711da3306b14..8dba26d3de07 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -33,8 +33,8 @@ static int link_quirk;
 module_param(link_quirk, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(link_quirk, "Don't clear the chain bit on a link TRB");
 
-static unsigned int quirks;
-module_param(quirks, uint, S_IRUGO);
+static unsigned long long quirks;
+module_param(quirks, ullong, S_IRUGO);
 MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default");
 
 /* TODO: copied from ehci-hcd.c - can this be refactored? */
@@ -4963,7 +4963,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t 
get_quirks)
return retval;
xhci_dbg(xhci, "Called HCD init\n");
 
-   xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%08x\n",
+   xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%016llx\n",
  xhci->hcc_params, xhci->hci_version, xhci->quirks);
 
return 0;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 6dfc4867dbcf..42848dfc3445 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1787,7 +1787,7 @@ struct xhci_hcd {
 #define XHCI_STATE_DYING   (1 << 0)
 #define XHCI_STATE_HALTED  (1 << 1)
 #define XHCI_STATE_REMOVING(1 << 2)
-   unsigned intquirks;
+   unsigned long long  quirks;
 #defineXHCI_LINK_TRB_QUIRK (1 << 0)
 #define XHCI_RESET_EP_QUIRK(1 << 1)
 #define XHCI_NEC_HOST  (1 << 2)
-- 
2.14.2

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

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On Mon, 07 May 2018 06:00:56 +0100,
Bockholdt Arne wrote:
> 
> 
> 
> On Thu, 2018-05-03 at 10:41 +0100, Marc Zyngier wrote:
> > I'm talking about making the whole workaround dependent on the USB
> > controller being behind an iommu. No iommu, no workaround (because it
> > is
> > likely that there is no problem in that case).
> > 
> My server doesn't have an IOMMU at all and is affected by the bug. I'm
> the original reporter of the problem.

No, you are just reporting a regression caused by the workaround for
the original issue, which has to do with transitioning from 64bit
addresses to 32bit addresses.

Resting the controller solves that problem, but generates other issues
(yours and a few others). The current approach is to drop the
mandatory reset and to use a different sequence, only if the device is
behind an IOMMU.

So in your case, we won't do anything at all, which is what seem to
work for most people. Only a subset of the platforms that would be
affected will have another workaround. Machines that don't have an
IOMMU *and* perform an 64/32 address range transition may see memory
corruption or other side effects, but that's nothing new (the HW looks
terminally broken).

Hope this makes it clear.

M.

-- 
Jazz is not dead, it just smell funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On 03/05/18 11:41, Ard Biesheuvel wrote:
> On 3 May 2018 at 11:41, Marc Zyngier <marc.zyng...@arm.com> wrote:
>> On 03/05/18 09:42, Faiz Abbas wrote:
>>> Hi Marc,
>>>
>>> On Thursday 03 May 2018 01:44 PM, Marc Zyngier wrote:
>>>> On 03/05/18 05:49, Faiz Abbas wrote:
>>>>> Hi Everyone,
>>>>>
>>>>> On Wednesday 11 April 2018 07:32 PM, Marc Zyngier wrote:
>>>>>> On Wed, 11 Apr 2018 14:11:52 +0100,
>>>>>> Bockholdt Arne wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> is there anything new, I've just tried the new stable 4.16.1 kernel
>>>>>>> without any change. The Renesys USB3 controller is still not
>>>>>>> functional. I'm willing to test any patch that is based on a stable
>>>>>>> kernel version because the machine is in production use.
>>>>>>
>>>>>> Have you tested the branch[1] I mentioned in my previous email?
>>>>>> Without your feedback, I cannot really make much progress on this.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>M.
>>>>>>
>>>>>> [1] https://www.spinics.net/lists/linux-usb/msg167301.html
>>>>>>
>>>>>
>>>>> I was also having problems with a Renesas card (01:00.0 USB controller
>>>>> [0c03]: Renesas Technology Corp. uPD720201 USB 3.0 Host Controller
>>>>> [1912:0014]) on a dra72x-evm. The kernel would just hang because of the
>>>>> xhci reset.
>>>>>
>>>>> Log:https://pastebin.ubuntu.com/p/dYYV9DZMwx/
>>>>>
>>>>> The patches on Marc's branch have fixed this for me as well. Thanks for
>>>>> the fix.
>>>> OK. I'll rebase this on a more recent version of the kernel, make it
>>>> conditional on having an iommu (as it seems to be the only affected
>>>> configuration), and post that to a wider audience.
>>>
>>> Just to be sure, you are talking about the original 32 bit DMA issue
>>> being conditional on iommu right? Because dra72x doesn't use iommu.
>>
>> I'm talking about making the whole workaround dependent on the USB
>> controller being behind an iommu. No iommu, no workaround (because it is
>> likely that there is no problem in that case).
>>
> 
> That is still only a partial solution, unfortunately.
> 
> On non-x86 systems with memory above and below the 4 GB mark, it is
> undefined which side the firmware will choose and which side the
> kernel will choose (although I suppose the kernel may attempt to
> preserve 32-bit addressable memory for peripherals that are only
> 32-bit capable)
> 
> This would be much easier to fix at the UEFI stub level (but still in
> kernel code, not firmware), by checking for PCI I/O protocol instances
> with the correct VID/PID and check whether the
> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is set, but that is
> also only a partial solution.

Unfortunately, I'm not sure there is a full solution that works for
everyone, and doesn't introduce regression. The reset method is proving
to be bad for a number of platform, so I'd rather have something that
narrowly matches the one broken case we know about.

Without knowing more about the internals of that controller, I'm not
convinced we can do much more...

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On 03/05/18 09:42, Faiz Abbas wrote:
> Hi Marc,
> 
> On Thursday 03 May 2018 01:44 PM, Marc Zyngier wrote:
>> On 03/05/18 05:49, Faiz Abbas wrote:
>>> Hi Everyone,
>>>
>>> On Wednesday 11 April 2018 07:32 PM, Marc Zyngier wrote:
>>>> On Wed, 11 Apr 2018 14:11:52 +0100,
>>>> Bockholdt Arne wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> is there anything new, I've just tried the new stable 4.16.1 kernel
>>>>> without any change. The Renesys USB3 controller is still not
>>>>> functional. I'm willing to test any patch that is based on a stable
>>>>> kernel version because the machine is in production use.
>>>>
>>>> Have you tested the branch[1] I mentioned in my previous email?
>>>> Without your feedback, I cannot really make much progress on this.
>>>>
>>>> Thanks,
>>>>
>>>>M.
>>>>
>>>> [1] https://www.spinics.net/lists/linux-usb/msg167301.html
>>>>
>>>
>>> I was also having problems with a Renesas card (01:00.0 USB controller
>>> [0c03]: Renesas Technology Corp. uPD720201 USB 3.0 Host Controller
>>> [1912:0014]) on a dra72x-evm. The kernel would just hang because of the
>>> xhci reset.
>>>
>>> Log:https://pastebin.ubuntu.com/p/dYYV9DZMwx/
>>>
>>> The patches on Marc's branch have fixed this for me as well. Thanks for
>>> the fix.
>> OK. I'll rebase this on a more recent version of the kernel, make it
>> conditional on having an iommu (as it seems to be the only affected
>> configuration), and post that to a wider audience.
> 
> Just to be sure, you are talking about the original 32 bit DMA issue
> being conditional on iommu right? Because dra72x doesn't use iommu.

I'm talking about making the whole workaround dependent on the USB
controller being behind an iommu. No iommu, no workaround (because it is
likely that there is no problem in that case).

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On 03/05/18 05:49, Faiz Abbas wrote:
> Hi Everyone,
> 
> On Wednesday 11 April 2018 07:32 PM, Marc Zyngier wrote:
>> On Wed, 11 Apr 2018 14:11:52 +0100,
>> Bockholdt Arne wrote:
>>>
>>> Hi all,
>>>
>>> is there anything new, I've just tried the new stable 4.16.1 kernel
>>> without any change. The Renesys USB3 controller is still not
>>> functional. I'm willing to test any patch that is based on a stable
>>> kernel version because the machine is in production use.
>>
>> Have you tested the branch[1] I mentioned in my previous email?
>> Without your feedback, I cannot really make much progress on this.
>>
>> Thanks,
>>
>>  M.
>>  
>> [1] https://www.spinics.net/lists/linux-usb/msg167301.html
>>
> 
> I was also having problems with a Renesas card (01:00.0 USB controller
> [0c03]: Renesas Technology Corp. uPD720201 USB 3.0 Host Controller
> [1912:0014]) on a dra72x-evm. The kernel would just hang because of the
> xhci reset.
> 
> Log:https://pastebin.ubuntu.com/p/dYYV9DZMwx/
> 
> The patches on Marc's branch have fixed this for me as well. Thanks for
> the fix.
OK. I'll rebase this on a more recent version of the kernel, make it
conditional on having an iommu (as it seems to be the only affected
configuration), and post that to a wider audience.

I'll keep you guys on cc.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On Wed, 11 Apr 2018 14:11:52 +0100,
Bockholdt Arne wrote:
> 
> Hi all,
> 
> is there anything new, I've just tried the new stable 4.16.1 kernel
> without any change. The Renesys USB3 controller is still not
> functional. I'm willing to test any patch that is based on a stable
> kernel version because the machine is in production use.

Have you tested the branch[1] I mentioned in my previous email?
Without your feedback, I cannot really make much progress on this.

Thanks,

M.

[1] https://www.spinics.net/lists/linux-usb/msg167301.html

-- 
Jazz is not dead, it just smell funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] net: usb: asix88179_178a: de-duplicate code

[Marc Zyngier]

On Mon, 2 Apr 2018 07:43:49 +
Alexander Kurz  wrote:

> Remove the duplicated code for asix88179_178a bind and reset methods.
> 
> Signed-off-by: Alexander Kurz 
> ---
>  drivers/net/usb/ax88179_178a.c | 137 
> ++---
>  1 file changed, 31 insertions(+), 106 deletions(-)

The good news is that this patch doesn't seem to break anything this
time. A few remarks below:

> 
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index a6ef75907ae9..fea4c7b877cc 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1223,7 +1223,7 @@ static int ax88179_led_setting(struct usbnet *dev)
>   return 0;
>  }
>  
> -static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
> +static int ax88179_bind_or_reset(struct usbnet *dev, bool do_reset)
>  {
>   u8 buf[5];
>   u16 *tmp16;
> @@ -1231,12 +1231,11 @@ static int ax88179_bind(struct usbnet *dev, struct 
> usb_interface *intf)
>   struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
>   struct ethtool_eee eee_data;
>  
> - usbnet_get_endpoints(dev, intf);
> -

So you move this to "bind"...

>   tmp16 = (u16 *)buf;
>   tmp = (u8 *)buf;
>  
> - memset(ax179_data, 0, sizeof(*ax179_data));
> + if (!do_reset)
> + memset(ax179_data, 0, sizeof(*ax179_data));

... but not that. Why? They both are exclusive to the bind operation.

>  
>   /* Power up ethernet PHY */
>   *tmp16 = 0;
> @@ -1249,9 +1248,13 @@ static int ax88179_bind(struct usbnet *dev, struct 
> usb_interface *intf)
>   ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
>   msleep(100);
>  
> + if (do_reset)
> + ax88179_auto_detach(dev, 0);
> +
>   ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
>ETH_ALEN, dev->net->dev_addr);
> - memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
> + if (!do_reset)
> + memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
>  
>   /* RX bulk configuration */
>   memcpy(tmp, _BULKIN_SIZE[0], 5);
> @@ -1266,19 +1269,21 @@ static int ax88179_bind(struct usbnet *dev, struct 
> usb_interface *intf)
>   ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH,
> 1, 1, tmp);
>  
> - dev->net->netdev_ops = _netdev_ops;
> - dev->net->ethtool_ops = _ethtool_ops;
> - dev->net->needed_headroom = 8;
> - dev->net->max_mtu = 4088;
> -
> - /* Initialize MII structure */
> - dev->mii.dev = dev->net;
> - dev->mii.mdio_read = ax88179_mdio_read;
> - dev->mii.mdio_write = ax88179_mdio_write;
> - dev->mii.phy_id_mask = 0xff;
> - dev->mii.reg_num_mask = 0xff;
> - dev->mii.phy_id = 0x03;
> - dev->mii.supports_gmii = 1;
> + if (!do_reset) {
> + dev->net->netdev_ops = _netdev_ops;
> + dev->net->ethtool_ops = _ethtool_ops;
> + dev->net->needed_headroom = 8;
> + dev->net->max_mtu = 4088;
> +
> + /* Initialize MII structure */
> + dev->mii.dev = dev->net;
> + dev->mii.mdio_read = ax88179_mdio_read;
> + dev->mii.mdio_write = ax88179_mdio_write;
> + dev->mii.phy_id_mask = 0xff;
> + dev->mii.reg_num_mask = 0xff;
> + dev->mii.phy_id = 0x03;
> + dev->mii.supports_gmii = 1;
> + }
>  
>   dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> NETIF_F_RXCSUM;
> @@ -1330,6 +1335,13 @@ static int ax88179_bind(struct usbnet *dev, struct 
> usb_interface *intf)
>   return 0;
>  }
>  
> +static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
> +{
> + usbnet_get_endpoints(dev, intf);
> +
> + return ax88179_bind_or_reset(dev, false);
> +}

I find the whole construct extremely messy.

You shouldn't need to "bind or reset" the adapter. You first reset it
(that's the HW part), and you then bind it (that's the SW part). I
understand that the code is quite messy already, and that you're trying
to improve it. I'm just not convinced that having a single function
containing everything and the kitchen sink is the most sensible
approach.

Instead, you probably want to extract stuff that needs to be done at
reset time from what can be done at bind time, and keep the two quite
separate. The fact that bind is mostly a superset of reset is a bit of
an odd thing, and it'd be good to get to the bottom of that (I fully
admit that my understanding of USB networking is close to zero).

I came up with the below hack on top of your patch, and things seem to
still behave.

Thanks,

M.

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index fea4c7b877cc..aed98d400d7a 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1223,7 +1223,7 @@ static int 

Re: [PATCH v3 2/2] net: usb: asix88179_178a: de-duplicate code

[Marc Zyngier]

[dropping Freddy as I'm getting bounces from asix.com.tw]

On Mon, 2 Apr 2018 15:21:08 + Alexander Kurz  wrote:

Alexander,

> Hi Marc, David,
> with the v2 patch ("net: usb: asix88179_178a: de-duplicate code")
> I made an embarrasly stupid mistake of removing the wrong function.
> The v2 patch accidentially changed ax88179_link_reset() instead of 
> ax88179_reset(). Hunk 6 of v2 ("net: usb: asix88179_178a: de-duplicate 
> code") is just utterly wrong.

OK, that explains what I saw here.

> ax88179_bind() and ax88179_reset() were the correct targets to be 
> de-duplicated, as done in the v3 patch.
> 
> Sorry for this, Alexander

We all make mistakes, so let's try to learn from them.

You can improve your process by testing what you're about to send (a
very basic requirement), and documenting the changes you make on each
version you send (a cover letter is a good place to put it).

Answering reviewer questions helps building trust between the
contributor and the maintainer on the receiving end of the patch, and
you probably want to answer them before sending a new version so that a
proper discussion can take place, specially if the reviewer doesn't
quite see what you're aiming for.

I'll comment on the patch separately.

Hope this helps,

M.
-- 
Without deviation from the norm, progress is not possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] net: usb: asix88179_178a: de-duplicate code

[Marc Zyngier]

On Mon, 02 Apr 2018 08:43:49 +0100,
Alexander Kurz wrote:

Alexander,

> 
> Remove the duplicated code for asix88179_178a bind and reset methods.
> 
> Signed-off-by: Alexander Kurz 
> ---
>  drivers/net/usb/ax88179_178a.c | 137 
> ++---
>  1 file changed, 31 insertions(+), 106 deletions(-)

What has changed between this patch and the previous one? Having a bit
of a change-log would certainly help. Also, I would have appreciated a
reply to the questions I had on v2 before you posted a third version.

> 
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index a6ef75907ae9..fea4c7b877cc 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1223,7 +1223,7 @@ static int ax88179_led_setting(struct usbnet *dev)
>   return 0;
>  }
>  
> -static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
> +static int ax88179_bind_or_reset(struct usbnet *dev, bool do_reset)
>  {
>   u8 buf[5];
>   u16 *tmp16;
> @@ -1231,12 +1231,11 @@ static int ax88179_bind(struct usbnet *dev, struct 
> usb_interface *intf)
>   struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
>   struct ethtool_eee eee_data;
>  
> - usbnet_get_endpoints(dev, intf);
> -
>   tmp16 = (u16 *)buf;
>   tmp = (u8 *)buf;
>  
> - memset(ax179_data, 0, sizeof(*ax179_data));
> + if (!do_reset)
> + memset(ax179_data, 0, sizeof(*ax179_data));
>  
>   /* Power up ethernet PHY */
>   *tmp16 = 0;
> @@ -1249,9 +1248,13 @@ static int ax88179_bind(struct usbnet *dev, struct 
> usb_interface *intf)
>   ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
>   msleep(100);
>  
> + if (do_reset)
> + ax88179_auto_detach(dev, 0);
> +
>   ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
>ETH_ALEN, dev->net->dev_addr);
> - memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
> + if (!do_reset)
> + memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
>  
>   /* RX bulk configuration */
>   memcpy(tmp, _BULKIN_SIZE[0], 5);
> @@ -1266,19 +1269,21 @@ static int ax88179_bind(struct usbnet *dev, struct 
> usb_interface *intf)
>   ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH,
> 1, 1, tmp);
>  
> - dev->net->netdev_ops = _netdev_ops;
> - dev->net->ethtool_ops = _ethtool_ops;
> - dev->net->needed_headroom = 8;
> - dev->net->max_mtu = 4088;
> -
> - /* Initialize MII structure */
> - dev->mii.dev = dev->net;
> - dev->mii.mdio_read = ax88179_mdio_read;
> - dev->mii.mdio_write = ax88179_mdio_write;
> - dev->mii.phy_id_mask = 0xff;
> - dev->mii.reg_num_mask = 0xff;
> - dev->mii.phy_id = 0x03;
> - dev->mii.supports_gmii = 1;
> + if (!do_reset) {
> + dev->net->netdev_ops = _netdev_ops;
> + dev->net->ethtool_ops = _ethtool_ops;
> + dev->net->needed_headroom = 8;
> + dev->net->max_mtu = 4088;
> +
> + /* Initialize MII structure */
> + dev->mii.dev = dev->net;
> + dev->mii.mdio_read = ax88179_mdio_read;
> + dev->mii.mdio_write = ax88179_mdio_write;
> + dev->mii.phy_id_mask = 0xff;
> + dev->mii.reg_num_mask = 0xff;
> + dev->mii.phy_id = 0x03;
> + dev->mii.supports_gmii = 1;
> + }
>  
>   dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> NETIF_F_RXCSUM;
> @@ -1330,6 +1335,13 @@ static int ax88179_bind(struct usbnet *dev, struct 
> usb_interface *intf)
>   return 0;
>  }
>  
> +static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
> +{
> + usbnet_get_endpoints(dev, intf);
> +
> + return ax88179_bind_or_reset(dev, false);
> +}
> +
>  static void ax88179_unbind(struct usbnet *dev, struct usb_interface *intf)
>  {
>   u16 tmp16;
> @@ -1530,94 +1542,7 @@ static int ax88179_link_reset(struct usbnet *dev)
>  
>  static int ax88179_reset(struct usbnet *dev)
>  {
> - u8 buf[5];
> - u16 *tmp16;
> - u8 *tmp;
> - struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
> - struct ethtool_eee eee_data;
> -
> - tmp16 = (u16 *)buf;
> - tmp = (u8 *)buf;
> -
> - /* Power up ethernet PHY */
> - *tmp16 = 0;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
> -
> - *tmp16 = AX_PHYPWR_RSTCTL_IPRL;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
> - msleep(200);
> -
> - *tmp = AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
> - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
> - msleep(100);
> -
> - /* Ethernet PHY Auto Detach*/
> - ax88179_auto_detach(dev, 0);
> -
> - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN,
> -  

Re: [PATCH v2 2/2] net: usb: asix88179_178a: de-duplicate code

[Marc Zyngier]

Alexander, David,

On 2018-03-08 11:19, Alexander Kurz wrote:

Remove the duplicated code for asix88179_178a bind and reset methods.

Signed-off-by: Alexander Kurz 
---
 drivers/net/usb/ax88179_178a.c | 117
+++--
 1 file changed, 31 insertions(+), 86 deletions(-)


This patch (which is now in -next) causes an infinite stream of:

[   14.922412] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link 
status is: 1
[   14.954411] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link 
status is: 1
[   18.474372] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link 
status is: 1
[   18.506371] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link 
status is: 1
[   22.026327] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link 
status is: 1
[   22.058328] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link 
status is: 1
[   25.578285] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link 
status is: 1
[   25.610286] ax88179_178a 2-1:1.0 enx000ec6d7246f: ax88179 - Link 
status is: 1

[...]

and never comes really up on my arm64 box. Reverting it immediately 
fixes the issue.


Looking more closely at the patch, it raises a couple of questions:



diff --git a/drivers/net/usb/ax88179_178a.c 
b/drivers/net/usb/ax88179_178a.c

index a6ef75907ae9..fb1b78d4b9ef 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1223,7 +1223,7 @@ static int ax88179_led_setting(struct usbnet 
*dev)

return 0;
 }

-static int ax88179_bind(struct usbnet *dev, struct usb_interface 
*intf)
+static int ax88179_link_bind_or_reset(struct usbnet *dev, bool 
do_reset)

 {
u8 buf[5];
u16 *tmp16;
@@ -1231,12 +1231,11 @@ static int ax88179_bind(struct usbnet *dev,
struct usb_interface *intf)
struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
struct ethtool_eee eee_data;

-   usbnet_get_endpoints(dev, intf);
-
tmp16 = (u16 *)buf;
tmp = (u8 *)buf;

-   memset(ax179_data, 0, sizeof(*ax179_data));
+   if (!do_reset)
+   memset(ax179_data, 0, sizeof(*ax179_data));

/* Power up ethernet PHY */
*tmp16 = 0;
@@ -1249,9 +1248,13 @@ static int ax88179_bind(struct usbnet *dev,
struct usb_interface *intf)
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
msleep(100);

+   if (do_reset)
+   ax88179_auto_detach(dev, 0);


This wasn't initially present. As such, it doesn't really match the 
"de-duplication" goal of that patch.



+
ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
 ETH_ALEN, dev->net->dev_addr);
-   memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
+   if (!do_reset)
+   memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);

/* RX bulk configuration */
memcpy(tmp, _BULKIN_SIZE[0], 5);
@@ -1266,19 +1269,21 @@ static int ax88179_bind(struct usbnet *dev,
struct usb_interface *intf)
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH,
  1, 1, tmp);

-   dev->net->netdev_ops = _netdev_ops;
-   dev->net->ethtool_ops = _ethtool_ops;
-   dev->net->needed_headroom = 8;
-   dev->net->max_mtu = 4088;
-
-   /* Initialize MII structure */
-   dev->mii.dev = dev->net;
-   dev->mii.mdio_read = ax88179_mdio_read;
-   dev->mii.mdio_write = ax88179_mdio_write;
-   dev->mii.phy_id_mask = 0xff;
-   dev->mii.reg_num_mask = 0xff;
-   dev->mii.phy_id = 0x03;
-   dev->mii.supports_gmii = 1;
+   if (!do_reset) {
+   dev->net->netdev_ops = _netdev_ops;
+   dev->net->ethtool_ops = _ethtool_ops;
+   dev->net->needed_headroom = 8;
+   dev->net->max_mtu = 4088;
+
+   /* Initialize MII structure */
+   dev->mii.dev = dev->net;
+   dev->mii.mdio_read = ax88179_mdio_read;
+   dev->mii.mdio_write = ax88179_mdio_write;
+   dev->mii.phy_id_mask = 0xff;
+   dev->mii.reg_num_mask = 0xff;
+   dev->mii.phy_id = 0x03;
+   dev->mii.supports_gmii = 1;
+   }

dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
  NETIF_F_RXCSUM;
@@ -1330,6 +1335,13 @@ static int ax88179_bind(struct usbnet *dev,
struct usb_interface *intf)
return 0;
 }

+static int ax88179_bind(struct usbnet *dev, struct usb_interface 
*intf)

+{
+   usbnet_get_endpoints(dev, intf);
+
+   return ax88179_link_bind_or_reset(dev, false);
+}
+
 static void ax88179_unbind(struct usbnet *dev, struct usb_interface 
*intf)

 {
u16 tmp16;
@@ -1458,74 +1470,7 @@ ax88179_tx_fixup(struct usbnet *dev, struct
sk_buff *skb, gfp_t flags)

 static int ax88179_link_reset(struct usbnet *dev)
 {
-   struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
-   u8 tmp[5], link_sts;
-   u16 

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On 25/03/18 15:39, Ard Biesheuvel wrote:
> 
> 
>> On 25 Mar 2018, at 15:14, Marc Zyngier <marc.zyng...@arm.com> wrote:
>>
>> On Sun, 25 Mar 2018 14:26:58 +0100
>> Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
>>
>>>> On 25 March 2018 at 13:52, Marc Zyngier <marc.zyng...@arm.com> wrote:
>>>> On Sun, 25 Mar 2018 13:38:19 +0100,
>>>> Ard Biesheuvel wrote:  
>>>>>
>>>>>> On 25 March 2018 at 13:31, Marc Zyngier <marc.zyng...@arm.com> wrote:  
>>>>>> On Sun, 25 Mar 2018 12:57:55 +0100,
>>>>>> Ard Biesheuvel wrote:  
>>>>>>>
>>>>>>>> On 25 March 2018 at 12:51, Marc Zyngier <marc.zyng...@arm.com> wrote:  
>>>>>>>> On Sun, 25 Mar 2018 11:48:35 +0100,
>>>>>>>> Ard Biesheuvel wrote:
>>>>>>>>
>>>>>>>> Hi Ard,
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>> I finally found some time to work on this, and came up with an
>>>>>>>>>> alternative approach (it turns out that this chip is even more
>>>>>>>>>> braindead than I thought).
>>>>>>>>>>
>>>>>>>>>> It is slightly scary, in the sense that the USB controller seems to
>>>>>>>>>> perform memory accesses even when halted, and can generate faults,
>>>>>>>>>> but it works just fine on my system. And with this, we can drop the
>>>>>>>>>> hard reset at boot time. I'm still on the fence to limit it to 
>>>>>>>>>> systems
>>>>>>>>>> with an iommu though.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Marc,
>>>>>>>>>
>>>>>>>>> I take it you tested this on Cello?  
>>>>>>>>
>>>>>>>> Tested on Cello indeed (I should have mentioned that the first place).
>>>>>>>>
>>>>>>>>> There, it might make sense to
>>>>>>>>> limit this to systems with an IOMMU, but not in the general case, I
>>>>>>>>> think. The reason is that it is not guaranteed that the firmware will
>>>>>>>>> use 32-bit addressable allocations for these data structures, even if
>>>>>>>>> the kernel is able to without an IOMMU. (UEFI on arm64 will not prefer
>>>>>>>>> 32-bit addressable memory for PCI DMA if it is available, and usually
>>>>>>>>> serves heap allocations [such as the ones used for these data
>>>>>>>>> structures] starting at the top of DRAM)  
>>>>>>>>
>>>>>>>> My main worry is that this controller will happily try and DMA from
>>>>>>>> zero as we wipe the 64bit registers, even when halted. On Seattle (and
>>>>>>>> thus Cello), this is just fine as there is nothing there, and the
>>>>>>>> controller aborts with the HSE bit set.
>>>>>>>>
>>>>>>>> On other systems, where memory actually exists at 0, who knows what
>>>>>>>> this is going to do? On the other hand, this is not worse than the
>>>>>>>> current situation, where we could end-up with any odd address...
>>>>>>>>
>>>>>>>
>>>>>>> Is the PCI_COMMAND_MASTER bit enabled at this point? What happens if
>>>>>>> you clear it first?  
>>>>>>
>>>>>> Tried that. No difference whatsoever, as I still get a fault with the
>>>>>> device accessing address 0, and being caught by the iommu.
>>>>>>
>>>>>
>>>>> Wow so this device is more broken than I thought.  
>>>>
>>>> That's my impression as well. I came to the conclusion that the only
>>>> way to make it behave is to crash it first, and then to reset it.
>>>>
>>>
>>> OK, so what if it doesn't crash? Without an IOMMU, it is quite likely
>>> that putting zeroes in the lower half of a 64-bit memory address
>>> produces a physical address that is valid, and so the device will
>>> still misbehave in that case.
>>>
>>> If making it crash is what kicks it into submission, could we perhaps
>>> use U64_MAX instead?
>>
>> Just tried that. It gets into some even uglier state, and never
>> recovers. Even doing U64_MAX, fault, and then zero doesn't help. The
>> problem is that it dies with something in the top 32 bits, and that's
>> pretty final.
>>
>> If really feels that without an iommu in the path, this device is
>> completely unsafe, and should never be fed 64bit addresses.
>>
> 
> ... unless we do the pci reset in the exitbootservices handler in
> uefi, which is probably the most robust way of handling this (or wire
> up the iommu)
> 
> i have cello smmu patches if you’re interested

Could be worth a try.

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On Sun, 25 Mar 2018 14:26:58 +0100
Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:

> On 25 March 2018 at 13:52, Marc Zyngier <marc.zyng...@arm.com> wrote:
> > On Sun, 25 Mar 2018 13:38:19 +0100,
> > Ard Biesheuvel wrote:  
> >>
> >> On 25 March 2018 at 13:31, Marc Zyngier <marc.zyng...@arm.com> wrote:  
> >> > On Sun, 25 Mar 2018 12:57:55 +0100,
> >> > Ard Biesheuvel wrote:  
> >> >>
> >> >> On 25 March 2018 at 12:51, Marc Zyngier <marc.zyng...@arm.com> wrote:  
> >> >> > On Sun, 25 Mar 2018 11:48:35 +0100,
> >> >> > Ard Biesheuvel wrote:
> >> >> >
> >> >> > Hi Ard,
> >> >> >
> >> >> > [...]
> >> >> >  
> >> >> >> > I finally found some time to work on this, and came up with an
> >> >> >> > alternative approach (it turns out that this chip is even more
> >> >> >> > braindead than I thought).
> >> >> >> >
> >> >> >> > It is slightly scary, in the sense that the USB controller seems to
> >> >> >> > perform memory accesses even when halted, and can generate faults,
> >> >> >> > but it works just fine on my system. And with this, we can drop the
> >> >> >> > hard reset at boot time. I'm still on the fence to limit it to 
> >> >> >> > systems
> >> >> >> > with an iommu though.
> >> >> >> >  
> >> >> >>
> >> >> >> Hi Marc,
> >> >> >>
> >> >> >> I take it you tested this on Cello?  
> >> >> >
> >> >> > Tested on Cello indeed (I should have mentioned that the first place).
> >> >> >  
> >> >> >> There, it might make sense to
> >> >> >> limit this to systems with an IOMMU, but not in the general case, I
> >> >> >> think. The reason is that it is not guaranteed that the firmware will
> >> >> >> use 32-bit addressable allocations for these data structures, even if
> >> >> >> the kernel is able to without an IOMMU. (UEFI on arm64 will not 
> >> >> >> prefer
> >> >> >> 32-bit addressable memory for PCI DMA if it is available, and usually
> >> >> >> serves heap allocations [such as the ones used for these data
> >> >> >> structures] starting at the top of DRAM)  
> >> >> >
> >> >> > My main worry is that this controller will happily try and DMA from
> >> >> > zero as we wipe the 64bit registers, even when halted. On Seattle (and
> >> >> > thus Cello), this is just fine as there is nothing there, and the
> >> >> > controller aborts with the HSE bit set.
> >> >> >
> >> >> > On other systems, where memory actually exists at 0, who knows what
> >> >> > this is going to do? On the other hand, this is not worse than the
> >> >> > current situation, where we could end-up with any odd address...
> >> >> >  
> >> >>
> >> >> Is the PCI_COMMAND_MASTER bit enabled at this point? What happens if
> >> >> you clear it first?  
> >> >
> >> > Tried that. No difference whatsoever, as I still get a fault with the
> >> > device accessing address 0, and being caught by the iommu.
> >> >  
> >>
> >> Wow so this device is more broken than I thought.  
> >
> > That's my impression as well. I came to the conclusion that the only
> > way to make it behave is to crash it first, and then to reset it.
> >  
> 
> OK, so what if it doesn't crash? Without an IOMMU, it is quite likely
> that putting zeroes in the lower half of a 64-bit memory address
> produces a physical address that is valid, and so the device will
> still misbehave in that case.
> 
> If making it crash is what kicks it into submission, could we perhaps
> use U64_MAX instead?

Just tried that. It gets into some even uglier state, and never
recovers. Even doing U64_MAX, fault, and then zero doesn't help. The
problem is that it dies with something in the top 32 bits, and that's
pretty final.

If really feels that without an iommu in the path, this device is
completely unsafe, and should never be fed 64bit addresses.

M.
-- 
Without deviation from the norm, progress is not possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On Sun, 25 Mar 2018 13:38:19 +0100,
Ard Biesheuvel wrote:
> 
> On 25 March 2018 at 13:31, Marc Zyngier <marc.zyng...@arm.com> wrote:
> > On Sun, 25 Mar 2018 12:57:55 +0100,
> > Ard Biesheuvel wrote:
> >>
> >> On 25 March 2018 at 12:51, Marc Zyngier <marc.zyng...@arm.com> wrote:
> >> > On Sun, 25 Mar 2018 11:48:35 +0100,
> >> > Ard Biesheuvel wrote:
> >> >
> >> > Hi Ard,
> >> >
> >> > [...]
> >> >
> >> >> > I finally found some time to work on this, and came up with an
> >> >> > alternative approach (it turns out that this chip is even more
> >> >> > braindead than I thought).
> >> >> >
> >> >> > It is slightly scary, in the sense that the USB controller seems to
> >> >> > perform memory accesses even when halted, and can generate faults,
> >> >> > but it works just fine on my system. And with this, we can drop the
> >> >> > hard reset at boot time. I'm still on the fence to limit it to systems
> >> >> > with an iommu though.
> >> >> >
> >> >>
> >> >> Hi Marc,
> >> >>
> >> >> I take it you tested this on Cello?
> >> >
> >> > Tested on Cello indeed (I should have mentioned that the first place).
> >> >
> >> >> There, it might make sense to
> >> >> limit this to systems with an IOMMU, but not in the general case, I
> >> >> think. The reason is that it is not guaranteed that the firmware will
> >> >> use 32-bit addressable allocations for these data structures, even if
> >> >> the kernel is able to without an IOMMU. (UEFI on arm64 will not prefer
> >> >> 32-bit addressable memory for PCI DMA if it is available, and usually
> >> >> serves heap allocations [such as the ones used for these data
> >> >> structures] starting at the top of DRAM)
> >> >
> >> > My main worry is that this controller will happily try and DMA from
> >> > zero as we wipe the 64bit registers, even when halted. On Seattle (and
> >> > thus Cello), this is just fine as there is nothing there, and the
> >> > controller aborts with the HSE bit set.
> >> >
> >> > On other systems, where memory actually exists at 0, who knows what
> >> > this is going to do? On the other hand, this is not worse than the
> >> > current situation, where we could end-up with any odd address...
> >> >
> >>
> >> Is the PCI_COMMAND_MASTER bit enabled at this point? What happens if
> >> you clear it first?
> >
> > Tried that. No difference whatsoever, as I still get a fault with the
> > device accessing address 0, and being caught by the iommu.
> >
> 
> Wow so this device is more broken than I thought.

That's my impression as well. I came to the conclusion that the only
way to make it behave is to crash it first, and then to reset it.

> In UEFI, we rely on clearing the bus master bit to ensure that all
> hardware stops doing DMA when ExitBootServices is called, but this is
> clearly not enough.

A sensible thing to do, but this device is pretty bonkers.

M.

-- 
Jazz is not dead, it just smell funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On Sun, 25 Mar 2018 12:57:55 +0100,
Ard Biesheuvel wrote:
> 
> On 25 March 2018 at 12:51, Marc Zyngier <marc.zyng...@arm.com> wrote:
> > On Sun, 25 Mar 2018 11:48:35 +0100,
> > Ard Biesheuvel wrote:
> >
> > Hi Ard,
> >
> > [...]
> >
> >> > I finally found some time to work on this, and came up with an
> >> > alternative approach (it turns out that this chip is even more
> >> > braindead than I thought).
> >> >
> >> > It is slightly scary, in the sense that the USB controller seems to
> >> > perform memory accesses even when halted, and can generate faults,
> >> > but it works just fine on my system. And with this, we can drop the
> >> > hard reset at boot time. I'm still on the fence to limit it to systems
> >> > with an iommu though.
> >> >
> >>
> >> Hi Marc,
> >>
> >> I take it you tested this on Cello?
> >
> > Tested on Cello indeed (I should have mentioned that the first place).
> >
> >> There, it might make sense to
> >> limit this to systems with an IOMMU, but not in the general case, I
> >> think. The reason is that it is not guaranteed that the firmware will
> >> use 32-bit addressable allocations for these data structures, even if
> >> the kernel is able to without an IOMMU. (UEFI on arm64 will not prefer
> >> 32-bit addressable memory for PCI DMA if it is available, and usually
> >> serves heap allocations [such as the ones used for these data
> >> structures] starting at the top of DRAM)
> >
> > My main worry is that this controller will happily try and DMA from
> > zero as we wipe the 64bit registers, even when halted. On Seattle (and
> > thus Cello), this is just fine as there is nothing there, and the
> > controller aborts with the HSE bit set.
> >
> > On other systems, where memory actually exists at 0, who knows what
> > this is going to do? On the other hand, this is not worse than the
> > current situation, where we could end-up with any odd address...
> >
> 
> Is the PCI_COMMAND_MASTER bit enabled at this point? What happens if
> you clear it first?

Tried that. No difference whatsoever, as I still get a fault with the
device accessing address 0, and being caught by the iommu.

M.

-- 
Jazz is not dead, it just smell funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On Sun, 25 Mar 2018 11:48:35 +0100,
Ard Biesheuvel wrote:

Hi Ard,

[...]

> > I finally found some time to work on this, and came up with an
> > alternative approach (it turns out that this chip is even more
> > braindead than I thought).
> >
> > It is slightly scary, in the sense that the USB controller seems to
> > perform memory accesses even when halted, and can generate faults,
> > but it works just fine on my system. And with this, we can drop the
> > hard reset at boot time. I'm still on the fence to limit it to systems
> > with an iommu though.
> >
> 
> Hi Marc,
> 
> I take it you tested this on Cello?

Tested on Cello indeed (I should have mentioned that the first place).

> There, it might make sense to
> limit this to systems with an IOMMU, but not in the general case, I
> think. The reason is that it is not guaranteed that the firmware will
> use 32-bit addressable allocations for these data structures, even if
> the kernel is able to without an IOMMU. (UEFI on arm64 will not prefer
> 32-bit addressable memory for PCI DMA if it is available, and usually
> serves heap allocations [such as the ones used for these data
> structures] starting at the top of DRAM)

My main worry is that this controller will happily try and DMA from
zero as we wipe the 64bit registers, even when halted. On Seattle (and
thus Cello), this is just fine as there is nothing there, and the
controller aborts with the HSE bit set.

On other systems, where memory actually exists at 0, who knows what
this is going to do? On the other hand, this is not worse than the
current situation, where we could end-up with any odd address...

M.

-- 
Jazz is not dead, it just smell funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On Fri, 02 Mar 2018 17:38:26 +,
Bockholdt Arne wrote:

Hi Arne,

> 
> On Thu, 2018-03-01 at 17:37 +, Marc Zyngier wrote:
> > On 01/03/18 08:01, Bockholdt Arne wrote:
> > > 
> > > On Thu, 2018-02-15 at 19:29 +, Marc Zyngier wrote:
> > > > [+ Ard, who helped me chasing the initial issue]
> > > > 
> > > > On 15/02/18 06:43, Bockholdt Arne wrote:
> > > > > Hi all,
> > > > > 
> > > > > on our Intel Atom C2578 server with a SuperMicro A1SAi board
> > > > > and a
> > > > > Renesas uPD720201 USB 3.0 host controller the controller has
> > > > > stopped
> > > > > working since kernel 4.13.x. Before that kernel the dmesg
> > > > > messages
> > > > > from
> > > > > XHCI were:
> > > > > 
> > > > > dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: xHCI Host
> > > > > Controller
> > > > > dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: new USB bus
> > > > > registered,
> > > > > assigned bus number 2
> > > > > dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: hcc params
> > > > > 0x014051cf
> > > > > hci version 0x100 quirks 0x0010
> > > > > dmesg-4.12.1-serverv4.log:usb usb2: Manufacturer: Linux 4.12.1-
> > > > > serverv4
> > > > > xhci-hcd
> > > > > dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: xHCI Host
> > > > > Controller
> > > > > dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: new USB bus
> > > > > registered,
> > > > > assigned bus number 3
> > > > > dmesg-4.12.1-serverv4.log:usb usb3: Manufacturer: Linux 4.12.1-
> > > > > serverv4
> > > > > xhci-hcd
> > > > > 
> > > > > After that the message look like that:
> > > > > 
> > > > > dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: Resetting
> > > > > dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: Refused to
> > > > > change
> > > > > power
> > > > > state, currently in D3
> > > > > dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: xHCI Host
> > > > > Controller
> > > > > dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: new USB bus
> > > > > registered,
> > > > > assigned bus number 2
> > > > > dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: Host halt
> > > > > failed,
> > > > > -19
> > > > > dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: can't setup:
> > > > > -19
> > > > > dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: USB bus 2
> > > > > deregistered
> > > > > dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: init
> > > > > :03:00.0
> > > > > fail, -19
> > > > > 
> > > > > I've tested it with 4.15.3 too, it's still the same. I've
> > > > > narrowed
> > > > > it
> > > > > down to the following patch:
> > > > > 
> > > > > commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7
> > > > > Author: Marc Zyngier <marc.zyng...@arm.com <mailto:marc.zyngier
> > > > > @arm
> > > > > .com>>
> > > > > Date:   Tue Aug 1 20:11:08 2017 -0500
> > > > > 
> > > > > xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA
> > > > > issue
> > > > > 
> > > > > Reverting the patch on top of 4.15.3 restores the USB3
> > > > > functionality on
> > > > > our server. Please let me know if there is anything I can do to
> > > > > fix
> > > > > the
> > > > > problem. Thank you.
> > > > 
> > > > Hi Arne,
> > > > 
> > > > This looks pretty bad. I suspect that once reset, the firmware is
> > > > lost.
> > > > I'll try to write a patch dumping some information about it.
> > > > 
> > > > Ard: Do you know if the Cello board has a SPI flash connected to
> > > > the
> > > > Renesas chip, from which it would load the firmware?
> > > > 
> > > > Another possibility is that power management kicks in, and that
> > > > the
> > > > endpoint is stuck there. Could also be firmware related, but not
> > > > only.
> > > > I'd we

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

On 01/03/18 08:01, Bockholdt Arne wrote:
> 
> On Thu, 2018-02-15 at 19:29 +, Marc Zyngier wrote:
>> [+ Ard, who helped me chasing the initial issue]
>>
>> On 15/02/18 06:43, Bockholdt Arne wrote:
>>> Hi all,
>>>
>>> on our Intel Atom C2578 server with a SuperMicro A1SAi board and a
>>> Renesas uPD720201 USB 3.0 host controller the controller has
>>> stopped
>>> working since kernel 4.13.x. Before that kernel the dmesg messages
>>> from
>>> XHCI were:
>>>
>>> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: xHCI Host
>>> Controller
>>> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: new USB bus
>>> registered,
>>> assigned bus number 2
>>> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: hcc params
>>> 0x014051cf
>>> hci version 0x100 quirks 0x0010
>>> dmesg-4.12.1-serverv4.log:usb usb2: Manufacturer: Linux 4.12.1-
>>> serverv4
>>> xhci-hcd
>>> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: xHCI Host
>>> Controller
>>> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: new USB bus
>>> registered,
>>> assigned bus number 3
>>> dmesg-4.12.1-serverv4.log:usb usb3: Manufacturer: Linux 4.12.1-
>>> serverv4
>>> xhci-hcd
>>>
>>> After that the message look like that:
>>>
>>> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: Resetting
>>> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: Refused to change
>>> power
>>> state, currently in D3
>>> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: xHCI Host
>>> Controller
>>> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: new USB bus
>>> registered,
>>> assigned bus number 2
>>> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: Host halt failed,
>>> -19
>>> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: can't setup: -19
>>> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: USB bus 2
>>> deregistered
>>> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: init :03:00.0
>>> fail, -19
>>>
>>> I've tested it with 4.15.3 too, it's still the same. I've narrowed
>>> it
>>> down to the following patch:
>>>
>>> commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7
>>> Author: Marc Zyngier <marc.zyng...@arm.com <mailto:marc.zyngier@arm
>>> .com>>
>>> Date:   Tue Aug 1 20:11:08 2017 -0500
>>>
>>> xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA
>>> issue
>>>
>>> Reverting the patch on top of 4.15.3 restores the USB3
>>> functionality on
>>> our server. Please let me know if there is anything I can do to fix
>>> the
>>> problem. Thank you.
>>
>> Hi Arne,
>>
>> This looks pretty bad. I suspect that once reset, the firmware is
>> lost.
>> I'll try to write a patch dumping some information about it.
>>
>> Ard: Do you know if the Cello board has a SPI flash connected to the
>> Renesas chip, from which it would load the firmware?
>>
>> Another possibility is that power management kicks in, and that the
>> endpoint is stuck there. Could also be firmware related, but not
>> only.
>> I'd welcome any idea on the subject, as I cannot reproduce this issue
>> on
>> the HW I have.
>>
>> It we cannot work out what exactly is causing this, we may have to
>> default to not resetting the part and rely on a command-line option
>> to
>> do it... I can't say I'm a fan.
>>
>> Thanks,
>>
>>  M.
>>
> 
> Hi Marc,
> 
> I've tested it with 4.15.7 and it's still there. Is there anything that
> I can do to help fixing this problem?

Would you mind trying the following patch and let me know if it helps?
It is not exactly pretty, but we can polish it if that solves your issue.

Thanks,

M.

>From 9a253773b289f781b7114e887481939b3021bb30 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyng...@arm.com>
Date: Thu, 1 Mar 2018 17:27:42 +
Subject: [PATCH] xhci: Only reset uPD72020x if programmed with 64bit DMA
 addresses

The Renesas uPD72020x USB controller misbehaves when switching
from 64bit DMA addresses to 32bit ones, and can only accept a
new programming if hard reset first.

But it also misbehaves (in a different way) if reset for no
good reason. So let's restrict the resetting to the cases where
we detect the 64/32 issue, and leave the device alone in the
other cases.

Fixes: 8466489ef5ba ("xhci: Reset Renesas uPD72020x USB controller for 32-bit 
DMA issue")
Signed-off-by: M

Re: patch 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 breaks Renesas USB3 controller functionality

[Marc Zyngier]

[+ Ard, who helped me chasing the initial issue]

On 15/02/18 06:43, Bockholdt Arne wrote:
> Hi all,
> 
> on our Intel Atom C2578 server with a SuperMicro A1SAi board and a
> Renesas uPD720201 USB 3.0 host controller the controller has stopped
> working since kernel 4.13.x. Before that kernel the dmesg messages from
> XHCI were:
> 
> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: xHCI Host Controller
> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: new USB bus registered,
> assigned bus number 2
> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: hcc params 0x014051cf
> hci version 0x100 quirks 0x0010
> dmesg-4.12.1-serverv4.log:usb usb2: Manufacturer: Linux 4.12.1-serverv4
> xhci-hcd
> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: xHCI Host Controller
> dmesg-4.12.1-serverv4.log:xhci_hcd :03:00.0: new USB bus registered,
> assigned bus number 3
> dmesg-4.12.1-serverv4.log:usb usb3: Manufacturer: Linux 4.12.1-serverv4
> xhci-hcd
> 
> After that the message look like that:
> 
> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: Resetting
> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: Refused to change power
> state, currently in D3
> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: xHCI Host Controller
> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: new USB bus registered,
> assigned bus number 2
> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: Host halt failed, -19
> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: can't setup: -19
> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: USB bus 2 deregistered
> dmesg-4.13.1-serverv4.log:xhci_hcd :03:00.0: init :03:00.0 fail, -19
> 
> I've tested it with 4.15.3 too, it's still the same. I've narrowed it
> down to the following patch:
> 
> commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7
> Author: Marc Zyngier <marc.zyng...@arm.com <mailto:marc.zyng...@arm.com>>
> Date:   Tue Aug 1 20:11:08 2017 -0500
> 
> xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue
> 
> Reverting the patch on top of 4.15.3 restores the USB3 functionality on
> our server. Please let me know if there is anything I can do to fix the
> problem. Thank you.
Hi Arne,

This looks pretty bad. I suspect that once reset, the firmware is lost.
I'll try to write a patch dumping some information about it.

Ard: Do you know if the Cello board has a SPI flash connected to the
Renesas chip, from which it would load the firmware?

Another possibility is that power management kicks in, and that the
endpoint is stuck there. Could also be firmware related, but not only.
I'd welcome any idea on the subject, as I cannot reproduce this issue on
the HW I have.

It we cannot work out what exactly is causing this, we may have to
default to not resetting the part and rely on a command-line option to
do it... I can't say I'm a fan.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue

[Marc Zyngier]

On 05/01/18 02:09, Troy Kisky wrote:
> On 12/29/2017 3:34 AM, Marc Zyngier wrote:
>> On Wed, 27 Dec 2017 20:37:07 +,
>> Troy Kisky wrote:
>>>
>>> On 12/27/2017 2:37 AM, Marc Zyngier wrote:
>>>> On Tue, 26 Dec 2017 21:57:58 +,
>>>> Troy Kisky wrote:
>>>>>
>>>>> On 12/26/2017 1:52 PM, Troy Kisky wrote:
>>>>>> On 12/25/2017 2:10 AM, Marc Zyngier wrote:
>>>>>>> On Sun, 24 Dec 2017 23:01:33 +0000,
>>>>>>> Troy Kisky wrote:
>>>>>>>>
>>>>>>>> commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7
>>>>>>>> Author: Marc Zyngier <marc.zyng...@arm.com>
>>>>>>>> Date:   Tue Aug 1 20:11:08 2017 -0500
>>>>>>>>
>>>>>>>> xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue
>>>>>>>> ...
>>>>>>>> ...
>>>>>>>> ...
>>>>>>>> --- a/drivers/usb/host/xhci-pci.c
>>>>>>>> +++ b/drivers/usb/host/xhci-pci.c
>>>>>>>> @@ -284,6 +284,13 @@ static int xhci_pci_probe(struct pci_dev *dev, 
>>>>>>>> const struct pci_device_id *id)
>>>>>>>>
>>>>>>>> driver = (struct hc_driver *)id->driver_data;
>>>>>>>>
>>>>>>>> +   /* For some HW implementation, a XHCI reset is just not 
>>>>>>>> enough... */
>>>>>>>> +   if (usb_xhci_needs_pci_reset(dev)) {
>>>>>>>> +   dev_info(>dev, "Resetting\n");
>>>>>>>> +   if (pci_reset_function_locked(dev))
>>>>>>>> +   dev_warn(>dev, "Reset failed");
>>>>>>>> +   }
>>>>>>>> +
>>>>>>>> 
>>>>>>>>
>>>>>>>>
>>>>>>>> This pci_reset_function_locked call, causes my i.mx6qp processor to
>>>>>>>> hang. It no longer responds to the MAGIC_SYSRQ key (break on serial
>>>>>>>> port).
>>>>>>>>
>>>>>>>>
>>>>>>>> If I comment it out, things return to normal when testing on
>>>>>>>> Linux-next (20171222).
>>>>>>>>
>>>>>>>> If you need more info, let me know.
>>>>>>>
>>>>>>> Well, for a start:
>>>>>>>
>>>>>>> - Does it fail with 4.13 or 4.14 too?
>>>>>>
>>>>>> v4.13-rc4 - good
>>>>>> v4.13-rc5 - fail
>>>>>> v4.13 - fail
>>>>>> v4.14 - fail
>>>>>>
>>>>>>
>>>>>>
>>>>>>> - Can you work out where it is locking up exactly (going slightly
>>>>>>>   deeper than the pci_reset_function_locked() call)?
>>>>>>
>>>>>> With this patch
>>>>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>>>>> index 05104bd2b611..8782945a9f70 100644
>>>>>> --- a/drivers/usb/host/xhci.c
>>>>>> +++ b/drivers/usb/host/xhci.c
>>>>>> @@ -4808,8 +4808,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
>>>>>> xhci_get_quirks_t get_quirks)
>>>>>>
>>>>>> mutex_init(>mutex);
>>>>>> xhci->cap_regs = hcd->regs;
>>>>>> +   pr_err("%s: 1\n", __func__);
>>>>>> xhci->op_regs = hcd->regs +
>>>>>> HC_LENGTH(readl(>cap_regs->hc_capbase));
>>>>>> +   pr_err("%s: 2\n", __func__);
>>>>>> xhci->run_regs = hcd->regs +
>>>>>> (readl(>cap_regs->run_regs_off) & RTSOFF_MASK);
>>>>>> /* Cache read-only capability registers */
>>>>>> _
>>>>>>
>>>>>> The last thing printed is
>>>>>> [  OK  ] Reached target System Initialization.
>>>>>> [  OK  ] Listening on D-Bus System Message Bus Socket.
>>>>>> [  OK  ] Reached target Sockets.
>>>>>> [  OK  ] Started Daily Cleanup of Temporary Directories.
&g

Re: xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue

[Marc Zyngier]

On Wed, 27 Dec 2017 20:37:07 +,
Troy Kisky wrote:
> 
> On 12/27/2017 2:37 AM, Marc Zyngier wrote:
> > On Tue, 26 Dec 2017 21:57:58 +,
> > Troy Kisky wrote:
> >>
> >> On 12/26/2017 1:52 PM, Troy Kisky wrote:
> >>> On 12/25/2017 2:10 AM, Marc Zyngier wrote:
> >>>> On Sun, 24 Dec 2017 23:01:33 +,
> >>>> Troy Kisky wrote:
> >>>>>
> >>>>> commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7
> >>>>> Author: Marc Zyngier <marc.zyng...@arm.com>
> >>>>> Date:   Tue Aug 1 20:11:08 2017 -0500
> >>>>>
> >>>>> xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue
> >>>>> ...
> >>>>> ...
> >>>>> ...
> >>>>> --- a/drivers/usb/host/xhci-pci.c
> >>>>> +++ b/drivers/usb/host/xhci-pci.c
> >>>>> @@ -284,6 +284,13 @@ static int xhci_pci_probe(struct pci_dev *dev, 
> >>>>> const struct pci_device_id *id)
> >>>>>
> >>>>> driver = (struct hc_driver *)id->driver_data;
> >>>>>
> >>>>> +   /* For some HW implementation, a XHCI reset is just not 
> >>>>> enough... */
> >>>>> +   if (usb_xhci_needs_pci_reset(dev)) {
> >>>>> +   dev_info(>dev, "Resetting\n");
> >>>>> +   if (pci_reset_function_locked(dev))
> >>>>> +   dev_warn(>dev, "Reset failed");
> >>>>> +   }
> >>>>> +
> >>>>> 
> >>>>>
> >>>>>
> >>>>> This pci_reset_function_locked call, causes my i.mx6qp processor to
> >>>>> hang. It no longer responds to the MAGIC_SYSRQ key (break on serial
> >>>>> port).
> >>>>>
> >>>>>
> >>>>> If I comment it out, things return to normal when testing on
> >>>>> Linux-next (20171222).
> >>>>>
> >>>>> If you need more info, let me know.
> >>>>
> >>>> Well, for a start:
> >>>>
> >>>> - Does it fail with 4.13 or 4.14 too?
> >>>
> >>> v4.13-rc4 - good
> >>> v4.13-rc5 - fail
> >>> v4.13 - fail
> >>> v4.14 - fail
> >>>
> >>>
> >>>
> >>>> - Can you work out where it is locking up exactly (going slightly
> >>>>   deeper than the pci_reset_function_locked() call)?
> >>>
> >>> With this patch
> >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> >>> index 05104bd2b611..8782945a9f70 100644
> >>> --- a/drivers/usb/host/xhci.c
> >>> +++ b/drivers/usb/host/xhci.c
> >>> @@ -4808,8 +4808,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
> >>> xhci_get_quirks_t get_quirks)
> >>>
> >>> mutex_init(>mutex);
> >>> xhci->cap_regs = hcd->regs;
> >>> +   pr_err("%s: 1\n", __func__);
> >>> xhci->op_regs = hcd->regs +
> >>> HC_LENGTH(readl(>cap_regs->hc_capbase));
> >>> +   pr_err("%s: 2\n", __func__);
> >>> xhci->run_regs = hcd->regs +
> >>> (readl(>cap_regs->run_regs_off) & RTSOFF_MASK);
> >>> /* Cache read-only capability registers */
> >>> _
> >>>
> >>> The last thing printed is
> >>> [  OK  ] Reached target System Initialization.
> >>> [  OK  ] Listening on D-Bus System Message Bus Socket.
> >>> [  OK  ] Reached target Sockets.
> >>> [  OK  ] Started Daily Cleanup of Temporary Directories.
> >>> [   15.770319] xhci_gen_setup: 1
> > 
> > OK, so it looks like the CPU is blocked reading from the
> > device. Almost feels like the device is not clocked (that's about the
> > only reason why the CPU would be stuck on such a read).
> > 
> >>>
> >>>
> >>>> - Do you get the symptom at boot time?
> >>>
> >>> Boot
> >>>
> >>>> On resume? Is the USB driver
> >>>>   compiled as a module or built-in?
> >>>
> >>>   INSTALL drivers/usb/host/xhci-hcd.ko
> >>>   INSTALL drivers/usb/host/xhci-pci.ko

Re: xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue

[Marc Zyngier]

On Tue, 26 Dec 2017 21:57:58 +,
Troy Kisky wrote:
> 
> On 12/26/2017 1:52 PM, Troy Kisky wrote:
> > On 12/25/2017 2:10 AM, Marc Zyngier wrote:
> >> On Sun, 24 Dec 2017 23:01:33 +,
> >> Troy Kisky wrote:
> >>>
> >>> commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7
> >>> Author: Marc Zyngier <marc.zyng...@arm.com>
> >>> Date:   Tue Aug 1 20:11:08 2017 -0500
> >>>
> >>> xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue
> >>> ...
> >>> ...
> >>> ...
> >>> --- a/drivers/usb/host/xhci-pci.c
> >>> +++ b/drivers/usb/host/xhci-pci.c
> >>> @@ -284,6 +284,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const 
> >>> struct pci_device_id *id)
> >>>
> >>> driver = (struct hc_driver *)id->driver_data;
> >>>
> >>> +   /* For some HW implementation, a XHCI reset is just not enough... 
> >>> */
> >>> +   if (usb_xhci_needs_pci_reset(dev)) {
> >>> +   dev_info(>dev, "Resetting\n");
> >>> +   if (pci_reset_function_locked(dev))
> >>> +   dev_warn(>dev, "Reset failed");
> >>> +   }
> >>> +
> >>> 
> >>>
> >>>
> >>> This pci_reset_function_locked call, causes my i.mx6qp processor to
> >>> hang. It no longer responds to the MAGIC_SYSRQ key (break on serial
> >>> port).
> >>>
> >>>
> >>> If I comment it out, things return to normal when testing on
> >>> Linux-next (20171222).
> >>>
> >>> If you need more info, let me know.
> >>
> >> Well, for a start:
> >>
> >> - Does it fail with 4.13 or 4.14 too?
> > 
> > v4.13-rc4 - good
> > v4.13-rc5 - fail
> > v4.13 - fail
> > v4.14 - fail
> > 
> > 
> > 
> >> - Can you work out where it is locking up exactly (going slightly
> >>   deeper than the pci_reset_function_locked() call)?
> > 
> > With this patch
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 05104bd2b611..8782945a9f70 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -4808,8 +4808,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
> > xhci_get_quirks_t get_quirks)
> > 
> > mutex_init(>mutex);
> > xhci->cap_regs = hcd->regs;
> > +   pr_err("%s: 1\n", __func__);
> > xhci->op_regs = hcd->regs +
> > HC_LENGTH(readl(>cap_regs->hc_capbase));
> > +   pr_err("%s: 2\n", __func__);
> > xhci->run_regs = hcd->regs +
> > (readl(>cap_regs->run_regs_off) & RTSOFF_MASK);
> > /* Cache read-only capability registers */
> > _
> > 
> > The last thing printed is
> > [  OK  ] Reached target System Initialization.
> > [  OK  ] Listening on D-Bus System Message Bus Socket.
> > [  OK  ] Reached target Sockets.
> > [  OK  ] Started Daily Cleanup of Temporary Directories.
> > [   15.770319] xhci_gen_setup: 1

OK, so it looks like the CPU is blocked reading from the
device. Almost feels like the device is not clocked (that's about the
only reason why the CPU would be stuck on such a read).

> > 
> > 
> >> - Do you get the symptom at boot time?
> > 
> > Boot
> > 
> >> On resume? Is the USB driver
> >>   compiled as a module or built-in?
> > 
> >   INSTALL drivers/usb/host/xhci-hcd.ko
> >   INSTALL drivers/usb/host/xhci-pci.ko
> >   INSTALL drivers/usb/host/xhci-renesas.ko
> > 
> >> - What is your exact platform (something a bit more precise than just
> >>   i.MX6...)?
> > 
> > It is a custom board not in mainline. It does need to load firmware.
> > 
> > I tried moving pci_reset_function_locked before 
> > renesas_check_if_fw_dl_is_needed
> > but that did not help.
> > 
> 
> 
> 
> Which I should say I also have this patch
> 
> 
> commit 01ac5cfd811088b5fe8c97f28f96086e30393c66
> Author: Christian Lamparter <chunk...@gmail.com>
> Date:   Wed Jun 8 00:14:57 2016 +0200
> 
> usb: xhci: handle uPD720201 and uPD720202 w/o ROM
> 
> This patch adds a firmware check for the uPD720201K8-711-BAC-A
> and uPD720202K8-711-BAA-A variant. Both of these chips are listed
> in Renesas' R19UH0078EJ0500 Rev.5.00 &

Re: xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue

[Marc Zyngier]

On Sun, 24 Dec 2017 23:01:33 +,
Troy Kisky wrote:
> 
> commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7
> Author: Marc Zyngier <marc.zyng...@arm.com>
> Date:   Tue Aug 1 20:11:08 2017 -0500
> 
> xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue
> ...
> ...
> ...
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -284,6 +284,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const 
> struct pci_device_id *id)
> 
> driver = (struct hc_driver *)id->driver_data;
> 
> +   /* For some HW implementation, a XHCI reset is just not enough... */
> +   if (usb_xhci_needs_pci_reset(dev)) {
> +   dev_info(>dev, "Resetting\n");
> +   if (pci_reset_function_locked(dev))
> +   dev_warn(>dev, "Reset failed");
> +   }
> +
> 
> 
> 
> This pci_reset_function_locked call, causes my i.mx6qp processor to
> hang. It no longer responds to the MAGIC_SYSRQ key (break on serial
> port).
> 
> 
> If I comment it out, things return to normal when testing on
> Linux-next (20171222).
> 
> If you need more info, let me know.

Well, for a start:

- Does it fail with 4.13 or 4.14 too?
- Can you work out where it is locking up exactly (going slightly
  deeper than the pci_reset_function_locked() call)?
- Do you get the symptom at boot time? On resume? Is the USB driver
  compiled as a module or built-in?
- What is your exact platform (something a bit more precise than just
  i.MX6...)?

Thanks,

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

Re: [PATCH v5 77/78] irqdomain: Convert to XArray

[Marc Zyngier]

On Fri, 15 Dec 2017 22:04:49 +,
Matthew Wilcox wrote:
> 
> From: Matthew Wilcox <mawil...@microsoft.com>
> 
> In a non-critical path, irqdomain wants to know how many entries are
> stored in the xarray, so add xa_count().  This is a pretty straightforward
> conversion; mostly just removing now-redundant locking.  The only thing
> of note is just how much simpler irq_domain_fix_revmap() becomes.
> 
> Signed-off-by: Matthew Wilcox <mawil...@microsoft.com>
> ---
>  include/linux/irqdomain.h | 10 --
>  include/linux/xarray.h|  1 +
>  kernel/irq/irqdomain.c| 39 ++-
>  lib/xarray.c  | 25 +
>  4 files changed, 40 insertions(+), 35 deletions(-)

I certainly welcome the simplification this provides.

As for xa_count(), I'm trying to convince myself that the code under
CONFIG_IRQ_DOMAIN_DEBUG is now completely superseded by
CONFIG_GENERIC_IRQ_DEBUGFS, and that there would be more value in
getting rid of it rather than keeping it on life support.

Until then, the 1:1 replacement is good enough.

Acked-by: Marc Zyngier <marc.zyng...@arm.com>

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

Re: [regression] Force hard reset of Renesas uPD72020x USB controller

[Marc Zyngier]

On Mon, Sep 18 2017 at  3:01:32 pm BST, Albert Weichselbraun 
<alb...@weichselbraun.net> wrote:
> On Mon, 2017-09-18 at 12:46 +0100, Marc Zyngier wrote:
>> On 18/09/17 09:49, Albert Weichselbraun wrote:
>> > Hi Marc,
>> > 
>> > 100% ack
>> > - Booting with a kernel that does not do a PCI reset yields the
>> >   following topology:
>> > 
>> > 
>> > -[:00] -
>> >  +-00.0  Intel Corporation 2nd Generation Core Processor Family
>> > DRAM
>> >  Controller
>> >  +-02.0  Intel Corporation 2nd Generation Core Processor Family
>> >  Integrated Graphics Controller
>> >  +-16.0  Intel Corporation 6 Series/C200 Series Chipset Family MEI
>> >  Controller #1
>> >  +-19.0  Intel Corporation 82579LM Gigabit Network Connection
>> >  +-1a.0  Intel Corporation 6 Series/C200 Series Chipset Family USB
>> >  Enhanced Host Controller #2
>> >  +-1b.0  Intel Corporation 6 Series/C200 Series Chipset Family High
>> >  Definition Audio Controller
>> >  +-1c.0-[02]--
>> >  +-1c.1-[03]00.0  Intel Corporation Centrino Advanced-N 6205
>> >   [Taylor Peak]
>> >  +-1c.3-[05-0c]00.0  Renesas Technology Corp. uPD720202 USB 3.0
>> >  Host Controller
>> >  +-1c.4-[0d]00.0  Ricoh Co Ltd MMC/SD Host Controller
>> >  +-1d.0  Intel Corporation 6 Series/C200 Series Chipset Family USB
>> >  Enhanced Host Controller #1
>> >  +-1f.0  Intel Corporation QM67 Express Chipset Family LPC
>> > Controller
>> >  +-1f.2  Intel Corporation 6 Series/C200 Series Chipset Family 6
>> > port
>> >  SATA AHCI Controller
>> >  \-1f.3  Intel Corporation 6 Series/C200 Series Chipset Family
>> > SMBus
>> >  Controller
>> > 
>> > 
>> > Unplugging and replugging the card (regardless of the kernel
>> > version)
>> > or booting with a kernel that does the reset leads to the card not
>> > being correctly recognized and the problems I have observed.
>> 
>> Hmmm. Just to make sure I understand you correctly:
>> 
>> With a non-reset kernel: if you boot *without* the card inserted, but
>> insert it at a later time, the card is unusable? If that's the case,
>> that's quite unexpected too...
>
> This is exactly what is happening when booting with a non-reset kernel.
> If you insert the card at a later time it is unusable and the system
> starts to behave sluggish. 
> There is also not a single line logged in /var/log/kern.log.
>
> Once the card is removed everything is back to normal.

Right. So that's just a new version of an existing problem. The issue is
that the new reset behaves like an eject/insert sequence as far as the
card is concerned, and this triggers a pre-existing issue.

That's not to say that we should ignore your report, but I think we
should try to fix the root cause, which is that your ExpressCard bridge
and your car don't seem to like each other very much. Has this been ever
reported on the PCI mailing list?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [regression] Force hard reset of Renesas uPD72020x USB controller

[Marc Zyngier]

On 18/09/17 09:49, Albert Weichselbraun wrote:
> Hi Marc,
> 
> 100% ack
> - Booting with a kernel that does not do a PCI reset yields the
>   following topology:
> 
> 
> -[:00] -
>  +-00.0  Intel Corporation 2nd Generation Core Processor Family DRAM
>  Controller
>  +-02.0  Intel Corporation 2nd Generation Core Processor Family
>  Integrated Graphics Controller
>  +-16.0  Intel Corporation 6 Series/C200 Series Chipset Family MEI
>  Controller #1
>  +-19.0  Intel Corporation 82579LM Gigabit Network Connection
>  +-1a.0  Intel Corporation 6 Series/C200 Series Chipset Family USB
>  Enhanced Host Controller #2
>  +-1b.0  Intel Corporation 6 Series/C200 Series Chipset Family High
>  Definition Audio Controller
>  +-1c.0-[02]--
>  +-1c.1-[03]00.0  Intel Corporation Centrino Advanced-N 6205
>   [Taylor Peak]
>  +-1c.3-[05-0c]00.0  Renesas Technology Corp. uPD720202 USB 3.0
>  Host Controller
>  +-1c.4-[0d]00.0  Ricoh Co Ltd MMC/SD Host Controller
>  +-1d.0  Intel Corporation 6 Series/C200 Series Chipset Family USB
>  Enhanced Host Controller #1
>  +-1f.0  Intel Corporation QM67 Express Chipset Family LPC Controller
>  +-1f.2  Intel Corporation 6 Series/C200 Series Chipset Family 6 port
>  SATA AHCI Controller
>  \-1f.3  Intel Corporation 6 Series/C200 Series Chipset Family SMBus
>  Controller
> 
> 
> Unplugging and replugging the card (regardless of the kernel version)
> or booting with a kernel that does the reset leads to the card not
> being correctly recognized and the problems I have observed.

Hmmm. Just to make sure I understand you correctly:

With a non-reset kernel: if you boot *without* the card inserted, but
insert it at a later time, the card is unusable? If that's the case,
that's quite unexpected too...

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [regression] Force hard reset of Renesas uPD72020x USB controller

[Marc Zyngier]

Hi Albert,

On 17/09/17 13:39, Albert Weichselbraun wrote:
> Dear all,
> 
> I ran into a regression with an ExpressCard/54 USB 3.0 expansion card
> that uses the Renesas uPD72020x chipset on a Lenovo X220i Laptop (the
> described behavior has been reproduced with kernel versions 4.12.8,
> 4.12.13, 4.13.1 and 4.13.2).
> 
> Once booting such a kernel, the system becomes very sluggish with
> considerable mouse pointer delays that are followed by display driver
> errors such as
>  - pipe A vblank wait timed out
>  - pipe B vblank wait timed out
> after some minutes.
> 
> Please refer to 
>   https://weichselbraun.net/tmp/dmesg-pipe-a-vblank-timeout.log
>   https://weichselbraun.net/tmp/dmesg-pipe-b-vblank-timeout.log
> for the corresponding log messages.
> 
> Removing the expansion card or booting with a pre 4.12.8 kernel
> resolves the issue.
> 
> I traced the problem back to the following patch that has been
> introduced in 4.12.8 and seems to force a PCI reset on the device. 
> 

[...]

> What seems to happen on my machine is that the reset fails leaving the
> device in an undefined state:
> 
> 
> Sep 17 08:35:19 trinity kernel: [2.874329] xhci_hcd :05:00.0: 
>   xHCI host controller not responding, assume dead
> Sep 17 08:35:19 trinity kernel: [2.874341] xhci_hcd :05:00.0: 
>   remove, state 1
> Sep 17 08:35:19 trinity kernel: [2.874350] usb usb4: USB 
>   disconnect, device number 1
> Sep 17 08:35:19 trinity kernel: [2.874608] xhci_hcd :05:00.0: 
>   USB bus 4 deregistered
> Sep 17 08:35:19 trinity kernel: [2.875433] xhci_hcd :05:00.0: 
>   remove, state 1
> Sep 17 08:35:19 trinity kernel: [2.875440] usb usb3: USB 
>   disconnect, device number 1
> Sep 17 08:35:19 trinity kernel: [2.875535] clocksource: Switched
> to 
>   clocksource tsc
> Sep 17 08:35:19 trinity kernel: [2.875769] xhci_hcd :05:00.0: 
>   Host halt failed, -19
> Sep 17 08:35:19 trinity kernel: [2.875777] xhci_hcd :05:00.0: 
>   Host not accessible, reset failed.
> Sep 17 08:35:19 trinity kernel: [2.876148] xhci_hcd :05:00.0: 
>   USB bus 3 deregistered
> 
> 
> The full dmesg output is available at
>   https://weichselbraun.net/tmp/dmesg-4.13.2-unpatched
> 
> Reverting the changes (tested on 4.12.8, 4.13.1 and 4.13.2) solves this
> issue on my system.  
>   https://weichselbraun.net/tmp/dmesg-4.13.2-patched
>  
> lspci:
>   05:00.0 USB controller: Renesas Technology Corp. uPD720202 USB 3.0 
> Host Controller (rev 02)
> 
> Please let me know if there is any additional information I can provide
> to help narrowing this down.

Thanks for the detailed report.

I wonder if the hotplug nature of the card could be playing some tricks
on us, and the PCI reset nuking some otherwise important programming
that are relevant to the ExpressCard bridge.

Do you get the same effect if you're plugging the card after the system
has booted? Could you please post your full PCI topology (lspci -vt)?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Workaround for uPD72020x USB3 chips

[Marc Zyngier]

On 13/07/17 12:36, Bjorn Helgaas wrote:
> On Thu, Jul 13, 2017 at 08:46:45AM +0100, Marc Zyngier wrote:
>> On 13/07/17 07:48, Ard Biesheuvel wrote:
>>> On 13 July 2017 at 04:12, Bjorn Helgaas <helg...@kernel.org> wrote:
>>>> On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
>>>>> Ard and myself have just spent quite some time lately trying to pin
>>>>> down an issue in the DMA code which was taking the form of a PCIe USB3
>>>>> controller issuing a DMA access at some bizarre address, and being
>>>>> caught red-handed by the IOMMU.
>>>>>
>>>>> After much head scratching and most of a week-end spent on tracing the
>>>>> damn thing, I'm now convinced that the DMA code is fine, the XHCI
>>>>> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
>>>>> nasty piece of work.
>>>>>
>>>>> The issue is as follow:
>>>>>
>>>>> - EFI initializes the controller using physical addresses above the
>>>>>   4GB limit (this is on an arm64 box where the memory starts at
>>>>>   0x80_...).
>>>>>
>>>>> - The kernel takes over, sends a XHCI reset to the controller, and
>>>>>   because we have an IOMMU sitting between the controller and memory,
>>>>>   provides *virtual* addresses. Trying to make things a bit faster for
>>>>>   our controller, it issues IOVAs in the low 4GB range).
>>>>>
>>>>> - Low and behold, the controller is now issuing transactions with a
>>>>>   0x80 prefix in front of our IOVA. Yes, the same prefix that was
>>>>>   programmed during the EFI configuration. IOMMU fault, not happy.
>>>>>
>>>>> If the kernel is hacked to only generate IOVAs that are more than
>>>>> 32bit wide, the HW behaves correctly. The only way I can explain this
>>>>> behaviour is that the HW latches the top 32bit of the ERST (it is
>>>>> always the ERST IOVA that appears in my traces) in some internal
>>>>> register, and that the XHCI reset fails to clear it. Writing zero in
>>>>> the top bits is not enough to clear it either.
>>>>>
>>>>> So far, the only solution we have for this lovely piece of kit is to
>>>>> force a PCI reset at probe time, which puts it right. The patches are
>>>>> pretty ugly, but that's the best I could come up with so far.
>>>>>
>>>>> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
>>>>> uPD720202 controllers.
>>>>>
>>>>> Marc Zyngier (2):
>>>>>   PCI: Implement pci_reset_function_locked
>>>>>   usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
>>>>> controller
>>>>>
>>>>>  drivers/pci/pci.c | 35 +++
>>>>>  drivers/usb/host/pci-quirks.c | 20 
>>>>>  drivers/usb/host/pci-quirks.h |  1 +
>>>>>  drivers/usb/host/xhci-pci.c   |  7 +++
>>>>>  include/linux/pci.h   |  1 +
>>>>>  5 files changed, 64 insertions(+)
>>>>
>>>> I provisionally applied this to pci/virtualization.  I'd like to have an
>>>> XHCI ack before going further, though.
>>>>
>>>> I assume this only affects boxes where the firmware uses addresses above
>>>> 4GB, i.e., not very many?  So this is v4.14 material?  Or do you think it's
>>>> important for v4.13?
>>>>
>>>
>>> As I mentioned, it would be nice if this could at least go into v4.11
>>> and later, given that v4.11 contains a patch that switches all PCI
>>> devices to 32-bit addressing only when the IOMMU is involved in DMA,
>>> and this is what triggered the issue on arm64 boards with such a PCI
>>> card and no DRAM below 4 GB.
>>
>> Agreed. It is likely that the issue will trigger on any 64bit->32bit
>> IOVA transition, not only EFI->kernel, such as a kexec from a 4.10 to a
>> 4.11 kernel.
>>
>> More importantly, this could have a dramatic effect on a system where
>> both the 32bit and 64bit address ranges are valid. In my case, I was
>> saved by the IOMMU blocking the DMA access, but imagine for a second the
>> device was using PAs... I'm not sure that this is completely
>> hypothetical, nor arm64 specific.
> 
> I did add a v4.11+ stable tag on your behalf, so it will get
> backported to eventually.  But your responses don't exactly answer my
> question about whether you want to start with this in v4.13 or v4.14.  

Thanks for that. The sooner the better, so I'd be tempted to say 4.13,
assuming we can have an Ack from the XHCI maintainer in a reasonable
time frame.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Workaround for uPD72020x USB3 chips

[Marc Zyngier]

On 13/07/17 07:48, Ard Biesheuvel wrote:
> On 13 July 2017 at 04:12, Bjorn Helgaas <helg...@kernel.org> wrote:
>> On Mon, Jul 10, 2017 at 04:52:28PM +0100, Marc Zyngier wrote:
>>> Ard and myself have just spent quite some time lately trying to pin
>>> down an issue in the DMA code which was taking the form of a PCIe USB3
>>> controller issuing a DMA access at some bizarre address, and being
>>> caught red-handed by the IOMMU.
>>>
>>> After much head scratching and most of a week-end spent on tracing the
>>> damn thing, I'm now convinced that the DMA code is fine, the XHCI
>>> driver is correct, but that the HW (a Renesas uPD720202 chip) is a
>>> nasty piece of work.
>>>
>>> The issue is as follow:
>>>
>>> - EFI initializes the controller using physical addresses above the
>>>   4GB limit (this is on an arm64 box where the memory starts at
>>>   0x80_...).
>>>
>>> - The kernel takes over, sends a XHCI reset to the controller, and
>>>   because we have an IOMMU sitting between the controller and memory,
>>>   provides *virtual* addresses. Trying to make things a bit faster for
>>>   our controller, it issues IOVAs in the low 4GB range).
>>>
>>> - Low and behold, the controller is now issuing transactions with a
>>>   0x80 prefix in front of our IOVA. Yes, the same prefix that was
>>>   programmed during the EFI configuration. IOMMU fault, not happy.
>>>
>>> If the kernel is hacked to only generate IOVAs that are more than
>>> 32bit wide, the HW behaves correctly. The only way I can explain this
>>> behaviour is that the HW latches the top 32bit of the ERST (it is
>>> always the ERST IOVA that appears in my traces) in some internal
>>> register, and that the XHCI reset fails to clear it. Writing zero in
>>> the top bits is not enough to clear it either.
>>>
>>> So far, the only solution we have for this lovely piece of kit is to
>>> force a PCI reset at probe time, which puts it right. The patches are
>>> pretty ugly, but that's the best I could come up with so far.
>>>
>>> Tested on a pair of AMD Opteron 1100 boxes with Renesas uPD720201 and
>>> uPD720202 controllers.
>>>
>>> Marc Zyngier (2):
>>>   PCI: Implement pci_reset_function_locked
>>>   usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB
>>> controller
>>>
>>>  drivers/pci/pci.c | 35 +++
>>>  drivers/usb/host/pci-quirks.c | 20 
>>>  drivers/usb/host/pci-quirks.h |  1 +
>>>  drivers/usb/host/xhci-pci.c   |  7 +++
>>>  include/linux/pci.h   |  1 +
>>>  5 files changed, 64 insertions(+)
>>
>> I provisionally applied this to pci/virtualization.  I'd like to have an
>> XHCI ack before going further, though.
>>
>> I assume this only affects boxes where the firmware uses addresses above
>> 4GB, i.e., not very many?  So this is v4.14 material?  Or do you think it's
>> important for v4.13?
>>
> 
> As I mentioned, it would be nice if this could at least go into v4.11
> and later, given that v4.11 contains a patch that switches all PCI
> devices to 32-bit addressing only when the IOMMU is involved in DMA,
> and this is what triggered the issue on arm64 boards with such a PCI
> card and no DRAM below 4 GB.

Agreed. It is likely that the issue will trigger on any 64bit->32bit
IOVA transition, not only EFI->kernel, such as a kexec from a 4.10 to a
4.11 kernel.

More importantly, this could have a dramatic effect on a system where
both the 32bit and 64bit address ranges are valid. In my case, I was
saved by the IOMMU blocking the DMA access, but imagine for a second the
device was using PAs... I'm not sure that this is completely
hypothetical, nor arm64 specific.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] PCI: Implement pci_reset_function_locked

[Marc Zyngier]

The implementation of PCI workarounds may require that the device
is reset from its probe function. This implies that the PCI device
lock is already held, and makes calling pci_reset_function impossible
(since it will itself try to take that lock).

This patch introduces pci_reset_function_locked, which is the equivalent
of pci_reset_function, except that it requires the PCI device lock to
be already held by the caller.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/pci/pci.c   | 35 +++
 include/linux/pci.h |  1 +
 2 files changed, 36 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 563901cd9c06..a2c3e8b94f65 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4287,6 +4287,41 @@ int pci_reset_function(struct pci_dev *dev)
 EXPORT_SYMBOL_GPL(pci_reset_function);
 
 /**
+ * pci_reset_function_locked - quiesce and reset a PCI device function
+ * @dev: PCI device to reset
+ *
+ * Some devices allow an individual function to be reset without affecting
+ * other functions in the same device.  The PCI device must be responsive
+ * to PCI config space in order to use this function.
+ *
+ * This function does not just reset the PCI portion of a device, but
+ * clears all the state associated with the device.  This function differs
+ * from __pci_reset_function in that it saves and restores device state
+ * over the reset. it also differs from pci_reset_function in that it
+ * requires the PCI device lock to be held.
+ *
+ * Returns 0 if the device function was successfully reset or negative if the
+ * device doesn't support resetting a single function.
+ */
+int pci_reset_function_locked(struct pci_dev *dev)
+{
+   int rc;
+
+   rc = pci_dev_reset(dev, 1);
+   if (rc)
+   return rc;
+
+   pci_dev_save_and_disable(dev);
+
+   rc = __pci_dev_reset(dev, 0);
+
+   pci_dev_restore(dev);
+
+   return rc;
+}
+EXPORT_SYMBOL_GPL(pci_reset_function_locked);
+
+/**
  * pci_try_reset_function - quiesce and reset a PCI device function
  * @dev: PCI device to reset
  *
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8039f9f0ca05..16be18678ca1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1049,6 +1049,7 @@ void pcie_flr(struct pci_dev *dev);
 int __pci_reset_function(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev);
+int pci_reset_function_locked(struct pci_dev *dev);
 int pci_try_reset_function(struct pci_dev *dev);
 int pci_probe_reset_slot(struct pci_slot *slot);
 int pci_reset_slot(struct pci_slot *slot);
-- 
2.11.0

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

[PATCH 2/2] usb: host: pci_quirks: Force hard reset of Renesas uPD72020x USB controller

[Marc Zyngier]

The Renesas uPD72020x XHCI controller seems to suffer from a
really annoying bug, where it may retain some of its DMA programming
across a XHCI reset, and despite the driver correctly programming new
DMA addresses. This is visible if the device has been using 64bit DMA
addresses, and is then switched to using 32bit DMA addresses. The top
32bits of the address (now zero) are ignored are replaced by the 32bit
from the *previous* programming. Sticking with 64bit DMA always works,
but doesn't seem very appropriate.

A PCI reset of the device restores the normal functionnality, which
is done at probe time. Unfortunately, this has to be done before
any quirk has been discovered, hence the intrusive nature of the fix.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/usb/host/pci-quirks.c | 20 
 drivers/usb/host/pci-quirks.h |  1 +
 drivers/usb/host/xhci-pci.c   |  7 +++
 3 files changed, 28 insertions(+)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index a9a1e4c40480..5bed002c7dd3 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1096,3 +1096,23 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
+
+bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
+{
+   /*
+* Our dear uPD72020{1,2} friend only partially resets when
+* asked to via the XHCI interface, and may end-up doing DMA
+* at the wrong addresses, as it keeps the top 32bit of some
+* addresses from its previous programming under obscure
+* circumstances.
+* Give it a good wack at probe time. Unfortunately, this
+* needs to happen before we've had a chance to discover any
+* quirk, or the system will be in a rather bad state.
+*/
+   if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
+   (pdev->device == 0x0014 || pdev->device == 0x0015))
+   return true;
+
+   return false;
+}
+EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index 0222195bd5b0..dcbe4b097b33 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -14,6 +14,7 @@ void usb_amd_quirk_pll_enable(void);
 void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
 void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
 void sb800_prefetch(struct device *dev, int on);
+bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
 #else
 struct pci_dev;
 static inline void usb_amd_quirk_pll_disable(void) {}
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 1bcf971141c0..3831705c2817 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -267,6 +267,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const 
struct pci_device_id *id)
 
driver = (struct hc_driver *)id->driver_data;
 
+   /* For some HW implementation, a XHCI reset is just not enough... */
+   if (usb_xhci_needs_pci_reset(dev)) {
+   dev_info(>dev, "Resetting\n");
+   if (pci_reset_function_locked(dev))
+   dev_warn(>dev, "Reset failed");
+   }
+
/* Prevent runtime suspending between USB-2 and USB-3 initialization */
pm_runtime_get_noresume(>dev);
 
-- 
2.11.0

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