linux-kernel - Re: [PATCH 1/1 v7] usb: serial: add support for CH348

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFBinCB4pDOoE9QCH966uyP0yaVm3CkAi3uncMqEDh19VSmbQw@mail.gmail.com>
Date: Tue, 14 Jan 2025 22:40:56 +0100
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Johan Hovold <johan@...nel.org>
Cc: Corentin Labbe <clabbe@...libre.com>, gregkh@...uxfoundation.org, 
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org, david@...t.cz
Subject: Re: [PATCH 1/1 v7] usb: serial: add support for CH348

Hi Johan,

On Tue, Jan 14, 2025 at 4:39 PM Johan Hovold <johan@...nel.org> wrote:
>
> Hi Martin (and Corentin),
>
> and sorry about the late reply here.
Sometimes there's more important things in life than USB to serial adapters.

[...]
> > > You could try increasing the buffer size to 2k and see how much is
> > > received on the other end if you submit one URB (e.g. does the hardware
> > > just drop the last 1k of data when the device fifo is full).
>
> > I have not tried this yet but if still relevant (after the info about
> > the THRE interrupt) then I can try it and share the results.
>
> It would only be to really confirm that this is how the vendor protocol
> and device works. Your call.
I can give that a try in the next few days and will report back.

[...]
> > > I understand, but the generic implementation is not a good fit here as
> > > it actively tries to make sure the device buffers are always full (e.g.
> > > by using two URBs back-to-back).
> > >
> > > If you can't find a way to make the hardware behave properly then a
> > > custom implementation using a single URBs is preferable over trying
> > > to limit the generic implementation like you did here. Perhaps bits can
> > > be reused anyway (e.g. chars_in_buffer if you use the write fifo).
>
> > I cannot find any other usb-serial driver which uses this pattern.
> > Most devices seem to be happy to take more data once they trigger the
> > write_bulk_callback but not ch348.
>
> Right. I think the io_edgeport driver maintains some kind of TxCredits.
> I guess that's related, but not sure how relevant that is here (and you
> probably shouldn't based anything on that old driver directly anyway).
>
> > If there's any other (even if it's not a usb-serial) driver that I can
> > use as a reference implementation for your suggestion?
> > I'm not sure whether to use a dedicated kthread, single threaded workqueue, ...
>
> Not sure what Corentin has been preparing, but yeah, you need some kind
> of deferred mechanism to make write() non-blocking and hold off sending
> more data to the device until you're sure there's room in its buffers. I
> guess a workqueue should do fine.
We're currently testing an updated driver based on a workqueue
(work_struct) and it's working fine.
The issue that Corentin reported is unrelated to the workqueue part.
At this point we're thinking it may be a regression in linux-next, but
we're running more tests to verify this.


Best regards,
Martin