Re: a few usb issues related to edge cases
on 20/12/2011 14:25 Andriy Gapon said the following: I just wanted to draw your attention to the fact that obtaining any locks in the kdb context (or USB polling code in general, even) is not a good idea. Chances of getting into trouble on those locks are probably quite moderate or even low, but they do exist. I am not sure if you are getting any bug reports about such troubles :-) Regular users probably do not use kdb too often and a panic for them is just a crash, so they likely do not expect anything usable/debuggable after that :-) Looking some more at the code I just got myself confused as to how the dumping to a umass device could work when the scheduler is stopped. It seems that the umass_command_start - usbd_transfer_start - usbd_callback_ss_done_defer functions would always put a transfer request onto a queue and try to wake up a thread to process that queue and the request. But that's obviously not going to work when the other thread is not going to be run. Have I missed a code path that leads directly to the controller in this context? Thank you for your help. -- Andriy Gapon ___ freebsd-usb@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-usb To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org
Re: a few usb issues related to edge cases
On Wednesday 21 December 2011 12:29:49 Andriy Gapon wrote: on 20/12/2011 14:25 Andriy Gapon said the following: I just wanted to draw your attention to the fact that obtaining any locks in the kdb context (or USB polling code in general, even) is not a good idea. Chances of getting into trouble on those locks are probably quite moderate or even low, but they do exist. I am not sure if you are getting any bug reports about such troubles :-) Regular users probably do not use kdb too often and a panic for them is just a crash, so they likely do not expect anything usable/debuggable after that :-) Looking some more at the code I just got myself confused as to how the dumping to a umass device could work when the scheduler is stopped. It seems that the umass_command_start - usbd_transfer_start - usbd_callback_ss_done_defer functions would always put a transfer request onto a queue and try to wake up a thread to process that queue and the request. But that's obviously not going to work when the other thread is not going to be run. Have I missed a code path that leads directly to the controller in this context? Thank you for your help. Hi, Those threads should be polled when calling usbd_transfer_poll(). I.E. the wakeup should be stubbed in the !scheduler_running case. --HPS ___ freebsd-usb@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-usb To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org
Re: a few usb issues related to edge cases
on 21/12/2011 18:38 Hans Petter Selasky said the following: On Wednesday 21 December 2011 12:29:49 Andriy Gapon wrote: on 20/12/2011 14:25 Andriy Gapon said the following: I just wanted to draw your attention to the fact that obtaining any locks in the kdb context (or USB polling code in general, even) is not a good idea. Chances of getting into trouble on those locks are probably quite moderate or even low, but they do exist. I am not sure if you are getting any bug reports about such troubles :-) Regular users probably do not use kdb too often and a panic for them is just a crash, so they likely do not expect anything usable/debuggable after that :-) Looking some more at the code I just got myself confused as to how the dumping to a umass device could work when the scheduler is stopped. It seems that the umass_command_start - usbd_transfer_start - usbd_callback_ss_done_defer functions would always put a transfer request onto a queue and try to wake up a thread to process that queue and the request. But that's obviously not going to work when the other thread is not going to be run. Have I missed a code path that leads directly to the controller in this context? Thank you for your help. Hi, Those threads should be polled when calling usbd_transfer_poll(). I.E. the wakeup should be stubbed in the !scheduler_running case. Ah, that's what I missed! -- Andriy Gapon ___ freebsd-usb@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-usb To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org
Re: a few usb issues related to edge cases
Now the juicy stuff :)
on 19/12/2011 16:30 Hans Petter Selasky said the following:
3. Looking at usbd_transfer_poll I see that it touches a lot of locks,
including taking the bus lock. As we've discussed before, this is not safe
in a particular context where the polling is supposed to be used - in the
kdb/ddb context. If the lock is already taken by another thread, then
instead of being able to use a USB keyboard a user would get even less
debug-able crash. Also, it seems that usbd_transfer_poll calls into the
usual state machine with various callbacks and dynamically made decisions
about whether to execute some actions directly or defer their execution to
a different thread.
This is an optimisation. If the current thread can do the job without a LOR,
then we do it right away. Else we let another thread do it. It is possible to
have a more simple model, but then you will also get more task switches.
I think that you are speaking here about the code behavior in the general
context.
And I want to limit this part of discussion to the contexts where
usbd_transfer_poll is actually used - kdb and panic. In those contexts there is
one and only one thread that must do all the work. Other threads are stopped
and frozen in the middle of whatever they were doing before kdb was entered or
panic called (provided SCHEDULER_STOPPED() == true).
That code also touches locks in various places. I
think that it would be more preferable to have a method that does the job
in a more straight-forward way, without touching any locks, ignoring the
usual code paths and assuming that no other treads are running in
parallel. Ditto for the method to submit a request.
The current USB code can be run fine without real locks, if you do a few
tricks. I have a single-threaded BSD-kernel replacement for this which works
like a charm for non-FreeBSD projects.
That's very cool. I believe that various implementations of USB stack for BIOS
and similar are also non-threaded.
I'm going to paste a few lines FYI:
Why not extend struct mtx to have two fields which are only used in case of
system polling (no scheduler running):
struct mtx {
xxx;
int owned_polling = 0;
struct mtx *parent_polling;
};
void
mtx_init(struct mtx *mtx, const char *name, const char *type, int opt)
{
mtx-owned = 0;
mtx-parent = mtx;
}
void
mtx_lock(struct mtx *mtx)
{
mtx = mtx-parent;
mtx-owned++;
}
void
mtx_unlock(struct mtx *mtx)
{
mtx = mtx-parent;
mtx-owned--;
}
int
mtx_owned(struct mtx *mtx)
{
mtx = mtx-parent;
return (mtx-owned != 0);
}
void
mtx_destroy(struct mtx *mtx)
{
/* NOP */
}
Maybe mtx_init, mtx_lock, mtx_unlock mtx_owned, mtx_destroy, etc, could be
function pointers, which are swapped at panic.
I am not sure if I got your idea right based on the pseudo-code above.
Right now in the head we already skip all locks when SCHEDULER_STOPPED() is
true. But, but, that doesn't cover all polling cases as the automatic lock
skipping is _not_ done for kdb. There are various reasons for that. That's why
the kdb_active flag should be checked by the code that can be executed in the
kdb context when dealing with locking or shared resources in general.
USB is SMP!
Right and that's good, but there are cases where SMP is effectively stopped.
Those are panic and kdb.
To run SMP code from a single thread, you need to create a
hiherachy of the threads:
1) Callbacks (Giant)
2) Callbacks (non-Giant)
3) Control EP (non-Giant)
4) Explore thread (non-Giant)
When the explore thread is busy, we look for work in the level above and so
on. The USB stack implements this principle, which is maybe not documented
anywhere btw. If you want more than code, you can hire me to do that.
The mtx-code above I believe is far less work than to make new code which
handles the polling case only.
The reason for the parent mutex field, is to allow easy grouping of mutexes,
so that USB easily can figure out what can run or not.
This all is really above my level of knowledge.
Basically my current intention is the following: make ukbd/usb code work in
panic+SCHEDULER_STOPPED case in the same way (or not worse, at least) as it
currently works for the kdb case. I don't have enough knowledge (and time) to
go beyond that.
I just wanted to draw your attention to the fact that obtaining any locks in the
kdb context (or USB polling code in general, even) is not a good idea.
Chances of getting into trouble on those locks are probably quite moderate or
even low, but they do exist. I am not sure if you are getting any bug reports
about such troubles :-) Regular users probably do not use kdb too often and a
panic for them is just a crash, so they likely do not expect anything
usable/debuggable after that :-)
P.S. Since most (but not all) locking operations in the USB code are already
wrapped under
Re: a few usb issues related to edge cases
On Monday 19 December 2011 13:16:17 Andriy Gapon wrote:
Hans Petter,
I think that I see some issues in the USB code that could cause problems in
some edge cases.
From easiest to hardest:
Hi,
1. I think that currently there is a LOR in usb_bus_shutdown. I think
that the following patch should fix it:
===
--- a/sys/dev/usb/controller/usb_controller.c
+++ b/sys/dev/usb/controller/usb_controller.c
@@ -479,6 +481,7 @@ usb_bus_shutdown(struct usb_proc_msg *pm)
bus_generic_shutdown(bus-bdev);
+ USB_BUS_UNLOCK(bus);
usbd_enum_lock(udev);
err = usbd_set_config_index(udev, USB_UNCONFIG_INDEX);
@@ -497,6 +500,7 @@ usb_bus_shutdown(struct usb_proc_msg *pm)
(bus-methods-set_hw_power_sleep) (bus, USB_HW_POWER_SHUTDOWN);
usbd_enum_unlock(udev);
+ USB_BUS_LOCK(bus);
}
You are right! I believe my kernel tests were run without WITNESS.
2. Somewhat related to the above. I think that because the USB subsystem
implements the shutdown method and detaches all its drivers, then the ukbd
driver won't be able to properly handle the 'shutdown -h' case where the
kernel asks to press any key to reboot at the end. Depending on which
thread wins the race (the one that executes the mainline shutdown code or
the USB explore thread that detaches USB devices) there will either an
immediate reboot or a later crash when any key is pressed.
This is not critical, but OTOH perhaps the USB subsystem doesn't have to do
the shutdown. As far as I can see a lot of the drivers just do nothing
for the shutdown, for better or for worth.
A side note: perhaps it would be a good idea to pass the 'how' value as an
additional parameter to device_shutdown.
The shutdown of USB is done to give USB devices at last chance to turn off or
reduce their current consumption.
In the old code the Host controller itself would be disabled, so keyboard
wouldn't have worked I believe like you suggest.
BTW: Shutdown should be executed after any Press any key to reboot. and
shutdown should be given time to complete, hence for USB this needs to happen
in sync with the rest of the USB system.
3. Looking at usbd_transfer_poll I see that it touches a lot of locks,
including taking the bus lock. As we've discussed before, this is not safe
in a particular context where the polling is supposed to be used - in the
kdb/ddb context. If the lock is already taken by another thread, then
instead of being able to use a USB keyboard a user would get even less
debug-able crash. Also, it seems that usbd_transfer_poll calls into the
usual state machine with various callbacks and dynamically made decisions
about whether to execute some actions directly or defer their execution to
a different thread.
This is an optimisation. If the current thread can do the job without a LOR,
then we do it right away. Else we let another thread do it. It is possible to
have a more simple model, but then you will also get more task switches.
That code also touches locks in various places. I
think that it would be more preferable to have a method that does the job
in a more straight-forward way, without touching any locks, ignoring the
usual code paths and assuming that no other treads are running in
parallel. Ditto for the method to submit a request.
The current USB code can be run fine without real locks, if you do a few
tricks. I have a single-threaded BSD-kernel replacement for this which works
like a charm for non-FreeBSD projects. I'm going to paste a few lines FYI:
Why not extend struct mtx to have two fields which are only used in case of
system polling (no scheduler running):
struct mtx {
xxx;
int owned_polling = 0;
struct mtx *parent_polling;
};
void
mtx_init(struct mtx *mtx, const char *name, const char *type, int opt)
{
mtx-owned = 0;
mtx-parent = mtx;
}
void
mtx_lock(struct mtx *mtx)
{
mtx = mtx-parent;
mtx-owned++;
}
void
mtx_unlock(struct mtx *mtx)
{
mtx = mtx-parent;
mtx-owned--;
}
int
mtx_owned(struct mtx *mtx)
{
mtx = mtx-parent;
return (mtx-owned != 0);
}
void
mtx_destroy(struct mtx *mtx)
{
/* NOP */
}
Maybe mtx_init, mtx_lock, mtx_unlock mtx_owned, mtx_destroy, etc, could be
function pointers, which are swapped at panic.
USB is SMP! To run SMP code from a single thread, you need to create a
hiherachy of the threads:
1) Callbacks (Giant)
2) Callbacks (non-Giant)
3) Control EP (non-Giant)
4) Explore thread (non-Giant)
When the explore thread is busy, we look for work in the level above and so
on. The USB stack implements this principle, which is maybe not documented
anywhere btw. If you want more than code, you can hire me to do that.
The mtx-code above I believe is far less work than to make new code which
handles the polling case only.
The reason for the parent mutex field, is to allow easy
Re: a few usb issues related to edge cases
First replying just to couple of points where there seems to be a misunderstanding. on 19/12/2011 16:30 Hans Petter Selasky said the following: 2. Somewhat related to the above. I think that because the USB subsystem implements the shutdown method and detaches all its drivers, then the ukbd driver won't be able to properly handle the 'shutdown -h' case where the kernel asks to press any key to reboot at the end. Depending on which thread wins the race (the one that executes the mainline shutdown code or the USB explore thread that detaches USB devices) there will either an immediate reboot or a later crash when any key is pressed. This is not critical, but OTOH perhaps the USB subsystem doesn't have to do the shutdown. As far as I can see a lot of the drivers just do nothing for the shutdown, for better or for worth. A side note: perhaps it would be a good idea to pass the 'how' value as an additional parameter to device_shutdown. The shutdown of USB is done to give USB devices at last chance to turn off or reduce their current consumption. In the old code the Host controller itself would be disabled, so keyboard wouldn't have worked I believe like you suggest. I am not sure about the old code, I have never checked it. But the atkbd definitely works at this stage. BTW: Shutdown should be executed after any Press any key to reboot. and shutdown should be given time to complete, hence for USB this needs to happen in sync with the rest of the USB system. Have you actually ever done shutdown -h? In other words do you know what the system halt is? :) I am not sure if it would be a good idea to declare a system as halted before shutdown_final hooks are executed. I would rather sacrifice the whole press a key interactivity and simply executed hlt. That's because I think that the system halt has a very limited usage, mostly in combination with UPS, where interactivity via console/keyboard is not very important. BTW, the reason that I suggested to pass 'how' to device_shutdown is to give drivers some choice. E.g. USB could the whole shutdown thing for the cases of poweroff and reboot, but keep the devices going for halt. But probably right now we just need to make a decision whether ukbd is going to support system halt or not. If not, then I think that usb_shutdown() must wait until the explore_proc terminates. If yes, then usb_shutdown() should become a noop. Or it could become quite smart to detach/poweroff other devices in such a way that ukbd still stays usable. But that's probably harder to implement. [snip] As a side note: we probably need a flag to mark certain things such as e.g. the ukbd driver as non recoverable, meaning that once those are used in the kdb context then there is no safe way to go back to normal system operation. I think you need to do shutdown _after_ the Press any key to reboot. A flag won't help. Umm, this suggestion was about entering and exiting KDB/DDB, not about shutdown/reboot. P.S. I've just looked at the code in stable/7 and it seems that it didn't actually unconfigured USB and detached device drivers. At least ohci_shutdown and ohci_shutdown are not called on FreeBSD. -- Andriy Gapon ___ freebsd-usb@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-usb To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org
Re: a few usb issues related to edge cases
On Monday 19 December 2011 16:06:13 Andriy Gapon wrote: First replying just to couple of points where there seems to be a misunderstanding. on 19/12/2011 16:30 Hans Petter Selasky said the following: 2. Somewhat related to the above. I think that because the USB subsystem implements the shutdown method and detaches all its drivers, then the ukbd driver won't be able to properly handle the 'shutdown -h' case where the kernel asks to press any key to reboot at the end. Depending on which thread wins the race (the one that executes the mainline shutdown code or the USB explore thread that detaches USB devices) there will either an immediate reboot or a later crash when any key is pressed. This is not critical, but OTOH perhaps the USB subsystem doesn't have to do the shutdown. As far as I can see a lot of the drivers just do nothing for the shutdown, for better or for worth. A side note: perhaps it would be a good idea to pass the 'how' value as an additional parameter to device_shutdown. The shutdown of USB is done to give USB devices at last chance to turn off or reduce their current consumption. In the old code the Host controller itself would be disabled, so keyboard wouldn't have worked I believe like you suggest. I am not sure about the old code, I have never checked it. But the atkbd definitely works at this stage. ATKBD is no comparison to UKBD :-) BTW: Shutdown should be executed after any Press any key to reboot. and shutdown should be given time to complete, hence for USB this needs to happen in sync with the rest of the USB system. Have you actually ever done shutdown -h? In other words do you know what the system halt is? :) No, I'm usually shutdown -p now. I am not sure if it would be a good idea to declare a system as halted before shutdown_final hooks are executed. I would rather sacrifice the whole press a key interactivity and simply executed hlt. That's because I think that the system halt has a very limited usage, mostly in combination with UPS, where interactivity via console/keyboard is not very important. BTW, the reason that I suggested to pass 'how' to device_shutdown is to give drivers some choice. E.g. USB could the whole shutdown thing for the cases of poweroff and reboot, but keep the devices going for halt. I see. But probably right now we just need to make a decision whether ukbd is going to support system halt or not. If not, then I think that usb_shutdown() must wait until the explore_proc terminates. If yes, then usb_shutdown() should become a noop. Or it could become quite smart to detach/poweroff other devices in such a way that ukbd still stays usable. But that's probably harder to implement. I will fix that. I see a missing wait there. Can I assume that we are allowed to sleep from device_shutdown() and that system timers still work? P.S. I've just looked at the code in stable/7 and it seems that it didn't actually unconfigured USB and detached device drivers. At least ohci_shutdown and ohci_shutdown are not called on FreeBSD. Hmm. --HPS ___ freebsd-usb@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-usb To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org
Re: a few usb issues related to edge cases
on 19/12/2011 17:11 Hans Petter Selasky said the following: I will fix that. I see a missing wait there. Can I assume that we are allowed to sleep from device_shutdown() and that system timers still work? I don't see any reason why either of these should be not true. Oh, and I see that you've already committed the change - thanks! -- Andriy Gapon ___ freebsd-usb@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-usb To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org
