Re: [patch] urndis: uncomment watchdog
On Tue, 30 Nov 2021 21:40:35 +0300
Mikhail wrote:
> Currently watchdog functionality for urndis driver is disabled
> (commented), I've tested it and it works properly - reset messages are
> correctly sent and cmplt packets are received according to RNDIS spec
> (I patched the driver to forcedly send reset message and act on it
> with device reset every 5 seconds). I suggest to uncomment it.
>
Well, I am sorry to perhaps hijack the thread, but it seems to me, it
is just your hardware. I am using urndis(4) interface occasionally,
with my cellphone, and it "just works".
so, if your hardware needs special treatment - may be it is better to
make a special case under if(){} branch?
--
With best regards,
Gregory Edigarov
Re: [patch] urndis: uncomment watchdog
On Wed, Dec 08, 2021 at 03:26:25PM +0100, Gerhard Roth wrote: > > I don't want to fight for this diff, if you think that it's too naive to > > expect proper reset from unresponsive device - that's fine, I'm ready to > > drop the diff, but who knows how those devices are engineered and trade > > of of not being able to run other watchdogs comparing to possible > > network recovery does look reasonable to me. > > > > I don't blame the idea of revitializing urndis_watchdog(). But that > code has been disabled for more than 10 years. And its quite different > for what all the other watchdog routines of USB network interface > drivers do. Maybe the code needs rethinking. I was looking on other implementations and didn't find any signs of the same protocol logic - with keepalives and reset messages, so this flow is pretty unique for urndis driver, and I currently don't understand how to re-do it to avoid waiting on timeout. It already looks pretty straightforward and complete.
Re: [patch] urndis: uncomment watchdog
On 12/8/21 15:13, Mikhail wrote:
On Wed, Dec 08, 2021 at 02:43:04PM +0100, Gerhard Roth wrote:
Well, the RNDIS device doesn't respond to REMOTE_NDIS_KEEPALIVE_MSG
messages anymore, but now you hope that it'll still process the
REMOTE_NDIS_RESET_MSG we are sending? Sounds like wishful thinking.
I'd say a usbd_reset_port() might be more effective.
BTW: I was wrong about the 5 seconds. In fact its 10 seconds since the
same timeout applies to the reset message.
I think if the device don't ack the keepalive message the driver will
just output an error to the log and return, there should be no second 5
sec timeout:
748 rval = urndis_ctrl_send(sc, keep, sizeof(*keep));
749 free(keep, M_TEMP, sizeof *keep);
750
751 if (rval != RNDIS_STATUS_SUCCESS) {
752 printf("%s: keepalive failed\n", DEVNAME(sc));
753 return rval;
754 }
755
756 if ((hdr = urndis_ctrl_recv(sc)) == NULL) {
757 printf("%s: unable to get keepalive response\n",
DEVNAME(sc));
758 return RNDIS_STATUS_FAILURE;
759 }
As you see it calls urndis_ctrl_recv() which in turn calls
urndis_ctrl_msg() which in turn calls usbd_do_request().
usbd_do_request() calls usbd_do_request_flags() with a timeout
of USBD_DEFAULT_TIMEOUT (which is 5 seconds). And
usbd_do_request_flags() sets up an xfer with the USBD_SYNCHRONOUS
flag. Hence usbd_transfer() will wait for the completion up to
USBD_DEFAULT_TIMEOUT seconds.
It may be that the transfer fails with USBD_IOERROR. In that case
the I/O completes faster.
I don't want to fight for this diff, if you think that it's too naive to
expect proper reset from unresponsive device - that's fine, I'm ready to
drop the diff, but who knows how those devices are engineered and trade
of of not being able to run other watchdogs comparing to possible
network recovery does look reasonable to me.
I don't blame the idea of revitializing urndis_watchdog(). But that
code has been disabled for more than 10 years. And its quite different
for what all the other watchdog routines of USB network interface
drivers do. Maybe the code needs rethinking.
smime.p7s
Description: S/MIME cryptographic signature
Re: [patch] urndis: uncomment watchdog
On Wed, Dec 08, 2021 at 02:43:04PM +0100, Gerhard Roth wrote:
> Well, the RNDIS device doesn't respond to REMOTE_NDIS_KEEPALIVE_MSG
> messages anymore, but now you hope that it'll still process the
> REMOTE_NDIS_RESET_MSG we are sending? Sounds like wishful thinking.
> I'd say a usbd_reset_port() might be more effective.
> BTW: I was wrong about the 5 seconds. In fact its 10 seconds since the
> same timeout applies to the reset message.
>
I think if the device don't ack the keepalive message the driver will
just output an error to the log and return, there should be no second 5
sec timeout:
748 rval = urndis_ctrl_send(sc, keep, sizeof(*keep));
749 free(keep, M_TEMP, sizeof *keep);
750
751 if (rval != RNDIS_STATUS_SUCCESS) {
752 printf("%s: keepalive failed\n", DEVNAME(sc));
753 return rval;
754 }
755
756 if ((hdr = urndis_ctrl_recv(sc)) == NULL) {
757 printf("%s: unable to get keepalive response\n",
DEVNAME(sc));
758 return RNDIS_STATUS_FAILURE;
759 }
I don't want to fight for this diff, if you think that it's too naive to
expect proper reset from unresponsive device - that's fine, I'm ready to
drop the diff, but who knows how those devices are engineered and trade
of of not being able to run other watchdogs comparing to possible
network recovery does look reasonable to me.
Re: [patch] urndis: uncomment watchdog
On 12/8/21 14:31, Mikhail wrote: On Wed, Dec 08, 2021 at 02:10:49PM +0100, Gerhard Roth wrote: Well, there's only one watchdog thread for all of the network interfaces. If it is blocked, not other watchdogs can run. I don't think this is a big loss. On one side - no other watchdogs can run for 5 secs, but on other side - watchdog can potentially recover the network service with automatic reset of urndis device and return network connectivity. Isn't it a fair trade of? Well, the RNDIS device doesn't respond to REMOTE_NDIS_KEEPALIVE_MSG messages anymore, but now you hope that it'll still process the REMOTE_NDIS_RESET_MSG we are sending? Sounds like wishful thinking. I'd say a usbd_reset_port() might be more effective. BTW: I was wrong about the 5 seconds. In fact its 10 seconds since the same timeout applies to the reset message. smime.p7s Description: S/MIME cryptographic signature
Re: [patch] urndis: uncomment watchdog
On Wed, Dec 08, 2021 at 02:10:49PM +0100, Gerhard Roth wrote: > Well, there's only one watchdog thread for all of the > network interfaces. If it is blocked, not other watchdogs > can run. I don't think this is a big loss. On one side - no other watchdogs can run for 5 secs, but on other side - watchdog can potentially recover the network service with automatic reset of urndis device and return network connectivity. Isn't it a fair trade of?
Re: [patch] urndis: uncomment watchdog
On 12/8/21 14:08, Mikhail wrote: On Wed, Dec 08, 2021 at 10:39:15AM +0100, Gerhard Roth wrote: urndis_watchdog() calls urndis_ctrl_keepalive() which sends an RNDIS keepalive msg and then waits for the reply with USBD_DEFAULT_TIMEOUT. That means if the device stopped responding, the if_watchdog_thread will be blocked for 5 seconds. Not sure if that's a good idea. Hello, I think this behavior is like it is supposed to work. What are drawbacks of having this keepalive thread being blocked while waiting the answer for keepalive? - in case of timeout we will have error message in the logs and user will be able to handle the problem manually. Well, there's only one watchdog thread for all of the network interfaces. If it is blocked, not other watchdogs can run. smime.p7s Description: S/MIME cryptographic signature
Re: [patch] urndis: uncomment watchdog
On Wed, Dec 08, 2021 at 10:39:15AM +0100, Gerhard Roth wrote: > urndis_watchdog() calls urndis_ctrl_keepalive() which sends an RNDIS > keepalive msg and then waits for the reply with USBD_DEFAULT_TIMEOUT. > That means if the device stopped responding, the if_watchdog_thread > will be blocked for 5 seconds. Not sure if that's a good idea. Hello, I think this behavior is like it is supposed to work. What are drawbacks of having this keepalive thread being blocked while waiting the answer for keepalive? - in case of timeout we will have error message in the logs and user will be able to handle the problem manually.
Re: [patch] urndis: uncomment watchdog
On 12/8/21 10:26, Mikhail wrote: On Tue, Nov 30, 2021 at 09:40:35PM +0300, Mikhail wrote: Currently watchdog functionality for urndis driver is disabled (commented), I've tested it and it works properly - reset messages are correctly sent and cmplt packets are received according to RNDIS spec (I patched the driver to forcedly send reset message and act on it with device reset every 5 seconds). I suggest to uncomment it. The hardware is Megafon 4G МM200-1: urndis0 at uhub3 port 2 configuration 1 interface 0 "Qualcomm MM200-1" rev 2.00/3.18 addr 5 Tests, comments or objections? Ping. Commenting was introduced by mk@ in 61cec9357e4f with the following note: At some point we probably want to use the watchdog functionality but the current code is completely untested so disable it entirely rather than enabling it this close to release. I've tested it and it works, maybe there will be a committer who can enable the watchdog? - the functionality is widely used in other drivers and seem to be useful in general. Hi, urndis_watchdog() calls urndis_ctrl_keepalive() which sends an RNDIS keepalive msg and then waits for the reply with USBD_DEFAULT_TIMEOUT. That means if the device stopped responding, the if_watchdog_thread will be blocked for 5 seconds. Not sure if that's a good idea. Gerhard smime.p7s Description: S/MIME cryptographic signature
Re: [patch] urndis: uncomment watchdog
On Tue, Nov 30, 2021 at 09:40:35PM +0300, Mikhail wrote: > Currently watchdog functionality for urndis driver is disabled > (commented), I've tested it and it works properly - reset messages are > correctly sent and cmplt packets are received according to RNDIS spec (I > patched the driver to forcedly send reset message and act on it with > device reset every 5 seconds). I suggest to uncomment it. > > The hardware is Megafon 4G МM200-1: > > urndis0 at uhub3 port 2 configuration 1 interface 0 "Qualcomm MM200-1" rev > 2.00/3.18 addr 5 > > Tests, comments or objections? Ping. Commenting was introduced by mk@ in 61cec9357e4f with the following note: > At some point we probably want to use the watchdog functionality but the > current code is completely untested so disable it entirely rather than > enabling it this close to release. I've tested it and it works, maybe there will be a committer who can enable the watchdog? - the functionality is widely used in other drivers and seem to be useful in general.
