Message-ID: <cf0f3894-3c62-40b4-af48-afac1c7d0379@ixit.cz>
Date: Fri, 29 Nov 2024 21:52:24 -0500
From: David Heidelberg <david@...t.cz>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 Johan Hovold <johan@...nel.org>
Cc: Corentin Labbe <clabbe@...libre.com>, gregkh@...uxfoundation.org,
 linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/1 v7] usb: serial: add support for CH348

Hello Martin,

are you planning sending v8 soon?

There seems to be general interest, if it makes working on it more joyful:
https://www.reddit.com/r/raspberry_pi/comments/1gznt7p/has_anyone_used_this_board_usb_to_8channel_serial/

Thank you
David

On 25/08/2024 06:56, Martin Blumenstingl wrote:
> Hi Johan,
> 
> On Tue, Jul 23, 2024 at 6:13 PM Johan Hovold <johan@...nel.org> wrote:
> [...]
>>>> Do you need to include any copyrights from the vendor driver? It looks
>>>> like at least some of the code below was copied from somewhere (but
>>>> perhaps not that much).
>>
>>> If you - or someone else - have any advice on this I'd be happy to go with that!
>>
>> If you copy code directly (even if you clean it up after) you should
>> include it, but not necessarily if you just use it for reference for the
>> protocol.
>>
>> It doesn't hurt mentioning anyway when you add the reference to the
>> vendor driver, for example:
>>
>>          Based on the XXX driver:
>>
>>                  https://...
>>
>>          Copyright YYY
> Thanks, I'll include that in the next version.
> 
> [...]
>>> For slow speeds I never receive the "Transmitter holding register
>>> empty" interrupt/signal when using the full TX buffer.
>>> It's not that the interrupt/signal is late - it just never arrives.
>>> I don't know why that is (whether the firmware tries to keep things
>>> "fair" for other ports, ...) though.
>>
>> Perhaps you can run some isolated experiments if you haven't already.
>> Submitting just a single URB with say 128, 512 or 1024 bytes of data and
>> see when/if you ever receive a transmitter holding empty interrupt.
>>
>> How does the vendor driver handle this? Does it really wait for the THRE
>> interrupt before submitting more data?
> The vendor driver:
> - first acquires a per-device (not per port) write_lock [0]
> - then waits for the (per-device, not per port) write buffer to be empty [1]
> - and only then submits more data to be transmitted [2]
> 
>> 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.
> 
> [...]
>>>>> +             * If we ingest more data then usb_serial_generic_write() will
>>>>> +             * internally try to process as much data as possible with any
>>>>> +             * number of URBs without giving us the chance to wait in
>>>>> +             * between transfers.
>>>>
>>>> If the hardware really works this way, then perhaps you should not use
>>>> the generic write implementation. Just maintain a single urb per port
>>>> and don't submit it until the device fifo is empty.
>>
>>> I tried to avoid having to copy & paste (which then also means having
>>> to maintain it down the line) most of the generic write
>>> implementation.
>>> This whole dance with waiting for the "Transmitter holding register
>>> empty" by the way was the reason why parts of the transmit buffer got
>>> lost, see the report from Nicolas in v6 [1]
>>
>> 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.
> 
> 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, ...
> 
>>>>> +static struct usb_serial_driver ch348_device = {
>>>>> +     .driver = {
>>>>> +             .owner = THIS_MODULE,
>>>>> +             .name = "ch348",
>>>>> +     },
>>>>> +     .id_table =             ch348_ids,
>>>>> +     .num_ports =            CH348_MAXPORT,
>>>>> +     .num_bulk_in =          1,
>>>>> +     .num_bulk_out =         1,
>>>>
>>>> Set both of these to 2 so that core verifies that you have all four
>>>> endpoints.
>>
>>> I will have to test this because I thought that:
>>> - using 2 here makes usb-serial allocate an URB as well and by default
>>> assign it to the first and second port
>>> - usb-serial should not touch the second bulk in / bulk out endpoint
>>> (as they're entirely vendor / chip specific)
>>
>> Setting these two should make core make sure that the endpoints exist,
>> and by default they will be assigned to the first and second port, but
>> you can override that calc_num_endpoints() (as you already do).
>>
>> For the second IN EP, you could even let core allocate the URB and use
>> that instead of doing so manually (e.g. by submitting yourself or using
>> the generic read implementation as mxuport does).
> Thanks for the hint - I have tried this and it indeed simplifies the code!
> 
> 
> Best regards,
> Martin
> 
> 
> [0] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1100
> [1] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1152-L1156
> [2] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1166

-- 
David Heidelberg

Powered by blists - more mailing lists