* 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
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Harry Yoo (Oracle) @ 2026-03-27 10:39 UTC (permalink / raw)
To: Mikhail Gavrilov
Cc: vbabka, akpm, hao.li, cl, rientjes, roman.gushchin, linux-mm,
linux-kernel, linux-usb, stern, linux, andy.shevchenko, hch,
Jeff.kirsher, Catalin Marinas
In-Reply-To: <CABXGCsO_C8++4+oPfZ+bQyrBnEGy5JFpXHkGNpfy+8=5BvVNfg@mail.gmail.com>
On Fri, Mar 27, 2026 at 03:25:00PM +0500, Mikhail Gavrilov wrote:
> On Fri, Mar 27, 2026 at 1:43 PM Harry Yoo (Oracle) <harry@kernel.org> wrote:
> >
> > Probably what I said was misleading...
> >
> > I didn't mean "the architecture is fine with overlapping cacheline".
> >
> > I meant "not defining ARCH_DMA_MINALIGN or defining it as smaller than
> > L1_CACHE_BYTES is how architectures tell kmalloc subsystem that
> > kmalloc objects don't have to be aligned with cacheline size."
>
> Hi Harry,
Hi Mikhail,
> You're right, I was wrong about dma_get_cache_alignment() -- on
> x86_64 without ARCH_HAS_DMA_MINALIGN it returns 1, not
> L1_CACHE_BYTES. Sorry for the confusion.
Don't worry!
> So your suggestion to suppress the warning in dma-debug when
> dma_get_cache_alignment() < L1_CACHE_BYTES would indeed work
> on x86_64 and other coherent platforms.
Thanks for confirming.
> I don't have a strong preference either way. Both approaches
> solve the problem:
>
> - slab side: prevents the overlap from happening
> - dma-debug side: tolerates the overlap when the arch says
> cacheline alignment isn't required for DMA
>
> Would you prefer I send a v2 with the dma-debug approach instead?
Yes please. I think keeping the same behavior regardless of the debug
option will be better in the long term.
> Happy to go whichever direction the maintainers prefer.
Thanks a lot for working on this!
--
Cheers,
Harry / Hyeonggon
^ permalink raw reply
* Re: [PATCH] hid: usbhid: fix deadlock in hid_post_reset()
From: Jiri Kosina @ 2026-03-27 10:34 UTC (permalink / raw)
To: Oliver Neukum; +Cc: bentiss, linux-input, linux-usb
In-Reply-To: <20260324142507.74873-1-oneukum@suse.com>
On Tue, 24 Mar 2026, Oliver Neukum wrote:
> You can build a USB device that includes a HID component
> and a storage or UAS component. The components can be reset
> only together. That means that hid_pre_reset() and hid_post_reset()
> are in the block IO error handling. Hence no memory allocation
> used in them may do block IO because the IO can deadlock
> on the mutex held while resetting a device and calling the
> interface drivers.
> Use GFP_NOIO for all allocations in them.
>
> Fixes: dc3c78e434690 ("HID: usbhid: Check HID report descriptor contents after device reset")
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
Thanks Oliver.
Did you find this just by code inspection, or was this reported with a
real device?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Mikhail Gavrilov @ 2026-03-27 10:25 UTC (permalink / raw)
To: Harry Yoo (Oracle)
Cc: vbabka, akpm, hao.li, cl, rientjes, roman.gushchin, linux-mm,
linux-kernel, linux-usb, stern, linux, andy.shevchenko, hch,
Jeff.kirsher, Catalin Marinas
In-Reply-To: <acZDJ4Nfw4x7HCSr@hyeyoo>
On Fri, Mar 27, 2026 at 1:43 PM Harry Yoo (Oracle) <harry@kernel.org> wrote:
>
> Probably what I said was misleading...
>
> I didn't mean "the architecture is fine with overlapping cacheline".
>
> I meant "not defining ARCH_DMA_MINALIGN or defining it as smaller than
> L1_CACHE_BYTES is how architectures tell kmalloc subsystem that
> kmalloc objects don't have to be aligned with cacheline size."
>
Hi Harry,
You're right, I was wrong about dma_get_cache_alignment() -- on
x86_64 without ARCH_HAS_DMA_MINALIGN it returns 1, not
L1_CACHE_BYTES. Sorry for the confusion.
So your suggestion to suppress the warning in dma-debug when
dma_get_cache_alignment() < L1_CACHE_BYTES would indeed work
on x86_64 and other coherent platforms.
I don't have a strong preference either way. Both approaches
solve the problem:
- slab side: prevents the overlap from happening
- dma-debug side: tolerates the overlap when the arch says
cacheline alignment isn't required for DMA
Would you prefer I send a v2 with the dma-debug approach instead?
Happy to go whichever direction the maintainers prefer.
--
Best Regards,
Mike Gavrilov.
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Harry Yoo (Oracle) @ 2026-03-27 8:43 UTC (permalink / raw)
To: Mikhail Gavrilov
Cc: vbabka, akpm, hao.li, cl, rientjes, roman.gushchin, linux-mm,
linux-kernel, linux-usb, stern, linux, andy.shevchenko, hch,
Jeff.kirsher, Catalin Marinas
In-Reply-To: <CABXGCsPQo=Npum2aHet3QkZWkWEnEVMAw0KQ1AHPrNjMn=9bmA@mail.gmail.com>
On Fri, Mar 27, 2026 at 01:07:21PM +0500, Mikhail Gavrilov wrote:
> On Fri, Mar 27, 2026 at 1:00 PM Harry Yoo (Oracle) <harry@kernel.org> wrote:
> >
> > But dma_get_cache_alignment() < L1_CACHE_BYTES means the architecture
> > actually allows overlapping cachelines, no?
>
> Hi Harry,
>
> On x86_64, dma_get_cache_alignment() returns L1_CACHE_BYTES (both
> are 64). The condition (dma_get_cache_alignment() < L1_CACHE_BYTES)
> would be false, so the check wouldn't suppress the warning.
How does dma_get_cache_alignment() return L1_CACHE_BYTES when
x86_64 doesn't define ARCH_HAS_DMA_MINALIGN?
> The problem isn't that the architecture allows overlapping --
Probably what I said was misleading...
I didn't mean "the architecture is fine with overlapping cacheline".
I meant "not defining ARCH_DMA_MINALIGN or defining it as smaller than
L1_CACHE_BYTES is how architectures tell kmalloc subsystem that
kmalloc objects don't have to be aligned with cacheline size."
> it's that kmalloc returns 8-byte aligned buffers that happen to land in
> the same 64-byte cacheline.
> The DMA debug code correctly identifies that two DMA mappings share
> a cacheline, but on coherent platforms this is harmless.
That happens only when the architecture can live with that.
> Adding a dev_is_dma_coherent() check in dma-debug would fix x86
> but would also silence the warning for any coherent device, including
> ones behind IOMMUs that might have non-coherent paths.
Sorry, I don't understand where the idea of adding a
dma_is_dma_coherent() check comes from ...
> That's why Alan's conclusion was that fixing the allocator side is safer --
> it doesn't weaken any debug checks, it just ensures the situation
> never arises.
--
Cheers,
Harry / Hyeonggon
^ permalink raw reply
* Re: [PATCH v2] usbip: tools: Add usbip host driver availability check
From: Zongmin Zhou @ 2026-03-27 8:39 UTC (permalink / raw)
To: Shuah Khan, Greg KH
Cc: i, linux-kernel, linux-usb, valentina.manea.m, Zongmin Zhou
In-Reply-To: <1e974e1d-afdc-41a5-95a7-45e19e77a087@linuxfoundation.org>
On 2026/3/27 02:43, Shuah Khan wrote:
> On 3/26/26 02:43, Greg KH wrote:
>> On Thu, Mar 26, 2026 at 11:10:02AM +0800, Zongmin Zhou wrote:
>>>
>>> On 2026/3/25 16:58, Greg KH wrote:
>>>> On Wed, Mar 25, 2026 at 10:26:34AM +0800, Zongmin Zhou wrote:
>>>>> From: Zongmin Zhou <zhouzongmin@kylinos.cn>
>>>>>
>>>>> Currently, usbip_generic_driver_open() doesn't verify that the
>>>>> required
>>>>> kernel module (usbip-host or usbip-vudc) is actually loaded.
>>>>> The function returns success even when no driver is present,
>>>>> leading to usbipd daemon run success without driver loaded.
>>>>>
>>>>> So add a check function to ensure usbip host driver has been loaded.
>>>>>
>>>>> Suggested-by: Shuah Khan <skhan@linuxfoundation.org>
>>>>> Signed-off-by: Zongmin Zhou <zhouzongmin@kylinos.cn>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Use system calls directly instead of checking sysfs dir.
>>>>>
>>>>> tools/usb/usbip/libsrc/usbip_device_driver.c | 7 +++++--
>>>>> tools/usb/usbip/libsrc/usbip_host_driver.c | 8 ++++++--
>>>>> 2 files changed, 11 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c
>>>>> b/tools/usb/usbip/libsrc/usbip_device_driver.c
>>>>> index 927a151fa9aa..45ab647ef241 100644
>>>>> --- a/tools/usb/usbip/libsrc/usbip_device_driver.c
>>>>> +++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
>>>>> @@ -136,10 +136,13 @@ static int usbip_device_driver_open(struct
>>>>> usbip_host_driver *hdriver)
>>>>> hdriver->ndevs = 0;
>>>>> INIT_LIST_HEAD(&hdriver->edev_list);
>>>>> - ret = usbip_generic_driver_open(hdriver);
>>>>> - if (ret)
>>>>> + if (system("/sbin/lsmod | grep -q usbip_vudc")){
>>>> What happens if the module is built into the kernel?
>>>>
>>>>> err("please load " USBIP_CORE_MOD_NAME ".ko and "
>>>>> USBIP_DEVICE_DRV_NAME ".ko!");
>>>>> + return -1;
>>>>> + }
>>>>> +
>>>>> + ret = usbip_generic_driver_open(hdriver);
>>>>> return ret;
>>>>> }
>>>>> diff --git a/tools/usb/usbip/libsrc/usbip_host_driver.c
>>>>> b/tools/usb/usbip/libsrc/usbip_host_driver.c
>>>>> index 573e73ec36bd..f0ac941d4f6e 100644
>>>>> --- a/tools/usb/usbip/libsrc/usbip_host_driver.c
>>>>> +++ b/tools/usb/usbip/libsrc/usbip_host_driver.c
>>>>> @@ -31,10 +31,14 @@ static int usbip_host_driver_open(struct
>>>>> usbip_host_driver *hdriver)
>>>>> hdriver->ndevs = 0;
>>>>> INIT_LIST_HEAD(&hdriver->edev_list);
>>>>> - ret = usbip_generic_driver_open(hdriver);
>>>>> - if (ret)
>>>>> + if (system("/sbin/lsmod | grep -q usbip_host")){
>>>> Same here, what happens if it is built in?
>>> Thank you for pointing this out.
>>> I apologize for not considering the built-in module case.
>>>
>>> You are right that using lsmod | grep would incorrectly fail when
>>> usbip_host
>>> is built into the kernel (CONFIG_USBIP_HOST=y).
>>> Usbip has always been built as a loadable module (.ko) by default,
>>> which led
>>> to this oversight.
>>>
>>> To address this issue, would the following approach be acceptable?
>>
>> Wait, what "issue" are you trying to fix here? Why can't you just check
>> for opening the correct device node when the host opens the file and if
>> that fails, report an error? Doesn't that happen today already?
>>
>
> The problem Zongmin is trying fix ish when usbipd starts, it looks for
> exported if any - if it doesn't find any it assumes there aren't any
> exported and doesn't detect that usbip_host driver isn't loaded.
>
> refresh_exported_devices() doesn't have the logic and it shouldn't
> include that logic because this hook is called periodically to
> refresh the list of exported devices. Starting usbipd and loading
> the driver are distinct steps and without any dependencies.
>
> This patch he is trying to add that detection so a message can be printed
> to say "load the driver".
>
> A message can be added easily to cover two cases:
>
> 1. usbip_host driver isn't loaded
> 2. There are no exported devices.
>
> refresh_exported_devices() will not find any devices in both
> of the above scenarios. It isn't an error if it can't find
> any exported devices.
>
> An informational message when refresh_exported_devices()
> when it doesn't find any devices could help users.
>
> Zongmin,
>
> Would adding a message that says
> "Check if usbip_host driver is loaded or export devices"
> solve the problem of hard to debug problem you are addressing here?
>
Shuah,
Your suggestion makes sense.
Adding an informational message when no devices are found would be a simple
and clean solution that helps users debug without being intrusive.
However, I plan to add the info() message in usbip_generic_driver_open()
instead of
refresh_exported_devices(), because:
- usbip_generic_driver_open() is called only once at initialization.
- refresh_exported_devices() is called periodically to refresh the
exported device list.
- When the server has no exported devices, having zero devices
is normal and not worth frequent info messages.
Theoretically, we only need to prompt once at startup. Is my
understanding correct?
I'll also remove the existing error messages like below,
since they cannot accurately determine whether the driver is loaded:
if (ret)
err("please load " USBIP_CORE_MOD_NAME ".ko and "
USBIP_HOST_DRV_NAME ".ko!");
Does this approach look acceptable?
Best regards,
--Zongmin Zhou
> thanks,
> -- Shuah
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Mikhail Gavrilov @ 2026-03-27 8:07 UTC (permalink / raw)
To: Harry Yoo (Oracle)
Cc: vbabka, akpm, hao.li, cl, rientjes, roman.gushchin, linux-mm,
linux-kernel, linux-usb, stern, linux, andy.shevchenko, hch,
Jeff.kirsher, Catalin Marinas
In-Reply-To: <acY5G-lKZQEsUbRw@hyeyoo>
On Fri, Mar 27, 2026 at 1:00 PM Harry Yoo (Oracle) <harry@kernel.org> wrote:
>
> But dma_get_cache_alignment() < L1_CACHE_BYTES means the architecture
> actually allows overlapping cachelines, no?
Hi Harry,
On x86_64, dma_get_cache_alignment() returns L1_CACHE_BYTES (both
are 64). The condition (dma_get_cache_alignment() < L1_CACHE_BYTES)
would be false, so the check wouldn't suppress the warning.
The problem isn't that the architecture allows overlapping -- it's
that kmalloc returns 8-byte aligned buffers that happen to land in
the same 64-byte cacheline. The DMA debug code correctly identifies
that two DMA mappings share a cacheline, but on coherent platforms
this is harmless.
Adding a dev_is_dma_coherent() check in dma-debug would fix x86 but
would also silence the warning for any coherent device, including
ones behind IOMMUs that might have non-coherent paths. That's why
Alan's conclusion was that fixing the allocator side is safer --
it doesn't weaken any debug checks, it just ensures the situation
never arises.
--
Best Regards,
Mike Gavrilov.
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Harry Yoo (Oracle) @ 2026-03-27 8:00 UTC (permalink / raw)
To: Mikhail Gavrilov
Cc: vbabka, akpm, hao.li, cl, rientjes, roman.gushchin, linux-mm,
linux-kernel, linux-usb, stern, linux, andy.shevchenko, hch,
Jeff.kirsher, Catalin Marinas
In-Reply-To: <CABXGCsMTk6kDuJxsTAUKP6npTvp4X6Ryng+P4xjMUsPbTA43+Q@mail.gmail.com>
On Fri, Mar 27, 2026 at 11:50:07AM +0500, Mikhail Gavrilov wrote:
> On Fri, Mar 27, 2026 at 11:38 AM Harry Yoo (Oracle) <harry@kernel.org> wrote:
> >
> > 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.
> >
> > Is it feasible to suppress the warning if dma_get_cache_alignment() is
> > smaller than L1_CACHE_BYTES?
>
> Hi Harry,
Hi Mikhail,
Please keep in mind that I have limited understanding of DMA API,
but just wanted to double check if there is (or isn't) a sane way to
fix it on dma-debug side.
> Good question. I considered the dma-debug side, but the issue is
> that the cacheline overlap check in add_dma_entry() is intentionally
> strict -- it catches real bugs on non-coherent architectures where
> two DMA buffers sharing a cacheline can corrupt data.
But dma_get_cache_alignment() < L1_CACHE_BYTES means the architecture
actually allows overlapping cachelines, no?
A non-coherent architecture where two DMA buffers sharing a cacheline
could corrupt data should define ARCH_DMA_MINALIGN >= L1_CACHE_BYTES.
I'm not sure what kind of a real bug this will hide,
or am I missing something?
> Alan Stern discussed this in the bugzilla [1] and concluded that
> the slab alignment approach "seems reasonable" [2],
As long as there's no good alternative way to fix, yeah.
> noting that turning on debugging should not affect the way the kernel
> behaves -- otherwise what you're debugging isn't the same as what normally
> happens.
Yeah, this is why I'm trying to double check if there's no feasible
alternative.
> But given the way the DMA API debugging is set up, I don't
> see any alternative."
I'm trying to say adding the (dma_get_cache_alignment() <
L1_CACHE_BYTES) check might be considered as an alternative ;)
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=215740#c31
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=215740#c44
--
Cheers,
Harry / Hyeonggon
^ permalink raw reply
* [Bug 215740] kernel warning: DMA-API: xhci_hcd: cacheline tracking EEXIST, overlapping mappings aren't supported
From: bugzilla-daemon @ 2026-03-27 7:32 UTC (permalink / raw)
To: linux-usb
In-Reply-To: <bug-215740-208809@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=215740
Andy Shevchenko (andy.shevchenko@gmail.com) changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |PATCH_ALREADY_AVAILABLE
--- Comment #53 from Andy Shevchenko (andy.shevchenko@gmail.com) ---
(In reply to Guenter Roeck from comment #52)
> comment #51: Sounds good. I'd suggest to go ahead.
Agree. I close this for now since we have a compromise solution publicly
available.
https://lore.kernel.org/all/20260327055846.248829-1-mikhail.v.gavrilov@gmail.com/
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH v5 phy-next 10/27] scsi: ufs: qcom: keep parallel track of PHY power state
From: Manivannan Sadhasivam @ 2026-03-27 6:52 UTC (permalink / raw)
To: Vladimir Oltean
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: <20260326080444.gbesciaa5zwvcgoy@skbuf>
On Thu, Mar 26, 2026 at 10:04:44AM +0200, Vladimir Oltean wrote:
> On Wed, Mar 25, 2026 at 01:57:31PM +0200, Vladimir Oltean wrote:
> > On Wed, Mar 25, 2026 at 05:21:14PM +0530, Manivannan Sadhasivam wrote:
> > > I believe I added the power_count check for phy_exit(). But since that got
> > > moved, the check becomes no longer necessary.
> >
> > FYI, the power_count keeps track of the balance of phy_power_on() and
> > phy_power_off() calls, whereas it is the init_count keeps track of
> > phy_init() and phy_exit() calls. They are only related to the extent
> > that you must respect the phy_init() -> phy_power_on() -> phy_power_off()
> > -> phy_exit() sequence. But in any case, both should be considered
> > PHY-internal fields. The "Order of API calls" section from
> > Documentation/driver-api/phy/phy.rst mentions the order that I just
> > described above, and consumers should just ensure they follow that.
>
> Ok, so we can close this topic of "checking the power_count not needed"
> by linking to the conversation which spun off here:
> https://lore.kernel.org/lkml/20260325120122.265973-1-manivannan.sadhasivam@oss.qualcomm.com/
>
Sure.
> Mani, I spent some more time to figure out what's really going on with
> this unexpected phy_power_off() call. Do you think you could
> regression-test the patch attached?
>
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
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply related
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Mikhail Gavrilov @ 2026-03-27 6:50 UTC (permalink / raw)
To: Harry Yoo (Oracle)
Cc: vbabka, akpm, hao.li, cl, rientjes, roman.gushchin, linux-mm,
linux-kernel, linux-usb, stern, linux, andy.shevchenko, hch,
Jeff.kirsher
In-Reply-To: <acYlxRBhSMcwBnja@hyeyoo>
On Fri, Mar 27, 2026 at 11:38 AM Harry Yoo (Oracle) <harry@kernel.org> wrote:
>
> 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.
>
> Is it feasible to suppress the warning if dma_get_cache_alignment() is
> smaller than L1_CACHE_BYTES?
Hi Harry,
Good question. I considered the dma-debug side, but the issue is
that the cacheline overlap check in add_dma_entry() is intentionally
strict -- it catches real bugs on non-coherent architectures where
two DMA buffers sharing a cacheline can corrupt data.
The check already has suppressions for DMA_ATTR_SKIP_CPU_SYNC,
DMA_ATTR_CPU_CACHE_CLEAN, and CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC +
swiotlb. Adding another special case (e.g. dev_is_dma_coherent())
would weaken the check for all coherent platforms, potentially
hiding real bugs on devices behind non-coherent IOMMUs.
Alan Stern discussed this in the bugzilla [1] and concluded that
the slab alignment approach "seems reasonable" [2], noting that
"turning on debugging should not affect the way the kernel behaves --
otherwise what you're debugging isn't the same as what normally
happens. But given the way the DMA API debugging is set up, I don't
see any alternative."
The memory overhead is only present when CONFIG_DMA_API_DEBUG is
enabled, which is a debug-only option not used in production.
That said, if you'd prefer a dma-debug side fix, I'm happy to
explore that direction instead.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=215740#c31
[2] https://bugzilla.kernel.org/show_bug.cgi?id=215740#c44
--
Best Regards,
Mike Gavrilov.
^ permalink raw reply
* [PATCH] usbip: vhci: reject RET_SUBMIT with inflated number_of_packets
From: Nathan Rebello @ 2026-03-27 6:44 UTC (permalink / raw)
To: linux-usb
Cc: gregkh, addcontent08, skhan, kyungtae.kim, stable, Nathan Rebello
When a USB/IP client receives a RET_SUBMIT response,
usbip_pack_ret_submit() unconditionally overwrites
urb->number_of_packets from the network PDU. This value is
subsequently used as the loop bound in usbip_recv_iso() and
usbip_pad_iso() to iterate over urb->iso_frame_desc[], a flexible
array whose size was fixed at URB allocation time based on the
*original* number_of_packets from the CMD_SUBMIT.
A malicious USB/IP server can set number_of_packets in the response
to a value larger than what was originally submitted, causing a heap
out-of-bounds write when usbip_recv_iso() writes to
urb->iso_frame_desc[i] beyond the allocated region.
KASAN confirmed this with kernel 7.0.0-rc5:
BUG: KASAN: slab-out-of-bounds in usbip_recv_iso+0x46a/0x640
Write of size 4 at addr ffff888106351d40 by task vhci_rx/69
The buggy address is located 0 bytes to the right of
allocated 320-byte region [ffff888106351c00, ffff888106351d40)
The server side (stub_rx.c) and gadget side (vudc_rx.c) already
validate number_of_packets in the CMD_SUBMIT path since commits
c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path to handle
malicious input") and b78d830f0049 ("usbip: fix vudc_rx: harden
CMD_SUBMIT path to handle malicious input"). The server side validates
against USBIP_MAX_ISO_PACKETS because no URB exists yet at that point.
On the client side we have the original URB, so we can use the tighter
bound: the response must not exceed the original number_of_packets.
This mirrors the existing validation of actual_length against
transfer_buffer_length in usbip_recv_xbuff(), which checks the
response value against the original allocation size.
Kelvin Mbogo's series ("usb: usbip: fix integer overflow in
usbip_recv_iso()", v2) hardens the receive-side functions themselves;
this patch complements that work by catching the bad value at its
source -- in usbip_pack_ret_submit() before the overwrite -- and
using the tighter per-URB allocation bound rather than the global
USBIP_MAX_ISO_PACKETS limit.
Fix this by checking rpdu->number_of_packets against
urb->number_of_packets in usbip_pack_ret_submit() before the
overwrite. On violation, clamp to zero so that usbip_recv_iso() and
usbip_pad_iso() safely return early.
Fixes: 0775a9cbc798 ("staging: usbip: vhci extension: modifications to the client side")
Cc: stable@vger.kernel.org
Signed-off-by: Nathan Rebello <nathan.c.rebello@gmail.com>
---
drivers/usb/usbip/usbip_common.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -470,7 +470,18 @@ static void usbip_pack_ret_submit(struct usbip_header *pdu, struct urb *urb,
urb->status = rpdu->status;
urb->actual_length = rpdu->actual_length;
urb->start_frame = rpdu->start_frame;
- urb->number_of_packets = rpdu->number_of_packets;
+ /*
+ * The number_of_packets field determines the length of
+ * iso_frame_desc[], which is a flexible array allocated
+ * at URB creation time. A response must never claim more
+ * packets than originally submitted; doing so would cause
+ * an out-of-bounds write in usbip_recv_iso() and
+ * usbip_pad_iso(). Clamp to zero on violation so both
+ * functions safely return early.
+ */
+ if (rpdu->number_of_packets < 0 ||
+ rpdu->number_of_packets > urb->number_of_packets)
+ rpdu->number_of_packets = 0;
+ urb->number_of_packets = rpdu->number_of_packets;
urb->error_count = rpdu->error_count;
}
}
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Guenter Roeck @ 2026-03-27 6:41 UTC (permalink / raw)
To: Mikhail Gavrilov, vbabka, harry.yoo, akpm
Cc: hao.li, cl, rientjes, roman.gushchin, linux-mm, linux-kernel,
linux-usb, stern, andy.shevchenko, hch, Jeff.kirsher
In-Reply-To: <20260327055846.248829-1-mikhail.v.gavrilov@gmail.com>
On 3/26/26 22:58, 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.
>
> Raise ARCH_KMALLOC_MINALIGN to L1_CACHE_BYTES when CONFIG_DMA_API_DEBUG
> is enabled, ensuring each kmalloc allocation occupies its own cacheline
> and eliminating the false positive.
>
> 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: 64 pairs allocated, all in separate cachelines, no warning
>
> Fixes: 2b4bbc6231d7 ("dma-debug: report -EEXIST errors in add_dma_entry")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215740
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Tested-by: Jeff Kirsher <Jeff.kirsher@gmail.com>
> Tested-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Thanks a lot for taking care of this!
FWIW:
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
>
> Reproducer module that triggers the bug reliably:
> https://bugzilla.kernel.org/attachment.cgi?id=309769
>
> include/linux/slab.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 15a60b501b95..f044956e17c1 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -536,6 +536,19 @@ static inline bool kmem_dump_obj(void *object) { return false; }
> #endif
> #endif
>
> +/*
> + * 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
> +
> #ifndef ARCH_KMALLOC_MINALIGN
> #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
> #elif ARCH_KMALLOC_MINALIGN > 8
^ permalink raw reply
* Re: [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Harry Yoo (Oracle) @ 2026-03-27 6:37 UTC (permalink / raw)
To: Mikhail Gavrilov
Cc: vbabka, akpm, hao.li, cl, rientjes, roman.gushchin, linux-mm,
linux-kernel, linux-usb, stern, linux, andy.shevchenko, hch,
Jeff.kirsher
In-Reply-To: <20260327055846.248829-1-mikhail.v.gavrilov@gmail.com>
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.
Is it feasible to suppress the warning if dma_get_cache_alignment() is
smaller than L1_CACHE_BYTES?
> 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.
>
> Raise ARCH_KMALLOC_MINALIGN to L1_CACHE_BYTES when CONFIG_DMA_API_DEBUG
> is enabled, ensuring each kmalloc allocation occupies its own cacheline
> and eliminating the false positive.
>
> 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: 64 pairs allocated, all in separate cachelines, no warning
>
> Fixes: 2b4bbc6231d7 ("dma-debug: report -EEXIST errors in add_dma_entry")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215740
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Tested-by: Jeff Kirsher <Jeff.kirsher@gmail.com>
> Tested-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
--
Cheers,
Harry / Hyeonggon
^ permalink raw reply
* Re: [PATCH v2 1/3] usb: usbip: fix integer overflow in usbip_recv_iso()
From: Greg KH @ 2026-03-27 6:25 UTC (permalink / raw)
To: Nathan Rebello; +Cc: linux-usb, addcontent08, skhan, kyungtae.kim
In-Reply-To: <20260327043153.643-1-nathan.c.rebello@gmail.com>
On Fri, Mar 27, 2026 at 12:31:53AM -0400, Nathan Rebello wrote:
> Hi Kelvin,
>
> Your series hardens usbip_recv_iso() and usbip_pad_iso() against
> malicious number_of_packets values, but the bad value still lands in
> urb->number_of_packets via usbip_pack_ret_submit() before those
> checks run.
>
> The patch below validates at the source — in usbip_pack_ret_submit()
> before the overwrite — and checks against the original
> urb->number_of_packets (the actual allocation bound) rather than
> USBIP_MAX_ISO_PACKETS. This is a tighter check because the URB may
> have been allocated for far fewer than 1024 packets.
>
> This could complement your series as an additional layer, or stand
> alone. Would be glad to rework this however the maintainers see fit —
> whether folded into your series or submitted separately.
Please submit it separately, on top of that series, to make it easier to
review and apply.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] mm/slab: align kmalloc to cacheline when DMA API debugging is active
From: Mikhail Gavrilov @ 2026-03-27 5:58 UTC (permalink / raw)
To: vbabka, harry.yoo, akpm
Cc: hao.li, cl, rientjes, roman.gushchin, linux-mm, linux-kernel,
linux-usb, stern, linux, andy.shevchenko, hch, Jeff.kirsher,
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.
Raise ARCH_KMALLOC_MINALIGN to L1_CACHE_BYTES when CONFIG_DMA_API_DEBUG
is enabled, ensuring each kmalloc allocation occupies its own cacheline
and eliminating the false positive.
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: 64 pairs allocated, all in separate cachelines, no warning
Fixes: 2b4bbc6231d7 ("dma-debug: report -EEXIST errors in add_dma_entry")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215740
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Suggested-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Jeff Kirsher <Jeff.kirsher@gmail.com>
Tested-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
---
Reproducer module that triggers the bug reliably:
https://bugzilla.kernel.org/attachment.cgi?id=309769
include/linux/slab.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 15a60b501b95..f044956e17c1 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -536,6 +536,19 @@ static inline bool kmem_dump_obj(void *object) { return false; }
#endif
#endif
+/*
+ * 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
+
#ifndef ARCH_KMALLOC_MINALIGN
#define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
#elif ARCH_KMALLOC_MINALIGN > 8
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v2 1/3] usb: usbip: fix integer overflow in usbip_recv_iso()
From: Nathan Rebello @ 2026-03-27 4:31 UTC (permalink / raw)
To: linux-usb; +Cc: addcontent08, gregkh, skhan, kyungtae.kim
In-Reply-To: <20260325104841.8282-1-addcontent08@gmail.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4092 bytes --]
Hi Kelvin,
Your series hardens usbip_recv_iso() and usbip_pad_iso() against
malicious number_of_packets values, but the bad value still lands in
urb->number_of_packets via usbip_pack_ret_submit() before those
checks run.
The patch below validates at the source — in usbip_pack_ret_submit()
before the overwrite — and checks against the original
urb->number_of_packets (the actual allocation bound) rather than
USBIP_MAX_ISO_PACKETS. This is a tighter check because the URB may
have been allocated for far fewer than 1024 packets.
This could complement your series as an additional layer, or stand
alone. Would be glad to rework this however the maintainers see fit —
whether folded into your series or submitted separately.
---
From: Nathan Rebello <nathan.c.rebello@gmail.com>
Subject: [PATCH] usbip: vhci: reject RET_SUBMIT with inflated
number_of_packets
When a USB/IP client receives a RET_SUBMIT response,
usbip_pack_ret_submit() unconditionally overwrites
urb->number_of_packets from the network PDU. This value is
subsequently used as the loop bound in usbip_recv_iso() and
usbip_pad_iso() to iterate over urb->iso_frame_desc[], a flexible
array whose size was fixed at URB allocation time based on the
*original* number_of_packets from the CMD_SUBMIT.
A malicious USB/IP server can set number_of_packets in the response
to a value larger than what was originally submitted, causing a heap
out-of-bounds write when usbip_recv_iso() writes to
urb->iso_frame_desc[i] beyond the allocated region.
KASAN confirmed this with kernel 7.0.0-rc5:
BUG: KASAN: slab-out-of-bounds in usbip_recv_iso+0x46a/0x640
Write of size 4 at addr ffff888106351d40 by task vhci_rx/69
The buggy address is located 0 bytes to the right of
allocated 320-byte region [ffff888106351c00, ffff888106351d40)
The server side (stub_rx.c) and gadget side (vudc_rx.c) already
validate number_of_packets in the CMD_SUBMIT path since commits
c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path to handle
malicious input") and b78d830f0049 ("usbip: fix vudc_rx: harden
CMD_SUBMIT path to handle malicious input"). The server side validates
against USBIP_MAX_ISO_PACKETS because no URB exists yet at that point.
On the client side we have the original URB, so we can use the tighter
bound: the response must not exceed the original number_of_packets.
This mirrors the existing validation of actual_length against
transfer_buffer_length in usbip_recv_xbuff(), which checks the
response value against the original allocation size.
Fix this by checking rpdu->number_of_packets against
urb->number_of_packets in usbip_pack_ret_submit() before the
overwrite. On violation, clamp to zero so that usbip_recv_iso() and
usbip_pad_iso() safely return early.
Fixes: 0775a9cbc798 ("staging: usbip: vhci extension: modifications to the client side")
Cc: stable@vger.kernel.org
Signed-off-by: Nathan Rebello <nathan.c.rebello@gmail.com>
---
drivers/usb/usbip/usbip_common.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -470,7 +470,18 @@ static void usbip_pack_ret_submit(struct usbip_header *pdu, struct urb *urb,
urb->status = rpdu->status;
urb->actual_length = rpdu->actual_length;
urb->start_frame = rpdu->start_frame;
- urb->number_of_packets = rpdu->number_of_packets;
+ /*
+ * The number_of_packets field determines the length of
+ * iso_frame_desc[], which is a flexible array allocated
+ * at URB creation time. A response must never claim more
+ * packets than originally submitted; doing so would cause
+ * an out-of-bounds write in usbip_recv_iso() and
+ * usbip_pad_iso(). Clamp to zero on violation so both
+ * functions safely return early.
+ */
+ if (rpdu->number_of_packets < 0 ||
+ rpdu->number_of_packets > urb->number_of_packets)
+ rpdu->number_of_packets = 0;
+ urb->number_of_packets = rpdu->number_of_packets;
urb->error_count = rpdu->error_count;
}
}
^ permalink raw reply
* Re: [PATCH] usb: gadget: u_ether: Fix race between gether_disconnect and eth_stop
From: Dayu Jiang @ 2026-03-27 2:52 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kuen-Han Tsai, David Brownell, linux-usb, linux-kernel, stable
In-Reply-To: <2026032614-most-opposing-4363@gregkh>
On Thu, Mar 26, 2026 at 11:35:31AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 26, 2026 at 03:53:13PM +0800, Dayu Jiang wrote:
> > On Wed, Mar 11, 2026 at 05:12:15PM +0800, Kuen-Han Tsai wrote:
> > > A race condition between gether_disconnect() and eth_stop() leads to a
> > > NULL pointer dereference. Specifically, if eth_stop() is triggered
> > > concurrently while gether_disconnect() is tearing down the endpoints,
> > > eth_stop() attempts to access the cleared endpoint descriptor, causing
> > > the following NPE:
> > >
> > > Unable to handle kernel NULL pointer dereference
> > > Call trace:
> > > __dwc3_gadget_ep_enable+0x60/0x788
> > > dwc3_gadget_ep_enable+0x70/0xe4
> > > usb_ep_enable+0x60/0x15c
> > > eth_stop+0xb8/0x108
> > >
> > > Because eth_stop() crashes while holding the dev->lock, the thread
> > > running gether_disconnect() fails to acquire the same lock and spins
> > > forever, resulting in a hardlockup:
> > >
> > > Core - Debugging Information for Hardlockup core(7)
> > > Call trace:
> > > queued_spin_lock_slowpath+0x94/0x488
> > > _raw_spin_lock+0x64/0x6c
> > > gether_disconnect+0x19c/0x1e8
> > > ncm_set_alt+0x68/0x1a0
> > > composite_setup+0x6a0/0xc50
> > >
> > Hi Greg,
> > Hit the same issue during NCM switch stress test.
> > Can you take a look at this patch and check if it’s ready for merge?
>
> This is already in my tree and in linux-next and will go to Linus this
> weekend.
Got it. Sorry for the multiple copies of the same email—our mail server had some issues and sent them repeatedly. My apologies for the noise.
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH] usb: gadget: u_ether: Fix race between gether_disconnect and eth_stop
From: Dayu Jiang @ 2026-03-27 2:44 UTC (permalink / raw)
To: Kuen-Han Tsai, Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel, stable
In-Reply-To: <20260311-gether-disconnect-npe-v1-1-454966adf7c7@google.com>
On Wed, Mar 11, 2026 at 05:12:15PM +0800, Kuen-Han Tsai wrote:
> A race condition between gether_disconnect() and eth_stop() leads to a
> NULL pointer dereference. Specifically, if eth_stop() is triggered
> concurrently while gether_disconnect() is tearing down the endpoints,
> eth_stop() attempts to access the cleared endpoint descriptor, causing
> the following NPE:
>
> Unable to handle kernel NULL pointer dereference
> Call trace:
> __dwc3_gadget_ep_enable+0x60/0x788
> dwc3_gadget_ep_enable+0x70/0xe4
> usb_ep_enable+0x60/0x15c
> eth_stop+0xb8/0x108
>
> Because eth_stop() crashes while holding the dev->lock, the thread
> running gether_disconnect() fails to acquire the same lock and spins
> forever, resulting in a hardlockup:
>
> Core - Debugging Information for Hardlockup core(7)
> Call trace:
> queued_spin_lock_slowpath+0x94/0x488
> _raw_spin_lock+0x64/0x6c
> gether_disconnect+0x19c/0x1e8
> ncm_set_alt+0x68/0x1a0
> composite_setup+0x6a0/0xc50
>
> The root cause is that the clearing of dev->port_usb in
> gether_disconnect() is delayed until the end of the function.
>
> Move the clearing of dev->port_usb to the very beginning of
> gether_disconnect() while holding dev->lock. This cuts off the link
> immediately, ensuring eth_stop() will see dev->port_usb as NULL and
> safely bail out.
>
Hi Greg,
Hit the same issue during NCM switch stress test.
Can you take a look at this patch and check if it’s ready for merge?
Thanks,
Dayu Jiang
> Fixes: 2b3d942c4878 ("usb ethernet gadget: split out network core")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
> ---
> drivers/usb/gadget/function/u_ether.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 338f6e2a85a9..2c970a0eafd9 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -1246,6 +1246,11 @@ void gether_disconnect(struct gether *link)
>
> DBG(dev, "%s\n", __func__);
>
> + spin_lock(&dev->lock);
> + dev->port_usb = NULL;
> + link->is_suspend = false;
> + spin_unlock(&dev->lock);
> +
> netif_stop_queue(dev->net);
> netif_carrier_off(dev->net);
>
> @@ -1283,11 +1288,6 @@ void gether_disconnect(struct gether *link)
> dev->header_len = 0;
> dev->unwrap = NULL;
> dev->wrap = NULL;
> -
> - spin_lock(&dev->lock);
> - dev->port_usb = NULL;
> - link->is_suspend = false;
> - spin_unlock(&dev->lock);
> }
> EXPORT_SYMBOL_GPL(gether_disconnect);
>
>
> ---
> base-commit: 1be3b77de4eb89af8ae2fd6610546be778e25589
> change-id: 20260311-gether-disconnect-npe-5861d9831dff
>
> Best regards,
> --
> Kuen-Han Tsai <khtsai@google.com>
>
^ permalink raw reply
* Re: [PATCH] usb: gadget: u_ether: Fix race between gether_disconnect and eth_stop
From: Dayu Jiang @ 2026-03-27 2:36 UTC (permalink / raw)
To: Kuen-Han Tsai, Greg Kroah-Hartman
Cc: David Brownell, linux-usb, linux-kernel, stable
In-Reply-To: <20260311-gether-disconnect-npe-v1-1-454966adf7c7@google.com>
On Wed, Mar 11, 2026 at 05:12:15PM +0800, Kuen-Han Tsai wrote:
> A race condition between gether_disconnect() and eth_stop() leads to a
> NULL pointer dereference. Specifically, if eth_stop() is triggered
> concurrently while gether_disconnect() is tearing down the endpoints,
> eth_stop() attempts to access the cleared endpoint descriptor, causing
> the following NPE:
>
> Unable to handle kernel NULL pointer dereference
> Call trace:
> __dwc3_gadget_ep_enable+0x60/0x788
> dwc3_gadget_ep_enable+0x70/0xe4
> usb_ep_enable+0x60/0x15c
> eth_stop+0xb8/0x108
>
> Because eth_stop() crashes while holding the dev->lock, the thread
> running gether_disconnect() fails to acquire the same lock and spins
> forever, resulting in a hardlockup:
>
> Core - Debugging Information for Hardlockup core(7)
> Call trace:
> queued_spin_lock_slowpath+0x94/0x488
> _raw_spin_lock+0x64/0x6c
> gether_disconnect+0x19c/0x1e8
> ncm_set_alt+0x68/0x1a0
> composite_setup+0x6a0/0xc50
>
> The root cause is that the clearing of dev->port_usb in
> gether_disconnect() is delayed until the end of the function.
>
> Move the clearing of dev->port_usb to the very beginning of
> gether_disconnect() while holding dev->lock. This cuts off the link
> immediately, ensuring eth_stop() will see dev->port_usb as NULL and
> safely bail out.
>
Hi Greg,
Hit the same issue during NCM switch stress test.
Can you take a look at this patch and check if it’s ready for merge?
Thanks,
Dayu Jiang
> Fixes: 2b3d942c4878 ("usb ethernet gadget: split out network core")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
> ---
> drivers/usb/gadget/function/u_ether.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 338f6e2a85a9..2c970a0eafd9 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -1246,6 +1246,11 @@ void gether_disconnect(struct gether *link)
>
> DBG(dev, "%s\n", __func__);
>
> + spin_lock(&dev->lock);
> + dev->port_usb = NULL;
> + link->is_suspend = false;
> + spin_unlock(&dev->lock);
> +
> netif_stop_queue(dev->net);
> netif_carrier_off(dev->net);
>
> @@ -1283,11 +1288,6 @@ void gether_disconnect(struct gether *link)
> dev->header_len = 0;
> dev->unwrap = NULL;
> dev->wrap = NULL;
> -
> - spin_lock(&dev->lock);
> - dev->port_usb = NULL;
> - link->is_suspend = false;
> - spin_unlock(&dev->lock);
> }
> EXPORT_SYMBOL_GPL(gether_disconnect);
>
>
> ---
> base-commit: 1be3b77de4eb89af8ae2fd6610546be778e25589
> change-id: 20260311-gether-disconnect-npe-5861d9831dff
>
> Best regards,
> --
> Kuen-Han Tsai <khtsai@google.com>
>
^ permalink raw reply
* Re: [PATCH 2/2] USB: serial: pl2303: add new PID to support PL256X (TYPE_MP)
From: Charles Yeh @ 2026-03-27 2:29 UTC (permalink / raw)
To: Greg KH; +Cc: johan, linux-usb, charles-yeh, kernel test robot
In-Reply-To: <2026032549-zodiac-navigator-23ac@gregkh>
> just redo the original patch and send it as a v2 as the documentation describes
> how to do.
Thanks for the guidance.
I have updated the original patch and submitted it as v2 as requested.
Thanks,
Charles.
^ permalink raw reply
* [Bug 215740] kernel warning: DMA-API: xhci_hcd: cacheline tracking EEXIST, overlapping mappings aren't supported
From: bugzilla-daemon @ 2026-03-27 0:04 UTC (permalink / raw)
To: linux-usb
In-Reply-To: <bug-215740-208809@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=215740
--- Comment #52 from Guenter Roeck (linux@roeck-us.net) ---
#51: ounds good. I'd suggest to go ahead.
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH v3 1/2] usb: core: use dedicated spinlock for offload state
From: Mathias Nyman @ 2026-03-26 23:58 UTC (permalink / raw)
To: Guan-Yu Lin, gregkh, mathias.nyman, perex, tiwai, quic_wcheng,
broonie, arnd, xiaopei01, wesley.cheng, hannelotta, sakari.ailus,
eadavis, stern, amardeep.rai, xu.yang_2, andriy.shevchenko,
nkapron
Cc: linux-usb, linux-kernel, linux-sound, stable
In-Reply-To: <20260324203851.4091193-2-guanyulin@google.com>
Hi
On 3/24/26 22:38, Guan-Yu Lin wrote:
> Replace the coarse USB device lock with a dedicated offload_lock
> spinlock to reduce contention during offload operations. Use
> offload_pm_locked to synchronize with PM transitions and replace
> the legacy offload_at_suspend flag.
>
> Optimize usb_offload_get/put by switching from auto-resume/suspend
> to pm_runtime_get_if_active(). This ensures offload state is only
> modified when the device is already active, avoiding unnecessary
> power transitions.
>
> Cc: stable@vger.kernel.org
> Fixes: ef82a4803aab ("xhci: sideband: add api to trace sideband usage")
> Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> ---
> diff --git a/drivers/usb/core/offload.c b/drivers/usb/core/offload.c
> index 7c699f1b8d2b..c24945294d7e 100644
> --- a/drivers/usb/core/offload.c
> +++ b/drivers/usb/core/offload.c
> @@ -25,33 +25,30 @@
> */
> int usb_offload_get(struct usb_device *udev)
> {
> - int ret;
> + int ret = 0;
>
> - usb_lock_device(udev);
> - if (udev->state == USB_STATE_NOTATTACHED) {
> - usb_unlock_device(udev);
> + if (!usb_get_dev(udev))
> return -ENODEV;
> - }
>
> - if (udev->state == USB_STATE_SUSPENDED ||
> - udev->offload_at_suspend) {
> - usb_unlock_device(udev);
> - return -EBUSY;
> + if (pm_runtime_get_if_active(&udev->dev) != 1) {
> + ret = -EBUSY;
> + goto err_rpm;
> }
>
> - /*
> - * offload_usage could only be modified when the device is active, since
> - * it will alter the suspend flow of the device.
> - */
> - ret = usb_autoresume_device(udev);
> - if (ret < 0) {
> - usb_unlock_device(udev);
> - return ret;
> + spin_lock(&udev->offload_lock);
> +
> + if (udev->offload_pm_locked) {
Could we get rid of 'udev->offload_pm_locked' and 'usb_offload_set_pm_locked()'
by calling a synchronous pm_runtime_get_sync() or pm_runtime_resume_and_get()?
This way we can ensure udev->offload_usage isn't modified mid runtime suspend or
resume as resume is guaranteed to have finished and suspend won't be called,
at least not for the runtime case.
> + ret = -EAGAIN;
> + goto err;
> }
>
> udev->offload_usage++;
> - usb_autosuspend_device(udev);
> - usb_unlock_device(udev);
> +
> +err:
> + spin_unlock(&udev->offload_lock);
> + pm_runtime_put_autosuspend(&udev->dev);
> +err_rpm:
> + usb_put_dev(udev);
>
> return ret;
> }
> @@ -69,35 +66,32 @@ EXPORT_SYMBOL_GPL(usb_offload_get);
> */
> int usb_offload_put(struct usb_device *udev)
> {
> - int ret;
> + int ret = 0;
>
> - usb_lock_device(udev);
> - if (udev->state == USB_STATE_NOTATTACHED) {
> - usb_unlock_device(udev);
> + if (!usb_get_dev(udev))
> return -ENODEV;
> - }
>
> - if (udev->state == USB_STATE_SUSPENDED ||
> - udev->offload_at_suspend) {
> - usb_unlock_device(udev);
> - return -EBUSY;
> + if (pm_runtime_get_if_active(&udev->dev) != 1) {
> + ret = -EBUSY;
> + goto err_rpm;
> }
>
> - /*
> - * offload_usage could only be modified when the device is active, since
> - * it will alter the suspend flow of the device.
> - */
> - ret = usb_autoresume_device(udev);
> - if (ret < 0) {
> - usb_unlock_device(udev);
> - return ret;
> + spin_lock(&udev->offload_lock);
> +
> + if (udev->offload_pm_locked) {
> + ret = -EAGAIN;
> + goto err;
Ending up here is about unlucky timing, i.e. usb_offload_put() is called while
device is pretending to suspend/resume. Result here is that udev->offload_usage is
not decremented, and usb device won't properly suspend anymore even if device is
no longer offloaded.
> }
>
> /* Drop the count when it wasn't 0, ignore the operation otherwise. */
> if (udev->offload_usage)
> udev->offload_usage--;
> - usb_autosuspend_device(udev);
> - usb_unlock_device(udev);
> +
> +err:
> + spin_unlock(&udev->offload_lock);
> + pm_runtime_put_autosuspend(&udev->dev);
> +err_rpm:
> + usb_put_dev(udev);
>
> return ret;
> }
> @@ -112,25 +106,52 @@ EXPORT_SYMBOL_GPL(usb_offload_put);
> * management.
> *
> * The caller must hold @udev's device lock. In addition, the caller should
> - * ensure downstream usb devices are all either suspended or marked as
> - * "offload_at_suspend" to ensure the correctness of the return value.
> + * ensure downstream usb devices are all marked as "offload_pm_locked" to
> + * ensure the correctness of the return value.
> *
> * Returns true on any offload activity, false otherwise.
> */
> bool usb_offload_check(struct usb_device *udev) __must_hold(&udev->dev->mutex)
> {
> struct usb_device *child;
> - bool active;
> + bool active = false;
> int port1;
>
> + spin_lock(&udev->offload_lock);
> + if (udev->offload_usage)
> + active = true;
> + spin_unlock(&udev->offload_lock);
> +> + if (active)
> + return true;
Not sure what the purpose of the spinlock is above
> +
> usb_hub_for_each_child(udev, port1, child) {
> usb_lock_device(child);
> active = usb_offload_check(child);
> usb_unlock_device(child);
> +
> if (active)
> - return true;
> + break;
> }
>
> - return !!udev->offload_usage;
> + return active;
> }
> EXPORT_SYMBOL_GPL(usb_offload_check);
> +
> +/**
> + * usb_offload_set_pm_locked - set the PM lock state of a USB device
> + * @udev: the USB device to modify
> + * @locked: the new lock state
> + *
> + * Setting @locked to true prevents offload_usage from being modified. This
> + * ensures that offload activities cannot be started or stopped during critical
> + * power management transitions, maintaining a stable state for the duration
> + * of the transition.
> + */
> +void usb_offload_set_pm_locked(struct usb_device *udev, bool locked)
> +{
> + spin_lock(&udev->offload_lock);
> + udev->offload_pm_locked = locked;
> + spin_unlock(&udev->offload_lock);
>
spinlock usage unclear here as well
Thanks
Mathias
^ permalink raw reply
* Re: [PATCH v3 1/1] usb: dwc3: Add optional VBUS regulator support to SpacemiT K1
From: Thinh Nguyen @ 2026-03-26 23:25 UTC (permalink / raw)
To: Chukun Pan
Cc: Thinh Nguyen, Yixun Lan, Ze Huang, Greg Kroah-Hartman,
linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, spacemit@lists.linux.dev
In-Reply-To: <20260326100010.3588454-2-amadeus@jmu.edu.cn>
On Thu, Mar 26, 2026, Chukun Pan wrote:
> Some SpacemiT K1 boards (like OrangePi R2S) provide USB VBUS
> through a controllable regulator. Add support for the optional
> vbus-supply property so the regulator can be properly managed
> in host mode instead of left always-on. Note that this doesn't
> apply to USB Hub downstream ports with different VBUS supplies.
>
> The enabled and disabled actions of the regulator are handled
> automatically by devm_regulator_get_enable_optional().
>
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
> drivers/usb/dwc3/dwc3-generic-plat.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-generic-plat.c b/drivers/usb/dwc3/dwc3-generic-plat.c
> index e846844e0023..64f5e9f20663 100644
> --- a/drivers/usb/dwc3/dwc3-generic-plat.c
> +++ b/drivers/usb/dwc3/dwc3-generic-plat.c
> @@ -12,6 +12,8 @@
> #include <linux/reset.h>
> #include <linux/regmap.h>
> #include <linux/mfd/syscon.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/otg.h>
> #include "glue.h"
>
> #define EIC7700_HSP_BUS_FILTER_EN BIT(0)
> @@ -69,6 +71,20 @@ static int dwc3_eic7700_init(struct dwc3_generic *dwc3g)
> return 0;
> }
>
> +static int dwc3_spacemit_k1_init(struct dwc3_generic *dwc3g)
> +{
> + struct device *dev = dwc3g->dev;
> +
> + if (usb_get_dr_mode(dev) == USB_DR_MODE_HOST) {
> + int ret = devm_regulator_get_enable_optional(dev, "vbus");
> +
> + if (ret && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "failed to enable VBUS\n");
> + }
> +
> + return 0;
> +}
> +
> static int dwc3_generic_probe(struct platform_device *pdev)
> {
> const struct dwc3_generic_config *plat_config;
> @@ -201,6 +217,11 @@ static const struct dev_pm_ops dwc3_generic_dev_pm_ops = {
> dwc3_generic_runtime_idle)
> };
>
> +static const struct dwc3_generic_config spacemit_k1_dwc3 = {
> + .init = dwc3_spacemit_k1_init,
> + .properties = DWC3_DEFAULT_PROPERTIES,
> +};
> +
> static const struct dwc3_generic_config fsl_ls1028_dwc3 = {
> .properties.gsbuscfg0_reqinfo = 0x2222,
> };
> @@ -211,7 +232,7 @@ static const struct dwc3_generic_config eic7700_dwc3 = {
> };
>
> static const struct of_device_id dwc3_generic_of_match[] = {
> - { .compatible = "spacemit,k1-dwc3", },
> + { .compatible = "spacemit,k1-dwc3", &spacemit_k1_dwc3},
> { .compatible = "fsl,ls1028a-dwc3", &fsl_ls1028_dwc3},
> { .compatible = "eswin,eic7700-dwc3", &eic7700_dwc3},
> { /* sentinel */ }
> --
> 2.34.1
>
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
BR,
Thinh
^ permalink raw reply
* Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error
From: Thinh Nguyen @ 2026-03-26 23:24 UTC (permalink / raw)
To: Mathias Nyman
Cc: Thinh Nguyen, linux-usb@vger.kernel.org,
stern@rowland.harvard.edu, michal.pecio@gmail.com,
oneukum@suse.com, niklas.neronin@linux.intel.com
In-Reply-To: <9cf4008e-2d12-4025-809a-8d9371f45dac@linux.intel.com>
On Thu, Mar 26, 2026, Mathias Nyman wrote:
> On 3/26/26 03:19, Thinh Nguyen wrote:
> > On Wed, Mar 25, 2026, Mathias Nyman wrote:
> > > On 3/25/26 03:52, Thinh Nguyen wrote:
> > > > On Mon, Mar 23, 2026, Mathias Nyman wrote:
> > > > > Avoid automatically restarting bulk or interrupt transfers after a
> > > > > URB is given back due to stall or error.
> > > > >
> > > > > Introduce a 'TD_TAINTED' state for pending TDs queued on a endpoint when
> > > > > it halted. The actual TD the endpoint halted on is marked TD_HALTED,
> > > > > and its URB is given back with proper EPROTO or EPIPE error code.
> > > > >
> > > > > Don't automatically restart an endpoint if the next queued TD after
> > > > > the TD_HALTED one is marked tainted.
> > > >
> > > > Sounds good for -EPROTO, but will a clear-halt ring the corresponding
> > > > the endpoint's doorbell for STALL endpoint?
> > > >
> > >
> > > With this change xhci would not restart the stalled endpoint after a clear-halt
> > > request. The first usb_enqueue() call after clear-halt would start it.
> > >
> > > Could make sense to restart the endpoint after a clear-halt, and just add a small
> > > debug message if the next queued URB is marked 'tainted'.
> > >
> >
> > The -EPROTO should be handled differently than -EPIPE. A STALL endpoint
> > is part of a normal usb flow. Should the class driver submit a new URB
> > while the endpoint is STALL, we would always expect a STALL error
> > response after the endpoint is restarted. That's not the case with
> > -EPROTO where the data may be corrupted and/or the host and device are
> > out of sync. We should not continue until the class driver do some
> > recovery. IMHO, we can keep the handling of -EPIPE how it was before.
> > Let the xHC tell whether the STALL error still persists instead of
> > managing it by the xhci driver.
> >
> I agree that that we should restart the endpoint if class/core enqueues a new
> URB _after_ xhci gave back an URB with EPIPE after endpoint STALL.
>
> But I don't think we should restart the ring to continue processing URBs that
> were queued before the endpoint stalled. This would prevent the class/core
> from even attempting to retire the pending URBs, something USB2.0 spec,
> '5.8.5 Bulk Transfer Data Sequences' requires:
>
> "If a halt condition is detected on a bulk pipe due to transmission errors or
> a STALL handshake being returned from the endpoint, all pending IRPs are
> retired. Removal of the halt condition is achieved via software intervention
> through a separate control pipe."
>
Fair point. Then the core will need to track the endpoint's STALL state
and parse the clear-halt request to know which endpoint and when to
clear the STALL before it can accept new URB. So the first usb_enqueue()
call after clear-halt can start the endpoint again. The xhci will also
need to have access to this state.
Currently you have the xhci driver to "retire" the halted URBs. However,
you also noted that class/core may attempt to retire the pending URBs.
Who's expected to handle the retirement here?
On a separate note, will you plan to implement the clear-halt for EPROTO
in xhci?
Thanks!
Thinh
^ 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