* [PATCH v4 2/5] dt-bindings: usb: generic-ohci: add AT91RM9200 OHCI binding support
From: Charan Pedumuru @ 2026-03-27 16:47 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Claudiu Beznea, Herve Codina, Nicolas Ferre,
Alexandre Belloni
Cc: linux-usb, devicetree, linux-arm-kernel, linux-kernel,
Charan Pedumuru
In-Reply-To: <20260327-atmel-usb-v4-0-eb8b6e49b29d@gmail.com>
Convert the Atmel AT91RM9200 OHCI USB host controller binding to DT schema
by defining it in the existing generic OHCI schema.
Signed-off-by: Charan Pedumuru <charan.pedumuru@gmail.com>
---
.../devicetree/bindings/usb/atmel-usb.txt | 27 --------------
.../devicetree/bindings/usb/generic-ohci.yaml | 41 ++++++++++++++++++++++
2 files changed, 41 insertions(+), 27 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt b/Documentation/devicetree/bindings/usb/atmel-usb.txt
index 12183ef47ee4..c09685283109 100644
--- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
+++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
@@ -1,32 +1,5 @@
Atmel SOC USB controllers
-OHCI
-
-Required properties:
- - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
- used in host mode.
- - reg: Address and length of the register set for the device
- - interrupts: Should contain ohci interrupt
- - clocks: Should reference the peripheral, host and system clocks
- - clock-names: Should contain three strings
- "ohci_clk" for the peripheral clock
- "hclk" for the host clock
- "uhpck" for the system clock
- - num-ports: Number of ports.
- - atmel,vbus-gpio: If present, specifies a gpio that needs to be
- activated for the bus to be powered.
- - atmel,oc-gpio: If present, specifies a gpio that needs to be
- activated for the overcurrent detection.
-
-usb0: ohci@500000 {
- compatible = "atmel,at91rm9200-ohci", "usb-ohci";
- reg = <0x00500000 0x100000>;
- clocks = <&uhphs_clk>, <&uhphs_clk>, <&uhpck>;
- clock-names = "ohci_clk", "hclk", "uhpck";
- interrupts = <20 4>;
- num-ports = <2>;
-};
-
EHCI
Required properties:
diff --git a/Documentation/devicetree/bindings/usb/generic-ohci.yaml b/Documentation/devicetree/bindings/usb/generic-ohci.yaml
index 961cbf85eeb5..d42f448fa204 100644
--- a/Documentation/devicetree/bindings/usb/generic-ohci.yaml
+++ b/Documentation/devicetree/bindings/usb/generic-ohci.yaml
@@ -55,6 +55,7 @@ properties:
- ti,ohci-omap3
- items:
- enum:
+ - atmel,at91rm9200-ohci
- cavium,octeon-6335-ohci
- nintendo,hollywood-usb-ohci
- nxp,ohci-nxp
@@ -137,6 +138,24 @@ properties:
The associated ISP1301 device. Necessary for the UDC controller for
connecting to the USB physical layer.
+ atmel,vbus-gpio:
+ description:
+ GPIO used to control or sense the USB VBUS power. Each entry
+ represents a VBUS-related GPIO; count and order may vary by hardware.
+ Entries follow standard GPIO specifier format. A value of 0 indicates
+ an unused or unavailable VBUS signal.
+ minItems: 1
+ maxItems: 3
+
+ atmel,oc-gpio:
+ description:
+ GPIO used to signal USB overcurrent condition. Each entry represents
+ an OC detection GPIO; count and order may vary by hardware. Entries
+ follow standard GPIO specifier format. A value of 0 indicates an
+ unused or unavailable OC signal.
+ minItems: 1
+ maxItems: 3
+
required:
- compatible
- reg
@@ -144,6 +163,28 @@ required:
allOf:
- $ref: usb-hcd.yaml
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: atmel,at91rm9200-ohci
+ then:
+ properties:
+ clock-names:
+ items:
+ - const: ohci_clk
+ - const: hclk
+ - const: uhpck
+
+ required:
+ - clocks
+ - clock-names
+
+ else:
+ properties:
+ atmel,vbus-gpio: false
+ atmel,oc-gpio: false
+
- if:
not:
properties:
--
2.53.0
^ permalink raw reply related
* [PATCH v4 1/5] arm: dts: at91: remove unused #address-cells/#size-cells from sam9x60 udc node
From: Charan Pedumuru @ 2026-03-27 16:47 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Claudiu Beznea, Herve Codina, Nicolas Ferre,
Alexandre Belloni
Cc: linux-usb, devicetree, linux-arm-kernel, linux-kernel,
Charan Pedumuru
In-Reply-To: <20260327-atmel-usb-v4-0-eb8b6e49b29d@gmail.com>
The UDC node does not define any child nodes, so the "#address-cells" and
"#size-cells" properties are unnecessary. Remove these unused properties
to simplify the devicetree node and keep it consistent with DT conventions.
Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Signed-off-by: Charan Pedumuru <charan.pedumuru@gmail.com>
---
arch/arm/boot/dts/microchip/sam9x60.dtsi | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
index b075865e6a76..e708b3df4ccd 100644
--- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
+++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
@@ -75,8 +75,6 @@ ahb {
ranges;
usb0: gadget@500000 {
- #address-cells = <1>;
- #size-cells = <0>;
compatible = "microchip,sam9x60-udc";
reg = <0x00500000 0x100000
0xf803c000 0x400>;
--
2.53.0
^ permalink raw reply related
* [PATCH v4 0/5] dt-bindings: usb: atmel: convert Atmel USB controller bindings to YAML
From: Charan Pedumuru @ 2026-03-27 16:47 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Claudiu Beznea, Herve Codina, Nicolas Ferre,
Alexandre Belloni
Cc: linux-usb, devicetree, linux-arm-kernel, linux-kernel,
Charan Pedumuru
This patch series converts the legacy text-based Device Tree bindings for
Atmel/Microchip USB controllers to DT schema (YAML) format.
Note:
The patch "dt-bindings: usb: atmel,at91sam9rl-udc: convert to DT schema"
depends on the patch "arm: dts: at91: remove unused #address-cells/#size-cells
from sam9x60 UDC node". If the DT schema patch is applied before the DTS
cleanup patch, `dtbs_check` will fail due to the presence of the removed
properties in the existing DTS.
Signed-off-by: Charan Pedumuru <charan.pedumuru@gmail.com>
---
Changes in v4:
- generic-ohci: Modify the commit message and modify description for the
properties "atmel,vbus-gpio" and "atmel,oc-gpio".
- atmel,at91rm9200-udc: Remove minItems for clocks and rename
unevaluatedProperties to additionalProperties.
- atmel,at91sam9rl-udc: Remove minItems for clocks and modify commit
message.
- all: Remove the corresponding text binding node for each patch from
the text binding file.
- Link to v3: https://lore.kernel.org/r/20260307-atmel-usb-v3-0-3dc48fe772be@gmail.com
Changes in v3:
- sam9x60: Add a new patch removing the unnecessary #address-cells and
#size-cells properties from the sam9x60 UDC node.
- atmel,at91sam9rl-udc: Remove #address-cells and #size-cells from the
atmel,at91sam9rl-udc binding properties.
- generic-ohci: Add an else condition to the generic-ohci schema properties
for improved validation precision.
- Link to v2: https://lore.kernel.org/r/20260224-atmel-usb-v2-0-6d6a615c9c47@gmail.com
Changes in v2:
- Drop the separate YAML patches for OHCI and EHCI.
- Add the compatibles "atmel,at91rm9200-ohci" and "atmel,at91sam9g45-ehci"
to the existing generic OHCI and EHCI binding files.
- Link to v1: https://lore.kernel.org/r/20260201-atmel-usb-v1-0-d1a3e93003f1@gmail.com
---
Charan Pedumuru (5):
arm: dts: at91: remove unused #address-cells/#size-cells from sam9x60 udc node
dt-bindings: usb: generic-ohci: add AT91RM9200 OHCI binding support
dt-bindings: usb: generic-ehci: fix schema structure and add at91sam9g45 constraints
dt-bindings: usb: atmel,at91rm9200-udc: convert to DT schema
dt-bindings: usb: atmel,at91sam9rl-udc: convert to DT schema
.../bindings/usb/atmel,at91rm9200-udc.yaml | 76 +++++++++++++
.../bindings/usb/atmel,at91sam9rl-udc.yaml | 74 ++++++++++++
.../devicetree/bindings/usb/atmel-usb.txt | 125 ---------------------
.../devicetree/bindings/usb/generic-ehci.yaml | 46 +++++---
.../devicetree/bindings/usb/generic-ohci.yaml | 41 +++++++
arch/arm/boot/dts/microchip/sam9x60.dtsi | 2 -
6 files changed, 224 insertions(+), 140 deletions(-)
---
base-commit: 3f24e4edcd1b8981c6b448ea2680726dedd87279
change-id: 20260129-atmel-usb-37f89a141e48
Best regards,
--
Charan Pedumuru <charan.pedumuru@gmail.com>
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Marek Szyprowski @ 2026-03-27 14:55 UTC (permalink / raw)
To: Catalin Marinas, Mikhail Gavrilov
Cc: vbabka, harry.yoo, akpm, hao.li, cl, rientjes, roman.gushchin,
linux-mm, linux-kernel, linux-usb, stern, linux, andy.shevchenko,
hch, Jeff.kirsher, Robin Murphy
In-Reply-To: <d2ed7315-72ff-43f3-bfaa-995025cb9419@samsung.com>
On 27.03.2026 15:09, Marek Szyprowski wrote:
> On 27.03.2026 13:26, Catalin Marinas wrote:
>> + Marek, Robin
>
> Thanks for adding me to the loop.
>
>> On Fri, Mar 27, 2026 at 10:58:46AM +0500, Mikhail Gavrilov wrote:
>>> When CONFIG_DMA_API_DEBUG is enabled, the DMA debug infrastructure
>>> tracks active mappings per cacheline and warns if two different DMA
>>> mappings share the same cacheline ("cacheline tracking EEXIST,
>>> overlapping mappings aren't supported").
>>>
>>> On x86_64, ARCH_KMALLOC_MINALIGN defaults to 8, so small kmalloc
>>> allocations (e.g. the 8-byte hub->buffer and hub->status in the USB
>>> hub driver) frequently land in the same 64-byte cacheline. When both
>>> are DMA-mapped, this triggers a false positive warning.
>>>
>>> This has been reported repeatedly since v5.14 (when the EEXIST check
>>> was added) across various USB host controllers and devices including
>>> xhci_hcd with USB hubs, USB audio devices, and USB ethernet adapters.
>> This indeed has come up regularly in the past years.
>>
>>> +/*
>>> + * Align memory allocations to cache lines if DMA API debugging is active
>>> + * to avoid false positive DMA overlapping error messages.
>>> + */
>>> +#ifdef CONFIG_DMA_API_DEBUG
>>> +#ifndef ARCH_KMALLOC_MINALIGN
>>> +#define ARCH_KMALLOC_MINALIGN L1_CACHE_BYTES
>>> +#elif ARCH_KMALLOC_MINALIGN < L1_CACHE_BYTES
>>> +#undef ARCH_KMALLOC_MINALIGN
>>> +#define ARCH_KMALLOC_MINALIGN L1_CACHE_BYTES
>>> +#endif
>>> +#endif
>> TL;DR: I think this is fine:
>>
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>> I'm not sure that's the best way to hide the warning but there
>> are no great solutions either. On one hand, we want the DMA debug to
>> capture potential problems on architectures it's not running on. OTOH,
>> we also want to avoid false positives on coherent architectures/devices.
>> I don't think reconciling the two requirements is easy.
>>
>> When DMA_API_DEBUG is enabled, the above will change the x86 behaviour
>> that could have implications beyond DMA (e.g. may not catch some buffer
>> overflow because it's within L1_CACHE_BYTES). Similarly for non-coherent
>> architectures that select DMA_BOUNCE_UNALIGNED_KMALLOC (arm64 and riscv
>> currently). arm64 defines ARCH_DMA_MINALIGN to 128 but
>> ARCH_KMALLOC_MINALIGN to 8 (why 128 is larger than L1_CACHE_BYTES is
>> another matter but let's ignore it for now).
>
> IMHO enabling DMA_API_DEBUG should not change the kernel behavior, so I would prefer fixing this in DMA-debug code somehow.
>
>> More of a thinking out loud, we have:
>>
>> 1. Coherent architectures - alignment doesn't matter
>>
>> 2. Non-coherent architectures with:
>> a) Sufficiently large ARCH_KMALLOC_MINALIGN
>> b) Small ARCH_KMALLOC_MINALIGN but DMA_BOUNCE_UNALIGNED_KMALLOC
>> c) Broken config - forgot to set ARCH_DMA_MINALIGN or bouncing
>>
>> We can ignore (2.c), the aim of the DMA debug is to catch wrong uses in
>> drivers. If drivers is the only goal, the above change will do when
>> running on (1) or (2.a) hardware - it will catch sub-L1_CACHE_BYTES
>> buffers from drivers while assuming kmalloc() machinery is safe.
>> However, if running on (2.b) it won't catch anything that may be
>> problematic on (2.a) since the DMA debug ignores the overlap.
>>
>> We could make DMA_BOUNCE_UNALIGNED_KMALLOC dependent on !DMA_API_DEBUG
>> but it would be nice to be able to sanity-check the bouncing logic.
>> Well, it wasn't checking it before and with commit 03521c892bb8
>> ("dma-debug: don't report false positives with
>> DMA_BOUNCE_UNALIGNED_KMALLOC"), we made this clear that overlapping will
>> be ignored.
>>
>> Irrespective of whether we disable bouncing with DMA_API_DEBUG, maybe we
>> could replace the above commit with:
>>
>> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
>> index 3928a509c44c..488045ef6245 100644
>> --- a/kernel/dma/mapping.c
>> +++ b/kernel/dma/mapping.c
>> @@ -175,7 +175,7 @@ dma_addr_t dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
>> if (!is_mmio)
>> kmsan_handle_dma(phys, size, dir);
>> trace_dma_map_phys(dev, phys, addr, size, dir, attrs);
>> - debug_dma_map_phys(dev, phys, size, dir, addr, attrs);
>> + debug_dma_map_phys(dev, dma_to_phys(addr), size, dir, addr, attrs);
>>
>> return addr;
>> }
>>
>> Anyway, this I think is unrelated to the proposed change affecting x86,
>> more of a how to make the DMA API debugging more useful when running on
>> arm64 or riscv.
>
> This is not enough, there is also a dma_map_sg_attrs() path.
>
> I've reverted 03521c892bb8 and added the following change:
>
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c index 55e7ca8ceb86..bbada41143ea 100644 --- a/kernel/dma/debug.c +++ b/kernel/dma/debug.c @@ -18,6 +18,7 @@ #include <linux/uaccess.h> #include <linux/export.h> #include <linux/device.h> +#include <linux/dma-direct.h> #include <linux/types.h> #include <linux/sched.h> #include <linux/ctype.h> @@ -1241,7 +1242,8 @@ void debug_dma_map_phys(struct device *dev, phys_addr_t phys, size_t size, entry->dev = dev; entry->type = dma_debug_phy; - entry->paddr = phys; + entry->paddr = IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) ? + dma_to_phys(dev, dma_addr) : phys; entry->dev_addr = dma_addr; entry->size = size; entry->direction = direction; @@ -1335,7 +1337,9 @@ void debug_dma_map_sg(struct device *dev, struct scatterlist *sg, entry->type = dma_debug_sg; entry->dev = dev; - entry->paddr = sg_phys(s); + entry->paddr = + IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) ? + dma_to_phys(dev, sg_dma_address(s)) : sg_phys(s);
> entry->size = sg_dma_len(s); entry->dev_addr = sg_dma_address(s); entry->direction = direction;
>
> thenran my tests on ARM64 and RV64 boards. Only one new warning has been reported (I didn't analyze it yet), so this might be indeed a better solution than skipping overlapping cache lines warnings when DMA_BOUNCE_UNALIGNED_KMALLOC is set.
>
Huh, the diff has been malformed by my mail client. Let's try again:
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 55e7ca8ceb86..bbada41143ea 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -18,6 +18,7 @@
#include <linux/uaccess.h>
#include <linux/export.h>
#include <linux/device.h>
+#include <linux/dma-direct.h>
#include <linux/types.h>
#include <linux/sched.h>
#include <linux/ctype.h>
@@ -1241,7 +1242,8 @@ void debug_dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
entry->dev = dev;
entry->type = dma_debug_phy;
- entry->paddr = phys;
+ entry->paddr = IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) ?
+ dma_to_phys(dev, dma_addr) : phys;
entry->dev_addr = dma_addr;
entry->size = size;
entry->direction = direction;
@@ -1335,7 +1337,9 @@ void debug_dma_map_sg(struct device *dev, struct scatterlist *sg,
entry->type = dma_debug_sg;
entry->dev = dev;
- entry->paddr = sg_phys(s);
+ entry->paddr =
+ IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) ?
+ dma_to_phys(dev, sg_dma_address(s)) : sg_phys(s);
entry->size = sg_dma_len(s);
entry->dev_addr = sg_dma_address(s);
entry->direction = direction;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply related
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Marek Szyprowski @ 2026-03-27 14:41 UTC (permalink / raw)
To: Mikhail Gavrilov, Vlastimil Babka (SUSE)
Cc: Catalin Marinas, harry.yoo, akpm, hao.li, cl, rientjes,
roman.gushchin, linux-mm, linux-kernel, linux-usb, stern, linux,
andy.shevchenko, hch, Jeff.kirsher, Robin Murphy
In-Reply-To: <CABXGCsNDfuLQ64r=sMyJ0UYbzZT_j8xsH5h0gpAGTPwV64Qp2g@mail.gmail.com>
On 27.03.2026 15:37, Mikhail Gavrilov wrote:
> On Fri, Mar 27, 2026 at 7:30 PM Vlastimil Babka (SUSE)
> <vbabka@kernel.org> wrote:
>> So what about Harry's proposal [1]? Mikhail seems to be on board? [2]
>>
>> It seems it would achieve the goal that enabling DMA_API_DEBUG doesn't
>> change the kernel behavior? But I don't know this area too well so
>> maybe there's a catch.
>>
>> [1] https://lore.kernel.org/all/acYlxRBhSMcwBnja@hyeyoo/
>> [2] https://lore.kernel.org/all/CABXGCsO_C8%2B%2B4%2BoPfZ%2BbQyrBnEGy5JFpXHkGNpfy%2B8%3D5BvVNfg@mail.gmail.com/
> Hi Vlastimil,
>
> Yes, I've already sent v2 based on Harry's suggestion:
> https://lore.kernel.org/all/20260327124156.24820-1-mikhail.v.gavrilov@gmail.com/
>
> It adds a dma_get_cache_alignment() >= L1_CACHE_BYTES check in
> add_dma_entry() instead of changing ARCH_KMALLOC_MINALIGN, so
> enabling DMA_API_DEBUG no longer affects allocator behavior.
This looks like a good fix, but let me think a bit more about all
possible cases.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Mikhail Gavrilov @ 2026-03-27 14:37 UTC (permalink / raw)
To: Vlastimil Babka (SUSE)
Cc: Marek Szyprowski, Catalin Marinas, harry.yoo, akpm, hao.li, cl,
rientjes, roman.gushchin, linux-mm, linux-kernel, linux-usb,
stern, linux, andy.shevchenko, hch, Jeff.kirsher, Robin Murphy
In-Reply-To: <bfa5ff0a-8ef9-46cb-8881-396f71ed5be0@kernel.org>
On Fri, Mar 27, 2026 at 7:30 PM Vlastimil Babka (SUSE)
<vbabka@kernel.org> wrote:
>
> So what about Harry's proposal [1]? Mikhail seems to be on board? [2]
>
> It seems it would achieve the goal that enabling DMA_API_DEBUG doesn't
> change the kernel behavior? But I don't know this area too well so
> maybe there's a catch.
>
> [1] https://lore.kernel.org/all/acYlxRBhSMcwBnja@hyeyoo/
> [2] https://lore.kernel.org/all/CABXGCsO_C8%2B%2B4%2BoPfZ%2BbQyrBnEGy5JFpXHkGNpfy%2B8%3D5BvVNfg@mail.gmail.com/
Hi Vlastimil,
Yes, I've already sent v2 based on Harry's suggestion:
https://lore.kernel.org/all/20260327124156.24820-1-mikhail.v.gavrilov@gmail.com/
It adds a dma_get_cache_alignment() >= L1_CACHE_BYTES check in
add_dma_entry() instead of changing ARCH_KMALLOC_MINALIGN, so
enabling DMA_API_DEBUG no longer affects allocator behavior.
--
Best Regards,
Mike Gavrilov.
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Vlastimil Babka (SUSE) @ 2026-03-27 14:30 UTC (permalink / raw)
To: Marek Szyprowski, Catalin Marinas, Mikhail Gavrilov
Cc: harry.yoo, akpm, hao.li, cl, rientjes, roman.gushchin, linux-mm,
linux-kernel, linux-usb, stern, linux, andy.shevchenko, hch,
Jeff.kirsher, Robin Murphy
In-Reply-To: <d2ed7315-72ff-43f3-bfaa-995025cb9419@samsung.com>
On 3/27/26 15:09, Marek Szyprowski wrote:
> Hi
>
> On 27.03.2026 13:26, Catalin Marinas wrote:
>> + Marek, Robin
>
> Thanks for adding me to the loop.
>
>> On Fri, Mar 27, 2026 at 10:58:46AM +0500, Mikhail Gavrilov wrote:
>>> When CONFIG_DMA_API_DEBUG is enabled, the DMA debug infrastructure
>>> tracks active mappings per cacheline and warns if two different DMA
>>> mappings share the same cacheline ("cacheline tracking EEXIST,
>>> overlapping mappings aren't supported").
>>>
>>> On x86_64, ARCH_KMALLOC_MINALIGN defaults to 8, so small kmalloc
>>> allocations (e.g. the 8-byte hub->buffer and hub->status in the USB
>>> hub driver) frequently land in the same 64-byte cacheline. When both
>>> are DMA-mapped, this triggers a false positive warning.
>>>
>>> This has been reported repeatedly since v5.14 (when the EEXIST check
>>> was added) across various USB host controllers and devices including
>>> xhci_hcd with USB hubs, USB audio devices, and USB ethernet adapters.
>> This indeed has come up regularly in the past years.
>>
>>> +/*
>>> + * Align memory allocations to cache lines if DMA API debugging is active
>>> + * to avoid false positive DMA overlapping error messages.
>>> + */
>>> +#ifdef CONFIG_DMA_API_DEBUG
>>> +#ifndef ARCH_KMALLOC_MINALIGN
>>> +#define ARCH_KMALLOC_MINALIGN L1_CACHE_BYTES
>>> +#elif ARCH_KMALLOC_MINALIGN < L1_CACHE_BYTES
>>> +#undef ARCH_KMALLOC_MINALIGN
>>> +#define ARCH_KMALLOC_MINALIGN L1_CACHE_BYTES
>>> +#endif
>>> +#endif
>> TL;DR: I think this is fine:
>>
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>> I'm not sure that's the best way to hide the warning but there
>> are no great solutions either. On one hand, we want the DMA debug to
>> capture potential problems on architectures it's not running on. OTOH,
>> we also want to avoid false positives on coherent architectures/devices.
>> I don't think reconciling the two requirements is easy.
>>
>> When DMA_API_DEBUG is enabled, the above will change the x86 behaviour
>> that could have implications beyond DMA (e.g. may not catch some buffer
>> overflow because it's within L1_CACHE_BYTES). Similarly for non-coherent
>> architectures that select DMA_BOUNCE_UNALIGNED_KMALLOC (arm64 and riscv
>> currently). arm64 defines ARCH_DMA_MINALIGN to 128 but
>> ARCH_KMALLOC_MINALIGN to 8 (why 128 is larger than L1_CACHE_BYTES is
>> another matter but let's ignore it for now).
>
> IMHO enabling DMA_API_DEBUG should not change the kernel behavior, so I
> would prefer fixing this in DMA-debug code somehow.
So what about Harry's proposal [1]? Mikhail seems to be on board? [2]
It seems it would achieve the goal that enabling DMA_API_DEBUG doesn't
change the kernel behavior? But I don't know this area too well so
maybe there's a catch.
[1] https://lore.kernel.org/all/acYlxRBhSMcwBnja@hyeyoo/
[2] https://lore.kernel.org/all/CABXGCsO_C8%2B%2B4%2BoPfZ%2BbQyrBnEGy5JFpXHkGNpfy%2B8%3D5BvVNfg@mail.gmail.com/
>
>> More of a thinking out loud, we have:
>>
>> 1. Coherent architectures - alignment doesn't matter
>>
>> 2. Non-coherent architectures with:
>> a) Sufficiently large ARCH_KMALLOC_MINALIGN
>> b) Small ARCH_KMALLOC_MINALIGN but DMA_BOUNCE_UNALIGNED_KMALLOC
>> c) Broken config - forgot to set ARCH_DMA_MINALIGN or bouncing
>>
>> We can ignore (2.c), the aim of the DMA debug is to catch wrong uses in
>> drivers. If drivers is the only goal, the above change will do when
>> running on (1) or (2.a) hardware - it will catch sub-L1_CACHE_BYTES
>> buffers from drivers while assuming kmalloc() machinery is safe.
>> However, if running on (2.b) it won't catch anything that may be
>> problematic on (2.a) since the DMA debug ignores the overlap.
>>
>> We could make DMA_BOUNCE_UNALIGNED_KMALLOC dependent on !DMA_API_DEBUG
>> but it would be nice to be able to sanity-check the bouncing logic.
>> Well, it wasn't checking it before and with commit 03521c892bb8
>> ("dma-debug: don't report false positives with
>> DMA_BOUNCE_UNALIGNED_KMALLOC"), we made this clear that overlapping will
>> be ignored.
>>
>> Irrespective of whether we disable bouncing with DMA_API_DEBUG, maybe we
>> could replace the above commit with:
>>
>> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
>> index 3928a509c44c..488045ef6245 100644
>> --- a/kernel/dma/mapping.c
>> +++ b/kernel/dma/mapping.c
>> @@ -175,7 +175,7 @@ dma_addr_t dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
>> if (!is_mmio)
>> kmsan_handle_dma(phys, size, dir);
>> trace_dma_map_phys(dev, phys, addr, size, dir, attrs);
>> - debug_dma_map_phys(dev, phys, size, dir, addr, attrs);
>> + debug_dma_map_phys(dev, dma_to_phys(addr), size, dir, addr, attrs);
>>
>> return addr;
>> }
>>
>> Anyway, this I think is unrelated to the proposed change affecting x86,
>> more of a how to make the DMA API debugging more useful when running on
>> arm64 or riscv.
>
> This is not enough, there is also a dma_map_sg_attrs() path.
>
> I've reverted 03521c892bb8 and added the following change:
>
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c index
> 55e7ca8ceb86..bbada41143ea 100644 --- a/kernel/dma/debug.c +++
> b/kernel/dma/debug.c @@ -18,6 +18,7 @@ #include <linux/uaccess.h>
> #include <linux/export.h> #include <linux/device.h> +#include
> <linux/dma-direct.h> #include <linux/types.h> #include <linux/sched.h>
> #include <linux/ctype.h> @@ -1241,7 +1242,8 @@ void
> debug_dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
> entry->dev = dev; entry->type = dma_debug_phy; - entry->paddr = phys; +
> entry->paddr = IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) ? +
> dma_to_phys(dev, dma_addr) : phys; entry->dev_addr = dma_addr;
> entry->size = size; entry->direction = direction; @@ -1335,7 +1337,9 @@
> void debug_dma_map_sg(struct device *dev, struct scatterlist *sg,
> entry->type = dma_debug_sg; entry->dev = dev; - entry->paddr =
> sg_phys(s); + entry->paddr = +
> IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) ? + dma_to_phys(dev,
> sg_dma_address(s)) : sg_phys(s); entry->size = sg_dma_len(s);
> entry->dev_addr = sg_dma_address(s); entry->direction = direction;
>
> thenran my tests on ARM64 and RV64 boards. Only one new warning has been
> reported (I didn't analyze it yet), so this might be indeed a better
> solution than skipping overlapping cache lines warnings when
> DMA_BOUNCE_UNALIGNED_KMALLOC is set.
>
> Best regards
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Marek Szyprowski @ 2026-03-27 14:09 UTC (permalink / raw)
To: Catalin Marinas, Mikhail Gavrilov
Cc: vbabka, harry.yoo, akpm, hao.li, cl, rientjes, roman.gushchin,
linux-mm, linux-kernel, linux-usb, stern, linux, andy.shevchenko,
hch, Jeff.kirsher, Robin Murphy
In-Reply-To: <acZ3ZUXhFHpSXzYS@arm.com>
Hi
On 27.03.2026 13:26, Catalin Marinas wrote:
> + Marek, Robin
Thanks for adding me to the loop.
> On Fri, Mar 27, 2026 at 10:58:46AM +0500, Mikhail Gavrilov wrote:
>> When CONFIG_DMA_API_DEBUG is enabled, the DMA debug infrastructure
>> tracks active mappings per cacheline and warns if two different DMA
>> mappings share the same cacheline ("cacheline tracking EEXIST,
>> overlapping mappings aren't supported").
>>
>> On x86_64, ARCH_KMALLOC_MINALIGN defaults to 8, so small kmalloc
>> allocations (e.g. the 8-byte hub->buffer and hub->status in the USB
>> hub driver) frequently land in the same 64-byte cacheline. When both
>> are DMA-mapped, this triggers a false positive warning.
>>
>> This has been reported repeatedly since v5.14 (when the EEXIST check
>> was added) across various USB host controllers and devices including
>> xhci_hcd with USB hubs, USB audio devices, and USB ethernet adapters.
> This indeed has come up regularly in the past years.
>
>> +/*
>> + * Align memory allocations to cache lines if DMA API debugging is active
>> + * to avoid false positive DMA overlapping error messages.
>> + */
>> +#ifdef CONFIG_DMA_API_DEBUG
>> +#ifndef ARCH_KMALLOC_MINALIGN
>> +#define ARCH_KMALLOC_MINALIGN L1_CACHE_BYTES
>> +#elif ARCH_KMALLOC_MINALIGN < L1_CACHE_BYTES
>> +#undef ARCH_KMALLOC_MINALIGN
>> +#define ARCH_KMALLOC_MINALIGN L1_CACHE_BYTES
>> +#endif
>> +#endif
> TL;DR: I think this is fine:
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>
> I'm not sure that's the best way to hide the warning but there
> are no great solutions either. On one hand, we want the DMA debug to
> capture potential problems on architectures it's not running on. OTOH,
> we also want to avoid false positives on coherent architectures/devices.
> I don't think reconciling the two requirements is easy.
>
> When DMA_API_DEBUG is enabled, the above will change the x86 behaviour
> that could have implications beyond DMA (e.g. may not catch some buffer
> overflow because it's within L1_CACHE_BYTES). Similarly for non-coherent
> architectures that select DMA_BOUNCE_UNALIGNED_KMALLOC (arm64 and riscv
> currently). arm64 defines ARCH_DMA_MINALIGN to 128 but
> ARCH_KMALLOC_MINALIGN to 8 (why 128 is larger than L1_CACHE_BYTES is
> another matter but let's ignore it for now).
IMHO enabling DMA_API_DEBUG should not change the kernel behavior, so I
would prefer fixing this in DMA-debug code somehow.
> More of a thinking out loud, we have:
>
> 1. Coherent architectures - alignment doesn't matter
>
> 2. Non-coherent architectures with:
> a) Sufficiently large ARCH_KMALLOC_MINALIGN
> b) Small ARCH_KMALLOC_MINALIGN but DMA_BOUNCE_UNALIGNED_KMALLOC
> c) Broken config - forgot to set ARCH_DMA_MINALIGN or bouncing
>
> We can ignore (2.c), the aim of the DMA debug is to catch wrong uses in
> drivers. If drivers is the only goal, the above change will do when
> running on (1) or (2.a) hardware - it will catch sub-L1_CACHE_BYTES
> buffers from drivers while assuming kmalloc() machinery is safe.
> However, if running on (2.b) it won't catch anything that may be
> problematic on (2.a) since the DMA debug ignores the overlap.
>
> We could make DMA_BOUNCE_UNALIGNED_KMALLOC dependent on !DMA_API_DEBUG
> but it would be nice to be able to sanity-check the bouncing logic.
> Well, it wasn't checking it before and with commit 03521c892bb8
> ("dma-debug: don't report false positives with
> DMA_BOUNCE_UNALIGNED_KMALLOC"), we made this clear that overlapping will
> be ignored.
>
> Irrespective of whether we disable bouncing with DMA_API_DEBUG, maybe we
> could replace the above commit with:
>
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 3928a509c44c..488045ef6245 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -175,7 +175,7 @@ dma_addr_t dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
> if (!is_mmio)
> kmsan_handle_dma(phys, size, dir);
> trace_dma_map_phys(dev, phys, addr, size, dir, attrs);
> - debug_dma_map_phys(dev, phys, size, dir, addr, attrs);
> + debug_dma_map_phys(dev, dma_to_phys(addr), size, dir, addr, attrs);
>
> return addr;
> }
>
> Anyway, this I think is unrelated to the proposed change affecting x86,
> more of a how to make the DMA API debugging more useful when running on
> arm64 or riscv.
This is not enough, there is also a dma_map_sg_attrs() path.
I've reverted 03521c892bb8 and added the following change:
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c index
55e7ca8ceb86..bbada41143ea 100644 --- a/kernel/dma/debug.c +++
b/kernel/dma/debug.c @@ -18,6 +18,7 @@ #include <linux/uaccess.h>
#include <linux/export.h> #include <linux/device.h> +#include
<linux/dma-direct.h> #include <linux/types.h> #include <linux/sched.h>
#include <linux/ctype.h> @@ -1241,7 +1242,8 @@ void
debug_dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
entry->dev = dev; entry->type = dma_debug_phy; - entry->paddr = phys; +
entry->paddr = IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) ? +
dma_to_phys(dev, dma_addr) : phys; entry->dev_addr = dma_addr;
entry->size = size; entry->direction = direction; @@ -1335,7 +1337,9 @@
void debug_dma_map_sg(struct device *dev, struct scatterlist *sg,
entry->type = dma_debug_sg; entry->dev = dev; - entry->paddr =
sg_phys(s); + entry->paddr = +
IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) ? + dma_to_phys(dev,
sg_dma_address(s)) : sg_phys(s); entry->size = sg_dma_len(s);
entry->dev_addr = sg_dma_address(s); entry->direction = direction;
thenran my tests on ARM64 and RV64 boards. Only one new warning has been
reported (I didn't analyze it yet), so this might be indeed a better
solution than skipping overlapping cache lines warnings when
DMA_BOUNCE_UNALIGNED_KMALLOC is set.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH v2 2/6] usb: xhci: tegra: Remove redundant mutex when setting phy mode
From: Thierry Reding @ 2026-03-27 14:06 UTC (permalink / raw)
To: Diogo Ivo
Cc: Thierry Reding, Mathias Nyman, Greg Kroah-Hartman,
Jonathan Hunter, JC Kuo, Vinod Koul, Kishon Vijay Abraham I,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
linux-usb, linux-tegra, linux-kernel, linux-phy, devicetree
In-Reply-To: <00aeda7a-e5e5-4779-b212-6e56c2c5ec31@tecnico.ulisboa.pt>
[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]
On Thu, Mar 26, 2026 at 02:17:33PM +0000, Diogo Ivo wrote:
> Hello,
>
> On 3/24/26 11:48, Thierry Reding wrote:
> > On Tue, Jan 27, 2026 at 03:11:48PM +0000, Diogo Ivo wrote:
> > > As the PHY subsystem already synchronizes concurrent accesses to a PHY
> > > instance with a core-internal mutex remove the driver specific mutex
> > > synchronization.
> > >
> > > Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> > > ---
> > > v1->v2:
> > > - New patch
> > > ---
> > > drivers/usb/host/xhci-tegra.c | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> > > index 8b492871d21d..927861ca14f2 100644
> > > --- a/drivers/usb/host/xhci-tegra.c
> > > +++ b/drivers/usb/host/xhci-tegra.c
> > > @@ -1357,15 +1357,11 @@ static void tegra_xhci_id_work(struct work_struct *work)
> > > dev_dbg(tegra->dev, "host mode %s\n", str_on_off(tegra->host_mode));
> > > - mutex_lock(&tegra->lock);
> > > -
> > > if (tegra->host_mode)
> > > phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_HOST);
> > > else
> > > phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_NONE);
> > > - mutex_unlock(&tegra->lock);
> > > -
> >
> > It looks to me like the mutex here is trying to protect against
> > tegra->host_mode changing while we're setting a different mode. That
> > doesn't seem to be taken care of by the PHY internal mutex.
>
> After taking another look at it I think I understand your point for the
> mutex, but in that case wouldn't it also need to be held in the writer
> of host_mode, tegra_xhci_id_notify()?
Yes, I think it probably would need to. I don't know how likely it is,
but I think the purpose of this is to protect against the ID notifier
firing quickly in succession. Although, given that this runs on a work
queue and work queue instances are non-reentrant to my knowledge, I
don't think we need the mutex here after all.
> This patch has been picked up as-is into usb-next so it would be nice to
> figure this out before it gets merged in the next merge window.
Given the above, I think it's fine. Maybe the commit message doesn't
give a correct reason for why we don't need the mutex, but the resulting
code looks like it should be fine regardless.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2] dt-bindings: usb: qcom,snps-dwc3: Document the Eliza compatible
From: Abel Vesa @ 2026-03-27 12:51 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Wesley Cheng
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel,
Krzysztof Kozlowski, Abel Vesa
Document the compatible for the Qualcomm Synopsys DWC3 glue controller
found on Eliza SoC.
It follows the same binding requirements as other recent Qualcomm
SoCs, so add it to the existing schema conditionals covering the
required properties.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
Changes in v2:
- Rebased on next-20260326.
- Picked up Krzysztof's R-b tag.
- Link to v1: https://patch.msgid.link/20260318-eliza-bindings-dwc3-v1-1-92bdf233cb87@oss.qualcomm.com
---
Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
index f879e2e104c4..e67a967c677f 100644
--- a/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
@@ -24,6 +24,7 @@ properties:
compatible:
items:
- enum:
+ - qcom,eliza-dwc3
- qcom,glymur-dwc3
- qcom,glymur-dwc3-mp
- qcom,ipq4019-dwc3
@@ -346,6 +347,7 @@ allOf:
compatible:
contains:
enum:
+ - qcom,eliza-dwc3
- qcom,milos-dwc3
- qcom,qcm2290-dwc3
- qcom,qcs615-dwc3
@@ -507,6 +509,7 @@ allOf:
compatible:
contains:
enum:
+ - qcom,eliza-dwc3
- qcom,ipq4019-dwc3
- qcom,ipq8064-dwc3
- qcom,kaanapali-dwc3
---
base-commit: e77a5a5cfe43b4c25bd44a3818e487033287517f
change-id: 20260318-eliza-bindings-dwc3-4b6e4ea45b93
Best regards,
--
Abel Vesa <abel.vesa@oss.qualcomm.com>
^ permalink raw reply related
* [PATCH v2] dma-debug: suppress cacheline overlap warning when arch has no DMA alignment requirement
From: Mikhail Gavrilov @ 2026-03-27 12:41 UTC (permalink / raw)
To: m.szyprowski, robin.murphy
Cc: iommu, linux-kernel, linux-usb, linux-mm, harry, vbabka, akpm,
stern, linux, andy.shevchenko, hch, Jeff.kirsher, catalin.marinas,
Mikhail Gavrilov
When CONFIG_DMA_API_DEBUG is enabled, the DMA debug infrastructure
tracks active mappings per cacheline and warns if two different DMA
mappings share the same cacheline ("cacheline tracking EEXIST,
overlapping mappings aren't supported").
On x86_64, ARCH_KMALLOC_MINALIGN defaults to 8, so small kmalloc
allocations (e.g. the 8-byte hub->buffer and hub->status in the USB
hub driver) frequently land in the same 64-byte cacheline. When both
are DMA-mapped, this triggers a false positive warning.
This has been reported repeatedly since v5.14 (when the EEXIST check
was added) across various USB host controllers and devices including
xhci_hcd with USB hubs, USB audio devices, and USB ethernet adapters.
The cacheline overlap is only a real concern on architectures that
require DMA buffer alignment to cacheline boundaries (i.e. where
ARCH_DMA_MINALIGN >= L1_CACHE_BYTES). On architectures like x86_64
where dma_get_cache_alignment() returns 1, the hardware is
cache-coherent and overlapping cacheline mappings are harmless.
Suppress the EEXIST warning when dma_get_cache_alignment() is less
than L1_CACHE_BYTES, indicating the architecture does not require
cacheline-aligned DMA buffers.
Verified with a kernel module reproducer that performs two kmalloc(8)
allocations back-to-back and DMA-maps both:
Before: allocations share a cacheline, EEXIST fires within ~50 pairs
After: same cacheline pair found, but no warning emitted
Fixes: 2b4bbc6231d7 ("dma-debug: report -EEXIST errors in add_dma_entry")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215740
Suggested-by: Harry Yoo <harry@kernel.org>
Tested-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
---
v1 -> v2:
- Moved fix from include/linux/slab.h (ARCH_KMALLOC_MINALIGN)
to kernel/dma/debug.c per Harry Yoo's suggestion.
- Instead of forcing cacheline-aligned allocations, suppress
the warning when the architecture has no DMA alignment
requirement (dma_get_cache_alignment() < L1_CACHE_BYTES).
v1: https://lore.kernel.org/all/20260327055846.248829-1-mikhail.v.gavrilov@gmail.com/
Reproducer module that triggers the bug reliably:
https://bugzilla.kernel.org/attachment.cgi?id=309769
kernel/dma/debug.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 0677918f06a8..1a725edbbbf6 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -615,6 +615,7 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs)
} else if (rc == -EEXIST &&
!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
!(entry->is_cache_clean && overlap_cache_clean) &&
+ dma_get_cache_alignment() >= L1_CACHE_BYTES &&
!(IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) &&
is_swiotlb_active(entry->dev))) {
err_printk(entry->dev, entry,
--
2.53.0
^ permalink raw reply related
* [PATCH 9/9] usb: xhci: optimize resuming from S4 (suspend-to-disk)
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin
In-Reply-To: <20260327123441.806564-1-niklas.neronin@linux.intel.com>
On resume from S4 (power loss after suspend/hibernation), the xHCI
driver previously freed, reallocated, and fully reinitialized all
data structures. Most of this is unnecessary because the data is
restored from a saved image; only the xHCI registers lose their values.
This patch optimizes S4 resume by performing only a host controller
reset, which includes:
* Freeing or clearing runtime-created data.
* Rewriting xHCI registers.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-mem.c | 4 +--
drivers/usb/host/xhci.c | 53 ++++++++++++++++++++++---------------
drivers/usb/host/xhci.h | 2 ++
3 files changed, 35 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index f1b4f06d4b8b..4156822eb000 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -936,7 +936,7 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
* that tt_info, then free the child first. Recursive.
* We can't rely on udev at this point to find child-parent relationships.
*/
-static void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
+void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
{
struct xhci_virt_device *vdev;
struct list_head *tt_list_head;
@@ -1905,7 +1905,7 @@ void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
EXPORT_SYMBOL_GPL(xhci_remove_secondary_interrupter);
/* Cleanup roothub bandwidth data */
-static void xhci_rh_bw_cleanup(struct xhci_hcd *xhci)
+void xhci_rh_bw_cleanup(struct xhci_hcd *xhci)
{
struct xhci_root_port_bw_info *rh_bw;
struct xhci_tt_bw_info *tt_info, *tt_next;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 232e6143ac4b..8fb2b91fc0cc 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1082,9 +1082,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
{
u32 command, temp = 0;
struct usb_hcd *hcd = xhci_to_hcd(xhci);
+ struct xhci_segment *seg;
int retval = 0;
bool pending_portevent = false;
bool suspended_usb3_devs = false;
+ bool reset_registers = false;
if (!hcd->state)
return 0;
@@ -1103,10 +1105,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
spin_lock_irq(&xhci->lock);
- if (xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
- power_lost = true;
-
- if (!power_lost) {
+ if (power_lost || xhci->broken_suspend || xhci->quirks & XHCI_RESET_ON_RESUME) {
+ xhci_dbg(xhci, "HC state lost, performing host controller reset\n");
+ reset_registers = true;
+ } else {
+ xhci_dbg(xhci, "HC state intact, continuing without reset\n");
/*
* Some controllers might lose power during suspend, so wait
* for controller not ready bit to clear, just as in xHC init.
@@ -1144,11 +1147,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
temp = readl(&xhci->op_regs->status);
if ((temp & (STS_SRE | STS_HCE)) && !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
- power_lost = true;
+ reset_registers = true;
}
}
- if (power_lost) {
+ if (reset_registers) {
if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
!(xhci_all_ports_seen_u0(xhci))) {
timer_delete_sync(&xhci->comp_mode_recovery_timer);
@@ -1172,27 +1175,33 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
if (retval)
return retval;
- xhci_dbg(xhci, "// Disabling event ring interrupts\n");
- temp = readl(&xhci->op_regs->status);
- writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
- xhci_disable_interrupter(xhci, xhci->interrupters[0]);
+ cancel_delayed_work_sync(&xhci->cmd_timer);
+
+ /* Delete all remaining commands */
+ xhci_cleanup_command_queue(xhci);
+
+ /* Clear data which is re-initilized during runtime */
+ xhci_for_each_ring_seg(xhci->interrupters[0]->event_ring->first_seg, seg)
+ memset(seg->trbs, 0, sizeof(union xhci_trb) * TRBS_PER_SEGMENT);
+
+ for (int i = xhci->max_slots; i > 0; i--)
+ xhci_free_virt_devices_depth_first(xhci, i);
+
+ xhci_rh_bw_cleanup(xhci);
+
+ xhci->cmd_ring_reserved_trbs = 0;
+ xhci_for_each_ring_seg(xhci->cmd_ring->first_seg, seg)
+ memset(seg->trbs, 0, sizeof(union xhci_trb) * TRBS_PER_SEGMENT);
- xhci_dbg(xhci, "cleaning up memory\n");
- xhci_mem_cleanup(xhci);
xhci_debugfs_exit(xhci);
- xhci_dbg(xhci, "xhci_stop completed - status = %x\n",
- readl(&xhci->op_regs->status));
-
- /* USB core calls the PCI reinit and start functions twice:
- * first with the primary HCD, and then with the secondary HCD.
- * If we don't do the same, the host will never be started.
- */
- retval = xhci_mem_init(xhci, GFP_KERNEL);
- if (retval)
- return retval;
xhci_init(hcd);
+ /*
+ * USB core calls the PCI reinit and start functions twice:
+ * first with the primary HCD, and then with the secondary HCD.
+ * If we don't do the same, the host will never be started.
+ */
xhci_dbg(xhci, "Start the primary HCD\n");
retval = xhci_run(hcd);
if (!retval && xhci->shared_hcd) {
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index ade0198bf9ea..a76e183515b3 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1792,6 +1792,7 @@ void xhci_dbg_trace(struct xhci_hcd *xhci, void (*trace)(struct va_format *),
void xhci_mem_cleanup(struct xhci_hcd *xhci);
int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags);
void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev, int slot_id);
+void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id);
int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id, struct usb_device *udev, gfp_t flags);
int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *udev);
void xhci_copy_ep0_dequeue_into_input_ctx(struct xhci_hcd *xhci,
@@ -1803,6 +1804,7 @@ void xhci_update_tt_active_eps(struct xhci_hcd *xhci,
struct xhci_virt_device *virt_dev,
int old_active_eps);
void xhci_clear_endpoint_bw_info(struct xhci_bw_info *bw_info);
+void xhci_rh_bw_cleanup(struct xhci_hcd *xhci);
void xhci_update_bw_info(struct xhci_hcd *xhci,
struct xhci_container_ctx *in_ctx,
struct xhci_input_control_ctx *ctrl_ctx,
--
2.50.1
^ permalink raw reply related
* [PATCH 8/9] usb: xhci: improve debug messages during suspend
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin
In-Reply-To: <20260327123441.806564-1-niklas.neronin@linux.intel.com>
Improve debug output for suspend failures, particularly when the controller
handshake does not complete. This will become important as upcoming patches
significantly rework the resume path, making more detailed suspend-side
messages valuable for debugging.
Add an explicit check of the Save/Restore Error (SRE) flag after a
successful Save State (CSS) operation. The xHCI specification
(note in section 4.23.2) states:
"After a Save or Restore State operation completes, the
Save/Restore Error (SRE) flag in USBSTS should be checked to
ensure the operation completed successfully."
Currently, the SRE error is only observed and warning is printed.
This patch does not introduce deeper error handling, as the correct
response is unclear and changes to suspend behavior may risk regressions
once the resume path is updated.
Additionally, simplify and clean up the suspend USBSTS CSS/SSS
handling code, improving readability and quirk handling for AMD
SNPS xHC controllers that occasionally do not clear the SSS bit.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci.c | 65 +++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 658419eb6827..232e6143ac4b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -957,11 +957,11 @@ static bool xhci_pending_portevent(struct xhci_hcd *xhci)
*/
int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
{
- int rc = 0;
+ int err;
unsigned int delay = XHCI_MAX_HALT_USEC * 2;
struct usb_hcd *hcd = xhci_to_hcd(xhci);
u32 command;
- u32 res;
+ u32 usbsts;
if (!hcd->state)
return 0;
@@ -1007,11 +1007,10 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
/* Some chips from Fresco Logic need an extraordinary delay */
delay *= (xhci->quirks & XHCI_SLOW_SUSPEND) ? 10 : 1;
- if (xhci_handshake(&xhci->op_regs->status,
- STS_HALT, STS_HALT, delay)) {
- xhci_warn(xhci, "WARN: xHC CMD_RUN timeout\n");
- spin_unlock_irq(&xhci->lock);
- return -ETIMEDOUT;
+ err = xhci_handshake(&xhci->op_regs->status, STS_HALT, STS_HALT, delay);
+ if (err) {
+ xhci_warn(xhci, "Clearing Run/Stop bit failed %d\n", err);
+ goto handshake_error;
}
xhci_clear_command_ring(xhci);
@@ -1022,28 +1021,34 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
command = readl(&xhci->op_regs->command);
command |= CMD_CSS;
writel(command, &xhci->op_regs->command);
+
+ err = xhci_handshake(&xhci->op_regs->status, STS_SAVE, 0, 20 * USEC_PER_MSEC);
+ usbsts = readl(&xhci->op_regs->status);
xhci->broken_suspend = 0;
- if (xhci_handshake(&xhci->op_regs->status,
- STS_SAVE, 0, 20 * 1000)) {
- /*
- * AMD SNPS xHC 3.0 occasionally does not clear the
- * SSS bit of USBSTS and when driver tries to poll
- * to see if the xHC clears BIT(8) which never happens
- * and driver assumes that controller is not responding
- * and times out. To workaround this, its good to check
- * if SRE and HCE bits are not set (as per xhci
- * Section 5.4.2) and bypass the timeout.
- */
- res = readl(&xhci->op_regs->status);
- if ((xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND) &&
- (((res & STS_SRE) == 0) &&
- ((res & STS_HCE) == 0))) {
- xhci->broken_suspend = 1;
- } else {
- xhci_warn(xhci, "WARN: xHC save state timeout\n");
- spin_unlock_irq(&xhci->lock);
- return -ETIMEDOUT;
+ if (err) {
+ /*
+ * AMD SNPS xHC 3.0 occasionally does not clear the
+ * SSS bit of USBSTS and when driver tries to poll
+ * to see if the xHC clears BIT(8) which never happens
+ * and driver assumes that controller is not responding
+ * and times out. To workaround this, its good to check
+ * if SRE and HCE bits are not set (as per xhci
+ * Section 5.4.2) and bypass the timeout.
+ */
+ if (!(xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND)) {
+ xhci_warn(xhci, "Controller Save State failed %d\n", err);
+ goto handshake_error;
}
+
+ if (usbsts & (STS_SRE | STS_HCE)) {
+ xhci_warn(xhci, "Controller Save State failed, USBSTS 0x%08x\n", usbsts);
+ goto handshake_error;
+ }
+
+ xhci_dbg(xhci, "SNPS broken suspend, save state unreliable\n");
+ xhci->broken_suspend = 1;
+ } else if (usbsts & STS_SRE) {
+ xhci_warn(xhci, "Suspend Save Error (SRE), USBSTS 0x%08x\n", usbsts);
}
spin_unlock_irq(&xhci->lock);
@@ -1059,7 +1064,11 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
__func__);
}
- return rc;
+ return 0;
+
+handshake_error:
+ spin_unlock_irq(&xhci->lock);
+ return -ETIMEDOUT;
}
EXPORT_SYMBOL_GPL(xhci_suspend);
--
2.50.1
^ permalink raw reply related
* [PATCH 7/9] usb: xhci: split core allocation and initialization
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin
In-Reply-To: <20260327123441.806564-1-niklas.neronin@linux.intel.com>
Separate allocation and initialization in the xHCI core:
* xhci_mem_init() now only handles memory allocation.
* xhci_init() now only handles initialization.
This split allows xhci_init() to be reused when resuming from S4
suspend-to-disk.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-mem.c | 3 +++
drivers/usb/host/xhci.c | 30 ++++++++++--------------------
2 files changed, 13 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 2cd6111c9707..f1b4f06d4b8b 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2421,6 +2421,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
dma_addr_t dma;
+ xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
+
/*
* xHCI section 5.4.6 - Device Context array must be
* "physically contiguous and 64-byte (cache line) aligned".
@@ -2510,6 +2512,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
if (xhci_setup_port_arrays(xhci, flags))
goto fail;
+ xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Finished %s", __func__);
return 0;
fail:
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 4e811a2668e6..658419eb6827 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -536,24 +536,13 @@ static void xhci_set_dev_notifications(struct xhci_hcd *xhci)
writel(dev_notf, &xhci->op_regs->dev_notification);
}
-/*
- * Initialize memory for HCD and xHC (one-time init).
- *
- * Program the PAGESIZE register, initialize the device context array, create
- * device contexts (?), set up a command ring segment (or two?), create event
- * ring (one for now).
- */
-static int xhci_init(struct usb_hcd *hcd)
+/* Setup basic xHCI registers */
+static void xhci_init(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- int retval;
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
- retval = xhci_mem_init(xhci, GFP_KERNEL);
- if (retval)
- return retval;
-
/* Set the Number of Device Slots Enabled to the maximum supported value */
xhci_enable_max_dev_slots(xhci);
@@ -589,7 +578,6 @@ static int xhci_init(struct usb_hcd *hcd)
}
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Finished %s", __func__);
- return 0;
}
/*-------------------------------------------------------------------------*/
@@ -1190,11 +1178,12 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
* first with the primary HCD, and then with the secondary HCD.
* If we don't do the same, the host will never be started.
*/
- xhci_dbg(xhci, "Initialize the xhci_hcd\n");
- retval = xhci_init(hcd);
+ retval = xhci_mem_init(xhci, GFP_KERNEL);
if (retval)
return retval;
+ xhci_init(hcd);
+
xhci_dbg(xhci, "Start the primary HCD\n");
retval = xhci_run(hcd);
if (!retval && xhci->shared_hcd) {
@@ -5526,12 +5515,13 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
- xhci_dbg(xhci, "Calling HCD init\n");
- /* Initialize HCD and host controller data structures. */
- retval = xhci_init(hcd);
+ /* Allocate xHCI data structures */
+ retval = xhci_mem_init(xhci, GFP_KERNEL);
if (retval)
return retval;
- xhci_dbg(xhci, "Called HCD init\n");
+
+ /* Initialize HCD and host controller data structures */
+ xhci_init(hcd);
if (xhci_hcd_is_usb3(hcd))
xhci_hcd_init_usb3_data(xhci, hcd);
--
2.50.1
^ permalink raw reply related
* [PATCH 5/9] usb: xhci: move ring initialization
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin
In-Reply-To: <20260327123441.806564-1-niklas.neronin@linux.intel.com>
Move ring initialization from xhci_ring_alloc() to xhci_ring_init().
Call xhci_ring_init() after xhci_ring_alloc(); in the future,
it can also be used to re-initialize the ring during resume.
Additionally, remove xhci_dbg_trace() from xhci_mem_init(). The command
ring's first segment DMA address is now printed during the trace call in
xhci_ring_init().
This refactoring lays also the groundwork for eventually replacing:
* xhci_dbc_ring_init()
* xhci_clear_command_ring()
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-mem.c | 19 ++++++++++++++-----
drivers/usb/host/xhci.c | 3 +++
drivers/usb/host/xhci.h | 1 +
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 45638ab13635..ca4463eebc49 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -129,6 +129,13 @@ static void xhci_initialize_ring_segments(struct xhci_hcd *xhci, struct xhci_rin
ring->last_seg->trbs[TRBS_PER_SEGMENT - 1].link.control |= cpu_to_le32(LINK_TOGGLE);
}
+void xhci_ring_init(struct xhci_hcd *xhci, struct xhci_ring *ring)
+{
+ xhci_initialize_ring_segments(xhci, ring);
+ xhci_initialize_ring_info(ring);
+ trace_xhci_ring_alloc(ring);
+}
+
/*
* Link the src ring segments to the dst ring.
* Set Toggle Cycle for the new ring if needed.
@@ -389,9 +396,6 @@ struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci, unsigned int num_segs,
if (ret)
goto fail;
- xhci_initialize_ring_segments(xhci, ring);
- xhci_initialize_ring_info(ring);
- trace_xhci_ring_alloc(ring);
return ring;
fail:
@@ -668,6 +672,8 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
cur_ring = stream_info->stream_rings[cur_stream];
if (!cur_ring)
goto cleanup_rings;
+
+ xhci_ring_init(xhci, cur_ring);
cur_ring->stream_id = cur_stream;
cur_ring->trb_address_map = &stream_info->trb_address_map;
/* Set deq ptr, cycle bit, and stream context type */
@@ -1011,6 +1017,8 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
if (!dev->eps[0].ring)
goto fail;
+ xhci_ring_init(xhci, dev->eps[0].ring);
+
dev->udev = udev;
/* Point to output device context in dcbaa. */
@@ -1492,6 +1500,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
virt_dev->eps[ep_index].skip = false;
ep_ring = virt_dev->eps[ep_index].new_ring;
+ xhci_ring_init(xhci, ep_ring);
/* Fill the endpoint context */
ep_ctx->ep_info = cpu_to_le32(EP_MAX_ESIT_PAYLOAD_HI(max_esit_payload) |
@@ -2370,6 +2379,8 @@ xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs,
if (!ir)
return NULL;
+ xhci_ring_init(xhci, ir->event_ring);
+
spin_lock_irq(&xhci->lock);
if (!intr_num) {
/* Find available secondary interrupter, interrupter 0 is reserved for primary */
@@ -2482,8 +2493,6 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
goto fail;
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Allocated command ring at %p", xhci->cmd_ring);
- xhci_dbg_trace(xhci, trace_xhci_dbg_init, "First segment DMA is 0x%pad",
- &xhci->cmd_ring->first_seg->dma);
/* Allocate and set up primary interrupter 0 with an event ring. */
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Allocating primary event ring");
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index facadf0f0d1e..170615dd1e93 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -564,6 +564,8 @@ static int xhci_init(struct usb_hcd *hcd)
/* Set the Number of Device Slots Enabled to the maximum supported value */
xhci_enable_max_dev_slots(xhci);
+ /* Initialize the Command ring */
+ xhci_ring_init(xhci, xhci->cmd_ring);
/*
* Reserve one command ring TRB for disabling LPM.
* Since the USB core grabs the shared usb_bus bandwidth mutex before
@@ -583,6 +585,7 @@ static int xhci_init(struct usb_hcd *hcd)
xhci_set_dev_notifications(xhci);
/* Initialize the Primary interrupter */
+ xhci_ring_init(xhci, xhci->interrupters[0]->event_ring);
xhci_add_interrupter(xhci, 0);
xhci->interrupters[0]->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2b0796f6d00e..ade0198bf9ea 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1823,6 +1823,7 @@ void xhci_ring_free(struct xhci_hcd *xhci, struct xhci_ring *ring);
int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
unsigned int num_trbs, gfp_t flags);
void xhci_initialize_ring_info(struct xhci_ring *ring);
+void xhci_ring_init(struct xhci_hcd *xhci, struct xhci_ring *ring);
void xhci_free_endpoint_ring(struct xhci_hcd *xhci,
struct xhci_virt_device *virt_dev,
unsigned int ep_index);
--
2.50.1
^ permalink raw reply related
* [PATCH 6/9] usb: xhci: move initialization for lifetime objects
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin
In-Reply-To: <20260327123441.806564-1-niklas.neronin@linux.intel.com>
Initialize objects that exist for the lifetime of the driver only once,
rather than repeatedly. These objects do not require re-initialization
after events such as S4 (suspend-to-disk).
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-mem.c | 1 -
drivers/usb/host/xhci.c | 15 ++++++++-------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index ca4463eebc49..2cd6111c9707 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2009,7 +2009,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
xhci->port_caps = NULL;
xhci->interrupters = NULL;
- xhci->page_size = 0;
xhci->usb2_rhub.bus_state.bus_suspended = 0;
xhci->usb3_rhub.bus_state.bus_suspended = 0;
}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 170615dd1e93..4e811a2668e6 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -549,13 +549,6 @@ static int xhci_init(struct usb_hcd *hcd)
int retval;
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
- spin_lock_init(&xhci->lock);
-
- INIT_LIST_HEAD(&xhci->cmd_list);
- INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
- init_completion(&xhci->cmd_ring_stop_completion);
- xhci_hcd_page_size(xhci);
- memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
retval = xhci_mem_init(xhci, GFP_KERNEL);
if (retval)
@@ -5525,6 +5518,14 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
}
+ spin_lock_init(&xhci->lock);
+ INIT_LIST_HEAD(&xhci->cmd_list);
+ INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
+ init_completion(&xhci->cmd_ring_stop_completion);
+ xhci_hcd_page_size(xhci);
+
+ memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
+
xhci_dbg(xhci, "Calling HCD init\n");
/* Initialize HCD and host controller data structures. */
retval = xhci_init(hcd);
--
2.50.1
^ permalink raw reply related
* [PATCH 4/9] usb: xhci: move reserving command ring trb
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin
In-Reply-To: <20260327123441.806564-1-niklas.neronin@linux.intel.com>
Move the command ring TRB reservation from xhci_mem_init() to xhci_init().
Function xhci_mem_init() is intended for memory allocation,
while xhci_init() is for initialization.
This split allows xhci_init() to be reused when resuming from S4
suspend-to-disk.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-mem.c | 7 -------
drivers/usb/host/xhci.c | 6 ++++++
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index d4a9dbed8f16..45638ab13635 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2485,13 +2485,6 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "First segment DMA is 0x%pad",
&xhci->cmd_ring->first_seg->dma);
- /*
- * Reserve one command ring TRB for disabling LPM.
- * Since the USB core grabs the shared usb_bus bandwidth mutex before
- * disabling LPM, we only need to reserve one TRB for all devices.
- */
- xhci->cmd_ring_reserved_trbs++;
-
/* Allocate and set up primary interrupter 0 with an event ring. */
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Allocating primary event ring");
xhci->interrupters = kcalloc_node(xhci->max_interrupters, sizeof(*xhci->interrupters),
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a04b1365bb6a..facadf0f0d1e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -564,6 +564,12 @@ static int xhci_init(struct usb_hcd *hcd)
/* Set the Number of Device Slots Enabled to the maximum supported value */
xhci_enable_max_dev_slots(xhci);
+ /*
+ * Reserve one command ring TRB for disabling LPM.
+ * Since the USB core grabs the shared usb_bus bandwidth mutex before
+ * disabling LPM, we only need to reserve one TRB for all devices.
+ */
+ xhci->cmd_ring_reserved_trbs = 1;
/* Set the address in the Command Ring Control register */
xhci_set_cmd_ring_deq(xhci);
--
2.50.1
^ permalink raw reply related
* [PATCH 3/9] usb: xhci: factor out roothub bandwidth cleanup
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin
In-Reply-To: <20260327123441.806564-1-niklas.neronin@linux.intel.com>
Introduce xhci_rh_bw_cleanup() to release all bandwidth tracking
structures associated with xHCI roothub ports.
The new helper clears:
* TT bandwidth entries
* Per-interval endpoint lists
This refactors and consolidates the existing per-port cleanup logic
previously embedded in xhci_mem_cleanup(), reducing duplication and
making the teardown sequence easier to follow.
The helper will also be reused for upcoming S4 resume handling.
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci-mem.c | 50 +++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 75bc1eb98b76..d4a9dbed8f16 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1895,10 +1895,36 @@ void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
}
EXPORT_SYMBOL_GPL(xhci_remove_secondary_interrupter);
+/* Cleanup roothub bandwidth data */
+static void xhci_rh_bw_cleanup(struct xhci_hcd *xhci)
+{
+ struct xhci_root_port_bw_info *rh_bw;
+ struct xhci_tt_bw_info *tt_info, *tt_next;
+ struct list_head *eps, *ep, *ep_next;
+
+ for (int i = 0; i < xhci->max_ports; i++) {
+ rh_bw = &xhci->rh_bw[i];
+
+ /* Clear and free all TT bandwidth entries */
+ list_for_each_entry_safe(tt_info, tt_next, &rh_bw->tts, tt_list) {
+ list_del(&tt_info->tt_list);
+ kfree(tt_info);
+ }
+
+ /* Clear per-interval endpoint lists */
+ for (int j = 0; j < XHCI_MAX_INTERVAL; j++) {
+ eps = &rh_bw->bw_table.interval_bw[j].endpoints;
+
+ list_for_each_safe(ep, ep_next, eps)
+ list_del_init(ep);
+ }
+ }
+}
+
void xhci_mem_cleanup(struct xhci_hcd *xhci)
{
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
- int i, j;
+ int i;
cancel_delayed_work_sync(&xhci->cmd_timer);
@@ -1917,15 +1943,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed command ring");
xhci_cleanup_command_queue(xhci);
- for (i = 0; i < xhci->max_ports && xhci->rh_bw; i++) {
- struct xhci_interval_bw_table *bwt = &xhci->rh_bw[i].bw_table;
- for (j = 0; j < XHCI_MAX_INTERVAL; j++) {
- struct list_head *ep = &bwt->interval_bw[j].endpoints;
- while (!list_empty(ep))
- list_del_init(ep->next);
- }
- }
-
for (i = xhci->max_slots; i > 0; i--)
xhci_free_virt_devices_depth_first(xhci, i);
@@ -1959,18 +1976,9 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
scratchpad_free(xhci);
- if (!xhci->rh_bw)
- goto no_bw;
+ if (xhci->rh_bw)
+ xhci_rh_bw_cleanup(xhci);
- for (i = 0; i < xhci->max_ports; i++) {
- struct xhci_tt_bw_info *tt, *n;
- list_for_each_entry_safe(tt, n, &xhci->rh_bw[i].tts, tt_list) {
- list_del(&tt->tt_list);
- kfree(tt);
- }
- }
-
-no_bw:
xhci->cmd_ring_reserved_trbs = 0;
xhci->usb2_rhub.num_ports = 0;
xhci->usb3_rhub.num_ports = 0;
--
2.50.1
^ permalink raw reply related
* [PATCH 2/9] usb: xhci: relocate Restore/Controller error check
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman
Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin, Andy Shevchenko
In-Reply-To: <20260327123441.806564-1-niklas.neronin@linux.intel.com>
A Restore Error or Host Controller Error indicates that the host controller
failed to resume after suspend. In such cases, the xhci driver is fully
re-initialized, similar to a post-hibernation scenario.
The existing error check is only relevant when 'power_lost' is false.
If 'power_lost' is true, a Restore or Controller error has no effect:
no warning is printed and the 'power_lost' state remains unchanged.
Move the entire error check into the if '!power_lost' condition
to make this dependency explicit and simplify the resume logic.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 810905b824d3..a04b1365bb6a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1140,16 +1140,13 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
spin_unlock_irq(&xhci->lock);
return -ETIMEDOUT;
}
- }
- temp = readl(&xhci->op_regs->status);
-
- /* re-initialize the HC on Restore Error, or Host Controller Error */
- if ((temp & (STS_SRE | STS_HCE)) &&
- !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
- if (!power_lost)
+ /* re-initialize the HC on Restore Error, or Host Controller Error */
+ temp = readl(&xhci->op_regs->status);
+ if ((temp & (STS_SRE | STS_HCE)) && !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
- power_lost = true;
+ power_lost = true;
+ }
}
if (power_lost) {
--
2.50.1
^ permalink raw reply related
* [PATCH 1/9] usb: xhci: simplify CMRT initialization logic
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman
Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin, Andy Shevchenko
In-Reply-To: <20260327123441.806564-1-niklas.neronin@linux.intel.com>
The function compliance_mode_recovery_timer_init() is called from
xhci_init() because the Compliance Mode Recovery Timer (CMRT) must be set
up before xhci_run() when the xhci driver is re-initialized.
To handle this case, the boolean flag 'comp_timer_running' was introduced
to track whether xhci_run() had already been called, ensuring that
xhci_resume() would not invoke compliance_mode_recovery_timer_init()
a second time.
This can be simplified by moving the 'done' label in xhci_resume() to
after the compliance_mode_recovery_timer_init() call. With this change,
the timer initialization runs only when the xhci driver has not been
re-initialized, making the 'comp_timer_running' flag unnecessary and
allowing it to be removed.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
drivers/usb/host/xhci.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ef6d8662adec..810905b824d3 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1084,7 +1084,6 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
u32 command, temp = 0;
struct usb_hcd *hcd = xhci_to_hcd(xhci);
int retval = 0;
- bool comp_timer_running = false;
bool pending_portevent = false;
bool suspended_usb3_devs = false;
@@ -1196,7 +1195,6 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
retval = xhci_init(hcd);
if (retval)
return retval;
- comp_timer_running = true;
xhci_dbg(xhci, "Start the primary HCD\n");
retval = xhci_run(hcd);
@@ -1265,16 +1263,16 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
usb_hcd_resume_root_hub(hcd);
}
}
-done:
+
/*
* If system is subject to the Quirk, Compliance Mode Timer needs to
* be re-initialized Always after a system resume. Ports are subject
* to suffer the Compliance Mode issue again. It doesn't matter if
* ports have entered previously to U0 before system's suspension.
*/
- if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && !comp_timer_running)
+ if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
compliance_mode_recovery_timer_init(xhci);
-
+done:
if (xhci->quirks & XHCI_ASMEDIA_MODIFY_FLOWCONTROL)
usb_asmedia_modifyflowcontrol(to_pci_dev(hcd->self.controller));
--
2.50.1
^ permalink raw reply related
* [PATCH 0/9] xhci: usb: optimize resuming from S4 (suspend-to-disk)
From: Niklas Neronin @ 2026-03-27 12:34 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb, raoxu, michal.pecio, Niklas Neronin
On resume from S4, the xHCI controller loses register state, but the
driver currently responds by tearing down and fully reinitializing all
xhci data structures.
This is unnecessary. Instead of freeing and reallocating driver structures,
simply reset the required memory and restore the hardware registers that
lost their contents across hibernation.
Changes since RFCv2:
* Fix virtual device itteration from max-ports to slots.
Niklas Neronin (9):
usb: xhci: simplify CMRT initialization logic
usb: xhci: relocate Restore/Controller error check
usb: xhci: factor out roothub bandwidth cleanup
usb: xhci: move reserving command ring trb
usb: xhci: move ring initialization
usb: xhci: move initialization for lifetime objects
usb: xhci: split core allocation and initialization
usb: xhci: improve debug messages during suspend
usb: xhci: optimize resuming from S4 (suspend-to-disk)
drivers/usb/host/xhci-mem.c | 82 +++++++++-------
drivers/usb/host/xhci.c | 183 +++++++++++++++++++-----------------
drivers/usb/host/xhci.h | 3 +
3 files changed, 148 insertions(+), 120 deletions(-)
--
2.50.1
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Andy Shevchenko @ 2026-03-27 12:34 UTC (permalink / raw)
To: Catalin Marinas
Cc: Mikhail Gavrilov, vbabka, harry.yoo, akpm, hao.li, cl, rientjes,
roman.gushchin, linux-mm, linux-kernel, linux-usb, stern, linux,
hch, Jeff.kirsher, Marek Szyprowski, Robin Murphy
In-Reply-To: <acZ3ZUXhFHpSXzYS@arm.com>
On Fri, Mar 27, 2026 at 2:26 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Mar 27, 2026 at 10:58:46AM +0500, Mikhail Gavrilov wrote:
> TL;DR: I think this is fine:
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>
> I'm not sure that's the best way to hide the warning but there
> are no great solutions either. On one hand, we want the DMA debug to
> capture potential problems on architectures it's not running on. OTOH,
> we also want to avoid false positives on coherent architectures/devices.
> I don't think reconciling the two requirements is easy.
>
> When DMA_API_DEBUG is enabled, the above will change the x86 behaviour
> that could have implications beyond DMA (e.g. may not catch some buffer
> overflow because it's within L1_CACHE_BYTES). Similarly for non-coherent
> architectures that select DMA_BOUNCE_UNALIGNED_KMALLOC (arm64 and riscv
> currently). arm64 defines ARCH_DMA_MINALIGN to 128 but
> ARCH_KMALLOC_MINALIGN to 8 (why 128 is larger than L1_CACHE_BYTES is
> another matter but let's ignore it for now).
Maybe for the cases where we do not warn we should introduce a
dev_dbg_/pr_debug_once()? At least users may be informed about potential issues.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Catalin Marinas @ 2026-03-27 12:26 UTC (permalink / raw)
To: Mikhail Gavrilov
Cc: vbabka, harry.yoo, akpm, hao.li, cl, rientjes, roman.gushchin,
linux-mm, linux-kernel, linux-usb, stern, linux, andy.shevchenko,
hch, Jeff.kirsher, Marek Szyprowski, Robin Murphy
In-Reply-To: <20260327055846.248829-1-mikhail.v.gavrilov@gmail.com>
+ Marek, Robin
On Fri, Mar 27, 2026 at 10:58:46AM +0500, Mikhail Gavrilov wrote:
> When CONFIG_DMA_API_DEBUG is enabled, the DMA debug infrastructure
> tracks active mappings per cacheline and warns if two different DMA
> mappings share the same cacheline ("cacheline tracking EEXIST,
> overlapping mappings aren't supported").
>
> On x86_64, ARCH_KMALLOC_MINALIGN defaults to 8, so small kmalloc
> allocations (e.g. the 8-byte hub->buffer and hub->status in the USB
> hub driver) frequently land in the same 64-byte cacheline. When both
> are DMA-mapped, this triggers a false positive warning.
>
> This has been reported repeatedly since v5.14 (when the EEXIST check
> was added) across various USB host controllers and devices including
> xhci_hcd with USB hubs, USB audio devices, and USB ethernet adapters.
This indeed has come up regularly in the past years.
> +/*
> + * Align memory allocations to cache lines if DMA API debugging is active
> + * to avoid false positive DMA overlapping error messages.
> + */
> +#ifdef CONFIG_DMA_API_DEBUG
> +#ifndef ARCH_KMALLOC_MINALIGN
> +#define ARCH_KMALLOC_MINALIGN L1_CACHE_BYTES
> +#elif ARCH_KMALLOC_MINALIGN < L1_CACHE_BYTES
> +#undef ARCH_KMALLOC_MINALIGN
> +#define ARCH_KMALLOC_MINALIGN L1_CACHE_BYTES
> +#endif
> +#endif
TL;DR: I think this is fine:
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
I'm not sure that's the best way to hide the warning but there
are no great solutions either. On one hand, we want the DMA debug to
capture potential problems on architectures it's not running on. OTOH,
we also want to avoid false positives on coherent architectures/devices.
I don't think reconciling the two requirements is easy.
When DMA_API_DEBUG is enabled, the above will change the x86 behaviour
that could have implications beyond DMA (e.g. may not catch some buffer
overflow because it's within L1_CACHE_BYTES). Similarly for non-coherent
architectures that select DMA_BOUNCE_UNALIGNED_KMALLOC (arm64 and riscv
currently). arm64 defines ARCH_DMA_MINALIGN to 128 but
ARCH_KMALLOC_MINALIGN to 8 (why 128 is larger than L1_CACHE_BYTES is
another matter but let's ignore it for now).
More of a thinking out loud, we have:
1. Coherent architectures - alignment doesn't matter
2. Non-coherent architectures with:
a) Sufficiently large ARCH_KMALLOC_MINALIGN
b) Small ARCH_KMALLOC_MINALIGN but DMA_BOUNCE_UNALIGNED_KMALLOC
c) Broken config - forgot to set ARCH_DMA_MINALIGN or bouncing
We can ignore (2.c), the aim of the DMA debug is to catch wrong uses in
drivers. If drivers is the only goal, the above change will do when
running on (1) or (2.a) hardware - it will catch sub-L1_CACHE_BYTES
buffers from drivers while assuming kmalloc() machinery is safe.
However, if running on (2.b) it won't catch anything that may be
problematic on (2.a) since the DMA debug ignores the overlap.
We could make DMA_BOUNCE_UNALIGNED_KMALLOC dependent on !DMA_API_DEBUG
but it would be nice to be able to sanity-check the bouncing logic.
Well, it wasn't checking it before and with commit 03521c892bb8
("dma-debug: don't report false positives with
DMA_BOUNCE_UNALIGNED_KMALLOC"), we made this clear that overlapping will
be ignored.
Irrespective of whether we disable bouncing with DMA_API_DEBUG, maybe we
could replace the above commit with:
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 3928a509c44c..488045ef6245 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -175,7 +175,7 @@ dma_addr_t dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
if (!is_mmio)
kmsan_handle_dma(phys, size, dir);
trace_dma_map_phys(dev, phys, addr, size, dir, attrs);
- debug_dma_map_phys(dev, phys, size, dir, addr, attrs);
+ debug_dma_map_phys(dev, dma_to_phys(addr), size, dir, addr, attrs);
return addr;
}
Anyway, this I think is unrelated to the proposed change affecting x86,
more of a how to make the DMA API debugging more useful when running on
arm64 or riscv.
--
Catalin
^ permalink raw reply related
* Re: [PATCH v5 phy-next 10/27] scsi: ufs: qcom: keep parallel track of PHY power state
From: Vladimir Oltean @ 2026-03-27 11:28 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: linux-phy, Vinod Koul, Neil Armstrong, dri-devel, freedreno,
linux-arm-kernel, linux-arm-msm, linux-can, linux-gpio, linux-ide,
linux-kernel, linux-media, linux-pci, linux-renesas-soc,
linux-riscv, linux-rockchip, linux-samsung-soc, linux-scsi,
linux-sunxi, linux-tegra, linux-usb, netdev, spacemit,
UNGLinuxDriver, James E.J. Bottomley, Martin K. Petersen,
Nitin Rawat
In-Reply-To: <gq4sswslkjaoe5hhxe2mz6z57uiumotqknkryadvfsstj4srx4@qgenqekgrqv4>
[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]
On Fri, Mar 27, 2026 at 12:22:46PM +0530, Manivannan Sadhasivam wrote:
> I tested the patch. But it fails ufs_qcom_power_up_sequence() if PHY was already
> powered on:
>
> [ 31.513321] qcom-qmp-ufs-phy 1d87000.phy: phy initialization timed-out
> [ 31.513335] ufshcd-qcom 1d84000.ufshc: Failed to calibrate PHY: -110
> [ 31.565273] ufshcd-qcom 1d84000.ufshc: Enabling the controller failed
>
> Funny thing is, it didn't affect the functionality since the UFS core retries
> ufshcd_hba_enable() and in the error path of ufs_qcom_power_up_sequence(),
> phy_power_off() gets called and that causes the next try to succeed. So it is
> evident that, if PHY was already powered ON, it should be powered off before
> ufs_qcom_phy_power_on(). And due to the UFS driver design,
> ufs_qcom_power_up_sequence() can get called multiple times. So we cannot just
> remove phy_power_off().
>
> Below diff on top of your patch fixes the issue:
>
> ```
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index ed067247d72a..2c9fe03f349e 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -567,6 +567,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> if (ret)
> return ret;
>
> + ufs_qcom_phy_power_off(host);
> +
> ret = ufs_qcom_phy_set_gear(host, mode);
> if (ret) {
> dev_err(hba->dev, "%s: phy_set_mode_ext() failed, ret = %d\n",
> ```
>
> - Mani
Understood. Thanks for testing.
I'm still not satisfied with this level of complexity. If I get you
right, ufs_qcom_phy_power_off() is still needed because phy_calibrate()
expects a "fresh after power on" state, otherwise it fails? That would
be the second reason, apart from the first one I already identified
(undo a phy_power_on() done prior to phy_init()).
If so, could you please test the 3 patches attached (no relationship
with anything else we've exchanged thus far)?
[-- Attachment #2: 0001-phy-qcom-qmp-ufs-support-dynamic-gear-changing.patch --]
[-- Type: text/x-diff, Size: 2013 bytes --]
From 2d42c2d40e6ddfd0c73fc39601f93f7b81a42401 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 27 Mar 2026 12:41:00 +0200
Subject: [PATCH 1/3] phy: qcom-qmp-ufs: support dynamic gear changing
Currently, phy_set_mode_ext() on the QMP UFS PHY expects the PHY to be
powered down, and it makes no change to the hardware state, instead
phy_power_on() followed by phy_calibrate() must be run afterwards.
"Order of API calls" from Documentation/driver-api/phy/phy.rst has a
roundabout and not really clear way of saying that both calling
sequences should be supported. This was further discussed here,
documentation is pending an update:
https://lore.kernel.org/linux-phy/E1vo0mF-00000007kbg-1OeA@rmk-PC.armlinux.org.uk/
By absorbing the phy_power_off() -> ... -> phy_power_on() ->
phy_configure() surrounding sequence into phy_set_mode_ext(), consumer
drivers can be greatly simplified, and we also have a proper
self-standing phy_set_mode_ext() implementation which does not rely on
other calls to do its job.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index df138a5442eb..e75b059bf246 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -2004,15 +2004,24 @@ static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
{
struct qmp_ufs *qmp = phy_get_drvdata(phy);
const struct qmp_phy_cfg *cfg = qmp->cfg;
+ bool powered_on = phy->power_count;
if (submode > cfg->max_supported_gear || submode == 0) {
dev_err(qmp->dev, "Invalid PHY submode %d\n", submode);
return -EINVAL;
}
+ if (powered_on)
+ qmp_ufs_power_off(phy);
+
qmp->mode = mode;
qmp->submode = submode;
+ if (powered_on) {
+ qmp_ufs_power_on(phy);
+ return qmp_ufs_phy_calibrate(phy);
+ }
+
return 0;
}
--
2.34.1
[-- Attachment #3: 0002-scsi-ufs-qcom-call-phy_init-before-phy_power_on.patch --]
[-- Type: text/x-diff, Size: 3707 bytes --]
From 8d156781d38597865da37a86417f553143d74eaa Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 27 Mar 2026 13:14:39 +0200
Subject: [PATCH 2/3] scsi: ufs: qcom: call phy_init() before phy_power_on()
The Qualcomm UFS host controller driver violates the Generic PHY API
expectation, documented in section "Order of API calls" from
Documentation/driver-api/phy/phy.rst, and then tries to hide it.
The expectation is that calls must be made in the phy_init() ->
phy_power_on() -> phy_power_off() -> phy_exit() sequence.
What we actually have is:
ufshcd_init()
-> ufshcd_hba_init()
-> ufshcd_setup_clocks(hba, true)
-> ufshcd_vops_setup_clocks(hba, true, POST_CHANGE)
-> ufs_qcom_setup_clocks(hba, true, POST_CHANGE)
-> phy_power_on(phy)
-> ufshcd_variant_hba_init()
-> ufs_qcom_init()
-> ufs_qcom_setup_clocks(hba, true, POST_CHANGE)
-> phy_power_on(phy)
-> ufshcd_hba_enable()
-> ufshcd_vops_hce_enable_notify()
-> ufs_qcom_hce_enable_notify()
-> ufs_qcom_power_up_sequence()
-> if (phy->power_count) phy_power_off(phy)
-> phy_init(phy)
This "works" because the way that the "phy_power_on was called before
phy_init\n" condition is detected in phy-core.c is if the power_count is
positive at the phy_init() call time.
By having that "if (phy->power_count) phy_power_off(phy)" logic, the
ufs-qcom.c technically sidesteps the test, but actually violates the
Generic PHY API even more (calls phy_power_on() *and* phy_power_off()
before phy_init()).
The reason why I stumbled upon this was that I was trying to remove
dereferences of phy->power_count from drivers. This is a PHY-internal
field, and using it from drivers is highly likely to be incorrect, as
this case showcases rather well.
As commit 77d2fa54a945 ("scsi: ufs: qcom : Refactor phy_power_on/off
calls") shows, this driver tries to couple the PHY power state with the
HBA clocks, for power saving reasons. I won't try to change that, I will
just move the phy_init() call earlier, to ufs_qcom_init().
After the phy_init() movement, ufs_qcom_power_up_sequence() should no
longer need to do either phy_init() nor the conditional phy_power_off().
However, phy_power_off() is still needed, for a separate reason which
will be dealt with separately.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Nitin Rawat <quic_nitirawa@quicinc.com>
v5->v6: rewrite after actually understanding the core issue
v4->v5: patch is new
---
drivers/ufs/host/ufs-qcom.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 375fd24ba458..ffa70c6c7143 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -513,13 +513,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
/* phy initialization - calibrate the phy */
- ret = phy_init(phy);
- if (ret) {
- dev_err(hba->dev, "%s: phy init failed, ret = %d\n",
- __func__, ret);
- return ret;
- }
-
ret = phy_set_mode_ext(phy, mode, host->phy_gear);
if (ret)
goto out_disable_phy;
@@ -1441,6 +1434,13 @@ static int ufs_qcom_init(struct ufs_hba *hba)
if (err)
goto out_variant_clear;
+ err = phy_init(host->generic_phy);
+ if (err) {
+ dev_err(hba->dev, "%s: phy_init failed, ret = %d\n",
+ __func__, err);
+ goto out_variant_clear;
+ }
+
ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
ufs_qcom_get_default_testbus_cfg(host);
--
2.34.1
[-- Attachment #4: 0003-scsi-ufs-qcom-make-use-of-QMP-PHY-dynamic-gear-switc.patch --]
[-- Type: text/x-diff, Size: 1696 bytes --]
From 88f4bdfee770cd433a940a14e318d8c8b5dfa516 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 27 Mar 2026 13:18:05 +0200
Subject: [PATCH 3/3] scsi: ufs: qcom: make use of QMP PHY dynamic gear
switching ability
The QMP UFS PHY can now tolerate having phy_set_mode_ext() being called
while the PHY is powered up. We no longer need to power it down, back up
and calibrate it.
Simplify ufs_qcom_power_up_sequence() by relying on just phy_set_mode_ext()
and let PHY power management be handled just by ufs_qcom_setup_clocks().
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/ufs/host/ufs-qcom.c | 25 +------------------------
1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index ffa70c6c7143..cf7b67f2021e 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -508,37 +508,14 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
if (ret)
return ret;
- if (phy->power_count)
- phy_power_off(phy);
-
-
/* phy initialization - calibrate the phy */
ret = phy_set_mode_ext(phy, mode, host->phy_gear);
if (ret)
- goto out_disable_phy;
-
- /* power on phy - start serdes and phy's power and clocks */
- ret = phy_power_on(phy);
- if (ret) {
- dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
- __func__, ret);
- goto out_disable_phy;
- }
-
- ret = phy_calibrate(phy);
- if (ret) {
- dev_err(hba->dev, "Failed to calibrate PHY: %d\n", ret);
- goto out_disable_phy;
- }
+ return ret;
ufs_qcom_select_unipro_mode(host);
return 0;
-
-out_disable_phy:
- phy_exit(phy);
-
- return ret;
}
/*
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 1/2] usb: chipidea: core: fix device mode not work in non-lpm
From: Xu Yang @ 2026-03-27 10:54 UTC (permalink / raw)
To: Peter Chen (CIX); +Cc: gregkh, jun.li, linux-usb, linux-kernel, imx
In-Reply-To: <acOKAeNKWT86veM+@nchen-desktop>
On Wed, Mar 25, 2026 at 03:08:49PM +0800, Peter Chen (CIX) wrote:
> On 26-03-19 17:57:15, Xu Yang wrote:
> > In current design, we expect 2 ci_irq() to handle ID and VBUS events in
> > usb role switch, like what ci_extcon_wakeup_int() does. Now we only call
> > ci_irq() once. However, this won't bring any issues in low power mode,
> > because ci_irq() just take the device out of low power mode, and then
> > ci_extcon_wakeup_int() will call ci_irq() twice. If the device is not in
> > suspend state, the device mode will not work properly because VBUS event
> > won'tbe handled (ID event has higher priority) at all.
>
> %s/won'tbe/won't be
OK.
>
> Is it possible change ci_irq_handler and handle both events?
Yes, we can change ci_irq_handler() and let it set both ci->id_event and
ci->b_sess_valid_event flags, then queue a ci_otg_work() to handle them
later.
I think this just unnecessarily call ci_irq_handler() to handle lpm/non-lpm
case as the final path is ci_otg_work() and it will handle lpm/non-lpm case
by naturally calling pm_runtime_get/put_sync(), otherwise it relies on
ci_extcon_wakeup_int() to achieve the same purpose.
Both methods work for me, may I know which one do you prefer? :)
>
> >
> > Although 2 consecutive ci_irq() can work around the issue, do not do it
> > because ci_usb_role_switch_set() may or not be in low power context which
> > make the ci_irq() purpose not unique here. Because the final processing
> > is in ci_otg_work(), just directly queue an otg work. This also refine
> > the logic for more clarity and not set changed flag.
> >
> > Fixes: e1b5d2bed67c ("usb: chipidea: core: handle usb role switch in a common way")
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Jun Li <jun.li@nxp.com>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> > drivers/usb/chipidea/core.c | 30 +++++++++++-------------------
> > 1 file changed, 11 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index fac11f20cf0a..1bd231a852a1 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -618,30 +618,22 @@ static int ci_usb_role_switch_set(struct usb_role_switch *sw,
> > struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> > struct ci_hdrc_cable *cable;
> >
> > - if (role == USB_ROLE_HOST) {
> > - cable = &ci->platdata->id_extcon;
> > - cable->changed = true;
> > + cable = &ci->platdata->id_extcon;
> > + if (role == USB_ROLE_HOST)
> > cable->connected = true;
> > - cable = &ci->platdata->vbus_extcon;
> > - cable->changed = true;
> > - cable->connected = false;
> > - } else if (role == USB_ROLE_DEVICE) {
> > - cable = &ci->platdata->id_extcon;
> > - cable->changed = true;
> > + else
> > cable->connected = false;
> > - cable = &ci->platdata->vbus_extcon;
> > - cable->changed = true;
> > +
> > + cable = &ci->platdata->vbus_extcon;
> > + if (role == USB_ROLE_DEVICE)
> > cable->connected = true;
> > - } else {
> > - cable = &ci->platdata->id_extcon;
> > - cable->changed = true;
> > - cable->connected = false;
> > - cable = &ci->platdata->vbus_extcon;
> > - cable->changed = true;
> > + else
> > cable->connected = false;
> > - }
> >
> > - ci_irq(ci);
> > + ci->id_event = true;
> > + ci->b_sess_valid_event = true;
>
> Why both ID and VBUS event are set as true unconditionally?
The main purpose is to simplify the handling of the various situations.
The usb role include below types:
- host
- device
- none.
1. host <--> none
Above change means ID change occur.
2. device <--> none
Above change means VBUS change occur.
3. host <--> device
Above change means ID and VBUS change occur.
By setting both of them as true, the logic can be simplified here and
ID and VBUS otg work will check if a real state change happen by comparing
old state and current OTGSC_ID/OTGSC_BSV bit.
Thanks,
Xu Yang
>
> --
>
> Best regards,
> Peter
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox