Re: aucat: suggest using -v100 ?
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 ?
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
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
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
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
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
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
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
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
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
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?)
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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)
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)
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
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
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
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
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)
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: UBC?
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?
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?
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
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
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
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
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
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
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
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
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
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
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?
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?
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.
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.
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
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
+
