Re: aucat: suggest using -v100 ?

2011-05-28 Thread Jacob Meuser
On Sat, May 28, 2011 at 09:48:52AM +0200, Marc Espie wrote:
 On Sat, May 28, 2011 at 12:43:29AM +, Jacob Meuser wrote:
  more to the point: OpenBSD's audio is a bit less sucky than it used to
  be.  the people who want to change the way aucat works are not the
  people who made audio less sucky.  I think the EPOCH shit in ports
  is totally fucked, but my opinion doesn't really matter, because I'm
  not a major contributor to ports.  I can accept that.  at least with
  aucat there are options to make it work to suit differing opinions/
  tastes.
 
 Care to elaborate ? do you see another way to fix that specific issue ?
 Namely, to ensure that version numbers go forward.

yeah, don't rely on upstream version numbering at all.  upstream
versioning should only be in the package name to identify which
upstream sources were used, not whether a package is newer than
another.

I explained this in more detail some time ago on ports@.  you replied
to me ...

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: aucat: suggest using -v100 ?

2011-05-27 Thread Jacob Meuser
On Thu, May 26, 2011 at 07:23:23AM -0500, Marco Peereboom wrote:
 sane defaults.  I agree with stu.

the current default is sane.  if aucat is quieter by default, then I
suspect there will be *more* complaints that speakers are too quiet.

can we please just accept that a 100% perfect for everyone solution
is not really possible?

 On Thu, May 26, 2011 at 12:01:22PM +0200, Alexandre Ratchov wrote:
  On Thu, May 26, 2011 at 10:31:33AM +0100, Stuart Henderson wrote:
   On 2011/05/26 09:56, Alexandre Ratchov wrote:
This is to suggest using -v100 to avoid volume jumps when more than
one stream are played at the same time.

OK?

Or should we just say ``for normal use: -v100'', assuming nowadays
nobody listens to a single stream at the same :)
   
   Would it make sense to make -v100 the default, then people who
   wish to retain full dynamic range can set -v127?
  
  I'd like to avoid having different defaults for files (-i) and
  sub-devices (-s). And making -v100 the default would be unpractical
  for playing files or for off-line processing.
  
  -- Alexandre

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: mixerctl(1) diff: sort output

2011-05-14 Thread Jacob Meuser
On Sat, May 14, 2011 at 07:29:01PM +0300, Sviatoslav Chagaev wrote:
 On Mon, 9 May 2011 12:44:54 +
 Jacob Meuser jake...@sdf.lonestar.org wrote:

  ok then, why do some devices have 'outputs.dac' yet others have
  'inputs.dac'?  what is the difference to the user?  which is more
  correct?  why?
  
 
 I have absolutely no idea. I'm not an audio equipment manufacturer. I
 don't understand how are these questions even relevant.

my point exactly.

  this has been the case since long before azalia(4).  if you cannot
  explain this, then you just don't understand.

you post a diff and assert that the class is the most important
part of the name, but when confronted with an example of how the
class is ambiguous, you cannot explain why.

and it's not because you aren't a hardware manufacturer.  I'm not
a hardware mfg either.  but I do work on OpenBSD's audio system.
I have spent many, many hours reading the code, fixing issues,
trying to figure out how to make things better.  and I can
confidently say that adding sorting to mixerctl is not the
answer.  in fact, that will make things worse, I guarantee it.

 Suppose I have bought a new computer, can I predict where exactly these
 controls will pop up in the output?

all azalia(4) will have outputs.master very near the end.  cannot say
exactly, as it depends on what controls the hardware supports.

  please do give an example of how sorting them makes finding what you're
  looking for easier.  please.  and it better be easier than piping
  through sort/grep.  I'll repeat that you cannot rely on 'inputs' or
  'outputs' having true meaning.  sorry, but you can't.  trust me on
  that one.
  
 
 Binary search.
 I might not remember the exact name of the variable.
 Why should I be suffering if someone has broken hardware? From all the
 computers I owned -- there wasn't a single time when vars were labeled
 incorrectly.
 Even if I have broken hardware, I will know this the first time I use
 it. No need to tell me that over and over again. This will not fix
 my hardware.

this is not an example, nor does it preclude using grep/sort.

it's not about broken hardware.  it's about inconsistent mixer(4)
implementations, which is largely due to poor documentation of
mixer(9).

 It doesn't matter how mixerctl and other *ctl are _implemented_. They
 should behave as similar as possible.

I already showed how the *ctl programs do NOT behave similarly, even
before you get to the listing of variables, if the *ctl programs
even list variables.  while I appreciate consistency, shooting yourself
in the foot to be consistent is not a good idea.

if you really want to fix the mixer(4) implementations, then you have
to fix the mixer(4) implementations *in the low level drivers*.  adding
sorting in mixerctl will make that harder, because then the controls
will not be in the same order as they are created by the drivers.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: mixerctl(1) diff: sort output

2011-05-14 Thread Jacob Meuser
On Sat, May 14, 2011 at 07:02:21PM +0300, Sviatoslav Chagaev wrote:

 I always thought that the idea behind the *ctl programs is to provide a
 way to configure totally different things in a similar manner.
 Therefore *ctl programs should behave as similarily as possible.

so where is your diff to add an abstraction layer to mixer(4), so
that mixerctl output is the same for *every* device?

that is the solution that would benefit everyone.

adding sorting to mixerctl is stupid.  it solves nothing and makes
maintainence of audio drivers more difficult.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: aucat(1) mixing: saturating-addition instead of add-and-divide-by-n_inputs

2011-05-11 Thread Jacob Meuser
On Wed, May 11, 2011 at 09:45:05AM +0300, Sviatoslav Chagaev wrote:
 On Wed, 11 May 2011 03:35:56 +
 Jacob Meuser jake...@sdf.lonestar.org wrote:
 
  clipping is better than normalizing?  really?
 
 Clipping might describe something like value0xff, so no, not
 clipping, saturating addition.

   + if (sum  ADATA_MIN)
   + sum = ADATA_MIN;
   + else if (sum  ADATA_MAX)
   + sum = ADATA_MAX;

http://en.wikipedia.org/wiki/Clipping_(audio)

 Try it and see for yourself.

it is like jackd, which I find annoying.  I agree, changing volume of
an active stream is annoying, but there is a reason for it.

  
  what about the case where aucat is used for offline mixing?
  
 
 What about it?

it's a very different use case than the one you're trying to fix.
I'm asking you how your fix is going to affect that use case.

  like the mixerctl change, you are taking away things that exist
  for good reason, because it makes *your* situation better in *your*
  opinion, when you can (mostly) have what you want with the current
  code (if you just try a little).
  
 
 I'm not taking anything away,

you most certainly are.  whether or not you believe the code to be
correct/useful is irrelevant, you are removing something.

 I'm setting things right.

lemme offer a suggestion.  the next time you run into something
that doesn't make any sense to you, check if someone else has already
questioned it and if any reason has already been given.  or at least
ask why instead of asserting that it is wrong.  that will get you
more sympathy, from me anyway.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: aucat(1) mixing: saturating-addition instead of add-and-divide-by-n_inputs

2011-05-10 Thread Jacob Meuser
clipping is better than normalizing?  really?

what about the case where aucat is used for offline mixing?

like the mixerctl change, you are taking away things that exist
for good reason, because it makes *your* situation better in *your*
opinion, when you can (mostly) have what you want with the current
code (if you just try a little).

On Wed, May 11, 2011 at 02:50:36AM +0300, Sviatoslav Chagaev wrote:
 I'm sitting at work, listening to music, debugging a web-application
 with JavaScript alert()s. Each time an alert window pops up, the
 browser plays a sound. For a brief moment, the volume drops twicefold
 then goes back to normal. This is annoying and doesn't make sense.
 
 In real life, if you are surrounded by multiple sound sources, their
 sound volumes will not be divided by the total amount of sound sources.
 Their sounds will add up until they blur and you can't distinguish
 anything anymore. Other operating systems, such as Macrohard Doors, do
 mixing by modeling this real world behaviour.
 
 In this sense, aucat violates the principle of least surprise.
 I'm used to how sound interacts in real world and then aucat steps in
 and introduces it's own laws of physics.
 
 To remedy this, aucat has an option -v, which lets you pre-divide the
 volume of inputs. This results in loss of dynamic range (quiet sounds
 might disappear and the maximum volume that you can set decreases). And
 also, if during usage the count of inputs raises above of what I
 predicted, the volume starts to jump up and down again.
 
 Experimentally, I've found that if you do a saturating addition between
 inputs, it sounds very much how it might have sounded in real world and
 how Macrohard Doors, among others, sounds like when playing
 multiple sounds.
 
 
 So, why is what I'm proposing better than what currently exists:
 
 * Resembles how sound behaves in real world more closely;
 * Doesn't violate the principle of least surprise;
 * No more annoying volume jumps up and down;
 * No need to use the -v option anymore / less stuff to remember / it
 just works;
 * No more choosing between being annoyed by volume jumps or loosing
 dynamic range.
 
 (This doesn't affect the -v option, it remains fully functional.)
 
 Tested on i386 and amd64 with 16 bits and 24 bits.
 
 
 Index: abuf.h
 ===
 RCS file: /OpenBSD/src/usr.bin/aucat/abuf.h,v
 retrieving revision 1.23
 diff -u -r1.23 abuf.h
 --- abuf.h21 Oct 2010 18:57:42 -  1.23
 +++ abuf.h10 May 2011 22:58:18 -
 @@ -46,7 +46,6 @@
   union {
   struct {
   int weight; /* dynamic range */ 
 - int maxweight;  /* max dynamic range allowed */
   unsigned vol;   /* volume within the dynamic range */
   unsigned done;  /* frames ready */
   unsigned xrun;  /* underrun policy */
 Index: aparams.h
 ===
 RCS file: /OpenBSD/src/usr.bin/aucat/aparams.h,v
 retrieving revision 1.11
 diff -u -r1.11 aparams.h
 --- aparams.h 5 Nov 2010 16:42:17 -   1.11
 +++ aparams.h 10 May 2011 22:58:18 -
 @@ -19,6 +19,8 @@
  
  #include sys/param.h
  
 +#include limits.h
 +
  #define NCHAN_MAX16  /* max channel in a stream */
  #define RATE_MIN 4000/* min sample rate */
  #define RATE_MAX 192000  /* max sample rate */
 @@ -64,6 +66,9 @@
  
  typedef short adata_t;
  
 +#define ADATA_MINSHRT_MIN
 +#define ADATA_MAXSHRT_MAX
 +
  #define ADATA_MUL(x,y)   (((int)(x) * (int)(y))  (ADATA_BITS - 
 1))
  #define ADATA_MULDIV(x,y,z)  ((int)(x) * (int)(y) / (int)(z))
  
 @@ -71,6 +76,9 @@
  
  typedef int adata_t;
  
 +#define ADATA_MIN(-0xff / 2)
 +#define ADATA_MAX(0xff / 2)
 +
  #if defined(__i386__)  defined(__GNUC__)
  
  static inline int
 @@ -119,6 +127,28 @@
  #else
  #error only 16-bit and 24-bit precisions are supported
  #endif
 +
 +/* saturating addition */
 +static inline adata_t
 +adata_sadd(register adata_t x, register adata_t y)
 +{
 +#if ADATA_BITS = 16
 + register int sum;
 +#else
 + register long long sum;
 +#endif
 +
 + sum = x;
 + sum += y;
 +
 + if (sum  ADATA_MIN)
 + sum = ADATA_MIN;
 + else if (sum  ADATA_MAX)
 + sum = ADATA_MAX;
 +
 + return (adata_t) sum;
 +}
 +#define ADATA_SADD(x,y)  adata_sadd(x,y)
  
  #define MIDI_MAXCTL  127
  #define MIDI_TO_ADATA(m) (aparams_ctltovol[m]  (ADATA_BITS - 16))
 Index: aproc.c
 ===
 RCS file: /OpenBSD/src/usr.bin/aucat/aproc.c,v
 retrieving revision 1.64
 diff -u -r1.64 aproc.c
 --- aproc.c   28 Apr 2011 07:20:03 -  1.64
 +++ aproc.c   10 May 2011 22:58:19 -
 @@ -617,6 +617,7 @@
   unsigned i, j, cc, istart, inext, onext, 

Re: mixerctl(1) diff: sort output

2011-05-09 Thread Jacob Meuser
On Mon, May 09, 2011 at 02:56:28PM +0300, Sviatoslav Chagaev wrote:
 On Mon, May 9, 2011 at 4:48 AM, Jacob Meuser jake...@sdf.lonestar.org
 wrote:
  On Mon, May 09, 2011 at 01:32:49AM +0300, Sviatoslav Chagaev wrote:
  * sorted output looks cleaner, prettier;
  * it's easier to find the variable you're looking for in a sorted
  output;
  * hierarchical variable names yet unordered? doesn't make sense;
  * this way mixerctl's behaviour will be closer to other *ctl programs
  which output variables in an ordered fashion (audioctl, sysctl,
  wsconsctl).
 
  these are all matters of opinion, except for hierarchical variable names
  which is not technically the case here.
 
 
 Okay, but what about making mixerctl more similar to other *ctl?

*sigh*

$ wsconsctl
(blah) Permission denied

$ usbhidctl
usage: (blah)

$ ikectl
missing argument:

$ gpioctl
usage: (blah)

or how about, what other *ctl program is almost entirely hardware
dependent?

  Before:
 
  note how the controls are grouped (mostly) by widget.  please read
  azalia(4).
 
 I did notice that.
 Unfortunately, I can't access my amd64 machine right now. It has
 like 2 to 3 times the amount of variables shown here and I think I saw
 some vars there, for which I couldn't figure out by which criteria were they
 ordered.
 If mixerctl's intention is that the most important thing is the widget,
 then the widget's name should've been placed in front.
 I tried this just now, putting widget in front, then the mixer class --
 it looks horrible, completely unscannable.
 Maybe the mixer class is the most important thing after all, since
 this is the most generic thing you can group various widgets by.

ok then, why do some devices have 'outputs.dac' yet others have
'inputs.dac'?  what is the difference to the user?  which is more
correct?  why?

this has been the case since long before azalia(4).  if you cannot
explain this, then you just don't understand.

 
  s@d630:0:/usr/src/usr.bin/mixerctl$ mixerctl
  outputs.hp_source=dac-0:1
  outputs.hp_dir=output
  outputs.hp_boost=off
  outputs.line-in_source=dac-2:3
  outputs.line-in_dir=input
  outputs.mic_dir=input-vr80
  outputs.spkr_source=dac-2:3
  outputs.spkr_dir=none
  outputs.spkr_boost=off
  inputs.dac-2:3_mute=off
  inputs.dac-2:3=152,152
  inputs.dac-0:1_mute=off
  inputs.dac-0:1=152,152
  inputs.sel_source=mic
  outputs.sel=126,126
  inputs.sel2_source=line-in
  outputs.sel2=126,126
  inputs.sel3_source=sel
  inputs.sel3_sel=119,119
  inputs.sel4_source=sel2
  inputs.sel4_sel2=119,119
  record.adc-0:1_source=sel3
  record.adc-0:1_mute=off
  record.adc-2:3_source=sel4
  record.adc-2:3_mute=off
  inputs.beep=85
 
  and at the end are the pseudo controls.  the ones most people are
  most interested in.  so even if the rest of the controls scroll by
  when you do a simple mixerctl, you see these controls.
 
 
 Just by looking at these variables, I cannot distinguish which ones are
 pseudo and which aren't. This is an implementation detail, I think
 the user of an interface shouldn't have to care about implementation
 details.
 I cannot provide any evidence that most people are or are not
 interested in those pseudo vars, but I thought most of the time
 people are interested in something like play an mp3 file, watch a
 video, simple stuff like that. In this case one would probably be most
 interested in outputs, volume controls...

as in, 'outputs.master' ... the one that's tied to the keyboard keys on
laptops.  this is by far the most important, most used mixer control.

 It's possible to adjust
 sorting and output these vars last, by modifying the cmp function.

that is ridiculous.

 And I can argue that it is still easier to find what you're looking for
 when the output is sorted, especially considering that the output of
 mixerctl can vary widely between different computers.

please do give an example of how sorting them makes finding what you're
looking for easier.  please.  and it better be easier than piping
through sort/grep.  I'll repeat that you cannot rely on 'inputs' or
'outputs' having true meaning.  sorry, but you can't.  trust me on
that one.

  outputs.hp_sense=plugged
  outputs.line-in_sense=unplugged
  outputs.spkr_muters=hp,line-in
  outputs.master=153,153
  outputs.master.mute=off
  outputs.master.slaves=dac-2:3,dac-0:1
  record.volume=0,0
  record.volume.mute=off
  record.volume.slaves=adc-0:1,adc-2:3
 
  After:
  s@d630:0:/usr/src/usr.bin/mixerctl$ ./mixerctl
  inputs.beep=85
  inputs.dac-0:1=152,152
  inputs.dac-0:1_mute=off
  inputs.dac-2:3=152,152
  inputs.dac-2:3_mute=off
  inputs.sel2_source=line-in
  inputs.sel3_sel=119,119
  inputs.sel3_source=sel
  inputs.sel4_sel2=119,119
  inputs.sel4_source=sel2
  inputs.sel_source=mic
  outputs.hp_boost=off
  outputs.hp_dir=output
  outputs.hp_sense=plugged
  outputs.hp_source=dac-0:1
  outputs.line-in_dir=input
  outputs.line-in_sense=unplugged
  outputs.line-in_source=dac-2:3
  outputs.master=153,153
  outputs.master.mute=off

Re: mixerctl(1) diff: sort output

2011-05-08 Thread Jacob Meuser
On Mon, May 09, 2011 at 01:32:49AM +0300, Sviatoslav Chagaev wrote:
 * sorted output looks cleaner, prettier;
 * it's easier to find the variable you're looking for in a sorted
 output;
 * hierarchical variable names yet unordered? doesn't make sense;
 * this way mixerctl's behaviour will be closer to other *ctl programs
 which output variables in an ordered fashion (audioctl, sysctl,
 wsconsctl).

these are all matters of opinion, except for hierarchical variable names
which is not technically the case here.

 Before:

note how the controls are grouped (mostly) by widget.  please read
azalia(4).

 s@d630:0:/usr/src/usr.bin/mixerctl$ mixerctl
 outputs.hp_source=dac-0:1
 outputs.hp_dir=output
 outputs.hp_boost=off
 outputs.line-in_source=dac-2:3
 outputs.line-in_dir=input
 outputs.mic_dir=input-vr80
 outputs.spkr_source=dac-2:3
 outputs.spkr_dir=none
 outputs.spkr_boost=off
 inputs.dac-2:3_mute=off
 inputs.dac-2:3=152,152
 inputs.dac-0:1_mute=off
 inputs.dac-0:1=152,152
 inputs.sel_source=mic
 outputs.sel=126,126
 inputs.sel2_source=line-in
 outputs.sel2=126,126
 inputs.sel3_source=sel
 inputs.sel3_sel=119,119
 inputs.sel4_source=sel2
 inputs.sel4_sel2=119,119
 record.adc-0:1_source=sel3
 record.adc-0:1_mute=off
 record.adc-2:3_source=sel4
 record.adc-2:3_mute=off
 inputs.beep=85

and at the end are the pseudo controls.  the ones most people are
most interested in.  so even if the rest of the controls scroll by
when you do a simple mixerctl, you see these controls.

 outputs.hp_sense=plugged
 outputs.line-in_sense=unplugged
 outputs.spkr_muters=hp,line-in
 outputs.master=153,153
 outputs.master.mute=off
 outputs.master.slaves=dac-2:3,dac-0:1
 record.volume=0,0
 record.volume.mute=off
 record.volume.slaves=adc-0:1,adc-2:3
 
 After:
 s@d630:0:/usr/src/usr.bin/mixerctl$ ./mixerctl
 inputs.beep=85
 inputs.dac-0:1=152,152
 inputs.dac-0:1_mute=off
 inputs.dac-2:3=152,152
 inputs.dac-2:3_mute=off
 inputs.sel2_source=line-in
 inputs.sel3_sel=119,119
 inputs.sel3_source=sel
 inputs.sel4_sel2=119,119
 inputs.sel4_source=sel2
 inputs.sel_source=mic
 outputs.hp_boost=off
 outputs.hp_dir=output
 outputs.hp_sense=plugged
 outputs.hp_source=dac-0:1
 outputs.line-in_dir=input
 outputs.line-in_sense=unplugged
 outputs.line-in_source=dac-2:3
 outputs.master=153,153
 outputs.master.mute=off
 outputs.master.slaves=dac-2:3,dac-0:1
 outputs.mic_dir=input-vr80
 outputs.sel=126,126
 outputs.sel2=126,126
 outputs.spkr_boost=off
 outputs.spkr_dir=none
 outputs.spkr_muters=hp,line-in
 outputs.spkr_source=dac-2:3
 record.adc-0:1_mute=off
 record.adc-0:1_source=sel3
 record.adc-2:3_mute=off
 record.adc-2:3_source=sel4
 record.volume=0,0
 record.volume.mute=off
 record.volume.slaves=adc-0:1,adc-2:3

I do not find this more useful.  prettier, perhaps, but not more useful.
in particular, this (further) breaks the widget-wise grouping on some
devices.  please read azalia(4), inputs and outputs is really just
a hint, and making it more precise is much more difficult than adding
sorting to mixerctl ...


 
 
 Index: mixerctl.c
 ===
 RCS file: /OpenBSD/src/usr.bin/mixerctl/mixerctl.c,v
 retrieving revision 1.29
 diff -u -r1.29 mixerctl.c
 --- mixerctl.c12 Nov 2009 07:27:31 -  1.29
 +++ mixerctl.c8 May 2011 22:25:03 -
 @@ -46,23 +46,21 @@
  #include string.h
  #include unistd.h
  
 -struct field *findfield(char *);
 -void adjlevel(char **, u_char *, int);
 -void catstr(char *, char *, char *);
 -void prfield(struct field *, char *, int, mixer_ctrl_t *);
 -void rdfield(int, struct field *, char *, int, char *);
 -__dead void usage(void);
 -
  #define FIELD_NAME_MAX   64
  
  struct field {
   char name[FIELD_NAME_MAX];
   mixer_ctrl_t *valp;
   mixer_devinfo_t *infp;
 -} *fields, *rfields;
 +};
  
 -mixer_ctrl_t *values;
 -mixer_devinfo_t *infos;
 +int   fieldcmp(const void *, const void *);
 +int   fieldnamecmp(const void *, const void *);
 +void  adjlevel(char **, u_char *, int);
 +void  catstr(char *, char *, char *);
 +void  prfield(struct field *, char *, int, mixer_ctrl_t *);
 +void  rdfield(int, struct field *, char *, int, char *);
 +__dead void   usage(void);
  
  void
  catstr(char *p, char *q, char *out)
 @@ -73,14 +71,19 @@
   strlcpy(out, tmp, FIELD_NAME_MAX);
  }
  
 -struct field *
 -findfield(char *name)
 +int
 +fieldcmp(const void *pa, const void *pb)
  {
 - int i;
 - for (i = 0; fields[i].name[0] != '\0'; i++)
 - if (strcmp(fields[i].name, name) == 0)
 - return fields[i];
 - return (0);
 + const struct field *a = pa, *b = pb;
 + return strcmp(a-name, b-name);
 +}
 +
 +int
 +fieldnamecmp(const void *pa, const void *pb)
 +{
 + const char *name = pa;
 + const struct field *f = pb;
 + return strcmp(name, f-name);
  }
  
  #define e_member_nameun.e.member[i].label.name
 @@ -241,12 +244,15 @@
  int
  

uvideo: kill a quirk, fix frame sizes

2011-03-27 Thread Jacob Meuser
dcoppa@ noticed that his camera still didn't work with ffmpeg after
the latest uvideo changes.  the problem was that the device reported
a ridiculously large maximum frame size, and then malloc(9) failed
because there wasn't enough free memory to allocate the frame buffer.

this is similar to a problem with other cameras, and is why uvideo(4)
has the UVIDEO_FLAG_FIX_MAX_VIDEO_FRAME_SIZE quirk.

however, these problems and the quirk only affect/fix uncompressed
video formats.  uncompressed video formats have a fixed bit-depth
per pixel.  that means that if you know how many pixels are in
a frame, you can figure out how many bytes are needed to store the
frame.  we always know the frame size and bit depth, so we can
always figure out the needed buffer size for uncompressed frames.

so, that's what the following diff does.  it calculates the frame
buffer size neeed for uncompressed frames and ignores values given
by the device.

I also noticed that one of my cameras will produce a short frame
almost every time capture is started, and occasionally during
capture.  this breaks ffmpeg.  also, the v4l2 spec says that
the read() method should never give out partial frames.  the spec
isn't so clear about mmap'd frames, but ffmpeg doesn't like them,
and I imagine other applications won't like them either.  so I
also extended the check that skips over-sized frames to skip
under-sized frames for uncompressed formats.

and finally, I moved the check of the data returned from a probe
request from the function that does the probe request to the
function that does the parameter negotiation.  also added a check
that we actually got the parameters we wanted.  without that check,
the driver could think one format is being used, but the device
is using a different one ...

please test with all uvideo(4) devices, especially these that were
using a quirk to fix the frame size:

USB_PRODUCT_CHENSOURCE_CM12402: Eagle IR Cam
USB_PRODUCT_MICRODIA_CAM_1: CAM_1 (Sonix web cam)
USB_PRODUCT_MICROSOFT_LIFECAM: Microsoft LifeCam

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: uvideo.c
===
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.158
diff -u -p uvideo.c
--- uvideo.c26 Mar 2011 19:50:52 -  1.158
+++ uvideo.c27 Mar 2011 19:04:00 -
@@ -249,7 +249,6 @@ struct video_hw_if uvideo_hw_if = {
 #define UVIDEO_FLAG_ISIGHT_STREAM_HEADER   0x1
 #define UVIDEO_FLAG_REATTACH   0x2
 #define UVIDEO_FLAG_VENDOR_CLASS   0x4
-#define UVIDEO_FLAG_FIX_MAX_VIDEO_FRAME_SIZE   0x8
 struct uvideo_devs {
struct usb_devno uv_dev;
char*ucode_name;
@@ -325,27 +324,6 @@ struct uvideo_devs {
NULL,
UVIDEO_FLAG_VENDOR_CLASS
},
-   {
-   /* Needs to fix dwMaxVideoFrameSize */
-   { USB_VENDOR_CHENSOURCE, USB_PRODUCT_CHENSOURCE_CM12402 },
-   NULL,
-   NULL,
-   UVIDEO_FLAG_FIX_MAX_VIDEO_FRAME_SIZE
-   },
-   {
-   /* Needs to fix dwMaxVideoFrameSize */
-   { USB_VENDOR_MICRODIA, USB_PRODUCT_MICRODIA_CAM_1 },
-   NULL,
-   NULL,
-   UVIDEO_FLAG_FIX_MAX_VIDEO_FRAME_SIZE
-   },
-   {
-   /* Needs to fix dwMaxVideoFrameSize */
-   { USB_VENDOR_MICROSOFT, USB_PRODUCT_MICROSOFT_LIFECAM },
-   NULL,
-   NULL,
-   UVIDEO_FLAG_FIX_MAX_VIDEO_FRAME_SIZE
-   },
 };
 #define uvideo_lookup(v, p) \
((struct uvideo_devs *)usb_lookup(uvideo_devs, v, p))
@@ -1077,15 +1055,20 @@ uvideo_vs_parse_desc_frame_sub(struct uvideo_softc *sc
sc-sc_fmtgrp[fmtidx].frame_cur = fd;
 
/*
-* On some broken device, dwMaxVideoFrameBufferSize is not correct.
-* So fix it by frame width/height (XXX YUV2 format only).
+* On some devices, dwMaxVideoFrameBufferSize is not correct.
+* Version 1.1 of the UVC spec says this field is deprecated.
+* For uncompressed pixel formats, the frame buffer size can
+* be determined by multiplying width, height, and bytes per pixel.
+* Uncompressed formats have a fixed number of bytes per pixel.
+* Bytes per pixel can vary with compressed formats.
 */
-   if (sc-sc_quirk 
-   sc-sc_quirk-flags  UVIDEO_FLAG_FIX_MAX_VIDEO_FRAME_SIZE 
-   sc-sc_fmtgrp[fmtidx].pixelformat == V4L2_PIX_FMT_YUYV) {
-   fbuf_size = UGETW(fd-wWidth) * UGETW(fd-wHeight) * 4;
-   DPRINTF(1, wWidth = %d, wHeight = %d\n,
-   UGETW(fd-wWidth), UGETW(fd-wHeight));
+   if (desc-bDescriptorSubtype == UDESCSUB_VS_FRAME_UNCOMPRESSED) {
+   fbuf_size = UGETW(fd-wWidth) * UGETW(fd-wHeight) *
+   sc-sc_fmtgrp[fmtidx].format-u.uc.bBitsPerPixel / NBBY;
+   DPRINTF(10, %s: %s: frame buffer size=%d 
+  

uvideo(4) diff needs testing

2011-03-20 Thread Jacob Meuser
while working on an update for ffmpeg, I noticed it's ability to
record from a video(4) device is now completely broken because video(4)
doesn't yet support VIDIOC_{S,G}_PARM.

so I added support for the VIDIOC_{S,G}_PARM ioctls to video(4) and
uvideo(4).

but then ffmpeg still couldn't record from a video device.  more
investigation found that uvideo(4) doesn't fill in the timetamp
for frames when using the mmap() interface.  so I added that too.

but still, recording did not work correctly.  it would work the
first time after plugging in a camera, but not any time after that.
eventually I found that the pointer to the current frame in the
mmap() buffer does not get reset when the mmap() buffer is freed
and reallocate once that was fixed, recording video from a video(4)
device then worked like it should.

so here's a diff that includes all the above mentioned changes,
plus a few more that seemed appropriate:
* the mjpeg and uncompreeesed frame descriptors are the same,
  so use a single struct, instead of two different one.  also
  unify the handling of frame descriptors.
* really quit using the frame index from the frame descriptor
  as an index to our array of frame descriptors.  this means the
  array now starts at 0 instead of 1, which makes the code less
  confusing.
* replace magic numbers with appropriate value (sizeof(struct
  usb_video_frame_desc) to be exact)
* fill in uvideo_enum_fivals(), which is used for
  VIDIOC_ENUM_FRAMEINTERVALS ioctl

please test with uvideo(4) devices to be sure this does not break
anything that is currently working.  this will be committed in pieces,
but I wanted to get this full diff out for testing purposes.  thanks.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: usb/uvideo.c
===
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.149
diff -u -p usb/uvideo.c
--- usb/uvideo.c25 Jan 2011 20:03:36 -  1.149
+++ usb/uvideo.c20 Mar 2011 21:10:57 -
@@ -48,6 +48,7 @@
 
 #include dev/video_if.h
 
+/* #define UVIDEO_DEBUG */
 #ifdef UVIDEO_DEBUG
 int uvideo_debug = 1;
 #define DPRINTF(l, x...) do { if ((l) = uvideo_debug) printf(x); } while (0)
@@ -93,10 +94,8 @@ usbd_status  uvideo_vs_parse_desc_format_mjpeg(struct u
 usbd_statusuvideo_vs_parse_desc_format_uncompressed(struct uvideo_softc *,
const usb_descriptor_t *);
 usbd_statusuvideo_vs_parse_desc_frame(struct uvideo_softc *);
-usbd_statusuvideo_vs_parse_desc_frame_mjpeg(struct uvideo_softc *,
+usbd_statusuvideo_vs_parse_desc_frame_sub(struct uvideo_softc *,
const usb_descriptor_t *);
-usbd_statusuvideo_vs_parse_desc_frame_uncompressed(struct uvideo_softc *,
-   const usb_descriptor_t *);
 usbd_statusuvideo_vs_parse_desc_alt(struct uvideo_softc *, int, int, int);
 usbd_statusuvideo_vs_set_alt(struct uvideo_softc *, usbd_interface_handle,
int);
@@ -152,13 +151,11 @@ void  uvideo_dump_desc_cs_endpoint(struct 
uvideo_softc
const usb_descriptor_t *);
 void   uvideo_dump_desc_colorformat(struct uvideo_softc *,
const usb_descriptor_t *);
-void   uvideo_dump_desc_frame_mjpeg(struct uvideo_softc *,
-   const usb_descriptor_t *);
 void   uvideo_dump_desc_format_mjpeg(struct uvideo_softc *,
const usb_descriptor_t *);
 void   uvideo_dump_desc_format_uncompressed(struct uvideo_softc *,
const usb_descriptor_t *);
-void   uvideo_dump_desc_frame_uncompressed(struct uvideo_softc *,
+void   uvideo_dump_desc_frame(struct uvideo_softc *,
const usb_descriptor_t *);
 void   uvideo_dump_desc_processing(struct uvideo_softc *,
const usb_descriptor_t *);
@@ -178,6 +175,8 @@ int uvideo_enum_fsizes(void *, struct 
v4l2_frmsizeenu
 intuvideo_enum_fivals(void *, struct v4l2_frmivalenum *);
 intuvideo_s_fmt(void *, struct v4l2_format *);
 intuvideo_g_fmt(void *, struct v4l2_format *);
+intuvideo_s_parm(void *, struct v4l2_streamparm *);
+intuvideo_g_parm(void *, struct v4l2_streamparm *);
 intuvideo_enum_input(void *, struct v4l2_input *);
 intuvideo_s_input(void *, int);
 intuvideo_reqbufs(void *, struct v4l2_requestbuffers *);
@@ -225,6 +224,8 @@ struct video_hw_if uvideo_hw_if = {
uvideo_enum_fivals, /* VIDIOC_ENUM_FRAMEINTERVALS */
uvideo_s_fmt,   /* VIDIOC_S_FMT */
uvideo_g_fmt,   /* VIDIOC_G_FMT */
+   uvideo_s_parm,  /* VIDIOC_S_PARM */
+   uvideo_g_parm,  /* VIDIOC_G_PARM */
uvideo_enum_input,  /* VIDIOC_ENUMINPUT */
uvideo_s_input, /* VIDIOC_S_INPUT */
uvideo_reqbufs, /* 

Re: thinkpad mute mic key

2011-03-14 Thread Jacob Meuser
I like this *MUCH* better than the way acpithinkpad volume buttons
currently work.  these buttons should be affecting the mixer, if
at all possible, instead of doing things behind the audio system's
back, which causes confusion.

On Sun, Mar 13, 2011 at 10:11:54PM +0300, Alexander Polakov wrote:
 Hi,
 
 This is a diff to add support for mute microphone key, which can be
 found on some thinkpad models, which is currently reported as
 
 acpithinkpad0: unknown event 0x101b
 
 A simple solution would be just add definition to acpithinkpad.c and set
 handled to 1, but I chose a bit harder way of adding proper support.
 My attempt below.
 
 
 Index: sys/dev/audio.c
 ===
 RCS file: /cvs/src/sys/dev/audio.c,v
 retrieving revision 1.111
 diff -u -r1.111 audio.c
 --- sys/dev/audio.c   18 Nov 2010 21:15:14 -  1.111
 +++ sys/dev/audio.c   13 Mar 2011 19:10:19 -
 @@ -244,7 +244,7 @@
  
  #if NWSKBD  0
  /* Mixer manipulation using keyboard */
 -int wskbd_set_mixervolume(long);
 +int wskbd_set_mixervolume(long, int);
  #endif
  
  int
 @@ -3358,20 +3358,23 @@
  
  #if NWSKBD  0
  int
 -wskbd_set_mixervolume(long dir)
 +wskbd_set_mixervolume(long dir, int out)
  {
   struct audio_softc *sc;
   mixer_devinfo_t mi;
   int error;
   u_int gain;
   u_char balance, mute;
 + struct au_mixer_ports *ports;
  
   if (audio_cd.cd_ndevs == 0 || (sc = audio_cd.cd_devs[0]) == NULL) {
   DPRINTF((wskbd_set_mixervolume: audio_cd\n));
   return (ENXIO);
   }
  
 - if (sc-sc_outports.master == -1) {
 + ports = out ? sc-sc_outports : sc-sc_inports;
 +
 + if (ports-master == -1) {
   DPRINTF((wskbd_set_mixervolume: master == -1\n));
   return (ENXIO);
   }
 @@ -3379,7 +3382,7 @@
   if (dir == 0) {
   /* Mute */
  
 - error = au_get_mute(sc, sc-sc_outports, mute);
 + error = au_get_mute(sc, ports, mute);
   if (error != 0) {
   DPRINTF((wskbd_set_mixervolume:
au_get_mute: %d\n, error));
 @@ -3388,7 +3391,7 @@
  
   mute = !mute;
  
 - error = au_set_mute(sc, sc-sc_outports, mute);
 + error = au_set_mute(sc, ports, mute);
   if (error != 0) {
   DPRINTF((wskbd_set_mixervolume:
au_set_mute: %d\n, error));
 @@ -3397,7 +3400,7 @@
   } else {
   /* Raise or lower volume */
  
 - mi.index = sc-sc_outports.master;
 + mi.index = ports-master;
   error = sc-hw_if-query_devinfo(sc-hw_hdl, mi);
   if (error != 0) {
   DPRINTF((wskbd_set_mixervolume:
 @@ -3405,14 +3408,14 @@
   return (error);
   }
  
 - au_get_gain(sc, sc-sc_outports, gain, balance);
 + au_get_gain(sc, ports, gain, balance);
  
   if (dir  0)
   gain += mi.un.v.delta;
   else
   gain -= mi.un.v.delta;
  
 - error = au_set_gain(sc, sc-sc_outports, gain, balance);
 + error = au_set_gain(sc, ports, gain, balance);
   if (error != 0) {
   DPRINTF((wskbd_set_mixervolume:
au_set_gain: %d\n, error));
 Index: sys/dev/acpi/acpiasus.c
 ===
 RCS file: /cvs/src/sys/dev/acpi/acpiasus.c,v
 retrieving revision 1.11
 diff -u -r1.11 acpiasus.c
 --- sys/dev/acpi/acpiasus.c   28 Aug 2010 17:59:17 -  1.11
 +++ sys/dev/acpi/acpiasus.c   13 Mar 2011 19:10:19 -
 @@ -90,7 +90,7 @@
  int  acpiasus_activate(struct device *, int);
  
  #if NAUDIO  0  NWSKBD  0
 -extern int wskbd_set_mixervolume(long dir);
 +extern int wskbd_set_mixervolume(long dir, int out);
  #endif
  
  struct cfattach acpiasus_ca = {
 @@ -173,15 +173,15 @@
  #if NAUDIO  0  NWSKBD  0
   case ASUS_NOTIFY_VOLUMEMUTE:
   workq_add_task(NULL, 0, (workq_fn)wskbd_set_mixervolume,
 - (void *)(long)0, NULL);
 + (void *)(long)0, (void *)(int)1);
   break;
   case ASUS_NOTIFY_VOLUMEDOWN:
   workq_add_task(NULL, 0, (workq_fn)wskbd_set_mixervolume,
 - (void *)(long)-1, NULL);
 + (void *)(long)-1, (void *)(int)1);
   break;
   case ASUS_NOTIFY_VOLUMEUP:
   workq_add_task(NULL, 0, (workq_fn)wskbd_set_mixervolume,
 - (void *)(long)1, NULL);
 + (void *)(long)1, (void *)(int)1);
   break;
  #else
   case ASUS_NOTIFY_VOLUMEMUTE:
 Index: sys/dev/acpi/acpithinkpad.c
 ===
 RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
 retrieving revision 1.25
 diff -u -r1.25 acpithinkpad.c
 --- 

mfi(4): use 64-bit frames (unconditionally?)

2011-03-14 Thread Jacob Meuser
the following diff make mfi(4) use 64-bit frames, and support 64-bit
dma addresses.  these changes are based on freebsd's mfi(4).  however,
freebsd only uses 64-bit frames 'if (sizeof(bus_addr_t)) == 8',
whereas this patch uses 64-bit frames unconditionally, for both 32-bit
and 64-bit platforms.  I did it unconditionally, because it makes the
code a bit simpler.

according to my tests, this does have a slightly negative speed
impact on i386.

I've tested it with the following script.

#!/bin/sh

for i in 1 2 3
do
cd /usr/obj
sudo rm -rf *

cd /usr/src
time (make -j4 obj  /dev/null 21  \
make -j4 includes  /dev/null 21  \
make -j4 build  /dev/null 21)
done


I ran this twice on each combination of i386/amd64, current code/with
patch.  these are the results (numbers after each run are the mean
averages for that run):

i386

  32-bit mfi frames (current code)

  run 1
   32m11.25s real33m48.65s user10m42.95s system
   32m25.02s real33m51.56s user10m50.86s system
   32m32.85s real33m51.49s user10m54.01s system

   32m23.04s 33m50.57s 10m49.27s

  run 2
   32m32.08s real33m51.22s user10m49.67s system
   32m26.76s real33m47.27s user10m52.08s system
   32m34.42s real33m46.22s user10m51.88s system

   32m31.09s 33m48.24s 10m51.21s

  64-bit mfi frames (with patch)

  run 1
   32m39.15s real33m38.95s user10m59.97s system
   32m24.67s real33m36.85s user11m6.44s system
   32m25.48s real33m35.04s user11m3.67s system

   32m29.76s 33m36.95s 11m3.20s

  run 2
   32m25.55s real33m39.58s user11m6.11s system
   33m04.37s real33m34.70s user11m9.84s system
   32m33.54s real33m41.38s user11m7.65s system

   32m41.15s 33m38.55s 11m7.87s


amd64

  32-bit mfi frames (current code)

  run 1
   17m43.44s real17m49.14s user 8m19.46s system
   17m51.65s real17m49.07s user 8m29.63s system
   17m50.89s real17m49.28s user 8m28.45s system

   17m48.66s 17m49.16s  8m25.85s

  run 2
   17m49.82s real17m47.05s user 8m32.91s system
   17m49.68s real17m51.12s user 8m27.38s system
   17m51.37s real17m47.93s user 8m33.09s system

   17m50.29s 17m48.70s  8m31.13s

  64-bit mfi frames (with patch)

  run 1
   17m44.04s real17m51.22s user 8m21.02s system
   17m51.86s real17m51.00s user 8m28.67s system
   17m51.87s real17m50.50s user 8m30.44s system

   17m49.26s 17m51.04s  8m26.71s

  run 2
   17m54.74s real17m54.06s user 8m29.78s system
   17m53.51s real17m49.95s user 8m33.98s system
   17m50.85s real17m49.76s user 8m32.85s system

   17m53.03s 17m51.26s  8m32.20s


so, the question is, is the impact on i386 enough to warrant using
32-bit frames on 32-bit platforms?  if so, should this be decided
at runtime or compile time?

any other thoughts?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: mfi.c
===
RCS file: /cvs/src/sys/dev/ic/mfi.c,v
retrieving revision 1.114
diff -u -p mfi.c
--- mfi.c   30 Dec 2010 08:53:50 -  1.114
+++ mfi.c   14 Mar 2011 20:05:59 -
@@ -618,7 +618,7 @@ int
 mfi_attach(struct mfi_softc *sc, enum mfi_iop iop)
 {
struct scsibus_attach_args saa;
-   uint32_tstatus, frames;
+   uint32_tstatus, frames, max_sgl;
int i;
 
switch (iop) {
@@ -648,7 +648,8 @@ mfi_attach(struct mfi_softc *sc, enum mfi_iop iop)
 
status = mfi_fw_state(sc);
sc-sc_max_cmds = status  MFI_STATE_MAXCMD_MASK;
-   sc-sc_max_sgl = (status  MFI_STATE_MAXSGL_MASK)  16;
+   max_sgl = (status  MFI_STATE_MAXSGL_MASK)  16;
+   sc-sc_max_sgl = min(max_sgl, (128 * 1024) / PAGE_SIZE + 1);
DNPRINTF(MFI_D_MISC, %s: max commands: %u, max sgl: %u\n,
DEVNAME(sc), sc-sc_max_cmds, sc-sc_max_sgl);
 
@@ -662,8 +663,7 @@ mfi_attach(struct mfi_softc *sc, enum mfi_iop iop)
}
 
/* frame memory */
-   /* we are not doing 64 bit IO so only calculate # of 32 bit frames */
-   frames = (sizeof(struct mfi_sg32) * sc-sc_max_sgl +
+   frames = (sizeof(struct mfi_sgl) * sc-sc_max_sgl +
MFI_FRAME_SIZE - 1) / MFI_FRAME_SIZE + 1;
sc-sc_frames_size = frames * MFI_FRAME_SIZE;
sc-sc_frames = mfi_allocmem(sc, sc-sc_frames_size * sc-sc_max_cmds);
@@ -1105,7 +1105,7 @@ mfi_create_sgl(struct mfi_ccb *ccb, int flags)
struct mfi_softc*sc = ccb-ccb_sc;
struct mfi_frame_header *hdr;
bus_dma_segment_t   *sgd;
-   union mfi_sgl   *sgl;
+   struct mfi_sgl  *sgl;
int error, i;
 
DNPRINTF(MFI_D_DMA, %s: mfi_create_sgl 

azalia(4) resume diff

2011-02-28 Thread Jacob Meuser
some ATI azalia controllers are brojen after resume, as in PR 6550.
the following diff gathers most of the pci conf register manipulation
done in azalia_pci_attach() into a new function, azalia_configure_pci(),
and calls that function in azalia_pci_attach() and azalia_resume().

this fixes one reported case of post-resume brokenness and doesn't
cause any problems on other machines I've tested on, including an
ATI device that was working.  waiting to hear back about the PR and
other reports, but wanted to get this out for testing asap.

please test.  thanks.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: azalia.c
===
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.190
diff -u -p azalia.c
--- azalia.c17 Feb 2011 17:38:55 -  1.190
+++ azalia.c28 Feb 2011 22:25:56 -
@@ -198,6 +198,7 @@ int azalia_pci_match(struct device *, void *, void *);
 void   azalia_pci_attach(struct device *, struct device *, void *);
 intazalia_pci_activate(struct device *, int);
 intazalia_pci_detach(struct device *, int);
+void   azalia_configure_pci(azalia_t *);
 intazalia_intr(void *);
 void   azalia_print_codec(codec_t *);
 intazalia_reset(azalia_t *);
@@ -375,66 +376,37 @@ azalia_pci_write(pci_chipset_tag_t pc, pcitag_t pa, in
pci_conf_write(pc, pa, (reg  ~0x03), pcival);
 }
 
-int
-azalia_pci_match(struct device *parent, void *match, void *aux)
-{
-   struct pci_attach_args *pa;
-
-   pa = aux;
-   if (PCI_CLASS(pa-pa_class) == PCI_CLASS_MULTIMEDIA
-PCI_SUBCLASS(pa-pa_class) == PCI_SUBCLASS_MULTIMEDIA_HDAUDIO)
-   return 1;
-   return 0;
-}
-
 void
-azalia_pci_attach(struct device *parent, struct device *self, void *aux)
+azalia_configure_pci(azalia_t *az)
 {
-   azalia_t *sc;
-   struct pci_attach_args *pa;
pcireg_t v;
-   pci_intr_handle_t ih;
-   const char *interrupt_str;
uint8_t reg;
 
-   sc = (azalia_t*)self;
-   pa = aux;
-
-   sc-dmat = pa-pa_dmat;
-
-   v = pci_conf_read(pa-pa_pc, pa-pa_tag, ICH_PCI_HDBARL);
-   v = PCI_MAPREG_TYPE_MASK | PCI_MAPREG_MEM_TYPE_MASK;
-   if (pci_mapreg_map(pa, ICH_PCI_HDBARL, v, 0,
-  sc-iot, sc-ioh, NULL, sc-map_size, 0)) {
-   printf(: can't map device i/o space\n);
-   return;
-   }
-
/* enable back-to-back */
-   v = pci_conf_read(pa-pa_pc, pa-pa_tag, PCI_COMMAND_STATUS_REG);
-   pci_conf_write(pa-pa_pc, pa-pa_tag, PCI_COMMAND_STATUS_REG,
+   v = pci_conf_read(az-pc, az-tag, PCI_COMMAND_STATUS_REG);
+   pci_conf_write(az-pc, az-tag, PCI_COMMAND_STATUS_REG,
v | PCI_COMMAND_BACKTOBACK_ENABLE);
 
/* traffic class select */
-   v = pci_conf_read(pa-pa_pc, pa-pa_tag, ICH_PCI_HDTCSEL);
-   pci_conf_write(pa-pa_pc, pa-pa_tag, ICH_PCI_HDTCSEL,
+   v = pci_conf_read(az-pc, az-tag, ICH_PCI_HDTCSEL);
+   pci_conf_write(az-pc, az-tag, ICH_PCI_HDTCSEL,
v  ~(ICH_PCI_HDTCSEL_MASK));
 
/* disable MSI, use INTx instead */
-   if (PCI_VENDOR(pa-pa_id) == PCI_VENDOR_INTEL) {
-   reg = azalia_pci_read(pa-pa_pc, pa-pa_tag, ICH_PCI_MMC);
+   if (PCI_VENDOR(az-pciid) == PCI_VENDOR_INTEL) {
+   reg = azalia_pci_read(az-pc, az-tag, ICH_PCI_MMC);
reg = ~(ICH_PCI_MMC_ME);
-   azalia_pci_write(pa-pa_pc, pa-pa_tag, ICH_PCI_MMC, reg);
+   azalia_pci_write(az-pc, az-tag, ICH_PCI_MMC, reg);
}
 
/* enable PCIe snoop */
-   switch (PCI_PRODUCT(pa-pa_id)) {
+   switch (PCI_PRODUCT(az-pciid)) {
case PCI_PRODUCT_ATI_SB450_HDA:
case PCI_PRODUCT_ATI_SBX00_HDA:
-   reg = azalia_pci_read(pa-pa_pc, pa-pa_tag, 
ATI_PCIE_SNOOP_REG);
+   reg = azalia_pci_read(az-pc, az-tag, ATI_PCIE_SNOOP_REG);
reg = ATI_PCIE_SNOOP_MASK;
reg |= ATI_PCIE_SNOOP_ENABLE;
-   azalia_pci_write(pa-pa_pc, pa-pa_tag, ATI_PCIE_SNOOP_REG, 
reg);
+   azalia_pci_write(az-pc, az-tag, ATI_PCIE_SNOOP_REG, reg);
break;
case PCI_PRODUCT_NVIDIA_MCP51_HDA:
case PCI_PRODUCT_NVIDIA_MCP55_HDA:
@@ -458,26 +430,26 @@ azalia_pci_attach(struct device *parent, struct device
case PCI_PRODUCT_NVIDIA_MCP89_HDA_2:
case PCI_PRODUCT_NVIDIA_MCP89_HDA_3:
case PCI_PRODUCT_NVIDIA_MCP89_HDA_4:
-   reg = azalia_pci_read(pa-pa_pc, pa-pa_tag,
+   reg = azalia_pci_read(az-pc, az-tag,
NVIDIA_HDA_OSTR_COH_REG);
reg |= NVIDIA_HDA_STR_COH_ENABLE;
-   azalia_pci_write(pa-pa_pc, pa-pa_tag,
+   azalia_pci_write(az-pc, az-tag,
NVIDIA_HDA_OSTR_COH_REG, reg);
 
-   reg = azalia_pci_read(pa-pa_pc, pa-pa_tag,
+   reg = 

Re: azalia(4) resume diff

2011-02-28 Thread Jacob Meuser
On Mon, Feb 28, 2011 at 08:56:57PM -0500, Brad wrote:
 On Mon, Feb 28, 2011 at 11:16:49PM +, Jacob Meuser wrote:
  some ATI azalia controllers are brojen after resume, as in PR 6550.
  the following diff gathers most of the pci conf register manipulation
  done in azalia_pci_attach() into a new function, azalia_configure_pci(),
  and calls that function in azalia_pci_attach() and azalia_resume().
  
  this fixes one reported case of post-resume brokenness and doesn't
  cause any problems on other machines I've tested on, including an
  ATI device that was working.  waiting to hear back about the PR and
  other reports, but wanted to get this out for testing asap.
  
  please test.  thanks.
 
 This doesn't change anything about the symptom mentioned in the PR
 and its still broken after resume.

to be honest, I didn't really expect it to.  it appears that the device
doesn't even get powered up after resume.  probably still some power
management issues.  it *used* to be, until fairly recently, that the
device was getting powered down even before azalia(4) could shut it
down.

 
 -- 
 This message has been scanned for viruses and
 dangerous content by MailScanner, and is
 believed to be clean.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: new usb quirk and gps device id that needs it

2011-02-11 Thread Jacob Meuser
On Thu, Feb 10, 2011 at 02:21:23AM -0800, Daniel C. Sinclair wrote:
 On Wed, Feb 9, 2011 at 10:52 PM, Jacob Meuser jake...@sdf.lonestar.org
 wrote:
  this actually works?  could you please send usbctl (from the usbutil
  package) output for this device?  I don't like adding more quirks.
  if the device has the bulk endpoints in the control interface, then the
  requirement that the endpoints be in a different interface is overly
  restrictive.
 
 DEVICE addr 2
 DEVICE descriptor:
 bLength=18 bDescriptorType=device(1) bcdUSB=2.00 bDeviceClass=2
 bDeviceSubClass=0
 bDeviceProtocol=0 bMaxPacketSize=64 idVendor=0x0e8d idProduct=0x3329
 bcdDevice=100
 iManufacturer=3(MTK) iProduct=4(GPS Receiver) iSerialNumber=0()
 bNumConfigurations=1
 
 CONFIGURATION descriptor 0:
 bLength=9 bDescriptorType=config(2) wTotalLength=67 bNumInterface=2
 bConfigurationValue=1 iConfiguration=0() bmAttributes=80 bMaxPower=500 mA
 
 INTERFACE descriptor 0:
 bLength=9 bDescriptorType=interface(4) bInterfaceNumber=0 bAlternateSetting=0
 bNumEndpoints=2 bInterfaceClass=10 bInterfaceSubClass=0
 bInterfaceProtocol=0 iInterface=1(GPS COM(data_if))
 
 ENDPOINT descriptor:
 bLength=7 bDescriptorType=endpoint(5) bEndpointAddress=1-in
 bmAttributes=bulk wMaxPacketSize=64 bInterval=0
 
 ENDPOINT descriptor:
 bLength=7 bDescriptorType=endpoint(5) bEndpointAddress=1-out
 bmAttributes=bulk wMaxPacketSize=64 bInterval=0
 
 INTERFACE descriptor 1:
 bLength=28 bDescriptorType=interface(4) bInterfaceNumber=1
 bAlternateSetting=0
 bNumEndpoints=1 bInterfaceClass=2 bInterfaceSubClass=2
 bInterfaceProtocol=1 iInterface=2(GPS COM(comm_if))
 
 ENDPOINT descriptor:
 bLength=7 bDescriptorType=endpoint(5) bEndpointAddress=2-in
 bmAttributes=interrupt wMaxPacketSize=64 bInterval=1
 
 current configuration 1
 
 --

does the following work for you?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: umodem.c
===
RCS file: /cvs/src/sys/dev/usb/umodem.c,v
retrieving revision 1.41
diff -u -p umodem.c
--- umodem.c25 Jan 2011 20:03:36 -  1.41
+++ umodem.c11 Feb 2011 16:34:47 -
@@ -170,9 +170,11 @@ umodem_get_caps(struct usb_attach_arg *uaa, int ctl_if
const usb_cdc_cm_descriptor_t *cmd;
const usb_cdc_acm_descriptor_t *acmd;
const usb_cdc_union_descriptor_t *uniond;
+   const usb_endpoint_descriptor_t *ed;
usbd_desc_iter_t iter;
-   int current_iface_no = -1;
+   int bulkin, bulkout, current_iface_no;
 
+   *data_iface_no = current_iface_no = bulkin = bulkout = -1;
*cm_cap = *acm_cap = 0;
usb_desc_iter_init(uaa-device, iter);
desc = usb_desc_iter_next(iter);
@@ -198,9 +200,26 @@ umodem_get_caps(struct usb_attach_arg *uaa, int ctl_if
*data_iface_no = uniond-bSlaveInterface[0];
break;
}
+   } else if (current_iface_no == ctl_iface_no 
+   desc-bDescriptorType == UDESC_CS_ENDPOINT) {
+   ed = (usb_endpoint_descriptor_t *)desc;
+   if (UE_GET_XFERTYPE(ed-bmAttributes) == UE_BULK) {
+   if (UE_GET_DIR(ed-bEndpointAddress) ==
+   UE_DIR_IN)
+   bulkin = ed-bEndpointAddress;
+   else
+   bulkout = ed-bEndpointAddress;
+   }
}
desc = usb_desc_iter_next(iter);
}
+
+   /*
+* Usually, there is a separate data interface.  However, some
+* devices have the bulk data endpoints in the control interface.
+*/
+   if (*data_iface_no == -1  (bulkin != -1  bulkout != -1))
+   *data_iface_no = ctl_iface_no;
 }
 
 int
@@ -234,10 +253,13 @@ umodem_match(struct device *parent, void *match, void 
if (ret == UMATCH_NONE)
return (ret);
 
-   /* umodem doesn't yet support devices without a data iface */
+   /*
+* Check that either the bulk endpoints are in the control interface
+* or that there is a data interface.
+*/
umodem_get_caps(uaa, id-bInterfaceNumber, data_iface_no,
cm_cap, acm_cap);
-   if (data_iface_no == 0)
+   if (data_iface_no == -1)
ret = UMATCH_NONE;
 
return (ret);
@@ -267,8 +289,8 @@ umodem_attach(struct device *parent, struct device *se
/* Get the capabilities. */
umodem_get_caps(uaa, id-bInterfaceNumber, data_iface_no,
sc-sc_cm_cap, sc-sc_acm_cap);
-   if (data_iface_no == 0) {
-   printf(%s: no pointer to data interface\n,
+   if (data_iface_no == -1) {
+   printf(%s: no interface with bulk endpoints\n,
   sc-sc_dev.dv_xname);
goto bad;
}



Re: new usb quirk and gps device id that needs it

2011-02-09 Thread Jacob Meuser
this actually works?  could you please send usbctl (from the usbutil
package) output for this device?  I don't like adding more quirks.
if the device has the bulk endpoints in the control interface, then the
requirement that the endpoints be in a different interface is overly
restrictive.

On Wed, Feb 09, 2011 at 06:54:06PM -0800, Daniel C. Sinclair wrote:
 Hello, I recently got a Qstarz BT-Q818XT GPS receiver
 (http://www.qstarz.com/Products/GPS%20Products/BT-Q818XT-F.htm) which
 contains an MTK vII/3329 chipset
 (http://www.mediatek.com/en/product/info.php?sn=50).  I got the
 following when I plugged it into my laptop which is running
 yesterday's snapshot:
 
 umodem0 at uhub1 port 2 configuration 1 interface 1 MTK GPS Receiver
 rev 2.00/1.00 addr 2
 umodem0: no pointer to data interface
 ugen0 at uhub1 port 2 configuration 1 MTK GPS Receiver rev 2.00/1.00 addr 2
 
 I came up with some patches that make my GPS work and now I get the following:
 
 umodem0 at uhub1 port 2 configuration 1 interface 1 MTK GPS Receiver
 rev 2.00/1.00 addr 2
 umodem0: data interface 0, has no CM over data, has no break
 umodem0: status change notification available
 ucom0 at umodem0
 
 The first patch adds a new class of USB quirks:
 
 Index: usb_quirks.h
 ===
 RCS file: /cvs/src/sys/dev/usb/usb_quirks.h,v
 retrieving revision 1.16
 diff -u -r1.16 usb_quirks.h
 --- usb_quirks.h  19 Jul 2010 05:08:37 -  1.16
 +++ usb_quirks.h  10 Feb 2011 01:37:18 -
 @@ -49,6 +49,7 @@
  #define UQ_MS_LEADING_BYTE   0x0001 /* mouse sends unknown leading byte 
 */
  #define UQ_EHCI_NEEDTO_DISOWN0x0002 /* must hand device over to 
 USB 1.1
   if attached to EHCI */
 +#define UQ_NO_CDC_UNION  0x0004 /* no CDC UNION descriptor */
  };
 
  extern const struct usbd_quirks usbd_no_quirk;
 Index: umodem.c
 ===
 RCS file: /cvs/src/sys/dev/usb/umodem.c,v
 retrieving revision 1.41
 diff -u -r1.41 umodem.c
 --- umodem.c  25 Jan 2011 20:03:36 -  1.41
 +++ umodem.c  10 Feb 2011 01:37:18 -
 @@ -267,10 +267,13 @@
   /* Get the capabilities. */
   umodem_get_caps(uaa, id-bInterfaceNumber, data_iface_no,
   sc-sc_cm_cap, sc-sc_acm_cap);
 - if (data_iface_no == 0) {
 - printf(%s: no pointer to data interface\n,
 -sc-sc_dev.dv_xname);
 - goto bad;
 +
 + if (!(usbd_get_quirks(sc-sc_udev)-uq_flags  UQ_NO_CDC_UNION)) {
 + if (data_iface_no == 0) {
 + printf(%s: no pointer to data interface\n,
 +sc-sc_dev.dv_xname);
 + goto bad;
 + }
   }
 
   printf(%s: data interface %d, has %sCM over data, has %sbreak\n,
 
 
 The second patch adds the vendor/device IDs and makes the GPS use the quirk:
 
 Index: usbdevs
 ===
 RCS file: /cvs/src/sys/dev/usb/usbdevs,v
 retrieving revision 1.540
 diff -u -r1.540 usbdevs
 --- usbdevs   1 Feb 2011 18:23:59 -   1.540
 +++ usbdevs   10 Feb 2011 01:37:53 -
 @@ -437,6 +437,7 @@
  vendor HAWKING   0x0e66  Hawking
  vendor FOSSIL0x0e67  Fossil
  vendor GMATE 0x0e7e  G.Mate
 +vendor MTK   0x0e8d  MTK
  vendor OTI   0x0ea0  Ours Technology
  vendor PILOTECH  0x0eaf  Pilotech
  vendor NOVATECH  0x0eb0  Nova Tech
 @@ -2565,6 +2566,9 @@
 
  /* MDS products */
  product MDS ISDBT0x0001  MDS ISDB-T tuner
 +
 +/* MTK products */
 +product MTK VII3329  0x3329  vII/3329 GPS Receiver
 
  /* Meinberg Funkuhren products */
  product MEINBERG USB5131 0x0301  USB 5131 DCF77 - Radio Clock
 Index: usb_quirks.c
 ===
 RCS file: /cvs/src/sys/dev/usb/usb_quirks.c,v
 retrieving revision 1.63
 diff -u -r1.63 usb_quirks.c
 --- usb_quirks.c  2 Dec 2010 06:39:09 -   1.63
 +++ usb_quirks.c  10 Feb 2011 01:37:54 -
 @@ -133,6 +133,8 @@
   { USB_VENDOR_VELLEMAN, USB_PRODUCT_VELLEMAN_K8055,  ANY,{ UQ_BAD_HID }},
   { USB_VENDOR_DREAMLINK, USB_PRODUCT_DREAMLINK_ULMB1,ANY,{ 
 UQ_BAD_HID }},
 
 + { USB_VENDOR_MTK, USB_PRODUCT_MTK_VII3329,  ANY,{ UQ_NO_CDC_UNION }},
 +
   { USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E220,   ANY,{ UQ_NO_STRINGS 
 }},
   { USB_VENDOR_SHANTOU, USB_PRODUCT_SHANTOU_DM9601, ANY, { UQ_NO_STRINGS }},
   { USB_VENDOR_RALINK, USB_PRODUCT_RALINK_RT2573, ANY,{ UQ_NO_STRINGS 
 }},
 
 
 A very similar thing was done in NetBSD last year.  See
 http://www.mail-archive.com/source-changes-full@netbsd.org/msg15959.html
 and http://www.mail-archive.com/source-changes-full@netbsd.org/msg15960.html
 
 Daniel

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - 

azalia: fix bug in connection list parsing

2011-02-07 Thread Jacob Meuser
widget connection lists are described as a series of nids.  the widget
gives the number of connection list entries.  if the most significant
bit of a connection list entry is set, all nids between the last nid
in the connection list and the nid in the entry with the most significant
bit set are connected.

this means the number of connected nids isn't necessarily the same as
the number of connection list entries.  azalia currently assumes they
are the same, and ends up ignoring some of the connections, which leads
to widgets being considered unusable.

the following runs through the connection list entries twice.  the
first is to get the number of connections.  then once the number of
connections is known, an array to hold the nids of the connected
widgets is allocated.  then loop over the connection list entries
again and insert the nids into the array.

this affects almost all Analog Devices (AD198x) and Sigmatel/IDT
(STAC*/92HD*) codecs.

please test and report to me.  please include a dmesg and a diff of
'mixerctl -v' output from before and after applying the diff.  thanks.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: azalia.c
===
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.188
diff -u -p azalia.c
--- azalia.c12 Sep 2010 03:17:34 -  1.188
+++ azalia.c8 Feb 2011 03:52:17 -
@@ -3330,7 +3330,7 @@ azalia_widget_init_connection(widget_t *this, const co
uint32_t result;
int err;
int i, j, k;
-   int length, bits, conn, last;
+   int length, nconn, bits, conn, last;
 
this-selected = -1;
if ((this-widgetcap  COP_AWCAP_CONNLIST) == 0)
@@ -3349,32 +3349,59 @@ azalia_widget_init_connection(widget_t *this, const co
if (length == 0)
return 0;
 
-   this-nconnections = length;
-   this-connections = malloc(sizeof(nid_t) * length, M_DEVBUF, M_NOWAIT);
+   /*
+* 'length' is the number of entries, not the number of
+* connections.  Find the number of connections, 'nconn', so
+* enough space can be allocated for the list of connected
+* nids.
+*/
+   nconn = last = 0;
+   for (i = 0; i  length;) {
+   err = azalia_comresp(codec, this-nid,
+   CORB_GET_CONNECTION_LIST_ENTRY, i, result);
+   if (err)
+   return err;
+   for (k = 0; i  length  (k  32 / bits); k++) {
+   conn = (result  (k * bits))  ((1  bits) - 1);
+   /* If high bit is set, this is the end of a continuous
+* list that started with the last connection.
+*/
+   if ((nconn  0)  (conn  (1  (bits - 1
+   nconn += (conn  ~(1  (bits - 1))) - last;
+   else
+   nconn++;
+   last = conn;
+   i++;
+   }
+   }
+
+   this-connections = malloc(sizeof(nid_t) * nconn, M_DEVBUF, M_NOWAIT);
if (this-connections == NULL) {
printf(%s: out of memory\n, XNAME(codec-az));
return ENOMEM;
}
-   for (i = 0; i  length;) {
+   for (i = 0; i  nconn;) {
err = azalia_comresp(codec, this-nid,
CORB_GET_CONNECTION_LIST_ENTRY, i, result);
if (err)
return err;
-   for (k = 0; i  length  (k  32 / bits); k++) {
+   for (k = 0; i  nconn  (k  32 / bits); k++) {
conn = (result  (k * bits))  ((1  bits) - 1);
/* If high bit is set, this is the end of a continuous
 * list that started with the last connection.
 */
if ((i  0)  (conn  (1  (bits - 1 {
-   last = this-connections[i - 1];
-   for (j = 1; i  length  j = conn - last; j++)
+   for (j = 1; i  nconn  j = conn - last; j++)
this-connections[i++] = last + j;
} else {
this-connections[i++] = conn;
}
+   last = conn;
}
}
-   if (length  0) {
+   this-nconnections = nconn;
+
+   if (nconn  0) {
err = azalia_comresp(codec, this-nid,
CORB_GET_CONNECTION_SELECT_CONTROL, 0, result);
if (err)



Re: fix race between usbdevs(8) and usb device attach/detach

2011-02-02 Thread Jacob Meuser
On Fri, Jan 28, 2011 at 09:17:22PM +, Jacob Meuser wrote:
 currently, the USB_DEVICEINFO ioctl (used by usbdevs(8)) races against
 usb device attach/detach.  this is particularly problematic for device
 detachment.  if the ioctl is running exactly when a hub with devices
 attached is removed, there's a good chance of crashing.
 
 the following diff makes the racing parts of the ioctl run as a usb_task
 in the task thread.  since attach and detach are also run as usb_tasks
 in the task thread, the ioctl and attach/detach no longer race.
 
 this also requires the addition of a new function, usb_wait_task(),
 which waits for a task to be completed.  I've also condensed the
 two members of struct usb_task that represent state into one member.
 
 *please test on as many systems as possible.*
 
 this won't make 4.9 without wide testing.  thanks.

I could use some more test reports.  this is in the latest snapshots for
all archs.  thanks.

 
 -- 
 jake...@sdf.lonestar.org
 SDF Public Access UNIX System - http://sdf.lonestar.org
 
 Index: usb.c
 ===
 RCS file: /cvs/src/sys/dev/usb/usb.c,v
 retrieving revision 1.72
 diff -u -p usb.c
 --- usb.c 25 Jan 2011 20:03:36 -  1.72
 +++ usb.c 28 Jan 2011 20:33:50 -
 @@ -275,15 +275,15 @@ usb_add_task(usbd_device_handle dev, struct usb_task *
  {
   int s;
  
 - DPRINTFN(2,(%s: task=%p onqueue=%d type=%d\n, __func__, task,
 - task-onqueue, task-type));
 + DPRINTFN(2,(%s: task=%p state=%d type=%d\n, __func__, task,
 + task-state, task-type));
  
   /* Don't add task if the device's root hub is dying. */
   if (usbd_is_dying(dev))
   return;
  
   s = splusb();
 - if (!task-onqueue) {
 + if (task-state != USB_TASK_STATE_ONQ) {
   switch (task-type) {
   case USB_TASK_TYPE_ABORT:
   TAILQ_INSERT_TAIL(usb_abort_tasks, task, next);
 @@ -295,7 +295,7 @@ usb_add_task(usbd_device_handle dev, struct usb_task *
   TAILQ_INSERT_TAIL(usb_generic_tasks, task, next);
   break;
   }
 - task-onqueue = 1;
 + task-state = USB_TASK_STATE_ONQ;
   task-dev = dev;
   }
   if (task-type == USB_TASK_TYPE_ABORT)
 @@ -310,38 +310,43 @@ usb_rem_task(usbd_device_handle dev, struct usb_task *
  {
   int s;
  
 - DPRINTFN(2,(%s: task=%p onqueue=%d type=%d\n, __func__, task,
 - task-onqueue, task-type));
 + DPRINTFN(2,(%s: task=%p state=%d type=%d\n, __func__, task,
 + task-state, task-type));
  
 + if (task-state != USB_TASK_STATE_ONQ)
 + return;
 +
   s = splusb();
 - if (task-onqueue) {
 - switch (task-type) {
 - case USB_TASK_TYPE_ABORT:
 - TAILQ_REMOVE(usb_abort_tasks, task, next);
 - break;
 - case USB_TASK_TYPE_EXPLORE:
 - TAILQ_REMOVE(usb_explore_tasks, task, next);
 - break;
 - case USB_TASK_TYPE_GENERIC:
 - TAILQ_REMOVE(usb_generic_tasks, task, next);
 - break;
 - }
 - task-onqueue = 0;
 +
 + switch (task-type) {
 + case USB_TASK_TYPE_ABORT:
 + TAILQ_REMOVE(usb_abort_tasks, task, next);
 + break;
 + case USB_TASK_TYPE_EXPLORE:
 + TAILQ_REMOVE(usb_explore_tasks, task, next);
 + break;
 + case USB_TASK_TYPE_GENERIC:
 + TAILQ_REMOVE(usb_generic_tasks, task, next);
 + break;
   }
 + task-state = USB_TASK_STATE_NONE;
 +
   splx(s);
  }
  
  void
 -usb_rem_wait_task(usbd_device_handle dev, struct usb_task *task)
 +usb_wait_task(usbd_device_handle dev, struct usb_task *task)
  {
   int s;
  
 - DPRINTFN(2,(%s: task=%p onqueue=%d type=%d\n, __func__, task,
 - task-onqueue, task-type));
 + DPRINTFN(2,(%s: task=%p state=%d type=%d\n, __func__, task,
 + task-state, task-type));
  
 + if (task-state == USB_TASK_STATE_NONE)
 + return;
 +
   s = splusb();
 - usb_rem_task(dev, task);
 - while (task-running) {
 + while (task-state != USB_TASK_STATE_NONE) {
   DPRINTF((%s: waiting for task to complete\n, __func__));
   tsleep(task, PWAIT, endtask, 0);
   }
 @@ -349,6 +354,13 @@ usb_rem_wait_task(usbd_device_handle dev, struct usb_t
  }
  
  void
 +usb_rem_wait_task(usbd_device_handle dev, struct usb_task *task)
 +{
 + usb_rem_task(dev, task);
 + usb_wait_task(dev, task);
 +}
 +
 +void
  usb_first_explore(void *arg)
  {
   struct usb_softc *sc = arg;
 @@ -406,15 +418,16 @@ usb_task_thread(void *arg)
   tsleep(usb_run_tasks, PWAIT, usbtsk, 0);
   continue;
   }
 - task-onqueue = 0;
   /* Don't execute the task if the root

fix race between usbdevs(8) and usb device attach/detach

2011-01-28 Thread Jacob Meuser
currently, the USB_DEVICEINFO ioctl (used by usbdevs(8)) races against
usb device attach/detach.  this is particularly problematic for device
detachment.  if the ioctl is running exactly when a hub with devices
attached is removed, there's a good chance of crashing.

the following diff makes the racing parts of the ioctl run as a usb_task
in the task thread.  since attach and detach are also run as usb_tasks
in the task thread, the ioctl and attach/detach no longer race.

this also requires the addition of a new function, usb_wait_task(),
which waits for a task to be completed.  I've also condensed the
two members of struct usb_task that represent state into one member.

*please test on as many systems as possible.*

this won't make 4.9 without wide testing.  thanks.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: usb.c
===
RCS file: /cvs/src/sys/dev/usb/usb.c,v
retrieving revision 1.72
diff -u -p usb.c
--- usb.c   25 Jan 2011 20:03:36 -  1.72
+++ usb.c   28 Jan 2011 20:33:50 -
@@ -275,15 +275,15 @@ usb_add_task(usbd_device_handle dev, struct usb_task *
 {
int s;
 
-   DPRINTFN(2,(%s: task=%p onqueue=%d type=%d\n, __func__, task,
-   task-onqueue, task-type));
+   DPRINTFN(2,(%s: task=%p state=%d type=%d\n, __func__, task,
+   task-state, task-type));
 
/* Don't add task if the device's root hub is dying. */
if (usbd_is_dying(dev))
return;
 
s = splusb();
-   if (!task-onqueue) {
+   if (task-state != USB_TASK_STATE_ONQ) {
switch (task-type) {
case USB_TASK_TYPE_ABORT:
TAILQ_INSERT_TAIL(usb_abort_tasks, task, next);
@@ -295,7 +295,7 @@ usb_add_task(usbd_device_handle dev, struct usb_task *
TAILQ_INSERT_TAIL(usb_generic_tasks, task, next);
break;
}
-   task-onqueue = 1;
+   task-state = USB_TASK_STATE_ONQ;
task-dev = dev;
}
if (task-type == USB_TASK_TYPE_ABORT)
@@ -310,38 +310,43 @@ usb_rem_task(usbd_device_handle dev, struct usb_task *
 {
int s;
 
-   DPRINTFN(2,(%s: task=%p onqueue=%d type=%d\n, __func__, task,
-   task-onqueue, task-type));
+   DPRINTFN(2,(%s: task=%p state=%d type=%d\n, __func__, task,
+   task-state, task-type));
 
+   if (task-state != USB_TASK_STATE_ONQ)
+   return;
+
s = splusb();
-   if (task-onqueue) {
-   switch (task-type) {
-   case USB_TASK_TYPE_ABORT:
-   TAILQ_REMOVE(usb_abort_tasks, task, next);
-   break;
-   case USB_TASK_TYPE_EXPLORE:
-   TAILQ_REMOVE(usb_explore_tasks, task, next);
-   break;
-   case USB_TASK_TYPE_GENERIC:
-   TAILQ_REMOVE(usb_generic_tasks, task, next);
-   break;
-   }
-   task-onqueue = 0;
+
+   switch (task-type) {
+   case USB_TASK_TYPE_ABORT:
+   TAILQ_REMOVE(usb_abort_tasks, task, next);
+   break;
+   case USB_TASK_TYPE_EXPLORE:
+   TAILQ_REMOVE(usb_explore_tasks, task, next);
+   break;
+   case USB_TASK_TYPE_GENERIC:
+   TAILQ_REMOVE(usb_generic_tasks, task, next);
+   break;
}
+   task-state = USB_TASK_STATE_NONE;
+
splx(s);
 }
 
 void
-usb_rem_wait_task(usbd_device_handle dev, struct usb_task *task)
+usb_wait_task(usbd_device_handle dev, struct usb_task *task)
 {
int s;
 
-   DPRINTFN(2,(%s: task=%p onqueue=%d type=%d\n, __func__, task,
-   task-onqueue, task-type));
+   DPRINTFN(2,(%s: task=%p state=%d type=%d\n, __func__, task,
+   task-state, task-type));
 
+   if (task-state == USB_TASK_STATE_NONE)
+   return;
+
s = splusb();
-   usb_rem_task(dev, task);
-   while (task-running) {
+   while (task-state != USB_TASK_STATE_NONE) {
DPRINTF((%s: waiting for task to complete\n, __func__));
tsleep(task, PWAIT, endtask, 0);
}
@@ -349,6 +354,13 @@ usb_rem_wait_task(usbd_device_handle dev, struct usb_t
 }
 
 void
+usb_rem_wait_task(usbd_device_handle dev, struct usb_task *task)
+{
+   usb_rem_task(dev, task);
+   usb_wait_task(dev, task);
+}
+
+void
 usb_first_explore(void *arg)
 {
struct usb_softc *sc = arg;
@@ -406,15 +418,16 @@ usb_task_thread(void *arg)
tsleep(usb_run_tasks, PWAIT, usbtsk, 0);
continue;
}
-   task-onqueue = 0;
/* Don't execute the task if the root hub is gone. */
-   if (usbd_is_dying(task-dev))
+   if (usbd_is_dying(task-dev)) {
+   

no /dev/usb means usb events are useless

2011-01-22 Thread Jacob Meuser
NetBSD (where our usb stack came from) has a /dev/usb device node.
this node exists primarily for reading usb events.  however, we
do not create /dev/usb, which means we have no way to get the usb
events.  usb events are device/driver attach/detachments.  we have
hotplug(4) to get this info.

I don't see any reason to keep the usb event handling.

thoughts?  ok?

PS notice how some drivers only do one of attach and detach instead of
both, and that some drivers don't do either ...

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: if_athn_usb.c
===
RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v
retrieving revision 1.6
diff -u -p -r1.6 if_athn_usb.c
--- if_athn_usb.c   8 Jan 2011 15:18:01 -   1.6
+++ if_athn_usb.c   23 Jan 2011 03:54:07 -
@@ -281,8 +281,6 @@ athn_usb_attach(struct device *parent, s
mountroothook_establish(athn_usb_attachhook, usc);
else
athn_usb_attachhook(usc);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_ATTACH, usc-sc_udev, sc-sc_dev);
 }
 
 int
@@ -304,7 +302,6 @@ athn_usb_detach(struct device *self, int
athn_usb_free_tx_list(usc);
athn_usb_free_rx_list(usc);
 
-   usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, usc-sc_udev, sc-sc_dev);
return (0);
 }
 
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.83
diff -u -p -r1.83 if_aue.c
--- if_aue.c6 Dec 2010 04:41:39 -   1.83
+++ if_aue.c23 Jan 2011 03:54:08 -
@@ -837,9 +837,6 @@ aue_attach(struct device *parent, struct
timeout_set(sc-aue_stat_ch, aue_tick, sc);
 
splx(s);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_ATTACH, sc-aue_udev,
-  sc-aue_dev);
 }
 
 int
@@ -886,9 +883,6 @@ aue_detach(struct device *self, int flag
usb_detach_wait(sc-aue_dev);
}
splx(s);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc-aue_udev,
-  sc-aue_dev);
 
return (0);
 }
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.104
diff -u -p -r1.104 if_axe.c
--- if_axe.c6 Dec 2010 04:41:39 -   1.104
+++ if_axe.c23 Jan 2011 03:54:09 -
@@ -814,9 +814,6 @@ axe_attach(struct device *parent, struct
timeout_set(sc-axe_stat_ch, axe_tick, sc);
 
splx(s);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_ATTACH, sc-axe_udev,
-  sc-axe_dev);
 }
 
 int
@@ -875,9 +872,6 @@ axe_detach(struct device *self, int flag
usb_detach_wait(sc-axe_dev);
}
splx(s);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc-axe_udev,
-  sc-axe_dev);
 
return (0);
 }
Index: if_cdce.c
===
RCS file: /cvs/src/sys/dev/usb/if_cdce.c,v
retrieving revision 1.48
diff -u -p -r1.48 if_cdce.c
--- if_cdce.c   16 Jan 2011 22:35:29 -  1.48
+++ if_cdce.c   23 Jan 2011 03:54:10 -
@@ -360,9 +360,6 @@ found:
 
sc-cdce_attached = 1;
splx(s);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_ATTACH, sc-cdce_udev,
-   sc-cdce_dev);
 }
 
 int
@@ -387,9 +384,6 @@ cdce_detach(struct device *self, int fla
 
sc-cdce_attached = 0;
splx(s);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc-cdce_udev,
-   sc-cdce_dev);
 
return (0);
 }
Index: if_cue.c
===
RCS file: /cvs/src/sys/dev/usb/if_cue.c,v
retrieving revision 1.58
diff -u -p -r1.58 if_cue.c
--- if_cue.c17 Dec 2010 13:48:06 -  1.58
+++ if_cue.c23 Jan 2011 03:54:10 -
@@ -542,9 +542,6 @@ cue_attach(struct device *parent, struct
timeout_set(sc-cue_stat_ch, cue_tick, sc);
 
splx(s);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_ATTACH, sc-cue_udev,
-   sc-cue_dev);
 }
 
 int
@@ -585,9 +582,6 @@ cue_detach(struct device *self, int flag
 #endif
 
splx(s);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc-cue_udev,
-   sc-cue_dev);
 
return (0);
 }
Index: if_kue.c
===
RCS file: /cvs/src/sys/dev/usb/if_kue.c,v
retrieving revision 1.62
diff -u -p -r1.62 if_kue.c
--- if_kue.c17 Dec 2010 13:48:06 -  1.62
+++ if_kue.c23 Jan 2011 03:54:11 -
@@ -543,9 +543,6 @@ kue_attach(struct device *parent, struct
mountroothook_establish(kue_attachhook, sc);
else
kue_attachhook(sc);
-
-   usbd_add_drv_event(USB_EVENT_DRIVER_ATTACH, sc-kue_udev,
-  sc-kue_dev);
 }
 
 int
Index: if_mos.c

Re: no /dev/usb means usb events are useless

2011-01-22 Thread Jacob Meuser
On Sun, Jan 23, 2011 at 05:03:18AM +, Jacob Meuser wrote:
 NetBSD (where our usb stack came from) has a /dev/usb device node.
 this node exists primarily for reading usb events.  however, we
 do not create /dev/usb, which means we have no way to get the usb
 events.  usb events are device/driver attach/detachments.  we have
 hotplug(4) to get this info.
 
 I don't see any reason to keep the usb event handling.
 
 thoughts?  ok?
 
 PS notice how some drivers only do one of attach and detach instead of
 both, and that some drivers don't do either ...

snip

 @@ -717,83 +630,6 @@ usbioctl(dev_t devt, u_long cmd, caddr_t
   return (0);
  }
  
 -int
 -usbpoll(dev_t dev, int events, struct proc *p)
 -{
 - int revents, mask, s;
 -
 - if (minor(dev) == USB_DEV_MINOR) {
 - revents = 0;
 - mask = POLLIN | POLLRDNORM;
 -
 - s = splusb();
 - if (events  mask  usb_nevents  0)
 - revents |= events  mask;
 - if (revents == 0  events  mask)
 - selrecord(p, usb_selevent);
 - splx(s);
 -
 - return (revents);
 - } else {
 - return (POLLERR);
 - }
 -}

sigh.  this is needed as well.  obviously.  sorry about that ...

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: conf.h
===
RCS file: /cvs/src/sys/sys/conf.h,v
retrieving revision 1.109
diff -u -p conf.h
--- conf.h  8 Jan 2011 19:45:09 -   1.109
+++ conf.h  23 Jan 2011 05:25:32 -
@@ -396,7 +396,7 @@ extern struct cdevsw cdevsw[];
 #definecdev_usb_init(c,n) { \
dev_init(c,n,open), dev_init(c,n,close), (dev_type_read((*))) enodev, \
(dev_type_write((*))) enodev, dev_init(c,n,ioctl), \
-   (dev_type_stop((*))) enodev, 0, dev_init(c,n,poll), \
+   (dev_type_stop((*))) enodev, 0, selfalse, \
(dev_type_mmap((*))) enodev }
 
 /* open, close, write, ioctl */



allow ugen(4) to attach to unused interfaces

2011-01-03 Thread Jacob Meuser
when a USB device is inserted, usbd_probe_and_attach() first tries to
find a driver that claims to support the device by matching the
vendor/product IDs.  if no such driver is found, usbd_probe_and_attach()
loops through each interface of each of the devices configurations,
trying to match interface (aka class) drivers to an interface.  if
the device has more than one interface *in a single configuration*
that matches drivers, then drivers are allowed to attach to each
interface.  for example:

uvideo0 at uhub5 port 1 configuration 1 interface 0 Logitech product 0x09a2 
rev 2.00/0.08 addr 4
video0 at uvideo0
uaudio0 at uhub5 port 1 configuration 1 interface 2 Logitech product 0x09a2 
rev 2.00/0.08 addr 4
uaudio0: audio rev 1.00, 2 mixer controls
audio1 at uaudio0

note how they are both attached at uhub5 port 1 configuration 1.

and:

uhidev0 at uhub1 port 2 configuration 1 interface 0 Logitech USB Receiver rev 
2.00/16.00 addr 2
uhidev0: iclass 3/1
ums0 at uhidev0: 16 buttons, Z dir
wsmouse1 at ums0 mux 0
uhidev1 at uhub1 port 2 configuration 1 interface 1 Logitech USB Receiver rev 
2.00/16.00 addr 2
uhidev1: iclass 3/0, 17 report ids
uhid0 at uhidev1 reportid 3: input=4, output=0, feature=0
uhid1 at uhidev1 reportid 16: input=6, output=6, feature=0
uhid2 at uhidev1 reportid 17: input=19, output=19, feature=0

note how both uhidev(4)s are attached at uhub1 port 2 configuration 1.

currently, usbd_probe_and_attach() makes a copy of the array of pointers
to the interface descriptors and passes these to the match/attach functions
in struct usb_attach_args (uaa).  when an interface is matched, the pointer
to the interface descriptor in uaa is set to NULL.  and, if a driver uses
other interfaces, the driver sets the pointer to those interface
descriptors in the uaa to NULL.  this way, what interfaces are available
is known, but this only really works at attach time.

currently, ugen(4) assumes it can use any interface in any configuration.
for this reason, ugen(4) is only attached if no interfaces have drivers
attached.

the following allows ugen(4) to be safely attached when there are unused
interfaces.  this is accomplished by:

* instead of NULLing pointers to interface descriptors in the uaa, mark
  interfaces as being claimed in the usbd_device's copy of the interface
  descriptors
* allow ugen(4) to be attached if there are unused interfaces in a
  configuration that has had drivers attached
* make ugen(4) aware that it may be sharing a device with (an)other
  driver(s), and if so:
  * do not let ugen(4) change the configuration
  * do not let ugen(4) access the already claimed interfaces

this allows, for example:

ulpt0 at uhub0 port 1 configuration 1 interface 1 HP Officejet 4500 G510g-m 
rev 2.00/1.00 addr 2
ulpt0: using bi-directional mode
ugen0 at uhub0 port 1 configuration 1 HP Officejet 4500 G510g-m rev 2.00/1.00 
addr 2

which means this MFP can be used by both lpd and sane, simultaneously.
this also requires a patch to libusb, to let it know that ugen(4) might
not be the first driver attached to the device.  that patch is at
http://jakemsr.trancell.org/libusb.diff

thoughts?  oks?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: if_cdce.c
===
RCS file: /cvs/src/sys/dev/usb/if_cdce.c,v
retrieving revision 1.47
diff -u -p if_cdce.c
--- if_cdce.c   27 Oct 2010 17:51:11 -  1.47
+++ if_cdce.c   3 Jan 2011 21:52:27 -
@@ -224,12 +224,12 @@ cdce_attach(struct device *parent, struct device *self
DPRINTF((cdce_attach: union interface: ctl=%d, data=%d\n,
ctl_ifcno, data_ifcno));
for (i = 0; i  uaa-nifaces; i++) {
-   if (uaa-ifaces[i] == NULL)
+   if (usbd_iface_claimed(sc-cdce_udev, i))
continue;
id = usbd_get_interface_descriptor(uaa-ifaces[i]);
if (id != NULL  id-bInterfaceNumber == data_ifcno) {
sc-cdce_data_iface = uaa-ifaces[i];
-   uaa-ifaces[i] = NULL;
+   usbd_claim_iface(sc-cdce_udev, i);
}
}
}
Index: if_urndis.c
===
RCS file: /cvs/src/sys/dev/usb/if_urndis.c,v
retrieving revision 1.27
diff -u -p if_urndis.c
--- if_urndis.c 27 Oct 2010 17:51:11 -  1.27
+++ if_urndis.c 3 Jan 2011 21:52:27 -
@@ -1389,12 +1389,12 @@ urndis_attach(struct device *parent, struct device *se
DPRINTF((urndis_attach: union interface: ctl %u, data %u\n,
if_ctl, if_data));
for (i = 0; i  uaa-nifaces; i++) {
-   if (uaa-ifaces[i] == NULL)
+   if (usbd_iface_claimed(sc-sc_udev, i))
continue;
 

Re: allow ugen(4) to attach to unused interfaces

2011-01-03 Thread Jacob Meuser
On Tue, Jan 04, 2011 at 02:13:05PM +0900, Yojiro UO wrote:
 as the ugen(4) is too flexible (and unsafe) than other usb device drivers, 
 i don't like this work to extend ugen(4)'s area. 
 
 I know many userland applications which supports multiple platform using
 ugen type interface (because they usually use libusb or ugen apis), but 
 it is no reason to support them without considerations.
 
 .
 
 From usb developer's view (include me), this work is seems to the so 
 useful to use unsupported / partially supported device. hmm...
 
 -- Yojiro UO

well, when ugen(4) is attached as a secondary device, it is not
allowed to change the configuration, and is only allowed access to
the unclaimed interfaces.  this is essentially the same as other
class/interface drivers.  normal ugen(4) attachments and drivers
that attach by matching vendor/pid are given full access to the
device.

there is no defined class for scanners.  so, I don't see how usb
scanners will be supported without a generic usb device driver,
except maybe something like uscanner ...

this saves me from having to disable ulpt, so that I can use my
mfp as a ugen, which then forces me to use CUPS for printing.
now I don't have to fiddle in ukc or use CUPS.

I don't disagree that the wrong device attaching as ugen could have
unforseen negative consequences.  you wouldn't want to allow
unrestricted use of a ugen attached to a umass interface that is
controlling a filesystem, for example.  but we attach umass there,
so that shouldn't happen.



 
 
 On 2011/01/04, at 8:18, Jacob Meuser wrote:
 
  when a USB device is inserted, usbd_probe_and_attach() first tries to
  find a driver that claims to support the device by matching the
  vendor/product IDs.  if no such driver is found, usbd_probe_and_attach()
  loops through each interface of each of the devices configurations,
  trying to match interface (aka class) drivers to an interface.  if
  the device has more than one interface *in a single configuration*
  that matches drivers, then drivers are allowed to attach to each
  interface.  for example:
  
  uvideo0 at uhub5 port 1 configuration 1 interface 0 Logitech product 
  0x09a2 rev 2.00/0.08 addr 4
  video0 at uvideo0
  uaudio0 at uhub5 port 1 configuration 1 interface 2 Logitech product 
  0x09a2 rev 2.00/0.08 addr 4
  uaudio0: audio rev 1.00, 2 mixer controls
  audio1 at uaudio0
  
  note how they are both attached at uhub5 port 1 configuration 1.
  
  and:
  
  uhidev0 at uhub1 port 2 configuration 1 interface 0 Logitech USB Receiver 
  rev 2.00/16.00 addr 2
  uhidev0: iclass 3/1
  ums0 at uhidev0: 16 buttons, Z dir
  wsmouse1 at ums0 mux 0
  uhidev1 at uhub1 port 2 configuration 1 interface 1 Logitech USB Receiver 
  rev 2.00/16.00 addr 2
  uhidev1: iclass 3/0, 17 report ids
  uhid0 at uhidev1 reportid 3: input=4, output=0, feature=0
  uhid1 at uhidev1 reportid 16: input=6, output=6, feature=0
  uhid2 at uhidev1 reportid 17: input=19, output=19, feature=0
  
  note how both uhidev(4)s are attached at uhub1 port 2 configuration 1.
  
  currently, usbd_probe_and_attach() makes a copy of the array of pointers
  to the interface descriptors and passes these to the match/attach functions
  in struct usb_attach_args (uaa).  when an interface is matched, the pointer
  to the interface descriptor in uaa is set to NULL.  and, if a driver uses
  other interfaces, the driver sets the pointer to those interface
  descriptors in the uaa to NULL.  this way, what interfaces are available
  is known, but this only really works at attach time.
  
  currently, ugen(4) assumes it can use any interface in any configuration.
  for this reason, ugen(4) is only attached if no interfaces have drivers
  attached.
  
  the following allows ugen(4) to be safely attached when there are unused
  interfaces.  this is accomplished by:
  
  * instead of NULLing pointers to interface descriptors in the uaa, mark
   interfaces as being claimed in the usbd_device's copy of the interface
   descriptors
  * allow ugen(4) to be attached if there are unused interfaces in a
   configuration that has had drivers attached
  * make ugen(4) aware that it may be sharing a device with (an)other
   driver(s), and if so:
   * do not let ugen(4) change the configuration
   * do not let ugen(4) access the already claimed interfaces
  
  this allows, for example:
  
  ulpt0 at uhub0 port 1 configuration 1 interface 1 HP Officejet 4500 
  G510g-m rev 2.00/1.00 addr 2
  ulpt0: using bi-directional mode
  ugen0 at uhub0 port 1 configuration 1 HP Officejet 4500 G510g-m rev 
  2.00/1.00 addr 2
  
  which means this MFP can be used by both lpd and sane, simultaneously.
  this also requires a patch to libusb, to let it know that ugen(4) might
  not be the first driver attached to the device.  that patch is at
  http://jakemsr.trancell.org/libusb.diff
  
  thoughts?  oks?
  
  -- 
  jake...@sdf.lonestar.org
  SDF Public Access UNIX System - http://sdf.lonestar.org
  
  Index: if_cdce.c

Re: usb wireless detach

2010-12-29 Thread Jacob Meuser
On Thu, Dec 16, 2010 at 12:28:56AM +, Jacob Meuser wrote:
 the following diff tries to make sure that no other processes/threads
 are or will be using the drivers software context when the driver is
 detached.

same treatment for otus(4), rsu(4) and urtwn(4)

ok?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org


Index: if_otus.c
===
RCS file: /cvs/src/sys/dev/usb/if_otus.c,v
retrieving revision 1.25
diff -u -p -r1.25 if_otus.c
--- if_otus.c   27 Dec 2010 03:03:50 -  1.25
+++ if_otus.c   30 Dec 2010 05:57:42 -
@@ -246,17 +246,32 @@ otus_detach(struct device *self, int fla
struct ifnet *ifp = sc-sc_ic.ic_if;
int s;
 
-   s = splnet();
-
-   /* Wait for all queued asynchronous commands to complete. */
-   while (sc-cmdq.queued  0)
-   tsleep(sc-cmdq, 0, cmdq, 0);
+   s = splusb();
 
if (timeout_initialized(sc-scan_to))
timeout_del(sc-scan_to);
if (timeout_initialized(sc-calib_to))
timeout_del(sc-calib_to);
 
+   /* Wait for all queued asynchronous commands to complete. */
+#if 0
+   while (sc-cmdq.queued  0)
+   tsleep(sc-cmdq, 0, cmdq, 0);
+#endif
+   /* the async commands are run in a task */
+   usb_rem_wait_task(sc-sc_udev, sc-sc_task);
+
+   /* but the task might not have run if it did not start before
+* usbd_deactivate() was called, so wakeup now.  we're
+* detaching, no need to try to run more commands.
+*/
+   if (sc-cmdq.queued  0) {
+   sc-cmdq.queued = 0;
+   wakeup(sc-cmdq);
+   }
+
+   usbd_ref_wait(sc-sc_udev);
+
if (ifp-if_softc != NULL) {
ifp-if_flags = ~(IFF_RUNNING | IFF_OACTIVE);
ieee80211_ifdetach(ifp);
@@ -732,8 +747,15 @@ otus_next_scan(void *arg)
 {
struct otus_softc *sc = arg;
 
+   if (usbd_is_dying(sc-sc_udev))
+   return;
+
+   usbd_ref_incr(sc-sc_udev);
+
if (sc-sc_ic.ic_state == IEEE80211_S_SCAN)
ieee80211_next_scan(sc-sc_ic.ic_if);
+
+   usbd_ref_decr(sc-sc_udev);
 }
 
 void
@@ -809,7 +831,8 @@ otus_newstate_cb(struct otus_softc *sc, 
 
case IEEE80211_S_SCAN:
(void)otus_set_chan(sc, ic-ic_bss-ni_chan, 0);
-   timeout_add_msec(sc-scan_to, 200);
+   if (!usbd_is_dying(sc-sc_udev))
+   timeout_add_msec(sc-scan_to, 200);
break;
 
case IEEE80211_S_AUTH:
@@ -830,7 +853,8 @@ otus_newstate_cb(struct otus_softc *sc, 
otus_newassoc(ic, ni, 1);
 
/* Start calibration timer. */
-   timeout_add_sec(sc-calib_to, 1);
+   if (!usbd_is_dying(sc-sc_udev))
+   timeout_add_sec(sc-calib_to, 1);
}
break;
}
@@ -1499,6 +1523,11 @@ otus_ioctl(struct ifnet *ifp, u_long cmd
struct ifreq *ifr;
int s, error = 0;
 
+   if (usbd_is_dying(sc-sc_udev))
+   return ENXIO;
+
+   usbd_ref_incr(sc-sc_udev);
+
s = splnet();
 
switch (cmd) {
@@ -1555,6 +1584,9 @@ otus_ioctl(struct ifnet *ifp, u_long cmd
}
 
splx(s);
+
+   usbd_ref_decr(sc-sc_udev);
+
return error;
 }
 
@@ -2176,12 +2208,20 @@ otus_calibrate_to(void *arg)
struct ieee80211_node *ni;
int s;
 
+   if (usbd_is_dying(sc-sc_udev))
+   return;
+
+   usbd_ref_incr(sc-sc_udev);
+
s = splnet();
ni = ic-ic_bss;
ieee80211_amrr_choose(sc-amrr, ni, ((struct otus_node *)ni)-amn);
splx(s);
 
-   timeout_add_sec(sc-calib_to, 1);
+   if (!usbd_is_dying(sc-sc_udev))
+   timeout_add_sec(sc-calib_to, 1);
+
+   usbd_ref_decr(sc-sc_udev);
 }
 
 int
Index: if_rsu.c
===
RCS file: /cvs/src/sys/dev/usb/if_rsu.c,v
retrieving revision 1.8
diff -u -p -r1.8 if_rsu.c
--- if_rsu.c27 Dec 2010 03:03:50 -  1.8
+++ if_rsu.c30 Dec 2010 05:57:42 -
@@ -343,12 +343,29 @@ rsu_detach(struct device *self, int flag
struct ifnet *ifp = sc-sc_ic.ic_if;
int s;
 
-   s = splnet();
-   /* Wait for all async commands to complete. */
-   rsu_wait_async(sc);
+   s = splusb();
 
if (timeout_initialized(sc-calib_to))
timeout_del(sc-calib_to);
+
+   /* Wait for all async commands to complete. */
+#if 0
+   rsu_wait_async(sc);
+#endif
+   /* the async commands are run in a task */
+   usb_rem_wait_task(sc-sc_udev, sc-sc_task);
+
+   /* but the task might not have run if it did not start before
+* usbd_deactivate() was called, so wakeup now.  we're
+* detaching, no need to try to run more commands.
+*/
+   if (sc

Re: usb wireless detach

2010-12-22 Thread Jacob Meuser
no feedback yet.  anyone care to comment on this?

On Thu, Dec 16, 2010 at 12:28:56AM +, Jacob Meuser wrote:
 the following diff tries to make sure that no other processes/threads
 are or will be using the drivers software context when the driver is
 detached.
 
 this diff covers rum(4), run(4), ural(4) and urtw(4).  without the
 diff, I can get the kernel to crash by starting a scan with
 the deice, then ejecting it while the scan is running.  with this
 diff, I cannot get a crash.
 
 normally, usb device detach happens in the usb_task thread.  the sole
 exception is when the usb bus itself is being detached.  this happens
 with cardbus devices, and in that case, the usb device detach happens
 in the generic workq thread.
 
 this adds a simple reference counting/waiting mechanism.  this is
 definitely needed for ioctl(2), which can sleep then start using the
 driver again.  it may not be needed for timeout(9)s now, but maybe
 someday it will.  another possible process using the device is the
 usb_task thread.  in the normal case, this is the same process that
 is detaching, so there is no concurrency issue.  there is already
 usb_rem_wait_task() to deal with detaches from other processes,
 because the bus driver needs it.
 
 this also adds more uses of usbd_is_dying() to check if the device
 has been detached:
 * before adding timeouts
 * when entering timeouts, usb_tasks, and ioctl
 
 also of note is the order things are done in the detach routines:
 1) remove pending timeouts
 2) remove (and possibly wait for) pending usb_tasks
 3) wait for other processes
 4) detach from the network stack
 5) cleanup/free usb resources
 
 thoughts?  ok?
 
 -- 
 jake...@sdf.lonestar.org
 SDF Public Access UNIX System - http://sdf.lonestar.org
 
 
 Index: if_ral.c
 ===
 RCS file: /cvs/src/sys/dev/usb/if_ral.c,v
 retrieving revision 1.117
 diff -u -p -r1.117 if_ral.c
 --- if_ral.c  6 Dec 2010 04:41:39 -   1.117
 +++ if_ral.c  15 Dec 2010 02:58:35 -
 @@ -363,17 +363,20 @@ ural_detach(struct device *self, int fla
  
   s = splusb();
  
 - if (ifp-if_softc != NULL) {
 - ieee80211_ifdetach(ifp);/* free all nodes */
 - if_detach(ifp);
 - }
 -
 - usb_rem_task(sc-sc_udev, sc-sc_task);
   if (timeout_initialized(sc-scan_to))
   timeout_del(sc-scan_to);
   if (timeout_initialized(sc-amrr_to))
   timeout_del(sc-amrr_to);
  
 + usb_rem_wait_task(sc-sc_udev, sc-sc_task);
 +
 + usbd_ref_wait(sc-sc_udev);
 +
 + if (ifp-if_softc != NULL) {
 + ieee80211_ifdetach(ifp);/* free all nodes */
 + if_detach(ifp);
 + }
 +
   if (sc-amrr_xfer != NULL) {
   usbd_free_xfer(sc-amrr_xfer);
   sc-amrr_xfer = NULL;
 @@ -547,8 +550,15 @@ ural_next_scan(void *arg)
   struct ieee80211com *ic = sc-sc_ic;
   struct ifnet *ifp = ic-ic_if;
  
 + if (usbd_is_dying(sc-sc_udev))
 + return;
 +
 + usbd_ref_incr(sc-sc_udev);
 +
   if (ic-ic_state == IEEE80211_S_SCAN)
   ieee80211_next_scan(ifp);
 +
 + usbd_ref_decr(sc-sc_udev);
  }
  
  void
 @@ -559,6 +569,9 @@ ural_task(void *arg)
   enum ieee80211_state ostate;
   struct ieee80211_node *ni;
  
 + if (usbd_is_dying(sc-sc_udev))
 + return;
 +
   ostate = ic-ic_state;
  
   switch (sc-sc_state) {
 @@ -574,7 +587,8 @@ ural_task(void *arg)
  
   case IEEE80211_S_SCAN:
   ural_set_chan(sc, ic-ic_bss-ni_chan);
 - timeout_add_msec(sc-scan_to, 200);
 + if (!usbd_is_dying(sc-sc_udev))
 + timeout_add_msec(sc-scan_to, 200);
   break;
  
   case IEEE80211_S_AUTH:
 @@ -1324,6 +1338,11 @@ ural_ioctl(struct ifnet *ifp, u_long cmd
   struct ifreq *ifr;
   int s, error = 0;
  
 + if (usbd_is_dying(sc-sc_udev))
 + return ENXIO;
 +
 + usbd_ref_incr(sc-sc_udev);
 +
   s = splnet();
  
   switch (cmd) {
 @@ -1387,6 +1406,8 @@ ural_ioctl(struct ifnet *ifp, u_long cmd
  
   splx(s);
  
 + usbd_ref_decr(sc-sc_udev);
 +
   return error;
  }
  
 @@ -2147,7 +2168,8 @@ ural_amrr_start(struct ural_softc *sc, s
i--);
   ni-ni_txrate = i;
  
 - timeout_add_sec(sc-amrr_to, 1);
 + if (!usbd_is_dying(sc-sc_udev))
 + timeout_add_sec(sc-amrr_to, 1);
  }
  
  void
 @@ -2157,6 +2179,11 @@ ural_amrr_timeout(void *arg)
   usb_device_request_t req;
   int s;
  
 + if (usbd_is_dying(sc-sc_udev))
 + return;
 +
 + usbd_ref_incr(sc-sc_udev);
 +
   s = splusb();
  
   /*
 @@ -2174,6 +2201,8 @@ ural_amrr_timeout(void *arg)
   (void)usbd_transfer(sc-amrr_xfer);
  
   splx(s);
 +
 + usbd_ref_decr(sc-sc_udev);
  }
  
  void
 @@ -2203,7 +2232,8 @@ ural_amrr_update(usbd_xfer_handle xfer, 
  
   ieee80211_amrr_choose(sc-amrr, sc-sc_ic.ic_bss, sc

Re: diff: hp usb printers quirk + bonus

2010-12-18 Thread Jacob Meuser
On Sat, Dec 18, 2010 at 10:14:54AM +0200, Vladimir Kirillov wrote:
 Hello, t...@!
 
 Since a lot of HP usb printers work badly (or do not work at all)
 with ulpt and they want all cups and hplip goo, I've added a
 new UQ_SHOULD_UGEN quirk to let ulpt(4) know when to skip the device.

I suggested a quirk to choose ugen recently and there was some
resistence.

however, I think in this case, perhaps forcing HP printers to not
match ulpt is the more correct thing to do.

 As a bonus, a usbdevs 0X-0x consistency conversion.

please don't mix differring purposes in the same patch.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: diff: hp usb printers quirk + bonus

2010-12-18 Thread Jacob Meuser
On Sat, Dec 18, 2010 at 08:48:10AM +, Jacob Meuser wrote:
 On Sat, Dec 18, 2010 at 10:14:54AM +0200, Vladimir Kirillov wrote:
  Hello, t...@!
  
  Since a lot of HP usb printers work badly (or do not work at all)
  with ulpt and they want all cups and hplip goo, I've added a
  new UQ_SHOULD_UGEN quirk to let ulpt(4) know when to skip the device.
 
 I suggested a quirk to choose ugen recently and there was some
 resistence.

and looking at your patch ... if there will be a quirk that says
to attach at ugen, it should really make it attach at ugen, immediately,
and not be a flag to be used in other drivers.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



usb: add missing autoconf activate functions

2010-12-18 Thread Jacob Meuser
this adds activate functions for drivers that don't have them.  also
add usbd_deactivate() in DVACT_DEACTIVATE for drivers that do have
activate functions but don't have any dying flag.

ok?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: if_cdcef.c
===
RCS file: /cvs/src/sys/dev/usb/if_cdcef.c,v
retrieving revision 1.25
diff -u -p if_cdcef.c
--- if_cdcef.c  29 Jun 2010 07:12:31 -  1.25
+++ if_cdcef.c  18 Dec 2010 22:57:04 -
@@ -87,6 +87,7 @@ struct cdcef_softc {
 
 intcdcef_match(struct device *, void *, void *);
 void   cdcef_attach(struct device *, struct device *, void *);
+intcdcef_activate(struct device *, int);
 
 usbf_statuscdcef_do_request(usbf_function_handle,
 usb_device_request_t *, void **);
@@ -106,7 +107,8 @@ struct mbuf *   cdcef_newbuf(void);
 void   cdcef_start_timeout (void *);
 
 struct cfattach cdcef_ca = {
-   sizeof(struct cdcef_softc), cdcef_match, cdcef_attach
+   sizeof(struct cdcef_softc), cdcef_match, cdcef_attach, NULL,
+   cdcef_activate
 };
 
 struct cfdriver cdcef_cd = {
@@ -264,6 +266,23 @@ cdcef_attach(struct device *parent, struct device *sel
 
sc-sc_attached = 1;
splx(s);
+}
+
+int
+cdcef_activate(struct device *self, int act)
+{
+   struct cdcef_softc *sc = (struct cdcef_softc *)self;
+
+   switch (act) {
+   case DVACT_ACTIVATE:
+   break;
+
+   case DVACT_DEACTIVATE:
+   usbd_deactivate(sc-sc_udev);
+   break;
+   }
+
+   return 0;
 }
 
 usbf_status
Index: if_otus.c
===
RCS file: /cvs/src/sys/dev/usb/if_otus.c,v
retrieving revision 1.24
diff -u -p if_otus.c
--- if_otus.c   30 Oct 2010 18:03:43 -  1.24
+++ if_otus.c   18 Dec 2010 22:57:05 -
@@ -105,6 +105,7 @@ static const struct usb_devno otus_devs[] = {
 intotus_match(struct device *, void *, void *);
 void   otus_attach(struct device *, struct device *, void *);
 intotus_detach(struct device *, int);
+intotus_activate(struct device *, int);
 void   otus_attachhook(void *);
 void   otus_get_chanlist(struct otus_softc *);
 intotus_load_firmware(struct otus_softc *, const char *,
@@ -179,7 +180,8 @@ struct cfdriver otus_cd = {
 };
 
 const struct cfattach otus_ca = {
-   sizeof (struct otus_softc), otus_match, otus_attach, otus_detach
+   sizeof (struct otus_softc), otus_match, otus_attach, otus_detach,
+   otus_activate
 };
 
 int
@@ -266,6 +268,23 @@ otus_detach(struct device *self, int flags)
splx(s);
 
usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc-sc_udev, sc-sc_dev);
+
+   return 0;
+}
+
+int
+otus_activate(struct device *self, int act)
+{
+   struct otus_softc *sc = (struct otus_softc *)self;
+
+   switch (act) {
+   case DVACT_ACTIVATE:
+   break;
+
+   case DVACT_DEACTIVATE:
+   usbd_deactivate(sc-sc_udev);
+   break;
+   }
 
return 0;
 }
Index: if_rsu.c
===
RCS file: /cvs/src/sys/dev/usb/if_rsu.c,v
retrieving revision 1.6
diff -u -p if_rsu.c
--- if_rsu.c15 Dec 2010 16:51:39 -  1.6
+++ if_rsu.c18 Dec 2010 22:57:05 -
@@ -129,6 +129,7 @@ static const struct usb_devno rsu_devs_noht[] = {
 intrsu_match(struct device *, void *, void *);
 void   rsu_attach(struct device *, struct device *, void *);
 intrsu_detach(struct device *, int);
+intrsu_activate(struct device *, int);
 intrsu_open_pipes(struct rsu_softc *);
 void   rsu_close_pipes(struct rsu_softc *);
 intrsu_alloc_rx_list(struct rsu_softc *);
@@ -199,7 +200,8 @@ const struct cfattach rsu_ca = {
sizeof(struct rsu_softc),
rsu_match,
rsu_attach,
-   rsu_detach
+   rsu_detach,
+   rsu_activate
 };
 
 int
@@ -361,6 +363,23 @@ rsu_detach(struct device *self, int flags)
 
usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc-sc_udev, sc-sc_dev);
return (0);
+}
+
+int
+rsu_activate(struct device *self, int act)
+{
+   struct rsu_softc *sc = (struct rsu_softc *)self;
+
+   switch (act) {
+   case DVACT_ACTIVATE:
+   break;
+
+   case DVACT_DEACTIVATE:
+   usbd_deactivate(sc-sc_udev);
+   break;
+   }
+
+   return 0;
 }
 
 int
Index: if_urtwn.c
===
RCS file: /cvs/src/sys/dev/usb/if_urtwn.c,v
retrieving revision 1.10
diff -u -p if_urtwn.c
--- if_urtwn.c  11 Dec 2010 21:07:38 -  1.10
+++ if_urtwn.c  18 Dec 2010 22:57:06 -
@@ -113,6 +113,7 @@ static const struct usb_devno urtwn_devs[] = {
 int

Re: usb: add missing autoconf activate functions

2010-12-18 Thread Jacob Meuser
On Sun, Dec 19, 2010 at 05:02:54AM +0200, Paul Irofti wrote:
 On Sat, Dec 18, 2010 at 11:11:35PM +, Jacob Meuser wrote:
  this adds activate functions for drivers that don't have them.  also
  add usbd_deactivate() in DVACT_DEACTIVATE for drivers that do have
  activate functions but don't have any dying flag.
  
  ok?
 
 About this interface, I looked around in the code and it seems to be
 used mostly in activate functions and in failure cases inside attach.

yes, it's used to set the driver's dying flag.  those are the most common
places a driver would be marked as dying.  maybe also in an interrupt
handler.

 My question is if its not better to make usbd_deactivate() a function
 similar to an activate() one. That way we can just add something like
 ubdi_activate in the cfattach instead of a wrapper to it in each driver.

hmm.  perhaps usbd_deactivate() is not the best name.

 I don't know what the plan is for usbd_deactivate thus what I'm
 proposing might not make sense. Just a thought.

currently it is just a wrapper for setting the dying flag in the driver's
usbd_device.  maybe it will do other things, but probably not.  the pupose
of setting the dying flag in the usbd_device is so that this flag can be
checked in generic stack functions, because the almost always have access
to the usbd_device.  if there were just the dying flag in the softc, that
isn't available outside the driver.

 Otherwise it would sure get rid of a lot of redundant code in dev/usb/*.

I don't get what you're saying here.

 usbd_set_dying(dev, flag)? Don't know.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



usb: don't wait if dying

2010-12-14 Thread Jacob Meuser
ok?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: usb_subr.c
===
RCS file: /cvs/src/sys/dev/usb/usb_subr.c,v
retrieving revision 1.75
diff -u -p usb_subr.c
--- usb_subr.c  6 Dec 2010 04:30:57 -   1.75
+++ usb_subr.c  14 Dec 2010 16:16:32 -
@@ -351,6 +351,9 @@ usb_delay_ms(usbd_bus_handle bus, u_int ms)
 void
 usbd_delay_ms(usbd_device_handle dev, u_int ms)
 {
+   if (usbd_is_dying(dev))
+   return;
+
usb_delay_ms(dev-bus, ms);
 }



Re: usb_{bulk,interrupt}_transfer() and PCATCH

2010-12-13 Thread Jacob Meuser
any further thoughts on this?

On Sun, Dec 12, 2010 at 01:02:41PM +, Jacob Meuser wrote:
 On Sat, Dec 11, 2010 at 08:56:38PM +, Jacob Meuser wrote:
  On Sat, Dec 11, 2010 at 08:35:00PM +, Jacob Meuser wrote:
   On Sat, Dec 11, 2010 at 08:14:24PM +0100, Mark Kettenis wrote:
 Date: Wed, 8 Dec 2010 06:07:30 +
 From: Jacob Meuser jake...@sdf.lonestar.org
 
 I recently got a hp officejet 4500.  it's a 3-in-1 
 printer/scanner/fax.
 printing works great with the hplip packages.  scanning doesn't work 
 at
 all.
 
 I tracked the problem to read() failing in usb_bulk_read() in libusb.
 errno was EINTR.  so I made this function just continue when read() 
 got
 EINTR.  I could then scan, but not very reliably.
 
 so I went into the kernel, and tracked it to usbdi_util.c:
 
 @ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
 
   error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
   splx(s);
   if (error) {
   DPRINTF((usbd_bulk_transfer: tsleep=%d\n, error));
   usbd_abort_pipe(pipe);
   return (USBD_INTERRUPTED);
   }
 
 on a hunch, I changed 'PZERO | PCATCH' to 'PWAIT'.  then scanning 
 became
 100% reliable.  even full-page 1200dpi (~525MB of data) worked
 perfectly.
 
 great.  but, what if we need to interrupt the transfer.  we don't want
 to hang here.
 
 well, this function takes a timeout.  so, it's possible to make it
 return, even if the transfer stalls.  but is this used?  I looked
 at the ports that use libusb.  setting 0/infinite timeout for bulk
 transfers is extremely rare, only set in code marked experimental
 or problematic.  in the kernel, there are however some callers that
 use USBD_NO_TIMEOUT.
 
 so, I offer the following.  if there's no timeout, behave like we do
 now and catch signals, and if there's a timeout, ignore signals.
 
 I did the same for interrupt transfers because the situation seems
 to be the same.
 
 thoughts?

Sorry, but this very much feels like you're hacking around a bug in
the application you're using.  If it installs signal handler without
specifying the SA_RESTART flag, it has to deal with read(2) failing
with EINTR.
   
   that application would be libusb itself.  is a library supposed
   to be messing with signals?
  
  to be clear, the actual read() is in usb_bulk_read() in libusb.
  so, the application doesn't really know that there's a read().
  
  I tried making libusb continue, but the problem is that when
  usbd_bulk_transfer catches the signal, it aborts everything in
  the pipe.  restarting from there correctly is complicated,
  probably not even possible in a sane way.
 
 I looked at all users of libusb.  none deal with usb_{bulk,interrupt}_read
 (the libusb frontend) quitting with EINTR.  none set SA_RESTART.
 
 usbd_{bulk,intr}_transfer() (in the usb(4) stack) do not obey
 SA_RESTART.  neither do the ugen(4) ends of those functions.
 
 usbd_transfer() does not catch signals.  in fact.  the only other
 thing in the usb stack that does catch signals is usbread(), which
 is an interface to read messages from the usb event queue.  the
 event queue is stored in software.  it doesn't handle tsleep()
 returning ERESTART either, btw.
 
 figuring out an absolute max timeout seems fairly straight-forward.
 timeouts are specified in miliseconds.  the usb frame rate is 1000 Hz.
 an empty frame signals the end of a transfer.  timeout = bytes,
 basically.
 
 now, usbd_intr_transfer() may be a different story.  that will block
 (if not set or non-blocking io) until there is data to read.  most
 users of this in libusb do set a timeout though.  some also restart
 the read if they get ETIMEDOUT.
 
 I think, catching signals is wrong.  at least, it's not expected by
 most of it's users and not even handled correctly in the kernel.
 
 further, by not setting a timeout, it leaves the user to figure out
 that something is wrong.
 
 
 
  
  
   
   
 Index: usbdi_util.c
 ===
 RCS file: /cvs/src/sys/dev/usb/usbdi_util.c,v
 retrieving revision 1.25
 diff -u -p usbdi_util.c
 --- usbdi_util.c  26 Jun 2008 05:42:19 -  1.25
 +++ usbdi_util.c  8 Dec 2010 04:58:49 -
 @@ -426,7 +426,7 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, 
 usbd_pipe_ha
  u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, 
 char *lbl)
  {
   usbd_status err;
 - int s, error;
 + int s, error, pri;
  
   usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
   usbd_bulk_transfer_cb);
 @@ -437,7 +437,8 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, 
 usbd_pipe_ha
   splx(s);
   return (err

Re: usb_{bulk,interrupt}_transfer() and PCATCH

2010-12-12 Thread Jacob Meuser
On Sat, Dec 11, 2010 at 08:56:38PM +, Jacob Meuser wrote:
 On Sat, Dec 11, 2010 at 08:35:00PM +, Jacob Meuser wrote:
  On Sat, Dec 11, 2010 at 08:14:24PM +0100, Mark Kettenis wrote:
Date: Wed, 8 Dec 2010 06:07:30 +
From: Jacob Meuser jake...@sdf.lonestar.org

I recently got a hp officejet 4500.  it's a 3-in-1 printer/scanner/fax.
printing works great with the hplip packages.  scanning doesn't work at
all.

I tracked the problem to read() failing in usb_bulk_read() in libusb.
errno was EINTR.  so I made this function just continue when read() got
EINTR.  I could then scan, but not very reliably.

so I went into the kernel, and tracked it to usbdi_util.c:

@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha

error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
splx(s);
if (error) {
DPRINTF((usbd_bulk_transfer: tsleep=%d\n, error));
usbd_abort_pipe(pipe);
return (USBD_INTERRUPTED);
}

on a hunch, I changed 'PZERO | PCATCH' to 'PWAIT'.  then scanning became
100% reliable.  even full-page 1200dpi (~525MB of data) worked
perfectly.

great.  but, what if we need to interrupt the transfer.  we don't want
to hang here.

well, this function takes a timeout.  so, it's possible to make it
return, even if the transfer stalls.  but is this used?  I looked
at the ports that use libusb.  setting 0/infinite timeout for bulk
transfers is extremely rare, only set in code marked experimental
or problematic.  in the kernel, there are however some callers that
use USBD_NO_TIMEOUT.

so, I offer the following.  if there's no timeout, behave like we do
now and catch signals, and if there's a timeout, ignore signals.

I did the same for interrupt transfers because the situation seems
to be the same.

thoughts?
   
   Sorry, but this very much feels like you're hacking around a bug in
   the application you're using.  If it installs signal handler without
   specifying the SA_RESTART flag, it has to deal with read(2) failing
   with EINTR.
  
  that application would be libusb itself.  is a library supposed
  to be messing with signals?
 
 to be clear, the actual read() is in usb_bulk_read() in libusb.
 so, the application doesn't really know that there's a read().
 
 I tried making libusb continue, but the problem is that when
 usbd_bulk_transfer catches the signal, it aborts everything in
 the pipe.  restarting from there correctly is complicated,
 probably not even possible in a sane way.

I looked at all users of libusb.  none deal with usb_{bulk,interrupt}_read
(the libusb frontend) quitting with EINTR.  none set SA_RESTART.

usbd_{bulk,intr}_transfer() (in the usb(4) stack) do not obey
SA_RESTART.  neither do the ugen(4) ends of those functions.

usbd_transfer() does not catch signals.  in fact.  the only other
thing in the usb stack that does catch signals is usbread(), which
is an interface to read messages from the usb event queue.  the
event queue is stored in software.  it doesn't handle tsleep()
returning ERESTART either, btw.

figuring out an absolute max timeout seems fairly straight-forward.
timeouts are specified in miliseconds.  the usb frame rate is 1000 Hz.
an empty frame signals the end of a transfer.  timeout = bytes,
basically.

now, usbd_intr_transfer() may be a different story.  that will block
(if not set or non-blocking io) until there is data to read.  most
users of this in libusb do set a timeout though.  some also restart
the read if they get ETIMEDOUT.

I think, catching signals is wrong.  at least, it's not expected by
most of it's users and not even handled correctly in the kernel.

further, by not setting a timeout, it leaves the user to figure out
that something is wrong.



 
 
  
  
Index: usbdi_util.c
===
RCS file: /cvs/src/sys/dev/usb/usbdi_util.c,v
retrieving revision 1.25
diff -u -p usbdi_util.c
--- usbdi_util.c26 Jun 2008 05:42:19 -  1.25
+++ usbdi_util.c8 Dec 2010 04:58:49 -
@@ -426,7 +426,7 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, 
usbd_pipe_ha
 u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, 
char *lbl)
 {
usbd_status err;
-   int s, error;
+   int s, error, pri;
 
usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
usbd_bulk_transfer_cb);
@@ -437,7 +437,8 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, 
usbd_pipe_ha
splx(s);
return (err);
}
-   error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
+   pri = timeout == USBD_NO_TIMEOUT ? (PZERO | PCATCH) : PWAIT;
+   error = tsleep((caddr_t)xfer, pri, lbl, 0

Re: usb_{bulk,interrupt}_transfer() and PCATCH

2010-12-11 Thread Jacob Meuser
On Sat, Dec 11, 2010 at 08:14:24PM +0100, Mark Kettenis wrote:
  Date: Wed, 8 Dec 2010 06:07:30 +
  From: Jacob Meuser jake...@sdf.lonestar.org
  
  I recently got a hp officejet 4500.  it's a 3-in-1 printer/scanner/fax.
  printing works great with the hplip packages.  scanning doesn't work at
  all.
  
  I tracked the problem to read() failing in usb_bulk_read() in libusb.
  errno was EINTR.  so I made this function just continue when read() got
  EINTR.  I could then scan, but not very reliably.
  
  so I went into the kernel, and tracked it to usbdi_util.c:
  
  @ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
  
  error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
  splx(s);
  if (error) {
  DPRINTF((usbd_bulk_transfer: tsleep=%d\n, error));
  usbd_abort_pipe(pipe);
  return (USBD_INTERRUPTED);
  }
  
  on a hunch, I changed 'PZERO | PCATCH' to 'PWAIT'.  then scanning became
  100% reliable.  even full-page 1200dpi (~525MB of data) worked
  perfectly.
  
  great.  but, what if we need to interrupt the transfer.  we don't want
  to hang here.
  
  well, this function takes a timeout.  so, it's possible to make it
  return, even if the transfer stalls.  but is this used?  I looked
  at the ports that use libusb.  setting 0/infinite timeout for bulk
  transfers is extremely rare, only set in code marked experimental
  or problematic.  in the kernel, there are however some callers that
  use USBD_NO_TIMEOUT.
  
  so, I offer the following.  if there's no timeout, behave like we do
  now and catch signals, and if there's a timeout, ignore signals.
  
  I did the same for interrupt transfers because the situation seems
  to be the same.
  
  thoughts?
 
 Sorry, but this very much feels like you're hacking around a bug in
 the application you're using.  If it installs signal handler without
 specifying the SA_RESTART flag, it has to deal with read(2) failing
 with EINTR.

that application would be libusb itself.  is a library supposed
to be messing with signals?


  Index: usbdi_util.c
  ===
  RCS file: /cvs/src/sys/dev/usb/usbdi_util.c,v
  retrieving revision 1.25
  diff -u -p usbdi_util.c
  --- usbdi_util.c26 Jun 2008 05:42:19 -  1.25
  +++ usbdi_util.c8 Dec 2010 04:58:49 -
  @@ -426,7 +426,7 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
   u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, char 
  *lbl)
   {
  usbd_status err;
  -   int s, error;
  +   int s, error, pri;
   
  usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
  usbd_bulk_transfer_cb);
  @@ -437,7 +437,8 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
  splx(s);
  return (err);
  }
  -   error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
  +   pri = timeout == USBD_NO_TIMEOUT ? (PZERO | PCATCH) : PWAIT;
  +   error = tsleep((caddr_t)xfer, pri, lbl, 0);
  splx(s);
  if (error) {
  DPRINTF((usbd_bulk_transfer: tsleep=%d\n, error));
  @@ -467,7 +468,7 @@ usbd_intr_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
   u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, char 
  *lbl)
   {
  usbd_status err;
  -   int s, error;
  +   int s, error, pri;
   
  usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
  usbd_intr_transfer_cb);
  @@ -478,7 +479,8 @@ usbd_intr_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
  splx(s);
  return (err);
  }
  -   error = tsleep(xfer, PZERO | PCATCH, lbl, 0);
  +   pri = timeout == USBD_NO_TIMEOUT ? (PZERO | PCATCH) : PWAIT;
  +   error = tsleep(xfer, pri, lbl, 0);
  splx(s);
  if (error) {
  DPRINTF((usbd_intr_transfer: tsleep=%d\n, error));

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: usb_{bulk,interrupt}_transfer() and PCATCH

2010-12-11 Thread Jacob Meuser
On Sat, Dec 11, 2010 at 08:35:00PM +, Jacob Meuser wrote:
 On Sat, Dec 11, 2010 at 08:14:24PM +0100, Mark Kettenis wrote:
   Date: Wed, 8 Dec 2010 06:07:30 +
   From: Jacob Meuser jake...@sdf.lonestar.org
   
   I recently got a hp officejet 4500.  it's a 3-in-1 printer/scanner/fax.
   printing works great with the hplip packages.  scanning doesn't work at
   all.
   
   I tracked the problem to read() failing in usb_bulk_read() in libusb.
   errno was EINTR.  so I made this function just continue when read() got
   EINTR.  I could then scan, but not very reliably.
   
   so I went into the kernel, and tracked it to usbdi_util.c:
   
   @ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
   
 error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
 splx(s);
 if (error) {
 DPRINTF((usbd_bulk_transfer: tsleep=%d\n, error));
 usbd_abort_pipe(pipe);
 return (USBD_INTERRUPTED);
 }
   
   on a hunch, I changed 'PZERO | PCATCH' to 'PWAIT'.  then scanning became
   100% reliable.  even full-page 1200dpi (~525MB of data) worked
   perfectly.
   
   great.  but, what if we need to interrupt the transfer.  we don't want
   to hang here.
   
   well, this function takes a timeout.  so, it's possible to make it
   return, even if the transfer stalls.  but is this used?  I looked
   at the ports that use libusb.  setting 0/infinite timeout for bulk
   transfers is extremely rare, only set in code marked experimental
   or problematic.  in the kernel, there are however some callers that
   use USBD_NO_TIMEOUT.
   
   so, I offer the following.  if there's no timeout, behave like we do
   now and catch signals, and if there's a timeout, ignore signals.
   
   I did the same for interrupt transfers because the situation seems
   to be the same.
   
   thoughts?
  
  Sorry, but this very much feels like you're hacking around a bug in
  the application you're using.  If it installs signal handler without
  specifying the SA_RESTART flag, it has to deal with read(2) failing
  with EINTR.
 
 that application would be libusb itself.  is a library supposed
 to be messing with signals?

to be clear, the actual read() is in usb_bulk_read() in libusb.
so, the application doesn't really know that there's a read().

I tried making libusb continue, but the problem is that when
usbd_bulk_transfer catches the signal, it aborts everything in
the pipe.  restarting from there correctly is complicated,
probably not even possible in a sane way.


 
 
   Index: usbdi_util.c
   ===
   RCS file: /cvs/src/sys/dev/usb/usbdi_util.c,v
   retrieving revision 1.25
   diff -u -p usbdi_util.c
   --- usbdi_util.c  26 Jun 2008 05:42:19 -  1.25
   +++ usbdi_util.c  8 Dec 2010 04:58:49 -
   @@ -426,7 +426,7 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, char 
   *lbl)
{
 usbd_status err;
   - int s, error;
   + int s, error, pri;

 usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
 usbd_bulk_transfer_cb);
   @@ -437,7 +437,8 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
 splx(s);
 return (err);
 }
   - error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
   + pri = timeout == USBD_NO_TIMEOUT ? (PZERO | PCATCH) : PWAIT;
   + error = tsleep((caddr_t)xfer, pri, lbl, 0);
 splx(s);
 if (error) {
 DPRINTF((usbd_bulk_transfer: tsleep=%d\n, error));
   @@ -467,7 +468,7 @@ usbd_intr_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, char 
   *lbl)
{
 usbd_status err;
   - int s, error;
   + int s, error, pri;

 usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
 usbd_intr_transfer_cb);
   @@ -478,7 +479,8 @@ usbd_intr_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
 splx(s);
 return (err);
 }
   - error = tsleep(xfer, PZERO | PCATCH, lbl, 0);
   + pri = timeout == USBD_NO_TIMEOUT ? (PZERO | PCATCH) : PWAIT;
   + error = tsleep(xfer, pri, lbl, 0);
 splx(s);
 if (error) {
 DPRINTF((usbd_intr_transfer: tsleep=%d\n, error));
 
 -- 
 jake...@sdf.lonestar.org
 SDF Public Access UNIX System - http://sdf.lonestar.org

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



usb_{bulk,interrupt}_transfer() and PCATCH

2010-12-07 Thread Jacob Meuser
I recently got a hp officejet 4500.  it's a 3-in-1 printer/scanner/fax.
printing works great with the hplip packages.  scanning doesn't work at
all.

I tracked the problem to read() failing in usb_bulk_read() in libusb.
errno was EINTR.  so I made this function just continue when read() got
EINTR.  I could then scan, but not very reliably.

so I went into the kernel, and tracked it to usbdi_util.c:

@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha

error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
splx(s);
if (error) {
DPRINTF((usbd_bulk_transfer: tsleep=%d\n, error));
usbd_abort_pipe(pipe);
return (USBD_INTERRUPTED);
}

on a hunch, I changed 'PZERO | PCATCH' to 'PWAIT'.  then scanning became
100% reliable.  even full-page 1200dpi (~525MB of data) worked
perfectly.

great.  but, what if we need to interrupt the transfer.  we don't want
to hang here.

well, this function takes a timeout.  so, it's possible to make it
return, even if the transfer stalls.  but is this used?  I looked
at the ports that use libusb.  setting 0/infinite timeout for bulk
transfers is extremely rare, only set in code marked experimental
or problematic.  in the kernel, there are however some callers that
use USBD_NO_TIMEOUT.

so, I offer the following.  if there's no timeout, behave like we do
now and catch signals, and if there's a timeout, ignore signals.

I did the same for interrupt transfers because the situation seems
to be the same.

thoughts?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: usbdi_util.c
===
RCS file: /cvs/src/sys/dev/usb/usbdi_util.c,v
retrieving revision 1.25
diff -u -p usbdi_util.c
--- usbdi_util.c26 Jun 2008 05:42:19 -  1.25
+++ usbdi_util.c8 Dec 2010 04:58:49 -
@@ -426,7 +426,7 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
 u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, char *lbl)
 {
usbd_status err;
-   int s, error;
+   int s, error, pri;
 
usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
usbd_bulk_transfer_cb);
@@ -437,7 +437,8 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
splx(s);
return (err);
}
-   error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
+   pri = timeout == USBD_NO_TIMEOUT ? (PZERO | PCATCH) : PWAIT;
+   error = tsleep((caddr_t)xfer, pri, lbl, 0);
splx(s);
if (error) {
DPRINTF((usbd_bulk_transfer: tsleep=%d\n, error));
@@ -467,7 +468,7 @@ usbd_intr_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
 u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, char *lbl)
 {
usbd_status err;
-   int s, error;
+   int s, error, pri;
 
usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
usbd_intr_transfer_cb);
@@ -478,7 +479,8 @@ usbd_intr_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
splx(s);
return (err);
}
-   error = tsleep(xfer, PZERO | PCATCH, lbl, 0);
+   pri = timeout == USBD_NO_TIMEOUT ? (PZERO | PCATCH) : PWAIT;
+   error = tsleep(xfer, pri, lbl, 0);
splx(s);
if (error) {
DPRINTF((usbd_intr_transfer: tsleep=%d\n, error));



umodem: don't attach if it can't be used

2010-11-29 Thread Jacob Meuser
umodem(4) doesn't support devices that don't have a data interface.
this patch moves a chunk of code from the driver's attach function
that iterates over the usb descriptors to find the data endpoint
and other capabilities to a separate function.  this function is
then used in the match function, and if the data interface isn't
found, returns UMATCH_NONE.

this keeps umodem from attaching to the MSP430 on the TI Launchapd,
which has no data interface.  (jasper@ has a port for mspdebug which
can be used with the Launchpad through ugen(4)).

this is like what I did before with umidi/uaudio ...

except I don't have a umodem(4) to actually test that I didn't break
this.  please test this if you have a umodem(4).  thanks.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: umodem.c
===
RCS file: /cvs/src/sys/dev/usb/umodem.c,v
retrieving revision 1.38
diff -u -p -r1.38 umodem.c
--- umodem.c24 Sep 2010 08:33:59 -  1.38
+++ umodem.c30 Nov 2010 01:04:08 -
@@ -91,7 +91,6 @@ struct umodem_softc {
 
int  sc_ctl_iface_no;
usbd_interface_handlesc_ctl_iface;  /* control interface */
-   int  sc_data_iface_no;
usbd_interface_handlesc_data_iface; /* data interface */
 
int  sc_cm_cap; /* CM capabilities */
@@ -148,6 +147,8 @@ void umodem_attach(struct device *, stru
 int umodem_detach(struct device *, int); 
 int umodem_activate(struct device *, int); 
 
+void umodem_get_caps(struct usb_attach_arg *, int, int *, int *, int *);
+
 struct cfdriver umodem_cd = { 
NULL, umodem, DV_DULL 
 }; 
@@ -160,21 +161,63 @@ const struct cfattach umodem_ca = { 
umodem_activate, 
 };
 
+void
+umodem_get_caps(struct usb_attach_arg *uaa, int ctl_iface_no,
+int *data_iface_no, int *cm_cap, int *acm_cap)
+{
+   const usb_descriptor_t *desc;
+   const usb_interface_descriptor_t *id;
+   const usb_cdc_cm_descriptor_t *cmd;
+   const usb_cdc_acm_descriptor_t *acmd;
+   const usb_cdc_union_descriptor_t *uniond;
+   usbd_desc_iter_t iter;
+   int current_iface_no = -1;
+
+   *cm_cap = *acm_cap = 0;
+   usb_desc_iter_init(uaa-device, iter);
+   desc = usb_desc_iter_next(iter);
+   while (desc) {
+   if (desc-bDescriptorType == UDESC_INTERFACE) {
+   id = (usb_interface_descriptor_t *)desc;
+   current_iface_no = id-bInterfaceNumber;
+   }
+   if (current_iface_no == ctl_iface_no 
+   desc-bDescriptorType == UDESC_CS_INTERFACE) {
+   switch(desc-bDescriptorSubtype) {
+   case UDESCSUB_CDC_CM:
+   cmd = (usb_cdc_cm_descriptor_t *)desc;
+   *cm_cap = cmd-bmCapabilities;
+   *data_iface_no = cmd-bDataInterface;
+   break;
+   case UDESCSUB_CDC_ACM:
+   acmd = (usb_cdc_acm_descriptor_t *)desc;
+   *acm_cap = acmd-bmCapabilities;
+   break;
+   case UDESCSUB_CDC_UNION:
+   uniond = (usb_cdc_union_descriptor_t *)desc;
+   *data_iface_no = uniond-bSlaveInterface[0];
+   break;
+   }
+   }
+   desc = usb_desc_iter_next(iter);
+   }
+}
+
 int
 umodem_match(struct device *parent, void *match, void *aux)
 {
struct usb_attach_arg *uaa = aux;
usb_interface_descriptor_t *id;
usb_device_descriptor_t *dd;
-   int ret;
+   int data_iface_no, cm_cap, acm_cap, ret = UMATCH_NONE;
 
if (uaa-iface == NULL)
-   return (UMATCH_NONE);
+   return (ret);
 
id = usbd_get_interface_descriptor(uaa-iface);
dd = usbd_get_device_descriptor(uaa-device);
if (id == NULL || dd == NULL)
-   return (UMATCH_NONE);
+   return (ret);
 
ret = UMATCH_NONE;
if (UGETW(dd-idVendor) == USB_VENDOR_KYOCERA 
@@ -188,6 +231,15 @@ umodem_match(struct device *parent, void
id-bInterfaceProtocol == UIPROTO_CDC_AT)
ret = UMATCH_IFACECLASS_IFACESUBCLASS_IFACEPROTO;
 
+   if (ret == UMATCH_NONE)
+   return (ret);
+
+   /* umodem doesn't yet support devices without a data iface */
+   umodem_get_caps(uaa, id-bInterfaceNumber, data_iface_no,
+   cm_cap, acm_cap);
+   if (data_iface_no == 0)
+   ret = UMATCH_NONE;
+
return (ret);
 }
 
@@ -199,14 +251,8 @@ umodem_attach(struct device *parent, str
usbd_device_handle dev = uaa-device;
usb_interface_descriptor_t *id;
usb_endpoint_descriptor_t *ed;
-   

Re: more usb detach love

2010-11-25 Thread Jacob Meuser
On Thu, Nov 25, 2010 at 12:45:10PM +, Kevin Chadwick wrote:
 On Wed, 24 Nov 2010 20:59:22 +
 Jacob Meuser jake...@sdf.lonestar.org wrote:
 
  thoughts?
 
 Probably not the thoughts your after especially on tech, but some of the
 panics I was seeing a while back (keep meaning to run more recent and
 proper tests with output, but time is short at the mo) would occur just
 after device removal and only with certain usb readers. If I remember
 right, usually after bb_reset messages. This along with other work on
 usb I've seen definately seems to ring bells.
 
 I'll try to find the time to see if my 4.8 snapshot can be paniced with
 some troublesome readers, though I'm not sure if the ones that were
 troublesome varied on 4.5/4.6? and 4.7. Anyway, If I can, I'll try
 your patch

dlg@ fixed some panics that were caused by removing sd(4) devices not
so long ago, e.g. sys/scsi/sd.c r1.212.  anyway, this patch doesn't
touch umass(4), so it probably isn't a whole lot of help for that
particular case.

for people testing this, I'm more interested that it doesn't break
anything, than that it fixes some particular case, because I already
know that it does fix some particular cases.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



more usb detach love

2010-11-24 Thread Jacob Meuser
when a usb device is removed from a port, a hub interrupt is
generated.  this causes a hub explore task to be scheduled,
which will find that the device has been removed then detach
the driver via usb_disconnect_port().  so there is some time
between when the device is removed and the driver detachment.
if the kernel tries to access the device after it has been
removed but before the driver is detached, a kernel crash is
likely. also, the kernel does not know the device has been
removed until the driver is detached; config_deactivate() is
never called.

the following patch adds a 'dying' field to struct usbd_device,
and a couple functions, usbd_deactivate() and usbd_is_dying().
usbd_deactivate() sets the new dying field, and usbd_is_dying()
checks whether the dying field is set.  usb_disconnect_port()
is changed to call config_deactivate() for devices to be detached
before calling config_detach(), and usbd_deactivate() is added
to the DVACT_DEACTIVATE case for many devices' activate()
functions.  also, usbd_is_dying() is either added or replaces
driver specific state variables at places where it would be
good to know if the hardware is still usable.

the idea is to give drivers a chance to quit accessing the
hardware before the driver is detached, so we don't detach
the driver while a request is stalled trying to read/write
nonexistent hardware.

oh, usbd_is_dying() also checks if the underlying bus is
detached, because if the bus is detached, the device has
also been detached.  I think this will avoid the crash
described in
http://marc.info/?l=openbsd-bugsm=129050458925460w=2
it seems that that device generates a hub interrupt (the
only thing that causes uhub_explore() to be run) when
detached.  on the one hand this makes sense: hub interrupt
means the status of the hub changed.  on the other, it's
nonsensical: the hub is gone, we don't need an interrupt to
know that the hub state has changed.

the idea is really from yuo@, in some commit messages from
a couple months ago.

I've been running this diff for quite some time, and it was
actually in snapshots for some time in early October.

thoughts?

p.s. yes, this doesn't convert *all* drivers at this time.
that will happen, I promise.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: if_atu.c
===
RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
retrieving revision 1.96
diff -u -p -r1.96 if_atu.c
--- if_atu.c27 Oct 2010 17:51:11 -  1.96
+++ if_atu.c24 Nov 2010 19:53:58 -
@@ -902,7 +902,7 @@ atu_internal_firmware(void *arg)
if (err != 0) {
printf(%s: %s loadfirmware error %d\n,
sc-atu_dev.dv_xname, name, err);
-   return;
+   goto fail;
}
 
ptr = firm;
@@ -918,7 +918,7 @@ atu_internal_firmware(void *arg)
DPRINTF((%s: dfu_getstatus failed!\n,
sc-atu_dev.dv_xname));
free(firm, M_DEVBUF);
-   return;
+   goto fail;
}
/* success means state = DnLoadIdle */
state = DFUState_DnLoadIdle;
@@ -940,7 +940,7 @@ atu_internal_firmware(void *arg)
DPRINTF((%s: dfu_dnload failed\n,
sc-atu_dev.dv_xname));
free(firm, M_DEVBUF);
-   return;
+   goto fail;
}
 
ptr += block_size;
@@ -969,14 +969,14 @@ atu_internal_firmware(void *arg)
if (err) {
DPRINTF((%s: dfu_getstatus failed!\n,
sc-atu_dev.dv_xname));
-   return;
+   goto fail;
}
 
DPRINTFN(15, (%s: sending remap\n, sc-atu_dev.dv_xname));
err = atu_usb_request(sc, DFU_REMAP, 0, 0, 0, NULL);
if ((err)  (!ISSET(sc-atu_quirk, ATU_QUIRK_NO_REMAP))) {
DPRINTF((%s: remap failed!\n, sc-atu_dev.dv_xname));
-   return;
+   goto fail;
}
 
/* after a lot of trying and measuring I found out the device needs
@@ -989,6 +989,9 @@ atu_internal_firmware(void *arg)
printf(%s: reattaching after firmware upload\n,
sc-atu_dev.dv_xname);
usb_needs_reattach(sc-atu_udev);
+
+fail:
+   usbd_deactivate(sc-atu_udev);
 }
 
 void
@@ -1169,7 +1172,7 @@ atu_task(void *arg)
 
DPRINTFN(10, (%s: atu_task\n, sc-atu_dev.dv_xname));
 
-   if (sc-sc_state != ATU_S_OK)
+   if (usbd_is_dying(sc-atu_udev))
return;
 
switch (sc-sc_cmd) {
@@ -1259,25 +1262,23 @@ atu_attach(struct device *parent, struct
u_int8_tmode, channel;
int i;
 
-   sc-sc_state = ATU_S_UNCONFIG;
+   

Re: usb mem wait

2010-11-22 Thread Jacob Meuser
On Mon, Nov 22, 2010 at 09:09:17AM -0500, Ted Unangst wrote:
 On Mon, Nov 22, 2010 at 12:06 AM, Jacob Meuser jake...@sdf.lonestar.org
 wrote:
  On Sun, Nov 21, 2010 at 06:47:01PM -0500, Ted Unangst wrote:
  instead of faking an assertwaitok check, let's use the real thing.  this
  is almost the opposite of progress on the whole bluetooth issue, but it
  shortens the stack trace considerably.  the curproc check isn't terribly
  accurate, either, so it misses bugs.
 
  i don't see any reason to avoid sleeping once we've decided that waiting
  is ok, so i changed that too.
 
  where was it decided that waiting is ok?  because it's not supposed to
  be interrupt context?  I dunno about that.  this function makes it into
  a lot of places, and I don't see any that wait.
 
 Well, it is waiting!  running btconfig up will panic later down the
 chain, with this function on the call stack.  Sorry, don't have the
 exact trace handy.  And it's already checking for curproc.  That's
 just a broken test for the same thing.

I don't understand the purpose of this change.  do you plan to change
the rest of the stack (usb_allocmem, *hci_allocm, etc) that calls
this function to wait for memory?  are you sure the only reason for
the M_NOWAITs is to not wait in interrupt context?  I'm not saying
this change is necessarily wrong, but by itself, it raises questions.

 
 
  Index: usb_mem.c
  ===
  RCS file: /home/tedu/cvs/src/sys/dev/usb/usb_mem.c,v
  retrieving revision 1.22
  diff -u -r1.22 usb_mem.c
  --- usb_mem.c 29 Sep 2010 20:06:38 -  1.22
  +++ usb_mem.c 21 Nov 2010 23:15:26 -
  @@ -98,12 +98,7 @@
DPRINTFN(5, (usb_block_allocmem: size=%lu align=%lu\n,
 (u_long)size, (u_long)align));
 
  -#ifdef DIAGNOSTIC
  - if (!curproc) {
  - printf(usb_block_allocmem: in interrupt context,
 size=%lu\n,
  - (unsigned long) size);
  - }
  -#endif
  + assertwaitok();
 
s = splusb();
/* First check the free list. */
  @@ -120,17 +115,8 @@
}
splx(s);
 
  -#ifdef DIAGNOSTIC
  - if (!curproc) {
  - printf(usb_block_allocmem: in interrupt context,
 failed\n);
  - return (USBD_NOMEM);
  - }
  -#endif
  -
DPRINTFN(6, (usb_block_allocmem: no free\n));
  - p = malloc(sizeof *p, M_USB, M_NOWAIT);
  - if (p == NULL)
  - return (USBD_NOMEM);
  + p = malloc(sizeof *p, M_USB, M_WAITOK);
 
p-tag = tag;
p-size = size;
 
  --
  jake...@sdf.lonestar.org
  SDF Public Access UNIX System - http://sdf.lonestar.org

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: usb mem wait

2010-11-21 Thread Jacob Meuser
On Sun, Nov 21, 2010 at 06:47:01PM -0500, Ted Unangst wrote:
 instead of faking an assertwaitok check, let's use the real thing.  this 
 is almost the opposite of progress on the whole bluetooth issue, but it 
 shortens the stack trace considerably.  the curproc check isn't terribly 
 accurate, either, so it misses bugs.
 
 i don't see any reason to avoid sleeping once we've decided that waiting 
 is ok, so i changed that too.

where was it decided that waiting is ok?  because it's not supposed to
be interrupt context?  I dunno about that.  this function makes it into
a lot of places, and I don't see any that wait.


 Index: usb_mem.c
 ===
 RCS file: /home/tedu/cvs/src/sys/dev/usb/usb_mem.c,v
 retrieving revision 1.22
 diff -u -r1.22 usb_mem.c
 --- usb_mem.c 29 Sep 2010 20:06:38 -  1.22
 +++ usb_mem.c 21 Nov 2010 23:15:26 -
 @@ -98,12 +98,7 @@
   DPRINTFN(5, (usb_block_allocmem: size=%lu align=%lu\n,
(u_long)size, (u_long)align));
  
 -#ifdef DIAGNOSTIC
 - if (!curproc) {
 - printf(usb_block_allocmem: in interrupt context, size=%lu\n,
 - (unsigned long) size);
 - }
 -#endif
 + assertwaitok();
  
   s = splusb();
   /* First check the free list. */
 @@ -120,17 +115,8 @@
   }
   splx(s);
  
 -#ifdef DIAGNOSTIC
 - if (!curproc) {
 - printf(usb_block_allocmem: in interrupt context, failed\n);
 - return (USBD_NOMEM);
 - }
 -#endif
 -
   DPRINTFN(6, (usb_block_allocmem: no free\n));
 - p = malloc(sizeof *p, M_USB, M_NOWAIT);
 - if (p == NULL)
 - return (USBD_NOMEM);
 + p = malloc(sizeof *p, M_USB, M_WAITOK);
  
   p-tag = tag;
   p-size = size;

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



usb timeout_del() during detach safety

2010-10-23 Thread Jacob Meuser
if a usb device is plugged in, then unplugged before the driver fully
attaches, the driver detach routine might try to release resources that
haven't been allocated.  timeout(9)s are one such resource.  the
following diff checks if a timeout exists with timeout_initialized()
before doing timeout_del().  I am doing this more or less unconditionally
for purposes of consistency.  it's clear to me that not all usb device
driver authors are aware of this issue.

ok?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: hidkbd.c
===
RCS file: /cvs/src/sys/dev/usb/hidkbd.c,v
retrieving revision 1.3
diff -u -p -r1.3 hidkbd.c
--- hidkbd.c1 Aug 2010 21:37:08 -   1.3
+++ hidkbd.c23 Oct 2010 15:48:55 -
@@ -224,7 +224,8 @@ hidkbd_detach(struct hidkbd *kbd, int fl
DPRINTF((hidkbd_detach: sc=%p flags=%d\n, kbd-sc_device, flags));
 
 #ifdef WSDISPLAY_COMPAT_RAWKBD
-   timeout_del(kbd-sc_rawrepeat_ch);
+   if (timeout_initialized(kbd-sc_rawrepeat_ch))
+   timeout_del(kbd-sc_rawrepeat_ch);
 #endif
 
if (kbd-sc_console_keyboard) {
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.80
diff -u -p -r1.80 if_aue.c
--- if_aue.c23 Oct 2010 15:42:09 -  1.80
+++ if_aue.c23 Oct 2010 15:48:56 -
@@ -855,7 +855,8 @@ aue_detach(struct device *self, int flag
if (!sc-aue_attached) 
return (0);
 
-   timeout_del(sc-aue_stat_ch);
+   if (timeout_initialized(sc-aue_stat_ch))
+   timeout_del(sc-aue_stat_ch);
 
/*
 * Remove any pending tasks.  They cannot be executing because they run
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.100
diff -u -p -r1.100 if_axe.c
--- if_axe.c23 Oct 2010 15:42:09 -  1.100
+++ if_axe.c23 Oct 2010 15:48:56 -
@@ -835,7 +835,8 @@ axe_detach(struct device *self, int flag
if (!sc-axe_attached)
return (0);
 
-   timeout_del(sc-axe_stat_ch);
+   if (timeout_initialized(sc-axe_stat_ch))
+   timeout_del(sc-axe_stat_ch);
 
if (sc-axe_ep[AXE_ENDPT_TX] != NULL)
usbd_abort_pipe(sc-axe_ep[AXE_ENDPT_TX]);
Index: if_cue.c
===
RCS file: /cvs/src/sys/dev/usb/if_cue.c,v
retrieving revision 1.54
diff -u -p -r1.54 if_cue.c
--- if_cue.c23 Oct 2010 15:42:09 -  1.54
+++ if_cue.c23 Oct 2010 15:48:57 -
@@ -561,7 +561,8 @@ cue_detach(struct device *self, int flag
if (!sc-cue_attached)
return (0);
 
-   timeout_del(sc-cue_stat_ch);
+   if (timeout_initialized(sc-cue_stat_ch))
+   timeout_del(sc-cue_stat_ch);
 
/*
 * Remove any pending task.  It cannot be executing because it run
Index: if_mos.c
===
RCS file: /cvs/src/sys/dev/usb/if_mos.c,v
retrieving revision 1.9
diff -u -p -r1.9 if_mos.c
--- if_mos.c23 Oct 2010 15:42:09 -  1.9
+++ if_mos.c23 Oct 2010 15:48:57 -
@@ -771,7 +771,8 @@ mos_detach(struct device *self, int flag
if (!sc-mos_attached)
return (0);
 
-   timeout_del(sc-mos_stat_ch);
+   if (timeout_initialized(sc-mos_stat_ch))
+   timeout_del(sc-mos_stat_ch);
 
if (sc-mos_ep[MOS_ENDPT_TX] != NULL)
usbd_abort_pipe(sc-mos_ep[MOS_ENDPT_TX]);
Index: if_otus.c
===
RCS file: /cvs/src/sys/dev/usb/if_otus.c,v
retrieving revision 1.19
diff -u -p -r1.19 if_otus.c
--- if_otus.c   23 Oct 2010 15:42:09 -  1.19
+++ if_otus.c   23 Oct 2010 15:48:58 -
@@ -250,8 +250,10 @@ otus_detach(struct device *self, int fla
while (sc-cmdq.queued  0)
tsleep(sc-cmdq, 0, cmdq, 0);
 
-   timeout_del(sc-scan_to);
-   timeout_del(sc-calib_to);
+   if (timeout_initialized(sc-scan_to))
+   timeout_del(sc-scan_to);
+   if (timeout_initialized(sc-calib_to))
+   timeout_del(sc-calib_to);
 
if (ifp-if_flags != 0) {   /* if_attach() has been called. */
ifp-if_flags = ~(IFF_RUNNING | IFF_OACTIVE);
Index: if_ral.c
===
RCS file: /cvs/src/sys/dev/usb/if_ral.c,v
retrieving revision 1.114
diff -u -p -r1.114 if_ral.c
--- if_ral.c23 Oct 2010 15:42:09 -  1.114
+++ if_ral.c23 Oct 2010 15:48:58 -
@@ -367,8 +367,10 @@ ural_detach(struct device *self, int fla
if_detach(ifp);
 
usb_rem_task(sc-sc_udev, sc-sc_task);
-   timeout_del(sc-scan_to);
-   timeout_del(sc-amrr_to);
+   if 

usb network detach safety

2010-10-23 Thread Jacob Meuser
like the timeout(9) diff, but for network interfaces.  I discussed this
a bit with damien@ when I ran into this with rum(4), and he suggested
testing if_flags.  I don't work on network drivers ...

ok?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: if_atu.c
===
RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
retrieving revision 1.95
diff -u -p -r1.95 if_atu.c
--- if_atu.c23 Oct 2010 15:42:09 -  1.95
+++ if_atu.c23 Oct 2010 16:16:02 -
@@ -1504,6 +1504,9 @@ atu_detach(struct device *self, int flag
if (sc-atu_ep[ATU_ENDPT_RX] != NULL)
usbd_abort_pipe(sc-atu_ep[ATU_ENDPT_RX]);
 
+   if (ifp-if_flags == 0)
+   return(0);
+
ieee80211_ifdetach(ifp);
if_detach(ifp);
}
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.81
diff -u -p -r1.81 if_aue.c
--- if_aue.c23 Oct 2010 16:14:06 -  1.81
+++ if_aue.c23 Oct 2010 16:16:02 -
@@ -872,8 +872,10 @@ aue_detach(struct device *self, int flag
 
mii_detach(sc-aue_mii, MII_PHY_ANY, MII_OFFSET_ANY);
ifmedia_delete_instance(sc-aue_mii.mii_media, IFM_INST_ANY);
-   ether_ifdetach(ifp);
-   if_detach(ifp);
+   if (ifp-if_flags != 0) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
 #ifdef DIAGNOSTIC
if (sc-aue_ep[AUE_ENDPT_TX] != NULL ||
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.101
diff -u -p -r1.101 if_axe.c
--- if_axe.c23 Oct 2010 16:14:07 -  1.101
+++ if_axe.c23 Oct 2010 16:16:02 -
@@ -864,8 +864,10 @@ axe_detach(struct device *self, int flag
 
mii_detach(sc-axe_mii, MII_PHY_ANY, MII_OFFSET_ANY);
ifmedia_delete_instance(sc-axe_mii.mii_media, IFM_INST_ANY);
-   ether_ifdetach(ifp);
-   if_detach(ifp);
+   if (ifp-if_flags != 0) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
 #ifdef DIAGNOSTIC
if (sc-axe_ep[AXE_ENDPT_TX] != NULL ||
Index: if_cdce.c
===
RCS file: /cvs/src/sys/dev/usb/if_cdce.c,v
retrieving revision 1.46
diff -u -p -r1.46 if_cdce.c
--- if_cdce.c   24 Sep 2010 08:33:58 -  1.46
+++ if_cdce.c   23 Oct 2010 16:16:03 -
@@ -380,9 +380,10 @@ cdce_detach(struct device *self, int fla
if (ifp-if_flags  IFF_RUNNING)
cdce_stop(sc);
 
-   ether_ifdetach(ifp);
-
-   if_detach(ifp);
+   if (ifp-if_flags != 0) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
sc-cdce_attached = 0;
splx(s);
Index: if_cue.c
===
RCS file: /cvs/src/sys/dev/usb/if_cue.c,v
retrieving revision 1.55
diff -u -p -r1.55 if_cue.c
--- if_cue.c23 Oct 2010 16:14:07 -  1.55
+++ if_cue.c23 Oct 2010 16:16:03 -
@@ -576,9 +576,10 @@ cue_detach(struct device *self, int flag
if (ifp-if_flags  IFF_RUNNING)
cue_stop(sc);
 
-   ether_ifdetach(ifp);
-
-   if_detach(ifp);
+   if (ifp-if_flags != 0) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
 #ifdef DIAGNOSTIC
if (sc-cue_ep[CUE_ENDPT_TX] != NULL ||
Index: if_kue.c
===
RCS file: /cvs/src/sys/dev/usb/if_kue.c,v
retrieving revision 1.60
diff -u -p -r1.60 if_kue.c
--- if_kue.c24 Sep 2010 08:33:58 -  1.60
+++ if_kue.c23 Oct 2010 16:16:04 -
@@ -570,9 +570,10 @@ kue_detach(struct device *self, int flag
if (ifp-if_flags  IFF_RUNNING)
kue_stop(sc);
 
-   ether_ifdetach(ifp);
-
-   if_detach(ifp);
+   if (ifp-if_flags != 0) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
 #ifdef DIAGNOSTIC
if (sc-kue_ep[KUE_ENDPT_TX] != NULL ||
Index: if_mos.c
===
RCS file: /cvs/src/sys/dev/usb/if_mos.c,v
retrieving revision 1.10
diff -u -p -r1.10 if_mos.c
--- if_mos.c23 Oct 2010 16:14:07 -  1.10
+++ if_mos.c23 Oct 2010 16:16:04 -
@@ -799,8 +799,10 @@ mos_detach(struct device *self, int flag
 
mii_detach(sc-mos_mii, MII_PHY_ANY, MII_OFFSET_ANY);
ifmedia_delete_instance(sc-mos_mii.mii_media, IFM_INST_ANY);
-   ether_ifdetach(ifp);
-   if_detach(ifp);
+   if (ifp-if_flags != 0) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
 #ifdef DIAGNOSTIC
if (sc-mos_ep[MOS_ENDPT_TX] != NULL ||
Index: if_ral.c

Re: usb network detach safety

2010-10-23 Thread Jacob Meuser
On Sat, Oct 23, 2010 at 04:20:12PM +, Jacob Meuser wrote:
 like the timeout(9) diff, but for network interfaces.  I discussed this
 a bit with damien@ when I ran into this with rum(4), and he suggested
 testing if_flags.  I don't work on network drivers ...

new version after feedback from deraadt.  check ifp-if_softc instead
of ifp-if_flags and just skip net detach instead of returning early.

ok? 

fwiw, the previous version was in snaps, along with some other
diffs, for a while, maybe the first two weeks of oct or so.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: if_atu.c
===
RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
retrieving revision 1.95
diff -u -p -r1.95 if_atu.c
--- if_atu.c23 Oct 2010 15:42:09 -  1.95
+++ if_atu.c23 Oct 2010 16:53:44 -
@@ -1504,8 +1504,10 @@ atu_detach(struct device *self, int flag
if (sc-atu_ep[ATU_ENDPT_RX] != NULL)
usbd_abort_pipe(sc-atu_ep[ATU_ENDPT_RX]);
 
-   ieee80211_ifdetach(ifp);
-   if_detach(ifp);
+   if (ifp-if_softc != NULL) {
+   ieee80211_ifdetach(ifp);
+   if_detach(ifp);
+   }
}
 
return(0);
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.81
diff -u -p -r1.81 if_aue.c
--- if_aue.c23 Oct 2010 16:14:06 -  1.81
+++ if_aue.c23 Oct 2010 16:53:45 -
@@ -872,8 +872,10 @@ aue_detach(struct device *self, int flag
 
mii_detach(sc-aue_mii, MII_PHY_ANY, MII_OFFSET_ANY);
ifmedia_delete_instance(sc-aue_mii.mii_media, IFM_INST_ANY);
-   ether_ifdetach(ifp);
-   if_detach(ifp);
+   if (ifp-if_softc != NULL) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
 #ifdef DIAGNOSTIC
if (sc-aue_ep[AUE_ENDPT_TX] != NULL ||
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.101
diff -u -p -r1.101 if_axe.c
--- if_axe.c23 Oct 2010 16:14:07 -  1.101
+++ if_axe.c23 Oct 2010 16:53:45 -
@@ -864,8 +864,10 @@ axe_detach(struct device *self, int flag
 
mii_detach(sc-axe_mii, MII_PHY_ANY, MII_OFFSET_ANY);
ifmedia_delete_instance(sc-axe_mii.mii_media, IFM_INST_ANY);
-   ether_ifdetach(ifp);
-   if_detach(ifp);
+   if (ifp-if_softc != NULL) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
 #ifdef DIAGNOSTIC
if (sc-axe_ep[AXE_ENDPT_TX] != NULL ||
Index: if_cdce.c
===
RCS file: /cvs/src/sys/dev/usb/if_cdce.c,v
retrieving revision 1.46
diff -u -p -r1.46 if_cdce.c
--- if_cdce.c   24 Sep 2010 08:33:58 -  1.46
+++ if_cdce.c   23 Oct 2010 16:53:46 -
@@ -380,9 +380,10 @@ cdce_detach(struct device *self, int fla
if (ifp-if_flags  IFF_RUNNING)
cdce_stop(sc);
 
-   ether_ifdetach(ifp);
-
-   if_detach(ifp);
+   if (ifp-if_softc != NULL) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
sc-cdce_attached = 0;
splx(s);
Index: if_cue.c
===
RCS file: /cvs/src/sys/dev/usb/if_cue.c,v
retrieving revision 1.55
diff -u -p -r1.55 if_cue.c
--- if_cue.c23 Oct 2010 16:14:07 -  1.55
+++ if_cue.c23 Oct 2010 16:53:46 -
@@ -576,9 +576,10 @@ cue_detach(struct device *self, int flag
if (ifp-if_flags  IFF_RUNNING)
cue_stop(sc);
 
-   ether_ifdetach(ifp);
-
-   if_detach(ifp);
+   if (ifp-if_softc != NULL) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
 #ifdef DIAGNOSTIC
if (sc-cue_ep[CUE_ENDPT_TX] != NULL ||
Index: if_kue.c
===
RCS file: /cvs/src/sys/dev/usb/if_kue.c,v
retrieving revision 1.60
diff -u -p -r1.60 if_kue.c
--- if_kue.c24 Sep 2010 08:33:58 -  1.60
+++ if_kue.c23 Oct 2010 16:53:46 -
@@ -570,9 +570,10 @@ kue_detach(struct device *self, int flag
if (ifp-if_flags  IFF_RUNNING)
kue_stop(sc);
 
-   ether_ifdetach(ifp);
-
-   if_detach(ifp);
+   if (ifp-if_softc != NULL) {
+   ether_ifdetach(ifp);
+   if_detach(ifp);
+   }
 
 #ifdef DIAGNOSTIC
if (sc-kue_ep[KUE_ENDPT_TX] != NULL ||
Index: if_mos.c
===
RCS file: /cvs/src/sys/dev/usb/if_mos.c,v
retrieving revision 1.10
diff -u -p -r1.10 if_mos.c
--- if_mos.c23 Oct 2010 16:14:07 -  1.10
+++ if_mos.c23 Oct 2010 16:53:47 -
@@ -799,8 +799,10 @@ mos_detach(struct device *self, int flag

usb xfer timeout issue

2010-10-17 Thread Jacob Meuser
when a usb transfer (xfer) is started, the underlying hci driver
adds a timeout that adds a usb task that aborts the xfer.

if an xfer is synchronous, the upper usb layer sleeps waiting for
the xfer to complete.  it will also be woken up by the above mentioned
abort task if the xfer times out.

but what happens if a synchronous xfer run in the task thread stalls?
it can't be aborted by the abort task, because that won't run until
after the xfer wakes up!

I'm pretty sure this is what's causing the boot hangs in PR 6491 and
the one reported on m...@.

afaics, this issue is not new, but now that we're using the task
thread for attach/detach, we're more likely to hit it.

the patch below solves the issue by running another task thread that
is to be used ONLY for abort tasks.  I first looked at making the
upper usb layer timeout on it's own, but that's not possible without
major redesign.  I also thought about making the abort tasks workq
tasks, but that doesn't work because there is more than one way to
abort an xfer, and we may need to abort the abort task.  workqs have
no way to remove tasks from the queue but this is possible with
usb tasks.

it's not terribly easy to trigger an xfer timeout, but I did get at
least two, and things did recover.

unless someone sees a problem, or has a better solution, I think this
should go in soon.

yes, I had to hit each driver that uses usb tasks, but I think it's
also better to be explicit, now that we have 3 different types of
usb tasks ...

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: ehci.c
===
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.112
diff -u -p ehci.c
--- ehci.c  29 Sep 2010 20:06:38 -  1.112
+++ ehci.c  17 Oct 2010 22:12:50 -
@@ -1221,7 +1221,7 @@ ehci_allocx(struct usbd_bus *bus)
if (xfer != NULL) {
memset(xfer, 0, sizeof(struct ehci_xfer));
usb_init_task(EXFER(xfer)-abort_task, ehci_timeout_task,
-   xfer);
+   xfer, USB_TASK_TYPE_ABORT);
EXFER(xfer)-ehci_xfer_flags = 0;
 #ifdef DIAGNOSTIC
EXFER(xfer)-isdone = 1;
Index: if_atu.c
===
RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
retrieving revision 1.94
diff -u -p if_atu.c
--- if_atu.c21 Nov 2009 14:18:34 -  1.94
+++ if_atu.c17 Oct 2010 22:12:51 -
@@ -1467,7 +1467,7 @@ atu_complete_attach(struct atu_softc *sc)
/* setup ifmedia interface */
ieee80211_media_init(ifp, atu_media_change, atu_media_status);
 
-   usb_init_task(sc-sc_task, atu_task, sc);
+   usb_init_task(sc-sc_task, atu_task, sc, USB_TASK_TYPE_GENERIC);
 
 #if NBPFILTER  0
bpfattach(sc-sc_radiobpf, sc-sc_ic.ic_if, DLT_IEEE802_11_RADIO,
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.79
diff -u -p if_aue.c
--- if_aue.c24 Sep 2010 08:33:58 -  1.79
+++ if_aue.c17 Oct 2010 22:12:51 -
@@ -734,8 +734,10 @@ aue_attach(struct device *parent, struct device *self,
return;
}
 
-   usb_init_task(sc-aue_tick_task, aue_tick_task, sc);
-   usb_init_task(sc-aue_stop_task, (void (*)(void *))aue_stop, sc);
+   usb_init_task(sc-aue_tick_task, aue_tick_task, sc,
+   USB_TASK_TYPE_GENERIC);
+   usb_init_task(sc-aue_stop_task, (void (*)(void *))aue_stop, sc,
+   USB_TASK_TYPE_GENERIC);
rw_init(sc-aue_mii_lock, auemii);
 
err = usbd_device2interface_handle(dev, AUE_IFACE_IDX, iface);
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.99
diff -u -p if_axe.c
--- if_axe.c24 Sep 2010 03:21:21 -  1.99
+++ if_axe.c17 Oct 2010 22:12:51 -
@@ -676,9 +676,11 @@ axe_attach(struct device *parent, struct device *self,
 
sc-axe_flags = axe_lookup(uaa-vendor, uaa-product)-axe_flags;
 
-   usb_init_task(sc-axe_tick_task, axe_tick_task, sc);
+   usb_init_task(sc-axe_tick_task, axe_tick_task, sc,
+   USB_TASK_TYPE_GENERIC);
rw_init(sc-axe_mii_lock, axemii);
-   usb_init_task(sc-axe_stop_task, (void (*)(void *))axe_stop, sc);
+   usb_init_task(sc-axe_stop_task, (void (*)(void *))axe_stop, sc,
+   USB_TASK_TYPE_GENERIC);
 
err = usbd_device2interface_handle(dev, AXE_IFACE_IDX, sc-axe_iface);
if (err) {
Index: if_cue.c
===
RCS file: /cvs/src/sys/dev/usb/if_cue.c,v
retrieving revision 1.53
diff -u -p if_cue.c
--- if_cue.c24 Sep 2010 08:33:58 -  1.53
+++ if_cue.c17 Oct 2010 22:12:51 -
@@ -470,8 +470,10 @@ cue_attach(struct device *parent, struct device *self,
sc-cue_product = 

uvideo: use the right frame index

2010-10-15 Thread Jacob Meuser
the fidx field of struct uvideo_res is used as the frame index in the
video stream probe/commit commands.  this must be the bFrameIndex
value from the frame descriptor, not the index of the frame descriptor
in our software array of frame descriptors.  although the uvc spec implies
that the numbers should be interchangable, there is hardware where this
is not the case.  this makes the Kodak/PixArt S100 work at 640x480.

ok?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: uvideo.c
===
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.142
diff -u -p uvideo.c
--- uvideo.c9 Oct 2010 09:48:03 -   1.142
+++ uvideo.c15 Oct 2010 19:04:56 -
@@ -1339,7 +1339,7 @@ uvideo_find_res(struct uvideo_softc *sc, int idx, int 
diff_best = diff;
r-width = w;
r-height = h;
-   r-fidx = i;
+   r-fidx = sc-sc_fmtgrp[idx].frame[i]-bFrameIndex;
}
DPRINTF(1, %s: %s: frame index %d: width=%d, height=%d\n,
DEVNAME(sc), __func__, i, w, h);



uvideo: choose best alternate setting

2010-10-15 Thread Jacob Meuser
choose the bAlternateSetting with the closest matching bandwidth,
not the first one we come across that has at least as much as
we need.  the bAlternateSettings aren't guaranteed to be ordered
by increasing bandwidth, and using one with too much bandwidth can
stall the usb pipes.  lets the Ricoh VGP-VCC7 in Vaio VGN-TZ330E 
work at frame sizes other than the largest.

ok?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: uvideo.c
===
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.142
diff -u -p uvideo.c
--- uvideo.c9 Oct 2010 09:48:03 -   1.142
+++ uvideo.c15 Oct 2010 20:52:06 -
@@ -1226,7 +1227,7 @@ uvideo_vs_set_alt(struct uvideo_softc *sc, usbd_interf
const usb_descriptor_t *desc;
usb_interface_descriptor_t *id;
usb_endpoint_descriptor_t *ed;
-   int i;
+   int i, diff, best_diff = INT_MAX;
usbd_status error;
uint32_t psize;
 
@@ -1253,19 +1254,25 @@ uvideo_vs_set_alt(struct uvideo_softc *sc, usbd_interf
/* save endpoint with requested bandwidth */
psize = UGETW(ed-wMaxPacketSize);
psize = UE_GET_SIZE(psize) * (1 + UE_GET_TRANS(psize));
-   if (psize = max_packet_size) {
+   if (psize = max_packet_size)
+   diff = psize - max_packet_size;
+   else
+   goto next;
+   if (diff  best_diff) {
sc-sc_vs_cur-endpoint = ed-bEndpointAddress;
sc-sc_vs_cur-curalt = id-bAlternateSetting;
sc-sc_vs_cur-psize = psize;
-   DPRINTF(1, %s: set alternate iface to , DEVNAME(sc));
-   DPRINTF(1, bAlternateSetting=0x%02x\n,
-   id-bAlternateSetting);
-   break;
+   if (diff == 0)
+   break;
}
 next:
desc = usb_desc_iter_next(iter);
}
 
+   DPRINTF(1, %s: set alternate iface to , DEVNAME(sc));
+   DPRINTF(1, bAlternateSetting=0x%02x psize=%d max_packet_size=%d\n,
+   sc-sc_vs_cur-curalt, sc-sc_vs_cur-psize, max_packet_size);
+
/* set alternate video stream interface */
error = usbd_set_interface(ifaceh, i);
if (error) {



Re: uvideo: choose best alternate setting

2010-10-15 Thread Jacob Meuser
On Fri, Oct 15, 2010 at 09:08:01PM +, Jacob Meuser wrote:
 choose the bAlternateSetting with the closest matching bandwidth,
 not the first one we come across that has at least as much as
 we need.  the bAlternateSettings aren't guaranteed to be ordered
 by increasing bandwidth, and using one with too much bandwidth can
 stall the usb pipes.  lets the Ricoh VGP-VCC7 in Vaio VGN-TZ330E 
 work at frame sizes other than the largest.
 
 ok?

d'oh.  this of course needs an addition ...

 -- 
 jake...@sdf.lonestar.org
 SDF Public Access UNIX System - http://sdf.lonestar.org
 
 Index: uvideo.c
 ===
 RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
 retrieving revision 1.142
 diff -u -p uvideo.c
 --- uvideo.c  9 Oct 2010 09:48:03 -   1.142
 +++ uvideo.c  15 Oct 2010 20:52:06 -
 @@ -1226,7 +1227,7 @@ uvideo_vs_set_alt(struct uvideo_softc *sc, usbd_interf
   const usb_descriptor_t *desc;
   usb_interface_descriptor_t *id;
   usb_endpoint_descriptor_t *ed;
 - int i;
 + int i, diff, best_diff = INT_MAX;
   usbd_status error;
   uint32_t psize;
  
 @@ -1253,19 +1254,25 @@ uvideo_vs_set_alt(struct uvideo_softc *sc, usbd_interf
   /* save endpoint with requested bandwidth */
   psize = UGETW(ed-wMaxPacketSize);
   psize = UE_GET_SIZE(psize) * (1 + UE_GET_TRANS(psize));
 - if (psize = max_packet_size) {
 + if (psize = max_packet_size)
 + diff = psize - max_packet_size;
 + else
 + goto next;
 + if (diff  best_diff) {

+   best_diff = diff;

   sc-sc_vs_cur-endpoint = ed-bEndpointAddress;
   sc-sc_vs_cur-curalt = id-bAlternateSetting;
   sc-sc_vs_cur-psize = psize;
 - DPRINTF(1, %s: set alternate iface to , DEVNAME(sc));
 - DPRINTF(1, bAlternateSetting=0x%02x\n,
 - id-bAlternateSetting);
 - break;
 + if (diff == 0)
 + break;
   }
  next:
   desc = usb_desc_iter_next(iter);
   }
  
 + DPRINTF(1, %s: set alternate iface to , DEVNAME(sc));
 + DPRINTF(1, bAlternateSetting=0x%02x psize=%d max_packet_size=%d\n,
 + sc-sc_vs_cur-curalt, sc-sc_vs_cur-psize, max_packet_size);
 +
   /* set alternate video stream interface */
   error = usbd_set_interface(ifaceh, i);
   if (error) {

... which I didn't notice because I get exact matches on all resolutions
on the 3 cameras I tested.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



auglx(4): suspend like it's AC'97

2010-09-12 Thread Jacob Meuser
also, there's a bit of extra copying from auich(4).  in auich, adj_rate
is the rate after adjusting for the base rate not being 48kHz.  there
is not such adjustment in auglx.

I do not have an auglx(4), so testing is appreciated.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: auglx.c
===
RCS file: /cvs/src/sys/dev/pci/auglx.c,v
retrieving revision 1.6
diff -u -p auglx.c
--- auglx.c 7 Sep 2010 16:21:44 -   1.6
+++ auglx.c 12 Sep 2010 05:34:12 -
@@ -197,9 +197,6 @@ struct auglx_softc {
struct ac97_codec_if*codec_if;
struct ac97_host_if  host_if;
 
-   /* power mgmt */
-   u_int16_tsc_ext_ctrl;
-
int  sc_dmamap_flags;
 };
 
@@ -551,7 +548,6 @@ auglx_set_params(void *v, int setmode, int usemode, st
struct auglx_softc *sc = v;
int error;
u_int orate;
-   u_int adj_rate;
 
if (setmode  AUMODE_PLAY) {
play-factor = 1;
@@ -726,28 +722,25 @@ auglx_set_params(void *v, int setmode, int usemode, st
play-bps = AUDIO_BPS(play-precision);
play-msb = 1;
 
-   orate = adj_rate = play-sample_rate;
+   orate = play-sample_rate;
 
-   play-sample_rate = adj_rate;
+   play-sample_rate = orate;
error = ac97_set_rate(sc-codec_if,
AC97_REG_PCM_LFE_DAC_RATE, play-sample_rate);
if (error)
return error;
 
-   play-sample_rate = adj_rate;
+   play-sample_rate = orate;
error = ac97_set_rate(sc-codec_if,
AC97_REG_PCM_SURR_DAC_RATE, play-sample_rate);
if (error)
return error;
 
-   play-sample_rate = adj_rate;
+   play-sample_rate = orate;
error = ac97_set_rate(sc-codec_if,
AC97_REG_PCM_FRONT_DAC_RATE, play-sample_rate);
if (error)
return error;
-
-   if (play-sample_rate == adj_rate)
-   play-sample_rate = orate;
}
 
if (setmode  AUMODE_RECORD) {
@@ -903,12 +896,10 @@ auglx_set_params(void *v, int setmode, int usemode, st
rec-bps = AUDIO_BPS(rec-precision);
rec-msb = 1;
 
-   orate = rec-sample_rate;
error = ac97_set_rate(sc-codec_if, AC97_REG_PCM_LR_ADC_RATE,
rec-sample_rate);
if (error)
return error;
-   rec-sample_rate = orate;
}
 
return 0;
@@ -1343,17 +1334,22 @@ int
 auglx_activate(struct device *self, int act)
 {
struct auglx_softc *sc = (struct auglx_softc *)self;
+   int rv = 0;
 
switch (act) {
+   case DVACT_ACTIVATE:
+   break;
+   case DVACT_QUIESCE:
+   rv = config_activate_children(self, act);
+   break;
case DVACT_SUSPEND:
-   auglx_read_codec(sc, AC97_REG_EXT_AUDIO_CTRL, sc-sc_ext_ctrl);
break;
case DVACT_RESUME:
-   auglx_reset_codec(sc);
-   delay(1000);
-   (sc-codec_if-vtbl-restore_ports)(sc-codec_if);
-   auglx_write_codec(sc, AC97_REG_EXT_AUDIO_CTRL, sc-sc_ext_ctrl);
+   ac97_resume(sc-host_if, sc-codec_if);
+   rv = config_activate_children(self, act);
break;
+   case DVACT_DEACTIVATE:
+   break;
}
-   return 0;
+   return (rv);
 }



auacer(4): another ac97(4)/audio(4) suspend/resume diff

2010-09-12 Thread Jacob Meuser
I have no auacer(4), testing appreciated.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: auacer.c
===
RCS file: /cvs/src/sys/dev/pci/auacer.c,v
retrieving revision 1.9
diff -u -p auacer.c
--- auacer.c7 Sep 2010 16:21:44 -   1.9
+++ auacer.c12 Sep 2010 07:09:53 -
@@ -1079,15 +1079,22 @@ int
 auacer_activate(struct device *self, int act)
 {
struct auacer_softc *sc = (struct auacer_softc *)self;
+   int rv = 0;
 
switch (act) {
+   case DVACT_ACTIVATE:
+   break;
+   case DVACT_QUIESCE:
+   rv = config_activate_children(self, act);
+   break;
case DVACT_SUSPEND:
break;
case DVACT_RESUME:
-   auacer_reset_codec(sc);
-   delay(1000);
-   (sc-codec_if-vtbl-restore_ports)(sc-codec_if);
+   ac97_resume(sc-host_if, sc-codec_if);
+   rv = config_activate_children(self, act);
break;
+   case DVACT_DEACTIVATE:
+   break;
}
-   return 0;
+   return (rv);
 }



autri(4): more ac97/audio suspend/resume

2010-09-12 Thread Jacob Meuser
I've no autri(4) either.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: autri.c
===
RCS file: /cvs/src/sys/dev/pci/autri.c,v
retrieving revision 1.27
diff -u -p autri.c
--- autri.c 7 Sep 2010 16:21:44 -   1.27
+++ autri.c 12 Sep 2010 07:23:11 -
@@ -622,17 +622,25 @@ int
 autri_activate(struct device *self, int act)
 {
struct autri_softc *sc = (struct autri_softc *)self;
+   int rv = 0;
 
switch (act) {
+   case DVACT_ACTIVATE:
+   break;
+   case DVACT_QUIESCE:
+   rv = config_activate_children(self, act);
+   break;
case DVACT_SUSPEND:
break;
case DVACT_RESUME:
autri_init(sc);
-   /*autri_reset_codec(sc-sc_codec);*/
-   
(sc-sc_codec.codec_if-vtbl-restore_ports)(sc-sc_codec.codec_if);
+   ac97_resume(sc-sc_codec.host_if, sc-sc_codec.codec_if);
+   rv = config_activate_children(self, act);
break;
+   case DVACT_DEACTIVATE:
+   break;
}
-   return 0;
+   return (rv);
 }
 
 int



Re: usb(4) threads

2010-08-22 Thread Jacob Meuser
On Sat, Aug 21, 2010 at 07:46:12PM -0700, Matthew Dempsky wrote:
 On Sun, Aug 22, 2010 at 01:41:54AM +, Jacob Meuser wrote:
  this is in the device task loop to catch that:
  
+   if (!TAILQ_EMPTY(usb_hub_tasks))
+   break;
 
 Ahh, I missed that.  So yes, that's fine then, just the code can be
 simplified a bit. :)
 
  that's a good question.  the current code doesn't deal with that.
  in fact, the current code doesn't even try to stop the thread.
 
 Perhaps this diff should not worry about stopping the thread either
 and just fix the discover prioritization issue?
 
 However, I suspect that by time the thread is being destroyed, all of
 the tasks should have already been removed.  So perhaps a few
 KASSERT(TAILQ_EMPTY(...)) statements somewhere would be appropriate.

this only affects hotplug style usb busses, like cardbus or pcmcia
usb adapters.  and currently, it's not possible to remove those devices
without crashing.  check the usb PRs.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: usb(4) threads

2010-08-22 Thread Jacob Meuser
new diff.  integrated matthew's comments and task thread
loop and:

* put the hub explore task on the task queue, don't run it
directly (except in usb_attach()).  add comments about why.
* make sure high speed hubs explore before companion low/
full speed hubs
* handle config_pending_{inrc,decr} correctly
* stop interrupts from starting an explore if the first explore
hasn't happened yet
* instead of using both discover and explore for the same
idea, just use explore
* usb_hub_tasks and usb_device_tasks are bad names.  use
usb_explore_tasks for the explore tasks and usb_tasks for
the generic task tailq.

I've tested this on several machines with several different
usb configurations at boot (and of course plugging/unplugging
devices at random after boot).  afaics, everything is working
properly.  bonus: device attachment order is more predictible.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: uhub.c
===
RCS file: /cvs/src/sys/dev/usb/uhub.c,v
retrieving revision 1.52
diff -u -p uhub.c
--- uhub.c  13 Nov 2009 18:06:57 -  1.52
+++ uhub.c  22 Aug 2010 21:28:17 -
@@ -586,5 +586,5 @@ uhub_intr(usbd_xfer_handle xfer, usbd_private_handle a
if (status == USBD_STALLED)
usbd_clear_endpoint_stall_async(sc-sc_ipipe);
else if (status == USBD_NORMAL_COMPLETION)
-   usb_needs_explore(sc-sc_hub);
+   usb_needs_explore(sc-sc_hub, 0);
 }
Index: usb.c
===
RCS file: /cvs/src/sys/dev/usb/usb.c,v
retrieving revision 1.62
diff -u -p usb.c
--- usb.c   9 Nov 2009 17:53:39 -   1.62
+++ usb.c   22 Aug 2010 21:28:17 -
@@ -81,8 +81,7 @@ extern intehcidebug;
 #endif
 /*
  * 0  - do usual exploration
- * 1  - do not use timeout exploration
- * 1 - do no exploration
+ * !0 - do no exploration
  */
 intusb_noexplore = 0;
 #else
@@ -95,18 +94,20 @@ struct usb_softc {
usbd_bus_handle  sc_bus;/* USB controller */
struct usbd_port sc_port;   /* dummy port for root hub */
 
-   struct proc *sc_event_thread;
+   struct usb_task  sc_explore_task;
 
char sc_dying;
 };
 
-TAILQ_HEAD(, usb_task) usb_all_tasks;
+TAILQ_HEAD(, usb_task) usb_explore_tasks;
+TAILQ_HEAD(, usb_task) usb_tasks;
 
-volatile int threads_pending = 0;
+int usb_run_tasks;
+int explore_pending;
 
-void   usb_discover(void *);
-void   usb_create_event_thread(void *);
-void   usb_event_thread(void *);
+void   usb_explore(void *);
+void   usb_first_explore(void *);
+void   usb_create_task_thread(void *);
 void   usb_task_thread(void *);
 struct proc *usb_task_thread_proc = NULL;
 
@@ -161,7 +162,7 @@ usb_attach(struct device *parent, struct device *self,
int speed;
struct usb_event ue;
 
-   DPRINTF((usbd_attach\n));
+   DPRINTF((usb_attach\n));
 
usbd_init();
sc-sc_bus = aux;
@@ -189,6 +190,11 @@ usb_attach(struct device *parent, struct device *self,
if (cold)
sc-sc_bus-use_polling++;
 
+   /* Don't let hub interrupts cause explore until ready. */
+   sc-sc_bus-config_pending = 1;
+
+   usb_init_task(sc-sc_explore_task, usb_explore, sc);
+
ue.u.ue_ctrlr.ue_bus = sc-sc_dev.dv_unit;
usb_add_event(USB_EVENT_CTRLR_ATTACH, ue);
 
@@ -229,31 +235,40 @@ usb_attach(struct device *parent, struct device *self,
if (cold)
sc-sc_bus-use_polling--;
 
-   config_pending_incr();
-   kthread_create_deferred(usb_create_event_thread, sc);
+   if (!sc-sc_dying) {
+   config_pending_incr();
+   kthread_create_deferred(usb_first_explore, sc);
+   }
 }
 
+/*
+ * Called by usbd_init when first usb is attached.
+ */
 void
-usb_create_event_thread(void *arg)
+usb_begin_tasks(void)
 {
-   struct usb_softc *sc = arg;
-   static int created = 0;
+   TAILQ_INIT(usb_explore_tasks);
+   TAILQ_INIT(usb_tasks);
+   usb_run_tasks = 1;
+   kthread_create_deferred(usb_create_task_thread, NULL);
+}
 
-   if (sc-sc_bus-usbrev == USBREV_2_0)
-   threads_pending++;
+/*
+ * Called by usbd_finish when last usb is detached.
+ */
+void
+usb_end_tasks(void)
+{
+   usb_run_tasks = 0;
+   wakeup(usb_run_tasks);
+}
 
-   if (kthread_create(usb_event_thread, sc, sc-sc_event_thread,
-   %s, sc-sc_dev.dv_xname))
-   panic(unable to create event thread for %s,
-   sc-sc_dev.dv_xname);
-
-   if (!created) {
-   created = 1;
-   TAILQ_INIT(usb_all_tasks);
-   if (kthread_create(usb_task_thread, NULL,
-   usb_task_thread_proc, usbtask))
-   panic(unable to create usb task thread);
-   }
+void
+usb_create_task_thread(void *arg)
+{
+   if (kthread_create(usb_task_thread, NULL,
+ 

usb(4) threads

2010-08-21 Thread Jacob Meuser
usb(4) has a per-instance event thread, and there is also
a single task thread that runs all other usb_tasks.

usb_tasks are added to a tailq and the task thread runs
the tasks one at a time.

the per-usb event threads only run root hub explorations.
these are responsible for determining when a device has been
added or removed and starts the appropriate driver attach/
detach sequence.

now, when a driver's detach routine is run, it removes any
usb_tasks the driver has added.  this is necessary because
the task may actually try to do something with the hardware,
which is not available if it has been removed.

the problem is, with one thread running the tasks, and
a different thread attaching and detaching devices, when a
device is removed, there's really no guarantee that a detach,
and thus task removal, will happen before a task for that
device is run.

so this diff turns hub explorations into usb_tasks, and runs
them from the task thread.  to make sure exploration tasks
run before other tasks, it uses two tailqs: one for hub
explorations, and one for other tasks.  in this way, we can
be more sure that hub explorations happen before other
tasks, and reduces the chance of running stale tasks.

comments?

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: usb.c
===
RCS file: /cvs/src/sys/dev/usb/usb.c,v
retrieving revision 1.62
diff -u -p usb.c
--- usb.c   9 Nov 2009 17:53:39 -   1.62
+++ usb.c   21 Aug 2010 03:27:04 -
@@ -95,18 +95,19 @@ struct usb_softc {
usbd_bus_handle  sc_bus;/* USB controller */
struct usbd_port sc_port;   /* dummy port for root hub */
 
-   struct proc *sc_event_thread;
+   struct usb_task  sc_explore_task;
 
char sc_dying;
 };
 
-TAILQ_HEAD(, usb_task) usb_all_tasks;
+TAILQ_HEAD(, usb_task) usb_hub_tasks;
+TAILQ_HEAD(, usb_task) usb_device_tasks;
+volatile int usb_run_tasks;
+volatile int discover_pending = 0;
 
-volatile int threads_pending = 0;
-
 void   usb_discover(void *);
-void   usb_create_event_thread(void *);
-void   usb_event_thread(void *);
+void   usb_first_discover(void *);
+void   usb_create_task_thread(void *);
 void   usb_task_thread(void *);
 struct proc *usb_task_thread_proc = NULL;
 
@@ -201,6 +202,8 @@ usb_attach(struct device *parent, struct device *self,
return;
}
 
+   usb_init_task(sc-sc_explore_task, usb_discover, sc);
+
err = usbd_new_device(sc-sc_dev, sc-sc_bus, 0, speed, 0,
  sc-sc_port);
if (!err) {
@@ -230,30 +233,32 @@ usb_attach(struct device *parent, struct device *self,
sc-sc_bus-use_polling--;
 
config_pending_incr();
-   kthread_create_deferred(usb_create_event_thread, sc);
+   kthread_create_deferred(usb_first_discover, sc);
 }
 
+/* called by usbd_init when first usb is attached */
 void
-usb_create_event_thread(void *arg)
+usb_begin_tasks(void)
 {
-   struct usb_softc *sc = arg;
-   static int created = 0;
+   TAILQ_INIT(usb_hub_tasks);
+   TAILQ_INIT(usb_device_tasks);
+   usb_run_tasks = 1;
+   kthread_create_deferred(usb_create_task_thread, NULL);
+}
 
-   if (sc-sc_bus-usbrev == USBREV_2_0)
-   threads_pending++;
+/* called by usbd_finish when lasst usb is detached */
+void
+usb_end_tasks(void)
+{
+   usb_run_tasks = 0;
+}
 
-   if (kthread_create(usb_event_thread, sc, sc-sc_event_thread,
-   %s, sc-sc_dev.dv_xname))
-   panic(unable to create event thread for %s,
-   sc-sc_dev.dv_xname);
-
-   if (!created) {
-   created = 1;
-   TAILQ_INIT(usb_all_tasks);
-   if (kthread_create(usb_task_thread, NULL,
-   usb_task_thread_proc, usbtask))
-   panic(unable to create usb task thread);
-   }
+void
+usb_create_task_thread(void *arg)
+{
+   if (kthread_create(usb_task_thread, NULL,
+   usb_task_thread_proc, usbtask))
+   panic(unable to create usb task thread);
 }
 
 /*
@@ -266,15 +271,17 @@ usb_add_task(usbd_device_handle dev, struct usb_task *
 {
int s;
 
+   DPRINTFN(2,(%s: task=%p onqueue=%d\n, __func__, task, task-onqueue));
+
s = splusb();
if (!task-onqueue) {
-   DPRINTFN(2,(usb_add_task: task=%p\n, task));
-   TAILQ_INSERT_TAIL(usb_all_tasks, task, next);
+   if (task-fun == usb_discover)
+   TAILQ_INSERT_TAIL(usb_hub_tasks, task, next);
+   else
+   TAILQ_INSERT_TAIL(usb_device_tasks, task, next);
task-onqueue = 1;
-   } else {
-   DPRINTFN(3,(usb_add_task: task=%p on q\n, task));
}
-   wakeup(usb_all_tasks);
+   wakeup(usb_run_tasks);
splx(s);
 }
 
@@ -283,21 +290,27 @@ usb_rem_task(usbd_device_handle dev, 

Re: usb(4) threads

2010-08-21 Thread Jacob Meuser
On Sat, Aug 21, 2010 at 04:31:58PM -0700, Matthew Dempsky wrote:
 On Sat, Aug 21, 2010 at 08:25:23AM +, Jacob Meuser wrote:
  +volatile int usb_run_tasks;
  +volatile int discover_pending = 0;
 
 Do these really need to be volatile?

no

 +/* called by usbd_finish when lasst usb is detached */
 
 s/lasst/last/

oops

 +void
 +usb_end_tasks(void)
 +{
 + usb_run_tasks = 0;
 +}
 
 I think we need a wakeup(usb_run_tasks) after the assignment.

yes

  -   s = splusb();
  -   for (;;) {
  -   task = TAILQ_FIRST(usb_all_tasks);
  -   if (task == NULL) {
  -   tsleep(usb_all_tasks, PWAIT, usbtsk, 0);
  -   task = TAILQ_FIRST(usb_all_tasks);
  +   while (usb_run_tasks) {
  +   s = splusb();
  +   while (usb_run_tasks 
  +   (!TAILQ_EMPTY(usb_hub_tasks) ||
  +   !TAILQ_EMPTY(usb_device_tasks))) {
  +
  +   task = TAILQ_FIRST(usb_hub_tasks);
  +   while (task != NULL  usb_run_tasks) {
  +   TAILQ_REMOVE(usb_hub_tasks, task, next);
  +   task-onqueue = 0;
  +   splx(s);
  +   task-fun(task-arg);
  +   s = splusb();
  +   task = TAILQ_FIRST(usb_hub_tasks);
  +   }
  +
  +   task = TAILQ_FIRST(usb_device_tasks);
  +   while (task != NULL  usb_run_tasks) {
  +   TAILQ_REMOVE(usb_device_tasks, task, next);
  +   task-onqueue = 0;
  +   splx(s);
  +   task-fun(task-arg);
  +   s = splusb();
  +   if (!TAILQ_EMPTY(usb_hub_tasks))
  +   break;
  +   task = TAILQ_FIRST(usb_device_tasks);
  +   }
  }
  -   DPRINTFN(2,(usb_task_thread: woke up task=%p\n, task));
  -   if (task != NULL) {
  -   TAILQ_REMOVE(usb_all_tasks, task, next);
  -   task-onqueue = 0;
  -   splx(s);
  -   task-fun(task-arg);
  -   s = splusb();
  +   splx(s);
  +
  +   if (usb_run_tasks) {
  +   /* XXX Is not timing out too optimistic? */
  +   tsleep(usb_run_tasks, PWAIT, usbtsk, 0);
  +   DPRINTFN(2,(%s: woke up\n, __func__));
  }
  }
 
 If a bunch of device tasks get enqueued and then while we're running
 them, a discover task gets enqueued, the device tasks will finish
 running before we run the discover task.

this is in the device task loop to catch that:

  +   if (!TAILQ_EMPTY(usb_hub_tasks))
  +   break;


 How about writing the task thread loop like this:
 
   s = splusb();
   for (usb_run_tasks) {
   if ((task = TAILQ_FIRST(usb_hub_tasks)) != NULL)
   TAILQ_REMOVE(usb_hub_tasks, task, next);
   else if ((task = TAILQ_FIRST(usb_device_tasks)) != NULL)
   TAILQ_REMOVE(usb_device_tasks, task, next);
   else {
   tsleep(usb_run_tasks, PWAIT, usbtsk, 0);
   continue;
   }
 
   task-onqueue = 0;
   splx(s);
   task-fun(task-arg);
   s = splusb();
   }
   splx(s);


that is much nicer :)

 Also, is there a chance that there are tasks still enqueued when
 usb_end_tasks() is called?  Do they need any cleanup?

that's a good question.  the current code doesn't deal with that.
in fact, the current code doesn't even try to stop the thread.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: [PATCH] flip order of steps in release(8)

2010-07-30 Thread Jacob Meuser
On Fri, Jul 30, 2010 at 09:20:30PM +0100, Jason McIntyre wrote:
 On Fri, Jul 30, 2010 at 09:48:43PM +0200, Mark Kettenis wrote:
   Date: Fri, 30 Jul 2010 20:33:23 +0100
   From: Jason McIntyre j...@kerhand.co.uk
   
   On Thu, Jul 29, 2010 at 03:22:12PM +0100, Jason McIntyre wrote:
On Tue, Jul 27, 2010 at 06:48:30PM -0400, Daniel Dickman wrote:
 I think it might be better to create missing directories (especially 
 /usr/obj) before creating the symlinks. This is one less opportunity 
 for 
 someone to mess up if /usr/obj is missing for some reason...
 

would anyone like to ok this diff, or point out any problems with it?
jmc

   
   no? so it's going in.
  
  Sorry, I don't think this makes sense.  I always start with doing a
  make obj.  It's way too easy to mess things things up if you forget to
  do that step, so running anything in my source tree without doing make
  obj first makes me very nervous.
  
  Does changing the order actually fix something?
 
 the idea is you make distrib-dirs before make obj. make obj will fail if
 you are missing dirs,

really?  obj dirs are for source building, distrib dirs are where the
resulting files go.  I don't think 'make obj' will fail without
'make distrib-dirs' first, but perhaps 'make build' will.  or do
you have a specific example?

 so it makes sense to have them in place before
 making obj.
 
 does that make sense or is there a flaw (the reason i asked daniel to
 post this to tech in the first place)?
 
 jmc

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: [PATCH] flip order of steps in release(8)

2010-07-30 Thread Jacob Meuser
On Fri, Jul 30, 2010 at 08:37:05PM -0400, Daniel Dickman wrote:
  Sorry, I don't think this makes sense.  I always start with doing a
  make obj.  It's way too easy to mess things things up if you forget to
  do that step, so running anything in my source tree without doing make
  obj first makes me very nervous.
 
  Does changing the order actually fix something?
 
 i believe so. when i followed the existing order in release(8) but
 without remembering to recreate /usr/obj first i got /usr/obj does
 not exist for every symlink being created.
 
# cd /usr/src  nice make obj
=== lib
=== lib/csu
=== lib/csu/alpha
/usr/src/lib/csu/alpha/obj - /usr/obj/lib/csu/alpha
/usr/obj does not exist
=== lib/csu/amd64
/usr/src/lib/csu/amd64/obj - /usr/obj/lib/csu/amd64
/usr/obj does not exist
=== lib/csu/arm
/usr/src/lib/csu/arm/obj - /usr/obj/lib/csu/arm
/usr/obj does not exist
=== lib/csu/hppa
/usr/src/lib/csu/hppa/obj - /usr/obj/lib/csu/hppa
/usr/obj does not exist

 
 flipping the steps will ensure that make obj won't have this issue.

how did you end up without /usr/obj?

$ tar tzf base48.tgz ./usr/obj
./usr/obj
$ 

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: Adding support for Roland UA-25EX Audio I/F

2010-07-04 Thread Jacob Meuser
On Sun, Jul 04, 2010 at 01:23:14PM +0200, Denis Fondras wrote:
 Hello all,
 
 I have a Roland UA-25EX audio interface (USB) and I'm trying to make
 it work with OpenBSD.
 
 I added the VID/PID (0582/00E6) to usbdevs, usbdevs.h and
 usbdevs_data.h so the device is detected. After kernel compilation
 it is not detected as uaudio0 (as I expected) but as ugen0
 
 For what I read, it works more or less like UA700 which is already
 part of OpenBSD kernel.
 
 Any idea of what I am missing ?

from what I see, it has a standards compliant mode and an advanced mode,
and there is a button on the back to turn off advanced mode.  you'll
almost assuredly need to turn advanced mode off.

http://alsa.opensrc.org/index.php/Edirol_UA-25EX

looks like a nice device.  two XLR inputs and phantom power.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: patch for wss(4), pss(4), ym(4) and gus(4) needs testing

2010-06-28 Thread Jacob Meuser
On Sun, Jun 13, 2010 at 08:58:17AM +, Jacob Meuser wrote:

 I haven't gotten any test results on this yet

it will be committed in the next day or so, so test and/or speak up
now if you care.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: patch for wss(4), pss(4), ym(4) and gus(4) needs testing

2010-06-13 Thread Jacob Meuser
ym(4) needs the following in addition.

I haven't gotten any test results on this yet ... a lot of the
code in audioce(4) and audiocs(4) in sparc64 and sparc come from
here ... the plan is to split the MI code out and share instead
of having it copied in several places.  so, this is not just for
the benefit of old x86 machines ... maybe this can be used on
the alpha too ...

I've tested on wss(4) and ym(4).  pss(4) manual says pss(4) doesn't
work.  if someone has a gus(4), it would be great if you could test.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: isa/ym_isapnp.c
===
RCS file: /cvs/src/sys/dev/isa/ym_isapnp.c,v
retrieving revision 1.8
diff -u -p isa/ym_isapnp.c
--- isa/ym_isapnp.c 14 Mar 2002 01:26:57 -  1.8
+++ isa/ym_isapnp.c 13 Jun 2010 08:48:56 -
@@ -113,6 +113,7 @@ ym_isapnp_attach(parent, self, aux)
sc-sc_ad1848.sc_iooffs = WSS_CODEC;
sc-sc_ad1848.mode = 2;
sc-sc_ad1848.MCE_bit = MODE_CHANGE_ENABLE;
+   sc-sc_ad1848.sc_flags = AD1848_FLAG_32REGS;
 
 #if NMIDI  0
sc-sc_mpu_sc.iobase = ia-ipa_io[3].base;



patch for wss(4), pss(4), ym(4) and gus(4) needs testing

2010-06-03 Thread Jacob Meuser
audio(9) has two different methods for managing DMA transfers.  the
original method is essentially a callback that gives the low level
driver the current kernel audio buffer position and block size
when each block is to be moved.  the newer (well, at least 12 yrs
old) method just gives the low level driver the blocksize and
kernel audio buffer start and end addresses when DMA is started.

the old method is useful in some circumstances, but is unnecessary
for most devices.  the drivers that use the AD1848 DMA engine
directly do not need to use the older method, but they were never
changed to the new method (in OpenBSD, NetBSD did change them 11
years ago).

so, this diff changes the ad1848 ISA drivers to use the newer method.

the CS4231 is very similar to the AD1848, and the CS4231 is also
supported by these drivers.  the biggest difference is that the
AD1848 only supports a single DMA channel, while the CS4231 supports
two DMA channels.  this means the AD1848 is not capable of full-
duplex but the CS4231 is.  single channel DMA operations is referred
to as Mode 1 and two channel DMA operation is referred to as
Mode 2.  currently, CS4231 chips are forced into Mode 2, even if
there is only one DMA channel available.  this makes recording
not work if there is only one DMA channel available.  so I also
made it so CS4231 can be used in Mode 1, and both playback and
recording work.  and in mode 2, full-duplex currently doesn't work
because of a couple reasons, but mainly because only one of the
capture or playback (instead of both) interrupt callbacks gets
called.  this diff fixes full-duplex mode for Mode 2 capable
devices as well.  but, there is a bandwidth limit.  for full-duplex
to work, you need to use either 8-bit samples, mono, or a sample
rate = 24000.

and finally, the AD1848 and CS4231 recalibrate their sample clock
after mode changes.  the tests to check if calibration has ended are
currently wrong, causing busy loops in the driver.  when starting
audio playback, my machine is noticibly stuck for a second or two.
this diff fixes that issue as well.

what I'm mostly looking for are tests that this doesn't break any
current setup.  I imagine these devices probably aren't used much
these days, but maybe I'm wrong.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: ic/cs4231reg.h
===
RCS file: /cvs/src/sys/dev/ic/cs4231reg.h,v
retrieving revision 1.6
diff -u -p ic/cs4231reg.h
--- ic/cs4231reg.h  26 Jun 2008 05:42:15 -  1.6
+++ ic/cs4231reg.h  3 Jun 2010 07:42:30 -
@@ -77,26 +77,32 @@
 #define CS_ALT_FEATURE20x11
 #define CS_LEFT_LINE_CONTROL   0x12
 #define CS_RIGHT_LINE_CONTROL  0x13
+#defineLINE_INPUT_ATTEN_BITS   0x1f
+#defineLINE_INPUT_ATTEN_MASK   0xe0
+#defineLINE_INPUT_MUTE 0x80
+#defineLINE_INPUT_MUTE_MASK0x7f
 #define CS_TIMER_LOW   0x14
 #define CS_TIMER_HIGH  0x15
 #define CS_UPPER_FREQUENCY_SEL 0x16
 #define CS_LOWER_FREQUENCY_SEL 0x17
 #define CS_IRQ_STATUS  0x18
+#defineCS_IRQ_PU   0x01/* Playback Underrun */
+#defineCS_IRQ_PO   0x02/* Playback Overrun */
+#defineCS_IRQ_CO   0x04/* Capture Overrrun */
+#defineCS_IRQ_CU   0x08/* Capture Underrun */
+#defineCS_IRQ_PI   0x10/* Playback Interrupt */
+#defineCS_IRQ_CI   0x20/* Capture Interrupt */
+#defineCS_IRQ_TI   0x40/* Timer Interrupt */
+#defineCS_IRQ_RES  0x80/* reserved */
 #define CS_VERSION_ID  0x19
 #define CS_MONO_IO_CONTROL 0x1A
+#defineMONO_INPUT_ATTEN_BITS   0x0f
+#defineMONO_INPUT_ATTEN_MASK   0xf0
+#defineMONO_OUTPUT_MUTE0x40
+#defineMONO_INPUT_MUTE 0x80
+#defineMONO_INPUT_MUTE_MASK0x7f
 #define CS_POWERDOWN_CONTROL   0x1B
 #define CS_REC_FORMAT  0x1C
 #define CS_XTAL_SELECT 0x1D
 #define CS_UPPER_REC_CNT   0x1E
 #define CS_LOWER_REC_CNT   0x1F
-
-#define MONO_INPUT_ATTEN_BITS  0x0f
-#define MONO_INPUT_ATTEN_MASK  0xf0
-#define MONO_OUTPUT_MUTE   0x40
-#define MONO_INPUT_MUTE0x80
-#define MONO_INPUT_MUTE_MASK   0x7f
-
-#define LINE_INPUT_ATTEN_BITS  0x1f
-#define LINE_INPUT_ATTEN_MASK  0xe0
-#define LINE_INPUT_MUTE0x80
-#define LINE_INPUT_MUTE_MASK   0x7f
Index: isa/ad1848.c
===
RCS file: /cvs/src/sys/dev/isa/ad1848.c,v
retrieving revision 1.33
diff -u -p isa/ad1848.c
--- isa/ad1848.c5 Nov 2007 00:17:28 -   1.33
+++ isa/ad1848.c3 Jun 2010 07:42:30 -
@@ -92,9 +92,11 @@
 #include 

Re: testers needed for auich(4)

2010-04-02 Thread Jacob Meuser
On Fri, Apr 02, 2010 at 06:57:54PM -0700, J.C. Roberts wrote:
 On Fri, 2 Apr 2010 22:45:12 +0200 Alexandre Ratchov a...@caoua.org
 wrote:
 
   A short summary of the changes:
   
   - According to specification AUICH_RR may only be set after DMA 
 is halted (AUICH_DCH is 0 in AUICH_STS). To accomplish this I 
   revived auich_halt_pipe();
   - auich_calibrate() did not clear interrupt and event bits in 
 AUICH_STS. Do that now. Further it did watch the CIV index 
   counter to see when all samples are processed. I changed it to 
   watch AUICH_STS instead and set LVI=CIV. therefore it won't 
   change the CIV counter.
   - my last patch introduced a small bug in auich_trigger_pipe() I 
 fixed that.
   
  
  I think we should put the corresponding chunk in, sooner
  than later.
  
  BTW, I start wondering why this calibration exists at all?
  do devices running at the wrong rate exist (which would mean
  they don't have the proper clock). If so should we start
  adding calibration code in all of our audio drivers?
  
  -- Alexandre
 
 Take a look at the last line of the dmesg below (taken just a moment
 ago). The system has been up for about a day, and the last line was
 added some time after it first booted up. It's typically not seen right
 after a fresh cold boot. A *nearly* identical system running a newer
 snapshot doesn't have the last speed tweak line. As for what exactly
 the speed tweak actually means, I'm not entirely certain. --It kind of
 smells like the improper clock you mentioned above, but it does seem to
 answer your question if devices running at the wrong rate exist.

 auich0: measured ac97 link rate at 48008 Hz, will use 48000 Hz

0.017% deviation is negligible.

src/regress/sys/dev/audio measures the difference in audio and system
clocks.  I wonder how those ISA cards compare to the auich ;)

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: auich: print ac97 rate message only if debug is enabled.

2010-03-15 Thread Jacob Meuser
On Thu, Mar 11, 2010 at 12:11:42PM +0100, David Coppa wrote:
 Hi,
 
 The following diff makes auich.c not print ac97 link rate messages unless
 AUICH_DEBUG is enabled.  This is to prevent kernel from yelling at me
 during boot when aucat is started:
 
 ...
 preserving editor files.
 starting network daemons: sshd sendmail inetd.
 starting local daemons: aucatauich0: measured ac97 link rate at 47995 Hz, 
 will use 48000 Hz
  dbus mpd.
 standard daemons: apmd hotplugd cron.
 Thu Mar 11 11:43:36 CET 2010
 
 OpenBSD/i386 (x41.dacolab.dom) (ttyC0)
 
 ...
 
 It's based on what NetBSD has already done:
 http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/pci/auich.c.diff?r1=1.135r2=1.136only_with_tag=MAIN
 
 suggestions and comments are welcome
 
 cheers,
 david
 
 
 Index: auich.c
 ===
 RCS file: /cvs/src/sys/dev/pci/auich.c,v
 retrieving revision 1.80
 diff -N -u -p auich.c
 --- auich.c   14 Jan 2010 18:15:27 -  1.80
 +++ auich.c   11 Mar 2010 10:37:47 -
 @@ -1857,11 +1857,11 @@ auich_calibrate(struct auich_softc *sc)
   else
   ac97rate = actual_48k_rate;
  
 - printf(%s: measured ac97 link rate at %d Hz,
 -sc-sc_dev.dv_xname, actual_48k_rate);
 + DPRINTF(AUICH_DEBUG_CODECIO, (%s: measured ac97 link rate at %d Hz,
 +sc-sc_dev.dv_xname, actual_48k_rate));
   if (ac97rate != actual_48k_rate)
 - printf(, will use %d Hz, ac97rate);
 - printf(\n);
 + DPRINTF(AUICH_DEBUG_CODECIO, (, will use %d Hz, ac97rate));
 + DPRINTF(AUICH_DEBUG_CODECIO, (\n));
  
   return (ac97rate);
  }

heh.  I was thinking of this recently as well.  definitely debug-only
material.  perhaps not necessary at all unless actual_48k_rate is very
different from ac97rate.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: UBC?

2010-02-02 Thread Jacob Meuser
On Tue, Feb 02, 2010 at 10:09:58AM -0700, Darrin Chandler wrote:
 On Tue, Feb 02, 2010 at 02:51:52AM +, Jacob Meuser wrote:
  are you talking about Bret's reply about buzzwords?  imo, that's what
  that reply was about, buzzwords.  there was nothing personal.  see,
  the way buzzwords work, is they get stuck in your (as in most people)
  head, then they come out when you have a idea but don't know quite
  how to express it.  which is clearly the situation here.
 
 It's not like UBC was totally unrelated to the topic. And that situation
 was already handled fine by Bob Beck in a more productive and
 informative way.
 
  I think readers of OpenBSD lists are far too sensitive.  there, that's
  a personal 'attack' on all y'all.
 
 Yeah, everything is fine the way it is. Alienating random users with
 real questions is the cost of making a good OS. They're just too
 sensitive.
 
 Except that I have a pretty thick skin and have been around on the lists
 enough to see how things work. So why did this thread bother me? Because
 there's a difference between some idiot asking idiotic questions on
 misc@ and getting blasted, and an inquiry into a pretty common, genuine
 issue on t...@.

he asked about UBC.  he did not originally present his real issue.

but whatever.  you can take it as a lesson to not hide your real
questions/issues behind buzzwords, or you can get upset.  it's
your choice.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: UBC?

2010-02-02 Thread Jacob Meuser
On Tue, Feb 02, 2010 at 11:58:37AM -0700, Darrin Chandler wrote:
 On Tue, Feb 02, 2010 at 06:36:14PM +, Jacob Meuser wrote:
  On Tue, Feb 02, 2010 at 10:09:58AM -0700, Darrin Chandler wrote:
   On Tue, Feb 02, 2010 at 02:51:52AM +, Jacob Meuser wrote:
are you talking about Bret's reply about buzzwords?  imo, that's what
that reply was about, buzzwords.  there was nothing personal.  see,
the way buzzwords work, is they get stuck in your (as in most people)
head, then they come out when you have a idea but don't know quite
how to express it.  which is clearly the situation here.
   
   It's not like UBC was totally unrelated to the topic. And that situation
   was already handled fine by Bob Beck in a more productive and
   informative way.
   
I think readers of OpenBSD lists are far too sensitive.  there, that's
a personal 'attack' on all y'all.
   
   Yeah, everything is fine the way it is. Alienating random users with
   real questions is the cost of making a good OS. They're just too
   sensitive.
   
   Except that I have a pretty thick skin and have been around on the lists
   enough to see how things work. So why did this thread bother me? Because
   there's a difference between some idiot asking idiotic questions on
   misc@ and getting blasted, and an inquiry into a pretty common, genuine
   issue on t...@.
  
  he asked about UBC.  he did not originally present his real issue.
  
  but whatever.  you can take it as a lesson to not hide your real
  questions/issues behind buzzwords, or you can get upset.  it's
  your choice.
 
 The real issue came out quickly in response to direct questions, and it
 came out in a civil and informative manner. No problem. So what's your
 point, really?

don't hide your questions behind buzzwords and then get upset when
someone makes fun of buzzwords.

 No, nevermind. I'm not interested in debating this.
 You're grasping at anything to prop up your position. Just hold firm and
 I'll stop bugging you about it. You win. Kind of.

win what?

 -- 
 Darrin Chandler|  Phoenix BSD User Group  |  MetaBUG
 dwchand...@stilyagin.com   |  http://phxbug.org/  |  http://metabug.org/
 http://www.stilyagin.com/  |  Daemons in the Desert   |  Global BUG Federation

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: UBC?

2010-02-01 Thread Jacob Meuser
On Mon, Feb 01, 2010 at 03:28:22PM -0700, Darrin Chandler wrote:
 On Mon, Feb 01, 2010 at 03:05:37PM -0700, Nick Bender wrote:
  On Mon, Feb 1, 2010 at 2:46 PM, Bob Beck b...@ualberta.ca wrote:
   On 1 February 2010 14:09, Donald Allen donaldcal...@gmail.com wrote:
   I have not responded to this thread because I was angered by it and
   did not want to respond in anger. That has passed. But this thread is
   unfortunately all too typical of a pattern of ridicule and downright
   nastiness that occurs much too often on the OpenBSD lists.
  
   
  
   Well I certainly didn't get that impression from this thread, Sorry
   you did..  Happy Trails.
  
  
  
  Ditto. Thought this thread was fairly productive:
  
  - question raised
  - clarification sought
  - clarification provided
  - solution given
  - bug found
  - patch proposed and debated
  
   You could enforce minimum levels
   of civility, as many such communities do, without impeding the flow of
   technical information a it, but you choose not to.
  
  Civility is in the eye of the moderator. I prefer my debate unfiltered...
 
 There was some belittling of Allen for asking about UBC. I don't think
 it added anything. Not even humor. Beck's initial response was a good
 one, clarifying and offering something to try, but at least one person
 jumped in with unhelpful comments. There were also informative and
 interesting replies, and in fact these made of the bulk of the thread.
 While this thread was relatively mild compared to some I think Allen has
 a point. There's just no need to escalate what should have been a purely
 technical discussion into an ad hominem attack.

are you talking about Bret's reply about buzzwords?  imo, that's what
that reply was about, buzzwords.  there was nothing personal.  see,
the way buzzwords work, is they get stuck in your (as in most people)
head, then they come out when you have a idea but don't know quite
how to express it.  which is clearly the situation here.

I think readers of OpenBSD lists are far too sensitive.  there, that's
a personal 'attack' on all y'all.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: spelling sys/arch/vax/qbus/uda.c

2009-12-15 Thread Jacob Meuser
On Tue, Dec 15, 2009 at 09:02:19PM -0500, Brad Tilley wrote:
 # cvs diff -Nup sys/arch/vax/qbus/uda.c
 Index: sys/arch/vax/qbus/uda.c
 ===
 RCS file: /cvs/src/sys/arch/vax/qbus/uda.c,v
 retrieving revision 1.6
 diff -N -u -p sys/arch/vax/qbus/uda.c
 --- sys/arch/vax/qbus/uda.c 12 Nov 2005 03:44:24 -  1.6
 +++ sys/arch/vax/qbus/uda.c 16 Dec 2009 02:01:27 -
 @@ -250,7 +250,7 @@ err2:   bus_dmamem_unmap(sc-sc_dmat,
 (caddr_t)sc-sc_
 
 /*
  * The only thing that differ UDA's and Tape ctlr's is
 -* their vcid. Beacuse there are no way to determine which
 +* their vcid. Because there are no way to determine which
  * ctlr type it is, we check what is generated and later
  * set the correct vcid.
  */


Because there is no way?

I think your patches for the gnu/ stuff need to go upstream.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: mixerctl(1) to sysctl(8); a simpler interface

2009-11-20 Thread Jacob Meuser
On Fri, Nov 20, 2009 at 03:45:31PM +0100, Peter Hessler wrote:
 mixerctl inputs.master=[0,255]

record.volume, actually.  historical/consistency reasons.

 mixerctl outputs.master=[0,255]
 
 those two should do what you want.  the rest are there for people that
 like to poke at their audio bits. 

yes.  note that on azalia, those two (record.volume and outputs.master)
are special.  their behaviour and configurability is explained in
azalia(4) (as of some weeks in -current, anyway).

recording is a little different though, as you should really be able
to choose the source.  usually the control to select the recording
source on azalia is record.adc-0:1_source.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: mixerctl(1) to sysctl(8); a simpler interface

2009-11-20 Thread Jacob Meuser
On Fri, Nov 20, 2009 at 08:16:40PM +0100, Atle Kristensen wrote:

 what about audioctl, I think that the defaults could be more sane on USB 
 audio's?
 (OpenBSD 4.6
 
 play.rate=32000
 play.channels=1
 play.precision=8

this was recently changed in -current to try to set 44.1 kHz stereo 16-bit
slinear by default, but of course it depends whether or not the device
supports those parameters.  it will get as close as possible.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: mixerctl(1) to sysctl(8); a simpler interface

2009-11-20 Thread Jacob Meuser
On Fri, Nov 20, 2009 at 08:28:34AM -0800, Ted Unangst wrote:
 On Nov 20, 2009, at 8:03 AM, Thomas Pfaff tpf...@tp76.info wrote:
 
 On Fri, 20 Nov 2009 15:45:31 +0100
 Peter Hessler phess...@theapt.org wrote:
 
 mixerctl inputs.master=[0,255]
 mixerctl outputs.master=[0,255]
 
 Still, is a simpler sysctl interface something people want?  mixerctl
 can be reserved for people wanting to screw around with the gazillion
 of options available.

I have been considering making 'outputs.master' and 'record.volume'
standard on all devices.  the way it's done in azalia(4) could be
done for any driver.  uaudio(4) would be the only difficult one,
the rest would be easy.

but that would mean things are a lot more consistent (same basic
controls on all audio devices), and would even simplify some code
in audio(4).

I just haven't decided what would be the best way to do that yet.
probably some of the code could be reused, but where to put that
code is the question.  i.e. a driver just needs to make lists
of device indexes, but the mixer code has to be part of the
hardware driver, not audio(4) ...

 Mixerctl and sysctl already have the same interface. And saying  
 mixerctl has too many options is silly considering how many options  
 sysctl has.
 
 If you don't like mixerctl, there's at least a half dozen mixers in  
 ports.

those don't help at all.  cmixer is the only one with a chance of
ever being really helpful, imo.  it's the only one that doesn't use
libossaudio (where the real mixer names are matched to OSS names,
ick).

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: azalia: converter channel names

2009-10-26 Thread Jacob Meuser
no comments?

On Fri, Oct 23, 2009 at 01:16:18AM +, Jacob Meuser wrote:
 this diff encodes the audio stream channel numbers converters will
 process into converter mixer names.
 
 e.g.
 
 inputs.dac  - inputs.dac-0:1
 inputs.dac2 - inputs.dac-2:3
 inputs.dac3 - inputs.dac-4:5
 
 pros:
 * easier to understand what channels are going where
 * very similar in format to aucat channel numbering
 
 cons:
 * different than any other driver
 
 overcoming the con is not hard.  software looking for 'dac' can also
 look for 'dac-0:'.  woudn't be surprised if programs are confused by
 'dac2' already anyway though.  otoh, it wouldn't really bother me to
 make such a change universal, but I'm not really pushing that either,
 at this point.
 
 btw, this part of the manual change:
 
  However, a dac that is connected to built-in speaker(s) or front
  panel headphone jack(s) by default will convert audio stream channels
  starting at 0 if the dac would otherwise not be converting any channels.
  For example, if dac-2:3 is the default dac for the built-in speakers in
  a laptop, dac-2:3 will convert channels 0 and 1 when a stereo audio
  stream is being played.  This is to allow simultaneous stereo playback
  on both the built-in speakers and a line or headphone jack.
 
 is not new behaviour, it just wasn't explained before.
 
 -- 
 jake...@sdf.lonestar.org
 SDF Public Access UNIX System - http://sdf.lonestar.org
 
 Index: share/man/man4/azalia.4
 ===
 RCS file: /cvs/src/share/man/man4/azalia.4,v
 retrieving revision 1.22
 diff -u -p share/man/man4/azalia.4
 --- share/man/man4/azalia.4   20 Oct 2009 06:31:26 -  1.22
 +++ share/man/man4/azalia.4   23 Oct 2009 00:44:35 -
 @@ -106,8 +106,7 @@ The widget type enumerator is used to distinguish diff
  of the same type.
  The enumeration starts at 2: the first widget of each type is not
  enumerated.
 -Except for dac and adc widget types, the enumeration order is
 -meaningless.
 +The enumeration order is meaningless.
  The property is optional.
  Generally, if there is no property, the mixer item is an amplifier gain
  control.
 @@ -117,22 +116,45 @@ The following are the widget type names used in mixer 
  .Bl -tag -width SPDIF-in
  .It Cm dac
  Digital to analog converter, usually used for playback.
 -These widgets are enumerated according to the channels they convert.
 -For example, if a codec has 3 stereo dacs, they would convert the
 -following channels: dac channels 0 and 1, dac2 channels 2 and 3,
 -dac3 channels 4 and 5.
 +The audio stream channel(s) these widgets will convert are encoded into
 +their name in the form of start channel:end channel.
 +For example,
 +.Cm dac-0:1
 +converts channels 0 and 1 (stereo).
 +However, a dac that is connected to built-in speaker(s) or front
 +panel headphone jack(s) by default will convert audio stream channels
 +starting at 0 if the dac would otherwise not be converting any channels.
 +For example, if
 +.Cm dac-2:3
 +is the default dac for the built-in speakers in a laptop,
 +.Cm dac-2:3
 +will convert channels 0 and 1 when a stereo audio stream is being played.
 +This is to allow simultaneous stereo playback on both the built-in speakers
 +and a line or headphone jack.
  .Pp
  .It Cm dig-dac
  Digital output converter, usually an S/PDIF transmitter.
 +The audio stream channel(s) these widgets will convert are encoded into
 +their name in the form of start channel:end channel.
 +For example,
 +.Cm dig-dac-0:1
 +converts channels 0 and 1 (stereo).
  .Pp
  .It Cm adc
  Analog to digital converter, usually used for recording.
 -These widgets are enumerated according to the channels they convert.
 -For example, if a codec has 2 stereo adcs, they would convert the
 -following channels: adc channels 0 and 1, adc2 channels 2 and 3.
 +The audio stream channel(s) these widgets will convert are encoded into
 +their name in the form of start channel:end channel.
 +For example,
 +.Cm adc-0:1
 +converts channels 0 and 1 (stereo).
  .Pp
  .It Cm dig-adc
  Digital input converter, usually an S/PDIF receiver.
 +The audio stream channel(s) these widgets will convert are encoded into
 +their name in the form of start channel:end channel.
 +For example,
 +.Cm dig-adc-0:1
 +converts channels 0 and 1 (stereo).
  .Pp
  .It Cm mix
  Sums multiple audio sources into a single stream, but
 Index: sys/dev/pci/azalia.c
 ===
 RCS file: /cvs/src/sys/dev/pci/azalia.c,v
 retrieving revision 1.158
 diff -u -p sys/dev/pci/azalia.c
 --- sys/dev/pci/azalia.c  13 Oct 2009 19:33:16 -  1.158
 +++ sys/dev/pci/azalia.c  23 Oct 2009 00:44:36 -
 @@ -2767,9 +2767,10 @@ int
  azalia_widget_label_widgets(codec_t *codec)
  {
   widget_t *w;
 + convgroup_t *group;
   int types[16];
   int pins[16];
 - int colors_used, use_colors;
 + int colors_used, use_colors, schan;
   int i, j

Re: audioctl minor fix

2009-10-22 Thread Jacob Meuser
On Thu, Oct 22, 2009 at 11:02:37PM +0200, Pawlowski Marcin Piotr wrote:
 On Thu, 22 Oct 2009 22:33:23 +0200
 Pawlowski Marcin Piotr pawlowski...@gmail.com wrote:
 
  Hi,
  I was playing with audioctl and found that:
  - fullduplex and full_duplex are the same so one of them is redundant

so?  did you consider that scripts may use one form and not
the other?  did you look at the commit history?

  - output is different from other *ctl tools

$ sudo sysctl -n kern.usermount=1
1
$ audioctl -n play.rate=44100
$ mixerctl -n outputs.master=200
outputs.master: 200,200 - 200,200

imo, that is a much more important inconsistency.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: audioctl minor fix

2009-10-22 Thread Jacob Meuser
On Fri, Oct 23, 2009 at 12:55:39AM +0200, Pawlowski Marcin Piotr wrote:
 On Thu, 22 Oct 2009 21:41:45 +
 Jacob Meuser jake...@sdf.lonestar.org wrote:
 
  On Thu, Oct 22, 2009 at 11:02:37PM +0200, Pawlowski Marcin Piotr
  wrote:
   On Thu, 22 Oct 2009 22:33:23 +0200
   Pawlowski Marcin Piotr pawlowski...@gmail.com wrote:
   
Hi,
I was playing with audioctl and found that:
- fullduplex and full_duplex are the same so one of them is
redundant
  
  so?  did you consider that scripts may use one form and not
  the other?  did you look at the commit history?
 
 I did looked at commit history but didn't found any info about why. I

indeed, it doesn't say why fullduplex was added when full_duplex
already existed.  tsk tsk.

but at this point they have both existed for 11 years.  I don't
see much point in possibly breaking someone's private scripts/
methods, just to remove a redundant line.

- output is different from other *ctl tools
  
  $ sudo sysctl -n kern.usermount=1
  1
  $ audioctl -n play.rate=44100
  $ mixerctl -n outputs.master=200
  outputs.master: 200,200 - 200,200
  
  imo, that is a much more important inconsistency.
  
 
 Done.

looks good at first inspection.  thanks!

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



azalia: converter channel names

2009-10-22 Thread Jacob Meuser
this diff encodes the audio stream channel numbers converters will
process into converter mixer names.

e.g.

inputs.dac  - inputs.dac-0:1
inputs.dac2 - inputs.dac-2:3
inputs.dac3 - inputs.dac-4:5

pros:
* easier to understand what channels are going where
* very similar in format to aucat channel numbering

cons:
* different than any other driver

overcoming the con is not hard.  software looking for 'dac' can also
look for 'dac-0:'.  woudn't be surprised if programs are confused by
'dac2' already anyway though.  otoh, it wouldn't really bother me to
make such a change universal, but I'm not really pushing that either,
at this point.

btw, this part of the manual change:

 However, a dac that is connected to built-in speaker(s) or front
 panel headphone jack(s) by default will convert audio stream channels
 starting at 0 if the dac would otherwise not be converting any channels.
 For example, if dac-2:3 is the default dac for the built-in speakers in
 a laptop, dac-2:3 will convert channels 0 and 1 when a stereo audio
 stream is being played.  This is to allow simultaneous stereo playback
 on both the built-in speakers and a line or headphone jack.

is not new behaviour, it just wasn't explained before.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: share/man/man4/azalia.4
===
RCS file: /cvs/src/share/man/man4/azalia.4,v
retrieving revision 1.22
diff -u -p share/man/man4/azalia.4
--- share/man/man4/azalia.4 20 Oct 2009 06:31:26 -  1.22
+++ share/man/man4/azalia.4 23 Oct 2009 00:44:35 -
@@ -106,8 +106,7 @@ The widget type enumerator is used to distinguish diff
 of the same type.
 The enumeration starts at 2: the first widget of each type is not
 enumerated.
-Except for dac and adc widget types, the enumeration order is
-meaningless.
+The enumeration order is meaningless.
 The property is optional.
 Generally, if there is no property, the mixer item is an amplifier gain
 control.
@@ -117,22 +116,45 @@ The following are the widget type names used in mixer 
 .Bl -tag -width SPDIF-in
 .It Cm dac
 Digital to analog converter, usually used for playback.
-These widgets are enumerated according to the channels they convert.
-For example, if a codec has 3 stereo dacs, they would convert the
-following channels: dac channels 0 and 1, dac2 channels 2 and 3,
-dac3 channels 4 and 5.
+The audio stream channel(s) these widgets will convert are encoded into
+their name in the form of start channel:end channel.
+For example,
+.Cm dac-0:1
+converts channels 0 and 1 (stereo).
+However, a dac that is connected to built-in speaker(s) or front
+panel headphone jack(s) by default will convert audio stream channels
+starting at 0 if the dac would otherwise not be converting any channels.
+For example, if
+.Cm dac-2:3
+is the default dac for the built-in speakers in a laptop,
+.Cm dac-2:3
+will convert channels 0 and 1 when a stereo audio stream is being played.
+This is to allow simultaneous stereo playback on both the built-in speakers
+and a line or headphone jack.
 .Pp
 .It Cm dig-dac
 Digital output converter, usually an S/PDIF transmitter.
+The audio stream channel(s) these widgets will convert are encoded into
+their name in the form of start channel:end channel.
+For example,
+.Cm dig-dac-0:1
+converts channels 0 and 1 (stereo).
 .Pp
 .It Cm adc
 Analog to digital converter, usually used for recording.
-These widgets are enumerated according to the channels they convert.
-For example, if a codec has 2 stereo adcs, they would convert the
-following channels: adc channels 0 and 1, adc2 channels 2 and 3.
+The audio stream channel(s) these widgets will convert are encoded into
+their name in the form of start channel:end channel.
+For example,
+.Cm adc-0:1
+converts channels 0 and 1 (stereo).
 .Pp
 .It Cm dig-adc
 Digital input converter, usually an S/PDIF receiver.
+The audio stream channel(s) these widgets will convert are encoded into
+their name in the form of start channel:end channel.
+For example,
+.Cm dig-adc-0:1
+converts channels 0 and 1 (stereo).
 .Pp
 .It Cm mix
 Sums multiple audio sources into a single stream, but
Index: sys/dev/pci/azalia.c
===
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.158
diff -u -p sys/dev/pci/azalia.c
--- sys/dev/pci/azalia.c13 Oct 2009 19:33:16 -  1.158
+++ sys/dev/pci/azalia.c23 Oct 2009 00:44:36 -
@@ -2767,9 +2767,10 @@ int
 azalia_widget_label_widgets(codec_t *codec)
 {
widget_t *w;
+   convgroup_t *group;
int types[16];
int pins[16];
-   int colors_used, use_colors;
+   int colors_used, use_colors, schan;
int i, j;
 
bzero(pins, sizeof(pins));
@@ -2817,63 +2818,56 @@ azalia_widget_label_widgets(codec_t *codec)
case COP_AWTYPE_AUDIO_OUTPUT:
if 

bogus uaudio quirk: UQ_AU_INP_ASYNC

2009-10-15 Thread Jacob Meuser
relevant part of original NetBSD commit message which added this:

  UQ_AU_INP_ASYNC for input devices that claim to be adaptive, but are
in fact asynchronous (an easy mistake to make unless you read the
specs carefully :)

the specs say no such thing.

the specs say:
a) input adaptive endpoints need a synchronization pipe
b) a device can use implied feedback in lieu of a synch-pipe
c) if bSynchAddress==0, a synch-pipe is not needed

point a) is very blatant in the usb audio spec.  point b) is only
mentioned in the general usb spec.  point c) is only mentioned once,
in the table describing the layout of the descriptor.

so, yes, this is difficult to understand, because it's hard to find
all the pieces to the puzzle.  of NetBSD, FreeBSD, ALSA and OSS, none of
them really handle the case of bSynchAddress==0 correctly.  I just
committed a fix to our driver.

anyway, I've googled for descriptor dumps of the devices this quirk
was applied to and found two of three:

yapphone: 
http://www.gentooforum.de/artikel/17083/voip-yapphone-wird-nicht-erkannt.html

plantronics: 
http://www.mail-archive.com/alsa-u...@lists.sourceforge.net/msg17344.html

as you can see they both have bSynchAddress==0 for the adaptive input
endpoints.  I couldn't find the avancelogic device, but I'm certain it's
the same situation.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: usb_quirks.h
===
RCS file: /cvs/src/sys/dev/usb/usb_quirks.h,v
retrieving revision 1.13
diff -u -p -r1.13 usb_quirks.h
--- usb_quirks.h29 Jun 2008 10:04:15 -  1.13
+++ usb_quirks.h15 Oct 2009 08:51:37 -
@@ -56,8 +56,6 @@ struct usbd_quirks {
 #define UQ_POWER_CLAIM 0x0200
/* don't adjust for fractional samples */
 #define UQ_AU_NO_FRAC  0x0400
-   /* input is async despite claim of adaptive */
-#define UQ_AU_INP_ASYNC0x0800
/* modem device breaks on cm over data */
 #define UQ_ASSUME_CM_OVER_DATA 0x1000
/* printer has broken bidir mode */
Index: usb_quirks.c
===
RCS file: /cvs/src/sys/dev/usb/usb_quirks.c,v
retrieving revision 1.52
diff -u -p -r1.52 usb_quirks.c
--- usb_quirks.c11 May 2009 08:07:42 -  1.52
+++ usb_quirks.c15 Oct 2009 08:51:37 -
@@ -105,12 +105,6 @@ const struct usbd_quirk_entry {
 
  { USB_VENDOR_TI, USB_PRODUCT_TI_UTUSB41,  0x110, { UQ_POWER_CLAIM }},
  { USB_VENDOR_TELEX, USB_PRODUCT_TELEX_MIC1,   0x009, { UQ_AU_NO_FRAC }},
- { USB_VENDOR_SILICONPORTALS, USB_PRODUCT_SILICONPORTALS_YAPPHONE,
-   0x100, { UQ_AU_INP_ASYNC }},
- { USB_VENDOR_AVANCELOGIC, USB_PRODUCT_AVANCELOGIC_USBAUDIO,
-   0x101, { UQ_AU_INP_ASYNC }},
- { USB_VENDOR_PLANTRONICS, USB_PRODUCT_PLANTRONICS_HEADSET,
-   0x004, { UQ_AU_INP_ASYNC }},
 
  { USB_VENDOR_TERRATEC, USB_PRODUCT_TERRATEC_AUREON,
ANY, { UQ_BAD_HID }},
Index: uaudio.c
===
RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
retrieving revision 1.63
diff -u -p -r1.63 uaudio.c
--- uaudio.c15 Oct 2009 08:47:44 -  1.63
+++ uaudio.c15 Oct 2009 08:51:38 -
@@ -1549,9 +1549,6 @@ uaudio_process_as(struct uaudio_softc *s
 
dir = UE_GET_DIR(ed-bEndpointAddress);
type = UE_GET_ISO_TYPE(ed-bmAttributes);
-   if ((usbd_get_quirks(sc-sc_udev)-uq_flags  UQ_AU_INP_ASYNC) 
-   dir == UE_DIR_IN  type == UE_ISO_ADAPT)
-   type = UE_ISO_ASYNC;
 
/* We can't handle endpoints that need a sync pipe yet. */
sync = FALSE;



uaudio diff to test

2009-10-10 Thread Jacob Meuser
maybe you've seen such a line in your dmesg after plugging in USB
audio devices:

uaudio0: ignored input endpoint of type adaptive
or
uaudio0: ignored output endpoint of type async

these input/adaptive and output/async endpoints need external help to
synchronize them, or more accurately, to keep their data rate as
constant as possible.  in the USB specification literature, the
synchronization method is referred to as feedback (USB 2.0 Spec.,
Section 5.2.14.2).  usually, it is the responsibility of the driver
to facilitate this feedback, but uaudio(4) does not do this yet, and
that is why you may see the above messages in your dmesg.

however, it is possible that the device is providing the synchronization
on it's own.  consider the following specification excerpts:

from USB Audio Specification 1.0, Section 3.7.2 AudioStreaming Interface:
  [...] Each AudioStreaming interface can have at most one isochronous
  data endpoint.  [...]  In some cases, the isochronous data endpoint
  is accompanied by an associated isochronous synch endpoint for
  synchronisation purposes.  The isochronous data endpoint is required
  to be the first endpoint in the AudioStreaming interface.  The synch
  endpoint always follows its associated data endpoint.

from USB 2.0 Specification, Section 5.12.4.3 Implicit Feedback
  In some cases, implementing a separate explicit feedback endpoint
  can be avoided.  If a device implements a group of isochronous data
  endpoints that are closely related and if:
* All the endpoints in the group are synchronized (i.e. use
  sample clocks that are derived from a common master clock)
* The group contains one or more isochronous data endpoints in one
  direction that normally would need explicit feedback
* The group contains at least one isochronous data endpoint in the
  opposite direction
  Under these circumstances, the device may elect not to implement
  a separate isochronous explicit feedback endpoint.  Instead, feedback
  information can be derived from the data endpoint in the opposite
  direction by observing its data rate.


I have two different audio devices that have input/async endpoints
in their AudioStreaming interface.  these devices should require a
synchronisation endpoint, as the first excerpt says.  however, they
do not provide any such endpoint.  both these audio devices do
provide both recording and playback capabilities (endpoints in both
directions), and as with most audio devices, both playback and
recording very likely use the same clock source.

so, it seems likely to me that these devices are using implicit
feedback and don't really need anything special from the driver.
the patch below allows input/adaptive and output/async endpoints
that have no corresponding synch endpoint to be used.

this lets me record with:

uaudio0 at uhub1 port 2 configuration 1 interface 0 Logitech Logitech USB 
Headset rev 1.10/10.12 addr 2
uaudio0: audio rev 1.00, 6 mixer controls
audio1 at uaudio0

and

uaudio0 at uhub1 port 2 configuration 1 interface 0 Ten X Technology, Inc. USB 
 AUDIO rev 1.10/1.0b addr 2
uaudio0: audio rev 1.00, 4 mixer controls
audio1 at uaudio0
(though this thing still has other issues)


Linux (well, ALSA) does the same thing, their comment is, check the
number of EP, since some devices have broken descriptors which fool us.
if it has only one EP, assume it as adaptive-out or async-in.

please test with any and all USB audio devices.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: uaudio.c
===
RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
retrieving revision 1.61
diff -u -p uaudio.c
--- uaudio.c21 Nov 2008 17:55:02 -  1.61
+++ uaudio.c10 Oct 2009 14:48:30 -
@@ -1555,21 +1555,27 @@ uaudio_process_as(struct uaudio_softc *sc, const char 
 
/* We can't handle endpoints that need a sync pipe yet. */
sync = FALSE;
-   if (dir == UE_DIR_IN  type == UE_ISO_ADAPT) {
-   sync = TRUE;
+   /* If there's only one endpoint, then the device is implementing
+* implicit feedback (USB Specification 2.0, Section 5.12.4.3),
+* and we don't have to configure a sync pipe.
+*/
+   if (id-bNumEndpoints  1) {
+   if (dir == UE_DIR_IN  type == UE_ISO_ADAPT) {
+   sync = TRUE;
 #ifndef UAUDIO_MULTIPLE_ENDPOINTS
-   printf(%s: ignored input endpoint of type adaptive\n,
-  sc-sc_dev.dv_xname);
-   return (USBD_NORMAL_COMPLETION);
+   printf(%s: ignored input endpoint of type adaptive\n,
+  sc-sc_dev.dv_xname);
+   return (USBD_NORMAL_COMPLETION);
 #endif
-   }
-   if (dir != UE_DIR_IN  type == UE_ISO_ASYNC) {
-   sync = TRUE;
+   }
+   if (dir != UE_DIR_IN  type == UE_ISO_ASYNC) {
+   

Re: mixerctl broken for sparc?

2009-06-28 Thread Jacob Meuser
On Thu, Jun 25, 2009 at 05:07:58PM +0100, Edd Barrett wrote:
 Hi,
 
 On Sat, Jun 20, 2009 at 1:57 AM, Jacob Meuserjake...@sdf.lonestar.org wrote:
  Any clues what may be wrong here? I am willing to have a look at the
  code myself if no-one has any ideas.
 
  please do ;) ?the mixer interface in audioce(4) looks sorta bogus.
  I guess the driver was not really finished? ?maybe the following will
  help get you started?
 
 I have been hacking and I have something which does allow me to hear sound 
 now.
 
 Before I post that diff can someone confirm that the ce4231 differs
 from the cs4231 driver which I stumbled across in the sparc/ (not 64)
 directory by accident. The code looks similar, but not the same. Does
 the actual hardware differ?

the audio codec is the same.  the bus architecture differs.  cs4231(4)
uses sbus while ce4231(4) uses ebus.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: mixerctl broken for sparc?

2009-06-28 Thread Jacob Meuser
On Sun, Jun 28, 2009 at 07:35:07PM +0100, Edd Barrett wrote:
 Hi Jacob,
 
 On Sun, Jun 28, 2009 at 04:50:41PM +, Jacob Meuser wrote:
  the audio codec is the same.  the bus architecture differs.  cs4231(4)
  uses sbus while ce4231(4) uses ebus.
 
 I can't send a diff, as my disk died. Hopefully new one on the way soon.
 I can describe what I have gathered so far. Comments encouraged, as I am
 not very confident with kernel hacking yet:
 
 a) The mixer controls need to be reversed. A lower mixer control is
 louder to the card (or something elsewhere is reversing the controls).

ah, yes.  for the DAC, each bit adds attenuation, not amplification.

 b) The range of volumes on the card is 0-64, where 0 is max. In the
 patch you sent you were multiplying by 4 ( 2). I think it need to be
 divided by 4 ( 2).

no.  you would do something like `252 - (bits  2)'.  some of the other
registers act differently.  google for 'cs4231 datasheet' if you haven't
already.

 c) The set_port() function seems to write straight to the card, whereas
 the cs4231 (sparc/dev) driver only modifies the volumes struct here. It
 must be updating later. Didn't get a chance to look before the disk
 died. Which do you think is right?

the sparc driver uses mapiodev().  I guess this is not available on
sparc64.

 d) It seems the current ce4231 driver modifies the master volume control
 whereas the cs4231 code changes one of the aux ports. Again, did not get
 a chance to test. Any ideas which is correct? It did seem that the
 master volume was clashing with that you told the dac to be.

yes, it does clash.  I'm pretty sure the master volume control is using
the wrong register.  look at the end of sys/dev/ic/ad1848reg.h.
that corresponds to the indirect registers on page 22 of the cs4231
datasheet.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org



Re: azalia(4) diff needs testing.

2009-06-24 Thread Jacob Meuser
only 4 people have ati or nvidia azalia(4)?

On Tue, Jun 23, 2009 at 01:42:49AM +, Jacob Meuser wrote:
 On Sun, Jun 21, 2009 at 01:07:37AM +, Jacob Meuser wrote:
  On Mon, Feb 09, 2009 at 12:40:31AM +, Owain Ainsworth wrote:
   On Sun, Feb 08, 2009 at 05:11:16PM -0500, Brad wrote:
Please test the following diff with any azalia(4) adapter, but 
especially
with any ATI or NVIDIA chipsets. Make sure sound still works properly
without any unusual sound artifacts.
   
   Surely this isn't much of an improvement. At least only map uncached
   on the hardware that needs it. Why give non-broken chips the cpu-time
   hit?
  
  I recently got some reports of broken sound with azalia, all with
  amd64.mp with nVidia chipsets.  at least two of the reports said
  that this is a new problem, that the devices were working properly
  a couple months ago.  one (not sure if it was working before) was
  fixed by switching from enabling pcie snoop to enabling
  BUS_DMA_NOCACHE.  haven't heard back from the others yet.
  
  please test the following if you have ATI or NVIDIA chipsets.
 
 haven't gotten much feedback on this one ...

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: azalia.c
===
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.140
diff -u -p azalia.c
--- azalia.c18 Jun 2009 08:19:03 -  1.140
+++ azalia.c20 Jun 2009 11:01:18 -
@@ -143,6 +143,7 @@ typedef struct azalia_t {
bus_dma_tag_t dmat;
pcireg_t pciid;
uint32_t subid;
+   int dma_nocache;
 
codec_t codecs[15];
int ncodecs;/* number of codecs */
@@ -361,7 +362,6 @@ azalia_pci_attach(struct device *parent, struct device
pcireg_t v;
pci_intr_handle_t ih;
const char *interrupt_str;
-   uint8_t reg;
 
sc = (azalia_t*)self;
pa = aux;
@@ -384,15 +384,10 @@ azalia_pci_attach(struct device *parent, struct device
v = pci_conf_read(pa-pa_pc, pa-pa_tag, 0x44);
pci_conf_write(pa-pa_pc, pa-pa_tag, 0x44, v  (~0x7));
  
-   /* enable PCIe snoop */
+   sc-dma_nocache = 0;
switch (PCI_PRODUCT(pa-pa_id)) {
case PCI_PRODUCT_ATI_SB450_HDA:
case PCI_PRODUCT_ATI_SBX00_HDA:
-   reg = azalia_pci_read(pa-pa_pc, pa-pa_tag, 
ATI_PCIE_SNOOP_REG);
-   reg = ATI_PCIE_SNOOP_MASK;
-   reg |= ATI_PCIE_SNOOP_ENABLE;
-   azalia_pci_write(pa-pa_pc, pa-pa_tag, ATI_PCIE_SNOOP_REG, 
reg);
-   break;
case PCI_PRODUCT_NVIDIA_MCP51_HDA:
case PCI_PRODUCT_NVIDIA_MCP55_HDA:
case PCI_PRODUCT_NVIDIA_MCP61_HDA_1:
@@ -415,10 +410,7 @@ azalia_pci_attach(struct device *parent, struct device
case PCI_PRODUCT_NVIDIA_MCP89_HDA_2:
case PCI_PRODUCT_NVIDIA_MCP89_HDA_3:
case PCI_PRODUCT_NVIDIA_MCP89_HDA_4:
-   reg = azalia_pci_read(pa-pa_pc, pa-pa_tag, 
NVIDIA_PCIE_SNOOP_REG);
-   reg = NVIDIA_PCIE_SNOOP_MASK;
-   reg |= NVIDIA_PCIE_SNOOP_ENABLE;
-   azalia_pci_write(pa-pa_pc, pa-pa_tag, NVIDIA_PCIE_SNOOP_REG, 
reg);
+   sc-dma_nocache = BUS_DMA_NOCACHE;
break;
}
 
@@ -1109,7 +1101,7 @@ azalia_alloc_dmamem(azalia_t *az, size_t size, size_t 
if (nsegs != 1)
goto free;
err = bus_dmamem_map(az-dmat, d-segments, 1, size,
-   d-addr, BUS_DMA_NOWAIT | BUS_DMA_COHERENT);
+   d-addr, BUS_DMA_NOWAIT | BUS_DMA_COHERENT | az-dma_nocache);
if (err)
goto free;
err = bus_dmamap_create(az-dmat, size, 1, size, 0,



Re: azalia(4) diff needs testing.

2009-06-20 Thread Jacob Meuser
On Mon, Feb 09, 2009 at 12:40:31AM +, Owain Ainsworth wrote:
 On Sun, Feb 08, 2009 at 05:11:16PM -0500, Brad wrote:
  Please test the following diff with any azalia(4) adapter, but especially
  with any ATI or NVIDIA chipsets. Make sure sound still works properly
  without any unusual sound artifacts.
 
 Surely this isn't much of an improvement. At least only map uncached
 on the hardware that needs it. Why give non-broken chips the cpu-time
 hit?

I recently got some reports of broken sound with azalia, all with
amd64.mp with nVidia chipsets.  at least two of the reports said
that this is a new problem, that the devices were working properly
a couple months ago.  one (not sure if it was working before) was
fixed by switching from enabling pcie snoop to enabling
BUS_DMA_NOCACHE.  haven't heard back from the others yet.

please test the following if you have ATI or NVIDIA chipsets.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: azalia.c
===
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.140
diff -u -p azalia.c
--- azalia.c18 Jun 2009 08:19:03 -  1.140
+++ azalia.c20 Jun 2009 11:01:18 -
@@ -143,6 +143,7 @@ typedef struct azalia_t {
bus_dma_tag_t dmat;
pcireg_t pciid;
uint32_t subid;
+   int dma_nocache;
 
codec_t codecs[15];
int ncodecs;/* number of codecs */
@@ -361,7 +362,6 @@ azalia_pci_attach(struct device *parent, struct device
pcireg_t v;
pci_intr_handle_t ih;
const char *interrupt_str;
-   uint8_t reg;
 
sc = (azalia_t*)self;
pa = aux;
@@ -384,15 +384,10 @@ azalia_pci_attach(struct device *parent, struct device
v = pci_conf_read(pa-pa_pc, pa-pa_tag, 0x44);
pci_conf_write(pa-pa_pc, pa-pa_tag, 0x44, v  (~0x7));
  
-   /* enable PCIe snoop */
+   sc-dma_nocache = 0;
switch (PCI_PRODUCT(pa-pa_id)) {
case PCI_PRODUCT_ATI_SB450_HDA:
case PCI_PRODUCT_ATI_SBX00_HDA:
-   reg = azalia_pci_read(pa-pa_pc, pa-pa_tag, 
ATI_PCIE_SNOOP_REG);
-   reg = ATI_PCIE_SNOOP_MASK;
-   reg |= ATI_PCIE_SNOOP_ENABLE;
-   azalia_pci_write(pa-pa_pc, pa-pa_tag, ATI_PCIE_SNOOP_REG, 
reg);
-   break;
case PCI_PRODUCT_NVIDIA_MCP51_HDA:
case PCI_PRODUCT_NVIDIA_MCP55_HDA:
case PCI_PRODUCT_NVIDIA_MCP61_HDA_1:
@@ -415,10 +410,7 @@ azalia_pci_attach(struct device *parent, struct device
case PCI_PRODUCT_NVIDIA_MCP89_HDA_2:
case PCI_PRODUCT_NVIDIA_MCP89_HDA_3:
case PCI_PRODUCT_NVIDIA_MCP89_HDA_4:
-   reg = azalia_pci_read(pa-pa_pc, pa-pa_tag, 
NVIDIA_PCIE_SNOOP_REG);
-   reg = NVIDIA_PCIE_SNOOP_MASK;
-   reg |= NVIDIA_PCIE_SNOOP_ENABLE;
-   azalia_pci_write(pa-pa_pc, pa-pa_tag, NVIDIA_PCIE_SNOOP_REG, 
reg);
+   sc-dma_nocache = BUS_DMA_NOCACHE;
break;
}
 
@@ -1109,7 +1101,7 @@ azalia_alloc_dmamem(azalia_t *az, size_t size, size_t 
if (nsegs != 1)
goto free;
err = bus_dmamem_map(az-dmat, d-segments, 1, size,
-   d-addr, BUS_DMA_NOWAIT | BUS_DMA_COHERENT);
+   d-addr, BUS_DMA_NOWAIT | BUS_DMA_COHERENT | az-dma_nocache);
if (err)
goto free;
err = bus_dmamap_create(az-dmat, size, 1, size, 0,



azalia: multichannel DAC selection

2009-05-17 Thread Jacob Meuser
the idea is that when a codec is capable of playing at least 6
channels, the output jacks should be connected to unique DACs
by default.  it's not possible to do multichannel playback if
all the output jacks are connected to the same DAC.

it is also common for multichannel capable codecs to have
+2 capabilities.  perhaps you have seen 7.1+2 audio in
motherboard literature.  that means the device is capable of
playing a 7.1 (8 channels, 2 each for front, rear, center/lfe,
side; 5.1 is 6 channels, 2 each for front, rear, center/lfe; if
it can to 7.1 it can do 5.1 as well) multichannel stream, and a
separate stereo stream, concurrently.  these codecs are usually
found in desktop machines, and there will usually be a headphone
jack in the front of the machine, in addition to several jacks
in the rear.  this diff tries to connect such a front headphone
jack to a unique DAC, so it can be used to play back a stereo
stream while a multichannel stream is also playing.  however, if
the front headphone DAC is not be used separately (which requires
using aucat(1) in a multi-stream configuration, which I won't
describe now as this is already getting long), it will be connected
to the first stereo channels.  in other words, if you are
just playing some stereo music, both the front jack (usually
green) on the rear panel, and the headphone jack (also usually
green) on the front panel will get the music.

testing this diff is actually pretty easy.  just send me a complete
dmesg and complete 'mixerctl -v output' after applying the diff,
building the kernel and rebooting.  I can figure out from that if
the diff is doing what it's supposed to do.

this needs a bit of testing.  many laptops are actually capable of
5.1 playback, so although this mostly affects desktop systems, please
test on laptops as well.

thanks.

-- 
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org

Index: azalia.c
===
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.131
diff -u -p azalia.c
--- azalia.c12 May 2009 09:32:28 -  1.131
+++ azalia.c17 May 2009 19:48:45 -
@@ -212,6 +212,7 @@ int azalia_codec_find_defadc_sub(codec_t *, nid_t, int
 intazalia_codec_init_volgroups(codec_t *);
 intazalia_codec_sort_pins(codec_t *);
 intazalia_codec_select_micadc(codec_t *);
+intazalia_codec_select_dacs(codec_t *);
 intazalia_codec_select_spkrdac(codec_t *);
 intazalia_codec_find_inputmixer(codec_t *);
 
@@ -1361,6 +1362,17 @@ azalia_codec_init(codec_t *this)
if (err)
return err;
 
+   /* If the codec can do multichannel, select different DACs for
+* the output pins.  Also select a unique DAC for the front
+* headphone pin, if one exists.
+*/
+   this-fhp_dac = -1;
+   if (this-na_dacs = 3  this-nopins = 3) {
+   err = azalia_codec_select_dacs(this);
+   if (err)
+   return err;
+   }
+
err = azalia_codec_select_spkrdac(this);
if (err)
return err;
@@ -1489,7 +1501,7 @@ azalia_codec_sort_pins(codec_t *this)
struct io_pin opins[MAX_PINS], opins_d[MAX_PINS];
struct io_pin ipins[MAX_PINS], ipins_d[MAX_PINS];
int nopins, nopins_d, nipins, nipins_d;
-   int prio, add, nd, conv;
+   int prio, loc, add, nd, conv;
int i, j, k;
 
nopins = nopins_d = nipins = nipins_d = 0;
@@ -1499,6 +1511,10 @@ azalia_codec_sort_pins(codec_t *this)
if (!w-enable || w-type != COP_AWTYPE_PIN_COMPLEX)
continue;
 
+   loc = 0;
+   if (this-na_dacs = 3)
+   loc = CORB_CD_LOC_GEO(w-d.pin.config);
+
prio = w-d.pin.association  4 | w-d.pin.sequence;
conv = -1;
 
@@ -1529,7 +1545,8 @@ azalia_codec_sort_pins(codec_t *this)
if (add  nopins  MAX_PINS) {
opins[nopins].nid = w-nid;
opins[nopins].conv = conv;
-   opins[nopins].prio = prio | (nd  8);
+   prio |= (nd  8) | (loc  9);
+   opins[nopins].prio = prio;
nopins++;
}
}
@@ -1695,6 +1712,80 @@ azalia_codec_sort_pins(codec_t *this)
 #undef MAX_PINS
 }
 
+int
+azalia_codec_select_dacs(codec_t *this)
+{
+   widget_t *w;
+   nid_t *convs;
+   int nconv, conv;
+   int i, j, k, err, isfhp;
+
+   convs = malloc(this-na_dacs * sizeof(nid_t), M_DEVBUF,
+   M_NOWAIT | M_ZERO);
+   if (convs == NULL)
+   return(ENOMEM);
+
+   nconv = 0;
+   for (i = 0; i  this-nopins; i++) {
+   isfhp = 0;
+   w = this-w[this-opins[i].nid];
+
+   if (w-d.pin.device == CORB_CD_HEADPHONE 
+