Re: Status of PCIe Hotplug?

2016-09-28 Thread John Baldwin
On Wednesday, September 28, 2016 11:55:47 AM Hans Petter Selasky wrote:
> On 09/28/16 11:31, Jan Henrik Sylvester wrote:
> > On 09/28/2016 10:32, Hans Petter Selasky wrote:
> >> Can you compile your kernel with debugging enabled so that we get the
> >> sourcelines in the dump below. Also please include some parts of the
> >
> > The kernel is from the yet-to-be-released 11.0-RELEASE installed by
> > freebsd-update. Now I have extracted kernel-dbg.txz, base-dbg.txz, and
> > lib32-dbg.txz from the 11.0-RELEAESE distribution, but the backtrace
> > still looks similar. Do have have to do anything else to use the debug
> > symbols or do I really have to compile a new kernel?
> >
> 
> Hi,
> 
> Can you try the attached patch. Does it make any difference?
> 
> device_delete_child() will delete devices starting at the leaves, while 
> USB expects parents to be detached first.
> 
> jhb: Any comments?

This is fine, though I feel like we should fix device_delete_child instead
as this has broken several places.  That is, I think device_delete_child
should do the detach first and then delete any dangling grandchildren
after device_detach() of the child.

-- 
John Baldwin
___
freebsd-usb@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to "freebsd-usb-unsubscr...@freebsd.org"


Re: USB ports on Lenovo T400 do not work after a suspend/resume

2013-07-08 Thread John Baldwin
On Sunday, June 30, 2013 10:22:09 am Ian Smith wrote:
 On Sat, 29 Jun 2013, Adrian Chadd wrote:
   On 27 June 2013 04:58, Ian Smith smi...@nimnet.asn.au wrote:
We don't yet know if this is a bus, ACPI /or USB issue.  Home yet? :)
   
   Yup:
   
   http://people.freebsd.org/~adrian/usb/
   
   dmesg.boot = dmesg at startup
   
   1 - after powerup, usb device in
   2 - after acpiconf -s3 suspend/resume, w/ a USB device plugged in
   3 - after acpiconf -s3 suspend/resume, with a USB device removed
   before suspend/resume
 
 After removing [numbers] (for WITNESS?), diff started making sense.  
 The below is between the first and second suspend/resume cycles in 
 dmesg-3.txt, encompassing the others.
 
 Nothing of note that I can see, if that usb hub-to-bus remapping is 
 normal.  As you said, 'CPU0: local APIC error 0x40' looks maybe sus.  
 Maybe someone who knows might comment on that?

From sys/amd64/include/apicreg.h:

/* fields in ESR */
#define APIC_ESR_SEND_CS_ERROR  0x0001
#define APIC_ESR_RECEIVE_CS_ERROR   0x0002
#define APIC_ESR_SEND_ACCEPT0x0004
#define APIC_ESR_RECEIVE_ACCEPT 0x0008
#define APIC_ESR_SEND_ILLEGAL_VECTOR0x0020
#define APIC_ESR_RECEIVE_ILLEGAL_VECTOR 0x0040
#define APIC_ESR_ILLEGAL_REGISTER   0x0080

Receive illegal vector (if look in Intel's SDM manuals) means it
got an interrupt vector  32 (probably zero).  Perhaps it asserted
an interrupt in an I/O APIC before the I/O APIC was properly reset?
Are you using MSI at all?

-- 
John Baldwin
___
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: kern_yield vs ukbd_yield

2011-12-12 Thread John Baldwin
On Monday, December 12, 2011 10:58:22 am Hans Petter Selasky wrote:
 On Monday 12 December 2011 16:55:38 m...@freebsd.org wrote:
  On Mon, Dec 12, 2011 at 12:03 AM, Andriy Gapon a...@freebsd.org wrote:
   on 11/12/2011 23:48 m...@freebsd.org said the following:
   On Sun, Dec 11, 2011 at 1:12 PM, Andriy Gapon a...@freebsd.org wrote:
   Does the following change do what I think that it does?
   Thank you!
   
   Author: Andriy Gapon a...@icyb.net.ua
   Date:   Thu Sep 1 16:50:13 2011 +0300
   
  ukbd: drop local duplicate of kern_yield and use that instead
   
   diff --git a/sys/dev/usb/input/ukbd.c b/sys/dev/usb/input/ukbd.c
   index 086c178..8078cbb 100644
   --- a/sys/dev/usb/input/ukbd.c
   +++ b/sys/dev/usb/input/ukbd.c
   @@ -399,33 +399,6 @@ ukbd_put_key(struct ukbd_softc *sc, uint32_t key)
}
   
static void
   -ukbd_yield(void)
   -{
   -   struct thread *td = curthread;
   -   uint32_t old_prio;
   -
   -   DROP_GIANT();
   -
   -   thread_lock(td);
   -
   -   /* get current priority */
   -   old_prio = td-td_base_pri;
   -
   -   /* set new priority */
   -   sched_prio(td, td-td_user_pri);
   -
   -   /* cause a task switch */
   -   mi_switch(SW_INVOL | SWT_RELINQUISH, NULL);
   -
   -   /* restore priority */
   -   sched_prio(td, old_prio);
   -
   -   thread_unlock(td);
   -
   -   PICKUP_GIANT();
   -}
   -
   -static void
ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait)
{
   
   @@ -439,7 +412,7 @@ ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait)
  while (sc-sc_inputs == 0) {
   
  /* give USB threads a chance to run */
   -   ukbd_yield();
   +   kern_yield(-1);
   
   Not quite.
   
   1) -1 should be spelled PRI_UNCHANGED, except ukbd_yield() uses
   td_user_pri, but then puts it back again, so I think UNCHANGED is what
   is meant.
   2) kern_yield() calls it a SW_VOL rather than SW_INVOL, which seems
   the desired behaviour here anyways, since this is an explicit (i.e.
   voluntary) yield.
   
   Thank you for the explanation.  So would you say that the patch is OK?
  
  As far as I know, yes.  There may be a difference in behaviour,
  though, while yielding, if the priority of the thread remains high (as
  this change would make it) -- I'm not completely sure how the
  scheduler chooses threads, because I'm pretty sure I've seen it take
  threads with lower (higher numbered) priorities even when there's
  runnable threads with a higher (lower numbered) priority available.

The scheduler generally should not do this with the following exceptions:

 1) for timesharing threads, priorities are in bands, so effectively the
pri / 4 is what is used for comparison and if two threads have the same
pri / 4 the scheduler will round-robin among htem.

 2) ULE might be a bit different because of the way it assigns threads to
CPUs, so if a CPU has two high priority threads and another CPU only
has a low priority thread, the second CPU will not run the second high
priority thread.  4BSD handles this case more correctly.

  It has always seemed weird to me that the priorities in the kernel are
  strictly higher than user-space -- but only after a prio change like
  that done implicitly by many of the calls to sleep(9).  So it may be
  that the better patch is to use PRI_USER, not PRI_UNCHANGED, which
  would revert any potentially changed thread prio (e.g. due to a
  sleep(9)) back to its user-space level, so that it contended as
  expected with other threads.

Realtime priorities (for rtprio user threads) are higher than the kernel
sleep priorities.  Also, keep in mind that a thread does not get an
automatic priority boost when it enters the kernel.  It only gets a boost
either temporarily from priority propagation, or a slightly longer (but
still temporary) boost from a sleep(9) call.

 Hi,
 
  hselasky@ or someone else familiar with the various usb threads would
  have to answer that.
 
 The problem is only during init() where the init thread has highest priority 
 and that doesn't allow other threads to run even if the scheduler is 
running!

Hmm, that should be fixed by lowering the relevant thread's priority.
Do you mean thread0 (the one doing all the SYSINIT's or thread we create for
init (pid 1) before it executes init?

-- 
John Baldwin
___
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: kern_yield vs ukbd_yield

2011-12-12 Thread John Baldwin
On Monday, December 12, 2011 2:13:49 pm m...@freebsd.org wrote:
 On Mon, Dec 12, 2011 at 11:05 AM, John Baldwin j...@freebsd.org wrote:
  On Monday, December 12, 2011 10:58:22 am Hans Petter Selasky wrote:
  On Monday 12 December 2011 16:55:38 m...@freebsd.org wrote:
   On Mon, Dec 12, 2011 at 12:03 AM, Andriy Gapon a...@freebsd.org wrote:
on 11/12/2011 23:48 m...@freebsd.org said the following:
On Sun, Dec 11, 2011 at 1:12 PM, Andriy Gapon a...@freebsd.org 
wrote:
Does the following change do what I think that it does?
Thank you!
   
Author: Andriy Gapon a...@icyb.net.ua
Date:   Thu Sep 1 16:50:13 2011 +0300
   
   ukbd: drop local duplicate of kern_yield and use that instead
   
diff --git a/sys/dev/usb/input/ukbd.c b/sys/dev/usb/input/ukbd.c
index 086c178..8078cbb 100644
--- a/sys/dev/usb/input/ukbd.c
+++ b/sys/dev/usb/input/ukbd.c
@@ -399,33 +399,6 @@ ukbd_put_key(struct ukbd_softc *sc, uint32_t 
key)
 }
   
 static void
-ukbd_yield(void)
-{
-   struct thread *td = curthread;
-   uint32_t old_prio;
-
-   DROP_GIANT();
-
-   thread_lock(td);
-
-   /* get current priority */
-   old_prio = td-td_base_pri;
-
-   /* set new priority */
-   sched_prio(td, td-td_user_pri);
-
-   /* cause a task switch */
-   mi_switch(SW_INVOL | SWT_RELINQUISH, NULL);
-
-   /* restore priority */
-   sched_prio(td, old_prio);
-
-   thread_unlock(td);
-
-   PICKUP_GIANT();
-}
-
-static void
 ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait)
 {
   
@@ -439,7 +412,7 @@ ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait)
   while (sc-sc_inputs == 0) {
   
   /* give USB threads a chance to run */
-   ukbd_yield();
+   kern_yield(-1);
   
Not quite.
   
1) -1 should be spelled PRI_UNCHANGED, except ukbd_yield() uses
td_user_pri, but then puts it back again, so I think UNCHANGED is what
is meant.
2) kern_yield() calls it a SW_VOL rather than SW_INVOL, which seems
the desired behaviour here anyways, since this is an explicit (i.e.
voluntary) yield.
   
Thank you for the explanation.  So would you say that the patch is OK?
  
   As far as I know, yes.  There may be a difference in behaviour,
   though, while yielding, if the priority of the thread remains high (as
   this change would make it) -- I'm not completely sure how the
   scheduler chooses threads, because I'm pretty sure I've seen it take
   threads with lower (higher numbered) priorities even when there's
   runnable threads with a higher (lower numbered) priority available.
 
  The scheduler generally should not do this with the following exceptions:
 
   1) for timesharing threads, priorities are in bands, so effectively the
 pri / 4 is what is used for comparison and if two threads have the same
 pri / 4 the scheduler will round-robin among htem.
 
   2) ULE might be a bit different because of the way it assigns threads to
 CPUs, so if a CPU has two high priority threads and another CPU only
 has a low priority thread, the second CPU will not run the second high
 priority thread.  4BSD handles this case more correctly.
 
   It has always seemed weird to me that the priorities in the kernel are
   strictly higher than user-space -- but only after a prio change like
   that done implicitly by many of the calls to sleep(9).  So it may be
   that the better patch is to use PRI_USER, not PRI_UNCHANGED, which
   would revert any potentially changed thread prio (e.g. due to a
   sleep(9)) back to its user-space level, so that it contended as
   expected with other threads.
 
  Realtime priorities (for rtprio user threads) are higher than the kernel
  sleep priorities.  Also, keep in mind that a thread does not get an
  automatic priority boost when it enters the kernel.  It only gets a boost
  either temporarily from priority propagation, or a slightly longer (but
  still temporary) boost from a sleep(9) call.
 
 I still don't understand why threads are boosting their priority while
 sleeping; if it's so they are woken preferentially, that makes some
 sense, but then they shouldn't be keeping the boosted priority after
 the thread is woken.  If a thread really needs higher priority to do
 its work, that should be an explicit call to sched_prio(9), rather
 than implicitly only when/if it first sleeps.

This is how the BSD scheduler traditionally boosted the priority of
interactive threads to favor them over CPU-bound threads.  That is, BSD
assumed that interactive threads are going to be blocking in the kernel
(on a socket, on a tty, etc.), then running for a little bit after waking
before blocking again.  The boost when you sleep is to let threads with
that work pattern always have a higher priority than

Re: use_generic and usb probing

2011-04-06 Thread John Baldwin
On Wednesday, April 06, 2011 3:33:47 am Hans Petter Selasky wrote:
 On Tuesday 05 April 2011 18:45:51 Andriy Gapon wrote:
  on 05/04/2011 15:55 Hans Petter Selasky said the following:
   On Tuesday 05 April 2011 14:50:43 Andriy Gapon wrote:
   I believe that newbus already supports ordering of children on a bus.
   
   BTW, does USB have to pass anything from probe to attach?
   
   Mostly only the driver info field. To avoid duplicate lookups.
   
   Duplicate lookup is of course not very nice, but duplicate probing pass
   is not nice either.
   
   This can all be avoided if the bus-drivers are sorted correctly before
   probing!
 
 Hi,
 
  
  Well, I think that that's what probe priorities actually for.
  I also think that typically ivars should be set by a bus driver.  So maybe
  it's not such a good idea to pass data from probe to attach via ivars in
  child drivers. But I could be mistaken about that.
  
 
 The same device_t is used to do all the probes!
 
  Practically speaking, you most likely don't have to worry about that for
  drivers that return BUS_PROBE_SPECIFIC (=0) from their probe methods.  And
  there is only a few generic drivers that can be handled on a case by
  case basis.
 
 There are more drivers that needs patching! Please scan all of the kernel 
 files for usbdi.h and the use_generic flag.
 
 After looking at subr_usb.c I see your solution is fine as long as the 
 PROBE() 
 method that it attaches is the last one called before ATTACH(). If this is 
 documented in how newbus should function, then please go ahead updating your 
 patch to cover all USB drivers using use_generic.

Yes.  The device_probe() routine is called for the best matching driver
(based on the return values from device_probe()) before device_attach() is
called.  Check device_probe_child() for the gory details including:

if (pri  0) {
/*
 * A bit bogus. Call the probe method again to make
 * sure that we have the right description.
 */
DEVICE_PROBE(child);
#if 0
child-flags |= DF_REBID;
#endif
} else
child-flags = ~DF_REBID;
child-state = DS_ALIVE;

-- 
John Baldwin
___
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: [RFC] Outline of USB process integration in the kernel taskqueue system

2010-11-08 Thread John Baldwin
On Saturday, November 06, 2010 4:33:17 pm Matthew Fleming wrote:
 On Sat, Nov 6, 2010 at 7:22 AM, Hans Petter Selasky hsela...@c2i.net wrote:
  Hi,
 
  On Saturday 06 November 2010 14:57:50 Matthew Fleming wrote:
 
  I think you're misunderstanding the existing taskqueue(9) implementation.
 
  As long as TQ_LOCK is held, the state of ta-ta_pending cannot change,
  nor can the set of running tasks.  So the order of checks is
  irrelevant.
 
  I agree that the order of checks is not important. That is not the problem.
 
  Cut  paste from suggested taskqueue patch from Fleming:
 
+int
   +taskqueue_cancel(struct taskqueue *queue, struct task *task)
   +{
   +   int rc;
   +
   +   TQ_LOCK(queue);
   +   if (!task_is_running(queue, task)) {
   +   if ((rc = task-ta_pending)  0)
   +   STAILQ_REMOVE(queue-tq_queue, task, task,
   ta_link); +   task-ta_pending = 0;
   +   } else {
   +   rc = -EBUSY;
 
  What happens in this case if ta_pending  0. Are you saying this is not
  possible? If ta_pending  0, shouldn't we also do a STAILQ_REMOVE() ?
 
 Ah!  I see what you mean.
 
 I'm not quite sure what the best thing to do here is; I agree it would
 be nice if taskqueue_cancel(9) dequeued the task, but I believe it
 also needs to indicate that the task is currently running.  I guess
 the best thing would be to return the old pending count by reference
 parameter, and 0 or EBUSY to also indicate if there is a task
 currently running.
 
 Adding jhb@ to this mail since he has good thoughts on interfacing.

I agree we should always dequeue when possible.  I think it should return
-EBUSY in that case.  That way code that uses 'cancel' followed by a
conditional 'drain' to implement a blocking 'cancel' will DTRT.

-- 
John Baldwin
___
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: [RFC] Outline of USB process integration in the kernel taskqueue system

2010-11-08 Thread John Baldwin
On Monday, November 08, 2010 10:34:33 am Matthew Fleming wrote:
 On Mon, Nov 8, 2010 at 6:47 AM, John Baldwin j...@freebsd.org wrote:
  On Saturday, November 06, 2010 4:33:17 pm Matthew Fleming wrote:
  On Sat, Nov 6, 2010 at 7:22 AM, Hans Petter Selasky hsela...@c2i.net 
  wrote:
   Hi,
  
   On Saturday 06 November 2010 14:57:50 Matthew Fleming wrote:
  
   I think you're misunderstanding the existing taskqueue(9) 
   implementation.
  
   As long as TQ_LOCK is held, the state of ta-ta_pending cannot change,
   nor can the set of running tasks.  So the order of checks is
   irrelevant.
  
   I agree that the order of checks is not important. That is not the 
   problem.
  
   Cut  paste from suggested taskqueue patch from Fleming:
  
 +int
+taskqueue_cancel(struct taskqueue *queue, struct task *task)
+{
+   int rc;
+
+   TQ_LOCK(queue);
+   if (!task_is_running(queue, task)) {
+   if ((rc = task-ta_pending)  0)
+   STAILQ_REMOVE(queue-tq_queue, task, task,
ta_link); +   task-ta_pending = 0;
+   } else {
+   rc = -EBUSY;
  
   What happens in this case if ta_pending  0. Are you saying this is not
   possible? If ta_pending  0, shouldn't we also do a STAILQ_REMOVE() ?
 
  Ah!  I see what you mean.
 
  I'm not quite sure what the best thing to do here is; I agree it would
  be nice if taskqueue_cancel(9) dequeued the task, but I believe it
  also needs to indicate that the task is currently running.  I guess
  the best thing would be to return the old pending count by reference
  parameter, and 0 or EBUSY to also indicate if there is a task
  currently running.
 
  Adding jhb@ to this mail since he has good thoughts on interfacing.
 
  I agree we should always dequeue when possible.  I think it should return
  -EBUSY in that case.  That way code that uses 'cancel' followed by a
  conditional 'drain' to implement a blocking 'cancel' will DTRT.
 
 Do we not also want the old ta_pending to be returned?  In the case
 where a task is pending and is also currently running (admittedly a
 narrow window), how would we do this?  This is why I suggested
 returning the old ta_pending by reference.  This also allows callers
 who don't care about the old pending to pass NULL and ignore it.

I would be fine with that then.  I wonder if taskqueue_cancel() could then
return a simple true/false.  False if the task is running, and true
otherwise?

-- 
John Baldwin
___
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: [RFC] Outline of USB process integration in the kernel taskqueue system

2010-11-08 Thread John Baldwin
On Monday, November 08, 2010 11:46:58 am Matthew Fleming wrote:
 On Mon, Nov 8, 2010 at 8:42 AM, John Baldwin j...@freebsd.org wrote:
  On Monday, November 08, 2010 10:34:33 am Matthew Fleming wrote:
  On Mon, Nov 8, 2010 at 6:47 AM, John Baldwin j...@freebsd.org wrote:
   On Saturday, November 06, 2010 4:33:17 pm Matthew Fleming wrote:
   On Sat, Nov 6, 2010 at 7:22 AM, Hans Petter Selasky hsela...@c2i.net 
   wrote:
Hi,
   
On Saturday 06 November 2010 14:57:50 Matthew Fleming wrote:
   
I think you're misunderstanding the existing taskqueue(9) 
implementation.
   
As long as TQ_LOCK is held, the state of ta-ta_pending cannot 
change,
nor can the set of running tasks.  So the order of checks is
irrelevant.
   
I agree that the order of checks is not important. That is not the 
problem.
   
Cut  paste from suggested taskqueue patch from Fleming:
   
  +int
 +taskqueue_cancel(struct taskqueue *queue, struct task *task)
 +{
 +   int rc;
 +
 +   TQ_LOCK(queue);
 +   if (!task_is_running(queue, task)) {
 +   if ((rc = task-ta_pending)  0)
 +   STAILQ_REMOVE(queue-tq_queue, task, task,
 ta_link); +   task-ta_pending = 0;
 +   } else {
 +   rc = -EBUSY;
   
What happens in this case if ta_pending  0. Are you saying this is 
not
possible? If ta_pending  0, shouldn't we also do a STAILQ_REMOVE() ?
  
   Ah!  I see what you mean.
  
   I'm not quite sure what the best thing to do here is; I agree it would
   be nice if taskqueue_cancel(9) dequeued the task, but I believe it
   also needs to indicate that the task is currently running.  I guess
   the best thing would be to return the old pending count by reference
   parameter, and 0 or EBUSY to also indicate if there is a task
   currently running.
  
   Adding jhb@ to this mail since he has good thoughts on interfacing.
  
   I agree we should always dequeue when possible.  I think it should return
   -EBUSY in that case.  That way code that uses 'cancel' followed by a
   conditional 'drain' to implement a blocking 'cancel' will DTRT.
 
  Do we not also want the old ta_pending to be returned?  In the case
  where a task is pending and is also currently running (admittedly a
  narrow window), how would we do this?  This is why I suggested
  returning the old ta_pending by reference.  This also allows callers
  who don't care about the old pending to pass NULL and ignore it.
 
  I would be fine with that then.  I wonder if taskqueue_cancel() could then
  return a simple true/false.  False if the task is running, and true
  otherwise?
 
 Sure, though since we don't really have a bool type in the kernel I'd
 still prefer to return an int with EBUSY meaning a task was running.

The only reason I would prefer the other sense (false if already running)
is that is what callout_stop()'s return value is (true if it stopped the
callout, false if it couldn't stop it)).

However, returning EBUSY is ok.  One difference is that callout_stop()
returns false if the callout is not pending (i.e. not on softclock).  Right
now taskqueue_cancel() returns 0 in that case (ta_pending == 0), but that is
probably fine as long as it is documented.  If you wanted to return EINVAL
instead, that would be fine, too.

-- 
John Baldwin
___
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: [RFC] Outline of USB process integration in the kernel taskqueue system

2010-11-05 Thread John Baldwin
On Thursday, November 04, 2010 5:49:22 pm Matthew Fleming wrote:
 On Thu, Nov 4, 2010 at 2:22 PM, John Baldwin j...@freebsd.org wrote:
  On Thursday, November 04, 2010 4:15:16 pm Hans Petter Selasky wrote:
  I think that if a task is currently executing, then there should be a drain
  method for that. I.E. two methods: One to stop and one to cancel/drain. Can
  you implement this?
 
  I agree, this would also be consistent with the callout_*() API if you had
  both stop() and drain() methods.
 
 Here's my proposed code.  Note that this builds but is not yet tested.
 
 
 Implement a taskqueue_cancel(9), to cancel a task from a queue.
 
 Requested by:   hps
 Original code:  jeff
 MFC after:  1 week
 
 
 http://people.freebsd.org/~mdf/bsd-taskqueue-cancel.diff

For FreeBSD taskqueue_cancel() should return EBUSY, not -EBUSY.  However, I
would prefer that it follow the semantics of callout_stop() and return true
if it stopped the task and false otherwise.  The Linux wrapper for
taskqueue_cancel() can convert the return value.

I'm not sure I like reusing the memory allocation flags (M_NOWAIT / M_WAITOK)
for this blocking flag.  In the case of callout(9) we just have two functions
that pass an internal boolean to the real routine (callout_stop() and
callout_drain() are wrappers for _callout_stop_safe()).  It is a bit
unfortunate that taskqueue_drain() already exists and has different semantics
than callout_drain().  It would have been nice to have the two APIs mirror each
other instead.

Hmm, I wonder if the blocking behavior cannot safely be provided by just
doing:

if (!taskqueue_cancel(queue, task, M_NOWAIT)
taskqueue_drain(queue, task);

If that works ok (I think it does), I would rather have taskqueue_cancel()
always be non-blocking.  Even though there is a race where the task could
be rescheduled by another thread in between cancel and drain, the race still
exists since if the task could be scheduled between the two, it could also
be scheduled just before the call to taskqueue_cancel() (in which case a
taskqueue_cancel(queue, task, M_WAITOK) would have blocked to wait for it
matching the taskqueue_drain() above).  The caller still always has to
provide synchronization for preventing a task's execution outright via their
own locking.

-- 
John Baldwin
___
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: [RFC] Outline of USB process integration in the kernel taskqueue system

2010-11-05 Thread John Baldwin
On Friday, November 05, 2010 9:50:10 am Matthew Fleming wrote:
 On Fri, Nov 5, 2010 at 5:58 AM, John Baldwin j...@freebsd.org wrote:
  On Thursday, November 04, 2010 5:49:22 pm Matthew Fleming wrote:
  On Thu, Nov 4, 2010 at 2:22 PM, John Baldwin j...@freebsd.org wrote:
   On Thursday, November 04, 2010 4:15:16 pm Hans Petter Selasky wrote:
   I think that if a task is currently executing, then there should be a 
   drain
   method for that. I.E. two methods: One to stop and one to cancel/drain. 
   Can
   you implement this?
  
   I agree, this would also be consistent with the callout_*() API if you 
   had
   both stop() and drain() methods.
 
  Here's my proposed code.  Note that this builds but is not yet tested.
 
 
  Implement a taskqueue_cancel(9), to cancel a task from a queue.
 
  Requested by:   hps
  Original code:  jeff
  MFC after:  1 week
 
 
  http://people.freebsd.org/~mdf/bsd-taskqueue-cancel.diff
 
  For FreeBSD taskqueue_cancel() should return EBUSY, not -EBUSY.  However, I
  would prefer that it follow the semantics of callout_stop() and return true
  if it stopped the task and false otherwise.  The Linux wrapper for
  taskqueue_cancel() can convert the return value.
 
 I used -EBUSY since positive return values reflect the old pending
 count.  ta_pending was zero'd, and I think needs to be to keep the
 task sane, because all of taskqueue(9) assumes a non-zero ta_pending
 means the task is queued.
 
 I don't know that the caller often needs to know the old value of
 ta_pending, but it seems simpler to return that as the return value
 and use -EBUSY than to use an optional pointer to a place to store the
 old ta_pending just so we can keep the error return positive.
 
 Note that phk (IIRC) suggested using -error in the returns for
 sbuf_drain to indicate the difference between success ( 0 bytes
 drained) and an error, so FreeBSD now has precedent.  I'm not entirely
 sure that's a good thing, since I am not generally fond of Linux's use
 of -error, but for some cases it is convenient.
 
 But, I'll do this one either way, just let me know if the above hasn't
 convinced you.

Hmm, I hadn't considered if callers would want to know the pending count of
the cancelled task.

  I'm not sure I like reusing the memory allocation flags (M_NOWAIT / 
  M_WAITOK)
  for this blocking flag.  In the case of callout(9) we just have two 
  functions
  that pass an internal boolean to the real routine (callout_stop() and
  callout_drain() are wrappers for _callout_stop_safe()).  It is a bit
  unfortunate that taskqueue_drain() already exists and has different 
  semantics
  than callout_drain().  It would have been nice to have the two APIs mirror 
  each
  other instead.
 
  Hmm, I wonder if the blocking behavior cannot safely be provided by just
  doing:
 
 if (!taskqueue_cancel(queue, task, M_NOWAIT)
 taskqueue_drain(queue, task);
 
 This seems reasonable and correct.  I will add a note to the manpage about 
 this.

In that case, would you be fine with dropping the blocking functionality from
taskqueue_cancel() completely and requiring code that wants the blocking
semantics to use a cancel followed by a drain?

-- 
John Baldwin
___
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: [RFC] Outline of USB process integration in the kernel taskqueue system

2010-11-05 Thread John Baldwin
On Friday, November 05, 2010 3:00:37 pm Hans Petter Selasky wrote:
 On Friday 05 November 2010 19:48:05 Matthew Fleming wrote:
  On Fri, Nov 5, 2010 at 11:45 AM, Hans Petter Selasky hsela...@c2i.net 
 wrote:
   On Friday 05 November 2010 19:39:45 Matthew Fleming wrote:
   True, but no taskqueue(9) code can handle that.  Only the caller can
   prevent a task from becoming enqueued again.  The same issue exists
   with taskqueue_drain().
   
   I find that strange, because that means if I queue a task again while it
   is running, then I doesn't get run? Are you really sure?
  
  If a task is currently running when enqueued, the task struct will be
  re-enqueued to the taskqueue.  When that task comes up as the head of
  the queue, it will be run again.
 
 Right, and the taskqueue_cancel has to cancel in that state to, but it 
 doesn't 
 because it only checks pending if !running() :-) ??

You can't close that race in taskqueue_cancel().  You have to manage that
race yourself in your task handler.  For the callout(9) API we are only
able to close that race if you use callout_init_mtx() so that the code
managing the callout wheel can make use of your lock to resolve the races.
If you use callout_init() you have to explicitly manage these races in your
callout handler.

-- 
John Baldwin
___
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: [RFC] Outline of USB process integration in the kernel taskqueue system

2010-11-04 Thread John Baldwin
On Thursday, November 04, 2010 9:55:09 am Matthew Fleming wrote:
 On Mon, Nov 1, 2010 at 1:15 PM, Hans Petter Selasky hsela...@c2i.net wrote:
  On Monday 01 November 2010 21:07:29 Matthew Fleming wrote:
  On Mon, Nov 1, 2010 at 12:54 PM, Hans Petter Selasky hsela...@c2i.net
  wrote:
   Hi!
  
   I've wrapped up an outline patch for what needs to be done to integrate
   the USB process framework into the kernel taskqueue system in a more
   direct way that to wrap it.
  
   The limitation of the existing taskqueue system is that it only
   guarantees execution at a given priority level. USB requires more. USB
   also requires a guarantee that the last task queued task also gets
   executed last. This is for example so that a deferred USB detach event
   does not happen before any pending deferred I/O for example in case of
   multiple occurring events.
  
   Mostly this new feature is targeted for GPIO-alike system using slow
   busses like the USB. Typical use case:
  
   2 tasks to program GPIO on.
   2 tasks to program GPIO off.
  
   Example:
  
   a) taskqueue_enqueue_odd(sc-sc_taskqueue, sc-sc_task_on[0], sc-
  
  sc_task_on[1]);
  
   b) taskqueue_enqueue_odd(sc-sc_taskqueue, sc-sc_task_off[0], sc-
  
  sc_task_off[1]);
  
   No matter how the call ordering of code-line a) and b), we are always
   guaranteed that the last queued state on or off is reached before the
   head of the taskqueue empties.
  
  
   In lack of a better name, the new function was called
   taskqueue_enqueue_odd [some people obviously think that USB processes
   are odd, but not taskqueues
  
   :-)]
 
  I'd like to make sure I understand the USB requirements.
 
  (1) does USB need the task priority field?  Many taskqueue(9) consumers do
  not.
 
  No, USB does not need a task priority field, but a sequence field associated
  with the task and task queue head to figure out which task was queued first
  without having to scan all the tasks queued.
 
  (2) if there was a working taskqueue_remove(9) that removed the task
  if pending or returned error if the task was currently running, would
  that be sufficient to implement the required USB functionality?
  (assuming that taskqueue_enqueue(9) put all tasks with equal priority
  in order of queueing).
 
  No, not completely. See comment above. I also need information about which
  task was queued first, or else I have to keep this information separately,
  which again, confuse people. The more layers the more confusion?
 
 I don't follow why keeping the information about which task was queued
 first privately rather than having taskqueue(9) maintain it is
 confusing.  So far, USB seems to be the only taskqueue consumer which
 needs this information, so it makes a lot more sense to me for it to
 be USB private.
 
 To my mind, there's primary operations, and secondary ones.  A
 secondary operation can be built from the primary ones.  It reads to
 me that, if there was a taskqueue_cancel(9) (and there is in Jeff's
 OFED branch) then you could build the functionality you want (and
 maybe you don't need cancel, even).  While there is sometimes an
 argument for making secondary operations available in a library, in
 this case you need extra data which most other taskqueue consumers do
 not.  That would break the KBI.  That is another argument in favor of
 keeping the implementation private to USB.

My sense is that I certainly agree.  The fact that you can't think of a good
name for the operation certainly indicates that it is not generic.
Unfortunately, I don't really understand the problem that is being solved.

I do agree that due to the 'pending' feature and automatic-coalescing of
task enqueue operations, taskqueues do not lend themselves to a barrier
operation.

-- 
John Baldwin
___
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: [RFC] Outline of USB process integration in the kernel taskqueue system

2010-11-04 Thread John Baldwin
On Thursday, November 04, 2010 4:15:16 pm Hans Petter Selasky wrote:
 On Thursday 04 November 2010 21:11:38 Matthew Fleming wrote:
  On Thu, Nov 4, 2010 at 12:11 PM, Hans Petter Selasky hsela...@c2i.net 
 wrote:
   On Thursday 04 November 2010 20:01:57 Matthew Fleming wrote:
   On Thu, Nov 4, 2010 at 11:41 AM, Hans Petter Selasky hsela...@c2i.net
   
   wrote:
On Thursday 04 November 2010 15:29:51 John Baldwin wrote:
 (and there is in Jeff's OFED branch)

Is there a link to this branch? I would certainly have a look at his
work and re-base my patch.
   
   It's on svn.freebsd.org:
   
   http://svn.freebsd.org/viewvc/base/projects/ofed/head/sys/kern/subr_task
   que ue.c?view=log
   http://svn.freebsd.org/viewvc/base?view=revisionrevision=209422
   
   For the purpose of speed, I'm not opposed to breaking the KBI by using
   a doubly-linked TAILQ, but I don't think the difference will matter
   all that often (perhaps I'm wrong and some taskqueues have dozens of
   pending tasks?)
   
   Thanks,
   matthew
   
   At first look I see that I need a non-blocking version of:
   
   taskqueue_cancel(
   
   At the point in the code where these functions are called I cannot block.
   Is this impossible to implement?
  
  It depends on whether the queue uses a MTX_SPIN or MTX_DEF.  It is not
  possible to determine whether a task is running without taking the
  taskqueue lock.  And it is certainly impossible to dequeue a task
  without the lock that was used to enqueue it.
  
  However, a variant that dequeued if the task was still pending, and
  returned failure otherwise (rather than sleeping) is definitely
  possible.
 
 I think that if a task is currently executing, then there should be a drain 
 method for that. I.E. two methods: One to stop and one to cancel/drain. Can 
 you implement this?

I agree, this would also be consistent with the callout_*() API if you had
both stop() and drain() methods.

-- 
John Baldwin
___
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: [RFC] Outline of USB process integration in the kernel taskqueue system

2010-11-01 Thread John Baldwin
On Monday, November 01, 2010 3:54:59 pm Hans Petter Selasky wrote:
 Hi!
 
 I've wrapped up an outline patch for what needs to be done to integrate the 
 USB process framework into the kernel taskqueue system in a more direct way 
 that to wrap it.
 
 The limitation of the existing taskqueue system is that it only guarantees 
 execution at a given priority level. USB requires more. USB also requires a 
 guarantee that the last task queued task also gets executed last. This is for 
 example so that a deferred USB detach event does not happen before any 
 pending 
 deferred I/O for example in case of multiple occurring events.
 
 Mostly this new feature is targeted for GPIO-alike system using slow busses 
 like the USB. Typical use case:
 
 2 tasks to program GPIO on.
 2 tasks to program GPIO off.
 
 Example:
 
 a) taskqueue_enqueue_odd(sc-sc_taskqueue, sc-sc_task_on[0], sc-
 sc_task_on[1]);
 
 
 b) taskqueue_enqueue_odd(sc-sc_taskqueue, sc-sc_task_off[0], sc-
 sc_task_off[1]);
 
 
 No matter how the call ordering of code-line a) and b), we are always 
 guaranteed that the last queued state on or off is reached before the 
 head 
 of the taskqueue empties.
 
 
 In lack of a better name, the new function was called taskqueue_enqueue_odd 
 [some people obviously think that USB processes are odd, but not taskqueues 
 :-)]

It feels like this should be something you could manage with a state machine
internal to USB rather than forcing that state into the taskqueue code itself.
If you wanted a simple barrier task (where a barrier task is always queued at
the tail of the list and all subsequent tasks are queued after the barrier task)
then I would be fine with adding that.   You could manage this without having
to alter the task KBI by having the taskqueue maintain a separate pointer to
the current barrier task and always enqueue entries after that task (the
barrier would be NULL before a barrier is queued, and set to NULL when a
barrier executes).

I think this sort of semantic is a bit simpler and also used in other parts of
the tree (e.g. in bio queues).

-- 
John Baldwin
___
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: sb600/sb700 ohci experimental patch

2009-09-28 Thread John Baldwin
On Sunday 27 September 2009 9:10:21 am Andriy Gapon wrote:
 on 27/09/2009 15:17 Andriy Gapon said the following:
  Another idea of working around this:
  1) in pci fixup code disable USB SMI for these chipsets
  2) (optional) in ohci code skip takeover step
  Sounds messy.
 
 BTW, just for the sake of experiment I did exactly what I suggested.
 I've got the following messages:
 
 kernel: ohci_controller_init:195: SMM active, request owner change
 kernel: usbus0: SMM does not respond, resetting
 kernel: ohci_controller_init:195: SMM active, request owner change
 kernel: usbus1: SMM does not respond, resetting
 kernel: ohci_controller_init:195: SMM active, request owner change
 kernel: usbus3: SMM does not respond, resetting
 kernel: ohci_controller_init:195: SMM active, request owner change
 kernel: usbus4: SMM does not respond, resetting
 kernel: ohci_controller_init:195: SMM active, request owner change
 kernel: usbus6: SMM does not respond, resetting
 
 And the register value stayed intact after initial programming, so no
 re-programming was needed.
 
 Here is the (dirty) hack:
 diff --git a/sys/dev/pci/fixup_pci.c b/sys/dev/pci/fixup_pci.c
 index 566e503..1463c24 100644
 --- a/sys/dev/pci/fixup_pci.c
 +++ b/sys/dev/pci/fixup_pci.c
 @@ -53,6 +53,7 @@ static int  fixup_pci_probe(device_t dev);
  static void  fixwsc_natoma(device_t dev);
  static void  fixc1_nforce2(device_t dev);
  static void  fixrtc_piix4(device_t dev);
 +static void  fixsmi_usb(device_t dev);
 
  static device_method_t fixup_pci_methods[] = {
  /* Device interface */
 @@ -84,6 +85,9 @@ fixup_pci_probe(device_t dev)
  case 0x01e010de: /* nVidia nForce2 */
   fixc1_nforce2(dev);
   break;
 +case 0x96001022: /* AMD SB700 */
 + fixsmi_usb(dev);
 + break;
  }
  return(ENXIO);
  }
 @@ -124,6 +128,21 @@
  }
 
 
 +/* Disable USB SMI */
 +static void
 +fixsmi_usb(device_t dev)
 +{
 +uint32_t features;
 +
 +dev = pci_find_device(0x1002, 0x4385);
 +features = pci_read_config(dev, 0x64, 4);
 +if (features  (1  15)) {
 + printf(Disabling USB SMI on SB7xx\n);
 + features = ~(1  15);
 + pci_write_config(dev, 0x64, features, 4);
 +}
 +}
 +
  /*
   * Set the SYSTEM_IDLE_TIMEOUT to 80 ns on nForce2 systems to work
   * around a hang that is triggered when the CPU generates a very fast

I don't think you can do this because it is a feature to not disable SMM if 
ohci(4) is not loaded so that a USB keyboard works when the USB driver isn't 
loaded via PS/2 emulation, even when the OS is running.  I am curious if we 
really need to do the handover for each controller or if disabling it for 
ohci0 effectively disables it for all controllers?  What do other OS's do?

-- 
John Baldwin
___
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: sb600/sb700 ohci experimental patch

2009-09-28 Thread John Baldwin
On Monday 28 September 2009 9:55:44 am Andriy Gapon wrote:
 on 28/09/2009 14:48 John Baldwin said the following:
  I don't think you can do this because it is a feature to not disable SMM 
  if 
  ohci(4) is not loaded so that a USB keyboard works when the USB driver 
  isn't 
  loaded via PS/2 emulation, even when the OS is running.
 
 Very good point.
 
  I am curious if we 
  really need to do the handover for each controller or if disabling it for 
  ohci0 effectively disables it for all controllers?  What do other OS's do?
  
 
 Don't have an answer about other OSes.
 But OHCI controllers have individual used by SMM bits and taking over one
 controller doesn't affect the bits of the other controllers - they remain set.
 Not that it means that SMM code actually keeps on controlling them.
 
 Actually, just checked - Linux also does it per controller:
 http://lxr.linux.no/#linux+v2.6.31/drivers/usb/host/ohci-hcd.c#L495

Hmm, it seems Linux now disables SMM for USB controllers (ohci, ehci, and uhci)
via PCI quirks rather than doing it in the device drivers themselves, which
matches your original suggestion.  I'm not sure how best to fix that while also
allowing USB to work w/o drivers loaded.

-- 
John Baldwin
___
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: USB polling (75% done)

2009-07-21 Thread John Baldwin
On Tuesday 21 July 2009 8:20:32 am Hans Petter Selasky wrote:
 On Monday 20 July 2009 23:51:41 Alfred Perlstein wrote:
  * Hans Petter Selasky hsela...@c2i.net [090715 13:37] wrote:
   Hi,
  
   I've added minimal polling support to the USB P4 repository now. Patch
   can be found here:
  
   http://perforce.freebsd.org/chv.cgi?CH=166148
  
   Dumping core to USB disk: Tested and works.
  
   Using USB keyboard in KDB: Does not work because Giant is not locked 
when
   calling into the UKBD's get char routine. UKBD is Giant locked. Someone
   familiar with the keyboard system on FreeBSD please step forward and fix
   this so that UKBD gets independent of the Giant mutex.
 
  the ukbd driver needs giant?
 
 I think the keyboard mux is under Giant, and does not have any concept about 
 mutexes. Most simple solution would be that DDB locks Giant before entering 
 into the keyboard code.

In general if you are in DDB you should not be using any locks at all.

-- 
John Baldwin
___
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: USB2 - umass problem

2009-02-04 Thread John Baldwin
On Wednesday 04 February 2009 4:58:02 am Vladimir Grebenschikov wrote:
 On Wed, 2009-02-04 at 10:44 +0100, Hans Petter Selasky wrote:
 
   By some reason devfs semantic was changed:
   Instead of /dev/cuaU0.[0-2] and /dev/ttyU0.[0-2], I've get
   /dev/cuaU[0-2] /dev/ttyU[0-2] and! /dev/cuau1 /dev/ttyu1
   What is reason for such change (additional port with lowercase 'u' and
   U[0-2] instead of more logical U0.[0-2]) ?
  
  It is because we are attaching drivers per interface instead of per 
device. A 
  new modem unit is allocated every time we find a modem, simply put. If the 
  modem has multiple instances in an interface, /dev/cuaU0.[0...] will be 
  created. Else /dev/cuaU... .
 
 What about /dev/cuau1 /dev/ttyu1 ?

You have a uart1 device.

-- 
John Baldwin
___
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: Soekris 4826 USB failure on FreeBSD 7.0

2008-04-24 Thread John Baldwin
On Thursday 24 April 2008 03:16:38 am Hans Petter Selasky wrote:
 On Thursday 24 April 2008, Geoffrey Mainland wrote:
  On Wed, Apr 23, 2008 at 07:31:59PM +0200, Hans Petter Selasky wrote:
   On Tuesday 22 April 2008, Geoffrey Mainland wrote:
Wow, this turns out to be much worse than I thought...I've tracked
down the problem to the commit of the new physical memory allocator
at Sat Jun 16 04:57:05 2007 UTC. Before that, no kern/122380; after
that, kern/122380 applies. Any ideas where to go from here?
  
   Hi,
  
   I've sometimes seen that the USB HC's do not always support 32 address
   lines. Not sure if that is the case for you. Then all DMA memory has to
   be allocated at a lower physical memory address. You can easily check
   this by changing the parameters used when creating DMA tags in the USB
   code.
  
   --HPS
 
  The new page allocator obviously tickled a bug somewhere in the current
  USB stack, but I'm happy to report that replacing it with the USB stack
  from your subversion repository fixed everything. Thank you! This fix
  has allowed me to move a large wireless testbed forward to FreeBSD 7.
 
  Geoff

 Hi,

 A wild guess of mine why the official USB stack in the 7-branch does not
 work: It might be that the loading of KVA into DMA is broken. I've found a
 couple of corner cases during my development where you have to generate the
 physaddr of the last page yourself in the busdma callback:

This would indicate a bug in the bus_dmamap_load() call (wrong length?) and 
that is going to hose you when you do the bus_dmamap_sync() for systems with 
bounce pages (not enough data will get copied back and forth?).  You need to 
track down the real bug and fix it rather than adding a hack in your callback 
routine.

-- 
John Baldwin
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: Soekris 4826 USB failure on FreeBSD 7.0

2008-04-24 Thread John Baldwin
On Thursday 24 April 2008 11:38:09 am Hans Petter Selasky wrote:
 On Thursday 24 April 2008, John Baldwin wrote:
 
  This would indicate a bug in the bus_dmamap_load() call (wrong length?) 
and
  that is going to hose you when you do the bus_dmamap_sync() for systems
  with bounce pages (not enough data will get copied back and forth?).  You
  need to track down the real bug and fix it rather than adding a hack in
  your callback routine.
 
 Hi John,
 
 The minimum segment size is PAGE_SIZE bytes in my DMA-tag. Bus-dma starts 
 counting at the memory location of the allocation. And not the memory 
 location aligned to PAGE_SIZE bytes.
 
 Memory pages:  |--|--|
 My allocation: |-X---|
 
 Sometimes when I allocate DMA memory I can end up having an allocation 
 crossing two contiguous memory pages in physical memory. In those cases 
 bus_dma does not give me the segment address of the second page in the 
 segment list, because the allocation is less than PAGE_SIZE bytes. It fits 
 within the values specified in the DMA tag. But in other cases, where the 
 pages are not contiguous in RAM, bus_dma will return two different segment 
 addresses.
 
 The question is: Is this a bug or is it a feature?

If you can't have a S/G element cross a page boundary, then use a 'boundary' 
of PAGE_SIZE when you create your tag.  As it is, you are (correctly for the 
tag you are creating) getting a single S/G entry that spans the entire 
region.  It sounds like you want two regions that is split on the page 
boundary.  If so, that is what the 'boundary' argument to bus_dmatag_create() 
is for.

-- 
John Baldwin
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: usb/118670: [ums] [patch] Razer Copperhead Laser Mouse shows up as keyboard

2008-02-27 Thread John Baldwin
The following reply was made to PR usb/118670; it has been noted by GNATS.

From: John Baldwin [EMAIL PROTECTED]
To: [EMAIL PROTECTED], [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]
Subject: Re: usb/118670: [ums] [patch] Razer Copperhead Laser Mouse shows up as 
keyboard
Date: Wed, 27 Feb 2008 16:41:13 -0500

 According to the USB spec, the protocol field is only defined if the subclass 
 is 1.  So a subclass of 0 is really a violation of the spec.  I think a 
 better fix is to put back the old hid_is_collection() test first and then 
 directly check the descriptor if that fails:
 
 Index: ums.c
 ===
 RCS file: /usr/cvs/src/sys/dev/usb/ums.c,v
 retrieving revision 1.97
 diff -u -r1.97 ums.c
 --- ums.c  26 Dec 2007 14:31:16 -  1.97
 +++ ums.c  27 Feb 2008 21:40:48 -
 @@ -198,7 +198,10 @@
if (err)
return (UMATCH_NONE);
  
 -  if (id-bInterfaceClass == UICLASS_HID 
 +  if (hid_is_collection(desc, size,
 +HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_MOUSE)))
 +  ret = UMATCH_IFACECLASS;
 +  else if (id-bInterfaceClass == UICLASS_HID 
id-bInterfaceSubClass == UISUBCLASS_BOOT 
id-bInterfaceProtocol == UIPROTO_MOUSE)
ret = UMATCH_IFACECLASS;
 
 -- 
 John Baldwin
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: New USB stack and Zero copy.

2007-07-09 Thread John Baldwin
On Saturday 07 July 2007 12:14:24 pm Hans Petter Selasky wrote:
 On Friday 06 July 2007 22:41, John Baldwin wrote:
  On Friday 06 July 2007 02:59:39 am Hans Petter Selasky wrote:
   On Friday 06 July 2007 01:35, John Baldwin wrote:
On Thursday 05 July 2007 04:25:17 pm John Baldwin wrote:
 On Thursday 05 July 2007 03:31:59 am Hans Petter Selasky wrote:
  On Wednesday 04 July 2007 19:35, John-Mark Gurney wrote:
   Hans Petter Selasky wrote this message on Wed, Jul 04, 2007 at
 
 
  Huh?  The bounce pages are preallocated, so bus_dmamap_load() isn't going
  to be allocating things.
 
 When are they allocated ?

UTSL.  bus_dma_tag_create() (if you pass BUS_DMA_ALLOCNOW) or 
bus_dmamap_create() for the first map belonging to a tag.

-- 
John Baldwin
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: New USB stack and Zero copy.

2007-07-06 Thread John Baldwin
On Friday 06 July 2007 02:59:39 am Hans Petter Selasky wrote:
 On Friday 06 July 2007 01:35, John Baldwin wrote:
  On Thursday 05 July 2007 04:25:17 pm John Baldwin wrote:
   On Thursday 05 July 2007 03:31:59 am Hans Petter Selasky wrote:
On Wednesday 04 July 2007 19:35, John-Mark Gurney wrote:
 Hans Petter Selasky wrote this message on Wed, Jul 04, 2007 at 09:01
  
   +0200:
  Also: How is the easiest way to load memory pages into DMA ? And I
 
  want
 
  that the loadig works like this, that when the page must be 
bounced
  it should not allocate a bounce buffer, hence I already have a
  bounce buffer. I only need to know which pages I can forward
  directly to the
  
   USB
  
  hardware, and the rest I will bounce somewhere else.

 Why do you not want to let bus_dma do the bouncing for you?  If it's
 to save a copy to another buffer, why don't you load the final 
buffer
 into bus_dma?
   
Because if I let bus_dma do the bounching, I cannot do this while
holding
 
  a
 
mutex, hence allocating DMA'able memory on the fly is not so good.
  
   This is not a hard problem to solve, every other driver using bus_dma
   solves it.  Just make sure your driver is in a sane state and drop the
   lock while you let bus_dmamap_load() map/copy things for you.
 
  Bah, backwards (was thinking of the fact that if you get EINPROGRESS you
  will have to drop the lock and just wait until the callback is called to
  make further progress on the request).  bus_dmamap_load() already
  _requires_ you to hold your mutex when you call it, so I don't really see
  what the issue is, unless you are assuming BUS_DMA_NOWAIT behavior and
  can't properly handle deferred callbacks.  You do have to drop the lock
  around bus_dmamem_alloc(), but not bus_dmamap_load().
 
 The thing is that allocating memory on the fly will be slow, and especially 
 when the _load() functions will allocate contiguous memory. This only 
 works fine when you load mbufs and things with size less than PAGE_SIZE 
 bytes ??

Huh?  The bounce pages are preallocated, so bus_dmamap_load() isn't going to 
be allocating things.

-- 
John Baldwin
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: New USB stack and Zero copy.

2007-07-05 Thread John Baldwin
On Thursday 05 July 2007 03:31:59 am Hans Petter Selasky wrote:
 On Wednesday 04 July 2007 19:35, John-Mark Gurney wrote:
  Hans Petter Selasky wrote this message on Wed, Jul 04, 2007 at 09:01 
+0200:
   Also: How is the easiest way to load memory pages into DMA ? And I want
   that the loadig works like this, that when the page must be bounced it
   should not allocate a bounce buffer, hence I already have a bounce
   buffer. I only need to know which pages I can forward directly to the 
USB
   hardware, and the rest I will bounce somewhere else.
 
  Why do you not want to let bus_dma do the bouncing for you?  If it's
  to save a copy to another buffer, why don't you load the final buffer
  into bus_dma?
 
 Because if I let bus_dma do the bounching, I cannot do this while holding a 
 mutex, hence allocating DMA'able memory on the fly is not so good.

This is not a hard problem to solve, every other driver using bus_dma solves 
it.  Just make sure your driver is in a sane state and drop the lock while 
you let bus_dmamap_load() map/copy things for you.

-- 
John Baldwin
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to [EMAIL PROTECTED]


Re: New USB stack and Zero copy.

2007-07-05 Thread John Baldwin
On Thursday 05 July 2007 04:25:17 pm John Baldwin wrote:
 On Thursday 05 July 2007 03:31:59 am Hans Petter Selasky wrote:
  On Wednesday 04 July 2007 19:35, John-Mark Gurney wrote:
   Hans Petter Selasky wrote this message on Wed, Jul 04, 2007 at 09:01 
 +0200:
Also: How is the easiest way to load memory pages into DMA ? And I 
want
that the loadig works like this, that when the page must be bounced it
should not allocate a bounce buffer, hence I already have a bounce
buffer. I only need to know which pages I can forward directly to the 
 USB
hardware, and the rest I will bounce somewhere else.
  
   Why do you not want to let bus_dma do the bouncing for you?  If it's
   to save a copy to another buffer, why don't you load the final buffer
   into bus_dma?
  
  Because if I let bus_dma do the bounching, I cannot do this while holding 
a 
  mutex, hence allocating DMA'able memory on the fly is not so good.
 
 This is not a hard problem to solve, every other driver using bus_dma solves 
 it.  Just make sure your driver is in a sane state and drop the lock while 
 you let bus_dmamap_load() map/copy things for you.

Bah, backwards (was thinking of the fact that if you get EINPROGRESS you will 
have to drop the lock and just wait until the callback is called to make 
further progress on the request).  bus_dmamap_load() already _requires_ you 
to hold your mutex when you call it, so I don't really see what the issue is, 
unless you are assuming BUS_DMA_NOWAIT behavior and can't properly handle 
deferred callbacks.  You do have to drop the lock around bus_dmamem_alloc(), 
but not bus_dmamap_load().

-- 
John Baldwin
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to [EMAIL PROTECTED]