PL2303 requires a full implementation of URB_FUNCTION_GET_STATUS_FROM_DEVICE

[svirot]

Similarly to #6, the PL2303 USB to Serial converter (idVendor 0x067b Prolific Technology, Inc. and
idProduct 0x2303 PL2303 Serial Port) seems to need a URB_FUNCTION_GET_STATUS_FROM_DEVICE to get initialised properly.
Unfortunately, the driver-side fix implemented in 7a92e8e is not enough as the hardware seems to be waiting for the status request (even though it returns 0x0000 as well).

Some Wireshark captures are available: PL2303.zip

• PL2303_direct.pcapng is a direct USB sniff, done without usbip but directly plugging the device into the client
• PL2303_notWorking_usbip_raw.pcapng is the raw TCP traffic when connecting the device through usbip (the server is using Linux kernel 5.4.28 armv7l)
• PL2303_notWorking_usbip_dissected.pcapng is the "dissected" usbip traffic from the previous file, easier to compare to the direct capture.

The driver debug messages have also been logged: PL2303.LOG

The first difference between the direct and the usbip capture is the lack of "GET STATUS" Request/Response (which is expected as usbip does not implement it). But the "SET CONFIGURATION" that follows receives a different response from the device, probably suggesting that the hardware needs the "GET STATUS" to startup properly (this is only my assumption).

Thanks for your help.

[cezanne]

@svirot : From PL2303.LOG, I found some warnings. How about checking #102 ?

[svirot]

Thanks for looking at this @cezanne.
The patch changes the behaviour slightly, adding some "URB Submit/Response" but the content looks slightly different. Regardless, the issue is still there, with a "This device cannot start. (Code 10)" on the Prolific driver.

Here are the text logs and the Wireshark capture:
PL2303_2020_04_22.zip

Thanks again for your help. Please do not hesitate to suggest other tests or changes. I'll try to take a look myself as well.

[svirot]

It seems the "out" URB_FUNCTION_CONTROL_TRANSFER calls that have an empty buffer are not processed properly because there is no buffer but the driver still tries to get it and errors.
So, I'd suggest to change all 4:
if (!in)
to
if (!in && get_read_payload_length(irp) > 0)

This gives better results. Here are the logs.
PL2303_2020_04_22_2.zip

Please share your view on this mod and the resulting logs.

[cezanne]

@svirot :

if (!in && get_read_payload_length(irp) > 0)

Yes, you're right. By the way, your PL2303_SV.LOG has several usbip_vhci:(WW) No transfer buffer entries. Didn't you apply your modification?

[svirot]

Very good spot @cezanne. These are on the write side, and not the read (my modification were only on the read side). They are actually the replies to the zero-buffer that were fixed with the updated "if" statement.
I have fixed it on my fork of the project, but let me know if you want me to give you a patch or create a pull request. Note that there is also an implementation of URB_FUNCTION_GET_STATUS_FROM_DEVICE, as initially suggested in this discussion/issue.

The result of all of this are quite promising as the error on the Prolific driver is now different. I get a "STATUS_DEVICE_POWER_FAILURE" right after the second "CLEAR FEATURE Request/Response". Looking at the debug log shows a "irp will be cancelled" which I'll need to understand and debug. Following that, the driver tries to send a lot of URB_FUNCTION_CONTROL_TRANSFER messages but they all result in a "(EE) Cannot get usbip header".

Here is the log, in case you have any suggestion:
PL2303_SV_2.LOG

Thanks for you help!

[cezanne]

@svirot : Please refer #102 (comment).

A previously mentioned code did not work correctly.
if (!in && get_read_payload_length(irp) > 0)

So the code has been fixed as follows:

usbip-win/driver/vhci/vhci_read.c

Line 566 in 930a1f9

if (!in && urb_ctltrans->TransferBufferLength > 0) {

[svirot]

@cezanne: Thanks for this fix, not knowing how things work, I was considering the 2 implementation equivalent and picked the wrong one :-).
Restarting from your current control_transfer branch (instead of the patched master from a week ago) and applying this change (4 times, for the 4 instances of !in) fixed the issue and made the PL2303 work over USBIP !
Thanks for your great help.

I've also tested it with my implementation of URB_FUNCTION_GET_STATUS_FROM_DEVICE which works as well but also does the "GET STATUS", so I'd encourage you to check that change and see if you want to add as well.

Thanks!

[cezanne]

@svirot :

I've also tested it with my implementation of URB_FUNCTION_GET_STATUS_FROM_DEVICE which works as well but also does the "GET STATUS", so I'd encourage you to check that change and see if you want to add as well.

Could you create a pull request for your implementation?
By the way, does your PL2303 work without actual implementation of URB_FUNCTION_GET_STATUS_FROM_DEVICE ?

[svirot]

Yes it does but there is a chance other devices need it.
I'll have a look at creating a pull request.

[koudis]

Hi all, i have LTE modem which requires GET_STATUS_FROM_DEVICE. so if that implementation works with PL2303 I can test it "by" my modem. Let me known when the PR i created. Dne so 25. 4. 2020 12:19 uživatel svirot <notifications@github.com> napsal:

…

Yes it does but there is a chance other devices need it. I'll have a look at creating a pull request. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#136 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ2XDHYQRFCP3VOV77ZLMDROK2LRANCNFSM4MD24HKA> .

[svirot]

@koudis: please check #145

[svirot]

Implemented in b8d86d0.
PL2303 work when using the control_transfer branch, hopefully merged into the master in the near future.

[cezanne]

@svirot : control_transfer branch is rebased on the master. Could you test it? If passed, I will merge into the master.

[svirot]

It looks good to me and works with the PL2303, thanks.

Should we created a dedicated bugfix commit to replace the 3 remaining if (!in) { in vhci_read.c with something equivalent to: if (!in && urb_ctltrans->TransferBufferLength > 0) { ? I'll create a pull request, but feel free to reject it if you feel it is not a necessary change.

[cezanne]

@svirot: Thanks for information.

Should we created a dedicated bugfix commit to replace the 3 remaining

Sure, I'll review your another PR.