Re: uvideo patches: Overview [0/4]

[Marcus Glocker]

On Tue, May 17, 2016 at 05:15:51PM -0700, patrick keshishian wrote:

> Greetings,
> 
> I have been looking at uvideo trying to model a new driver I'm
> attempting to port over and found a few issues (or what I precive
> as issues).
> 
> Since the list likes separate diffs for easier discussion, Here
> is my attempt to break them up in four emails. I think, with
> exception of one, all should apply and compile individually.
> 
> Here are description of patches in decreasing order of my
> confidence in proposing them:
> 
> 1/4: Incorrect enum used for v4l2_buf.flags.
>  This is a paste error I believe. Very simple diff
> 
> 2/4: Assumption on endpoint index to use in uvideo_vs_open() vs
>  actual saved endpoint address.
> 
> 3/4: In uvideo_vs_set_alt(), according to the comment within
>  while()-loop searches for an endpoint with requested
>  bandwidth, or best match. An iterator index (int i) is used
>  in the while()-loop, and eventually its value is used in
>  usbd_set_interface().
> 
>  Is the "matched" interface's bAlternateSetting not the
>  correct value to be used?

Agreed.  'i' should currently always be the same as 'curalt'.  Committed.
 
> 4/4: I don't believe V4L2_BUF_FLAG_QUEUED and V4L2_BUF_FLAG_DONE
>  flags are handled correctly in our uvideo driver.

Mirror downage: openbsd.cs.toronto.edu, obsdacvs.cs.toronto.edu, man.openbsd.org, cvsweb.openbsd.org

[Nick Holland]

Hi.

Due to a infrastructure upgrade, power to the mirror and other systems
at University of Toronto, will be interrupted sometime Thursday, after
9:30pm Toronto time (EDT -- UTC-4), and should be restored Friday by
7:30am EDT.

This will impact:
* openbsd.cs.toronto.edu
* obsdacvs.cs.toronto.edu
* http://cvsweb.openbsd.org
* http://man.openbsd.org

Sorry for any inconvenience this might cause.

Nick.

ptrace PT_IO write bug

[Mathieu -]

Hello everyone,

While playing a bit with ptrace to do some debugging I stumbled upon
something that looks like a bug.
While trying to write to the ptrace'd process using PT_IO in combinaison
with PIOD_WRITE_D I kept getting EFAULTs.
PIOD_READ_D would work fine on the same address though, and even more
weirdly PIOD_WRITE_I would also work fine on the same address.
Even more strange, PT_WRITE_D works fine too on the same address.
So in effect, only PT_IO + PIOD_WRITE_D would EFAULT on this address.

If this is the expected behavior (I really doubt it), then the man page
is wrong because it states clearly that *_I and *_D are equivalent.

Digging a bit on the implementation I traced it back to rev 1.33 of
sys_process.c.
The old implementation used procfs_domem to do the uvm_io call, and
based the decision as to use UVM_IO_FIXPROT or not on the uio.uio_rw
field (UIO_WRITE meant FIXPROT vs UIO_READ).
However the new implementation, in process_domem takes a third
parameter, req, the ptrace request and would use UVM_IO_FIXPROT only in
the PT_WRITE_I case (rings any bell?).
That's why PT_WRITE_D will EFAULT in any case.
Oh and PT_WRITE_I and PT_WRITE_D work because they both use PT_WRITE_I
as the req parameter :
 process_domem(p, t, , write ? PT_WRITE_I : 

So I came up with the following diff (kind of the big hammer approach),
which gets rid of the req parameters and base the UVM_IO_FIXPROT
decision on the uio.uio_rw field as the previous code (10 years ago!)
was doing.

nota: I was curious as to how gdb worked with this, but turns out they
just bruteforce both PIOD_WRITE_I and PIOD_WRITE_D flag until it works
:).

diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 60ec50e..4d589e7 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -368,8 +368,7 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = write ? UIO_WRITE : UIO_READ;
uio.uio_procp = p;
-   error = process_domem(p, t, , write ? PT_WRITE_I :
-   PT_READ_I);
+   error = process_domem(p, t, );
if (write == 0)
*retval = temp;
return (error);
@@ -387,23 +386,14 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
uio.uio_procp = p;
switch (piod.piod_op) {
case PIOD_READ_I:
-   req = PT_READ_I;
-   uio.uio_rw = UIO_READ;
-   break;
case PIOD_READ_D:
-   req = PT_READ_D;
uio.uio_rw = UIO_READ;
break;
case PIOD_WRITE_I:
-   req = PT_WRITE_I;
-   uio.uio_rw = UIO_WRITE;
-   break;
case PIOD_WRITE_D:
-   req = PT_WRITE_D;
uio.uio_rw = UIO_WRITE;
break;
case PIOD_READ_AUXV:
-   req = PT_READ_D;
uio.uio_rw = UIO_READ;
temp = tr->ps_emul->e_arglen * sizeof(char *);
if (uio.uio_offset > temp)
@@ -418,7 +408,7 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
default:
return (EINVAL);
}
-   error = process_domem(p, t, , req);
+   error = process_domem(p, t, );
piod.piod_len -= uio.uio_resid;
(void) copyout(, SCARG(uap, addr), sizeof(piod));
return (error);
@@ -711,7 +701,7 @@ process_checkioperm(struct proc *p, struct process *tr)
 }
 
 int
-process_domem(struct proc *curp, struct proc *p, struct uio *uio, int req)
+process_domem(struct proc *curp, struct proc *p, struct uio *uio)
 {
struct vmspace *vm;
int error;
@@ -734,11 +724,11 @@ process_domem(struct proc *curp, struct proc *p, struct 
uio *uio, int req)
vm->vm_refcnt++;
 
error = uvm_io(>vm_map, uio,
-   (req == PT_WRITE_I) ? UVM_IO_FIXPROT : 0);
+   (uio->uio_rw == UIO_WRITE) ? UVM_IO_FIXPROT : 0);
 
uvmspace_free(vm);
 
-   if (error == 0 && req == PT_WRITE_I)
+   if (error == 0 && uio->uio_rw == UIO_WRITE)
pmap_proc_iflush(p, addr, len);
 
return (error);
diff --git a/sys/sys/ptrace.h b/sys/sys/ptrace.h
index 3c8fda3..b8b76a0 100644
--- a/sys/sys/ptrace.h
+++ b/sys/sys/ptrace.h
@@ -116,7 +116,7 @@ int process_write_fpregs(struct proc *p, struct fpreg 
*regs);
 #endif
 intprocess_write_regs(struct proc *p, struct reg *regs);
 intprocess_checkioperm(struct proc *, struct process *);
-intprocess_domem(struct proc *, struct proc *, struct uio *, int);
+intprocess_domem(struct proc *, struct proc *, struct uio *);
 
 #ifndef FIX_SSTEP
 #define FIX_SSTEP(p)

Re: fputs(3) returns non-negative on success

[Todd C. Miller]

On Wed, 25 May 2016 14:46:24 -0600, "Todd C. Miller" wrote:

> Not necessarily zero.  Linux for example returns a non-zero value
> on success.  On failure it also sets the error indicator on the
> stream and sets errno.

We don't seem to document that stdio functions set the error indicator
on failure so it might be best to omit that bit for now.  It can
be added later in a full sweep of stdio.

 - todd

fputs(3) returns non-negative on success

[Todd C. Miller]

Not necessarily zero.  Linux for example returns a non-zero value
on success.  On failure it also sets the error indicator on the
stream and sets errno.

Mac OS X also have the following:

COMPATIBILITY
 fputs() now returns a non-negative number (as opposed to 0) on successful
 completion.  As a result, many tests (e.g., "fputs() == 0", "fputs() !=
 0") do not give the desired result.  Use "fputs() != EOF" or "fputs() ==
 EOF" to determine success or failure.

This is exactly what bit me so I wouldn't mind having a similar
note in our manual.

Thoughts?

 - todd

Index: lib/libc/stdio/fputs.3
===
RCS file: /cvs/src/lib/libc/stdio/fputs.3,v
retrieving revision 1.11
diff -u -p -u -r1.11 fputs.3
--- lib/libc/stdio/fputs.3  13 Jan 2015 14:02:30 -  1.11
+++ lib/libc/stdio/fputs.3  25 May 2016 20:39:34 -
@@ -60,15 +60,13 @@ and a terminating newline character,
 to the stream
 .Em stdout .
 .Sh RETURN VALUES
-The
-.Fn fputs
-function returns 0 on success and
+Upon successful completion a non-negative integer is returned.
+Otherwise,
 .Dv EOF
-on error;
-.Fn puts
-returns a non-negative integer on success and
-.Dv EOF
-on error.
+is returned, the global variable
+.Va errno
+is set to indicate the error,
+and the error indicator is set for the stream.
 .Sh ERRORS
 .Bl -tag -width Er
 .It Bq Er EBADF

Re: usbtv driver port (questions)

[patrick keshishian]

On Wed, May 25, 2016 at 01:57:36PM +0200, Marcus Glocker wrote:
> On Tue, May 24, 2016 at 04:46:57PM -0700, patrick keshishian wrote:
> 
> > Hi,
> > 
> > I have a "mostly working" driver port for Fushicai Audio-Video
> > Grabber (vendor 0x1b71 product 0x3002).
> > 
> > I've had the video bit working for a few days on amd64 and macppc.
> > I finally managed to get the audio bit working this morning on
> > amd64, but still an encoding issue (LE vs BE) on macppc.
> > 
> > I'd like to "polish up" the code for a post here, or off-list if
> > that is preferred, for review and ridicule. So I have some
> > questions which I don't think style(9) answers.
> > 
> > 1. Types: Original source used a few instances of "u16" types,
> >but I think OpenBSD prefers "u_int16_t" and "uint16_t".
> >Which to pick?
> 
> I vote for uint*_t.
>  
> > 2. Return values inside parenthesis or not?
> 
> The ongoing religious question :-)  Some people say return and sizeof
> shouldn't be used with parenthesis because they are no functions.  I
> still find it more readable with parenthesis.  We have both variations
> in the tree.  Your choice.
> 
> > 3. Original source filenames were hyphenated (e.g., usbtv-video.c).
> >OK to use underscore instead? I think it is preferred.
> >(Next question may trump this one)
> 
> How's about something a bit shorter, like uvcap.c (USB Video Capture)
> and then also call the driver uvcap?  Anyway just a proposal.  Otherwise
> yes, replace the '-' by '_' in the filename.
> 
> > 4. Original source has three .c and one .h file. They are dual-
> >licensed Linux sources: BSD and GPL.
> > 
> >I've borrowed from uvideo.c to get this driver working,
> >introducing a fourth .c.
> > 
> >I'm pretty sure OBSD doesn't want so many files introduced.
> >Should I combine the original .c files into one .c file, keep
> >the .h and the new .c file I introduced? Maintaining copyright
> >notices as-is (the original and one from uvideo.c).
> 
> For such a kind of driver I also would perfer one .c and one .h file.

Yes. Only concern I have is the slight variations in license
text. The original code is a two-clause BSD[0] while the uvideo.c
is the far simpler one.

> > 5. If tech@ is not the place to post an initial port effort,
> >especially from a first-timer, do let me know.
> 
> Well, I think tech@ is the right place.  Maybe we can take some stuff
> off-list.  Though myself has no such device yet to test further.

It is a low-end item, so I'm not sure if you really want one, but
would be glad to send one your way; just let me know where to ship
it to.

Also thanks for the suggestions!

--patrick

[0] https://github.com/torvalds/linux/tree/master/drivers/media/usb/usbtv

Re: usbtv driver port (questions)

[patrick keshishian]

On Wed, May 25, 2016 at 08:45:29AM -0400, Ian Darwin wrote:
> On 2016-05-25 7:57 AM, Marcus Glocker wrote:
> >On Tue, May 24, 2016 at 04:46:57PM -0700, patrick keshishian wrote:
> >
> >>Hi,
> >>
> >>I have a "mostly working" driver port for Fushicai Audio-Video
> >>Grabber (vendor 0x1b71 product 0x3002).
> >>
> >>I've had the video bit working for a few days on amd64 and macppc.
> >>I finally managed to get the audio bit working this morning on
> >>amd64, but still an encoding issue (LE vs BE) on macppc.
> >>
> >>...
> >>2. Return values inside parenthesis or not?
> >The ongoing religious question :-)  Some people say return and sizeof
> >shouldn't be used with parenthesis because they are not functions.  I
> >still find it more readable with parenthesis.  We have both variations
> >in the tree.  Your choice.
> "Obviously without" :-)
> >
> >>3. Original source filenames were hyphenated (e.g., usbtv-video.c).
> >>OK to use underscore instead? I think it is preferred.
> >>(Next question may trump this one)
> >How's about something a bit shorter, like uvcap.c (USB Video Capture)
> >and then also call the driver uvcap?  Anyway just a proposal.  Otherwise
> >yes, replace the '-' by '_' in the filename.
> >
> >
> There are (at least) three other popular usb video capture devices with
> totally different chipsets (Syntek, Empia, Somagic, besides this Fushicai
> one, "usbtv007"), so calling one of them uvcap might be annoying, like if
> one of our many network drivers were called "network". See
> https://linuxtv.org/wiki/index.php/Easycap.
> Does it make sense to start a naming convention like: utvsyn, utvemp,
> utvsom, utvfu, ...?

I like this scheme and utvfu.

Thanks,
--patrick

FYI - ftp5.usa.openbsd.org down tonight and tomorrow night

[Kurt Mosiejczuk]

ftp5 is hosted here at RIT and they are doing work on the substation that
supplies our electricity tonight (May 25th) and tomorrow night (May 26th).
The power goes out at 5pm EDT so I'm shutting everything down starting at
4pm.  Power comes back at 7am EDT tomorrow and Friday so it will be back up
by 8am EDT on Thursday and Friday.

Sorry for the late notice, I sent it to mirrors-discuss first and someone
pointed out I should send it here, but I've been running around getting
ready for the shutdown.

--Kurt Mosiejczuk

Re: [patch] Add percentage done to fsck_ffs SIGINFO output

[attila]

attila  writes:

> Hi tech@,
>
> The subject says it all.  I find myself doing this in my head as I hit
> ^T when fsck'ing.  FreeBSD has this, NetBSD does not, FWIW...
>

Whoops, that's wrong.  NetBSD does have it, it looks like FreeBSD in
this regard.  Sorry for the incorrect information.

> Feedback, comments most welcome.  Patch attached.  Tested lightly on
> i386 (after a hilarious few minutes trying to set up a scenario
> screwed up enough that fsck took long enough to sneak a ^T in there).
>
> Pax, -A

Pax, -A
--
http://haqistan.net/~attila | att...@stalphonsos.com | 0x62A729CF

[patch] Add percentage done to fsck_ffs SIGINFO output

[attila]

Hi tech@,

The subject says it all.  I find myself doing this in my head as I hit
^T when fsck'ing.  FreeBSD has this, NetBSD does not, FWIW...

Feedback, comments most welcome.  Patch attached.  Tested lightly on
i386 (after a hilarious few minutes trying to set up a scenario
screwed up enough that fsck took long enough to sneak a ^T in there).

Pax, -A
--
http://haqistan.net/~attila | att...@stalphonsos.com | 0x62A729CF
Index: pass1.c
===
RCS file: /cvs/src/sbin/fsck_ffs/pass1.c,v
retrieving revision 1.43
diff -u -p -r1.43 pass1.c
--- pass1.c	22 Aug 2015 06:00:27 -	1.43
+++ pass1.c	25 May 2016 17:19:47 -
@@ -55,8 +55,10 @@ static ino_t info_inumber;
 static int
 pass1_info(char *buf, size_t buflen)
 {
-	return (snprintf(buf, buflen, "phase 1, inode %llu/%llu",
+	return (snprintf(buf, buflen, "phase 1, inode %llu/%llu (%llu%%)",
 	(unsigned long long)info_inumber,
+	(unsigned long long)sblock.fs_ipg * sblock.fs_ncg,
+	(unsigned long long)info_inumber * 100 /
 	(unsigned long long)sblock.fs_ipg * sblock.fs_ncg) > 0);
 }
 
Index: pass1b.c
===
RCS file: /cvs/src/sbin/fsck_ffs/pass1b.c,v
retrieving revision 1.21
diff -u -p -r1.21 pass1b.c
--- pass1b.c	20 Jan 2015 18:22:21 -	1.21
+++ pass1b.c	25 May 2016 17:19:47 -
@@ -47,8 +47,10 @@ static ino_t info_inumber;
 static int
 pass1b_info(char *buf, size_t buflen)
 {
-	return (snprintf(buf, buflen, "phase 1b, inode %llu/%llu",
+	return (snprintf(buf, buflen, "phase 1b, inode %llu/%llu (%llu%%)",
 	(unsigned long long)info_inumber,
+	(unsigned long long)sblock.fs_ipg * sblock.fs_ncg,
+	(unsigned long long)info_inumber * 100 /
 	(unsigned long long)sblock.fs_ipg * sblock.fs_ncg) > 0);
 }
 
Index: pass2.c
===
RCS file: /cvs/src/sbin/fsck_ffs/pass2.c,v
retrieving revision 1.37
diff -u -p -r1.37 pass2.c
--- pass2.c	20 Jan 2015 18:22:21 -	1.37
+++ pass2.c	25 May 2016 17:19:47 -
@@ -56,8 +56,8 @@ static int info_pos;
 static int
 pass2_info1(char *buf, size_t buflen)
 {
-	return (snprintf(buf, buflen, "phase 2, directory %d/%d",
-	info_pos, info_max) > 0);
+	return (snprintf(buf, buflen, "phase 2, directory %d/%d (%d%%)",
+	info_pos, info_max, info_pos * 100 / info_max) > 0);
 }
 
 static int
Index: pass3.c
===
RCS file: /cvs/src/sbin/fsck_ffs/pass3.c,v
retrieving revision 1.18
diff -u -p -r1.18 pass3.c
--- pass3.c	20 Jan 2015 18:22:21 -	1.18
+++ pass3.c	25 May 2016 17:19:47 -
@@ -42,8 +42,8 @@ static int info_pos;
 static int
 pass3_info(char *buf, size_t buflen)
 {
-	return (snprintf(buf, buflen, "phase 3, directory %d/%ld",
-	info_pos, inplast) > 0);
+	return (snprintf(buf, buflen, "phase 3, directory %d/%ld (%ld%%)",
+	info_pos, inplast, info_pos * 100 / inplast) > 0);
 }
 
 void
Index: pass4.c
===
RCS file: /cvs/src/sbin/fsck_ffs/pass4.c,v
retrieving revision 1.24
diff -u -p -r1.24 pass4.c
--- pass4.c	20 Jan 2015 18:22:21 -	1.24
+++ pass4.c	25 May 2016 17:19:48 -
@@ -47,8 +47,10 @@ static ino_t info_inumber;
 static int
 pass4_info(char *buf, size_t buflen)
 {
-	return (snprintf(buf, buflen, "phase 4, inode %llu/%llu",
+	return (snprintf(buf, buflen, "phase 4, inode %llu/%llu (%llu%%)",
 	(unsigned long long)info_inumber,
+	(unsigned long long)lastino,
+	(unsigned long long)info_inumber * 100 /
 	(unsigned long long)lastino) > 0);
 }
 
Index: pass5.c
===
RCS file: /cvs/src/sbin/fsck_ffs/pass5.c,v
retrieving revision 1.48
diff -u -p -r1.48 pass5.c
--- pass5.c	20 Jan 2015 18:22:21 -	1.48
+++ pass5.c	25 May 2016 17:19:48 -
@@ -54,8 +54,8 @@ static int info_maxcg;
 static int
 pass5_info(char *buf, size_t buflen)
 {
-	return (snprintf(buf, buflen, "phase 5, cg %d/%d",
-	info_cg, info_maxcg) > 0);
+	return (snprintf(buf, buflen, "phase 5, cg %d/%d (%d%%)",
+	info_cg, info_maxcg, info_cg * 100 / info_maxcg) > 0);
 }
 
 void

Intel Wireless 8260 support for iwm(4)

[Stefan Sperling]

With the iwm(4) firmware update done, we can add support for 8260 devices.
These devices are currently showing up in new laptops.

The main differences over the 7000 series are the firmware file format,
firmware loading process, non-volatile memory on the chip, and,
unfortunately, yet another scan API.

The firmware is now signed by Intel and the signature is verified in
hardware. This setup is 100% FCC-approved. Regardless, the firmware
loading code tastes better than it smells since it was written under
the influence of French non-pasteurized cheese courtesy of miod@.

The new scan API uses a completely different set of commands and
sports more bells and whistles than we'll ever need.

This has already been tested by reyk@, robert@, Imre Vadasz, and
Bryan Vyhmeister. I'd like to let the firmware API changes committed earlier
today settle in for a few days before committing more code to the driver.
If you've got 8260 hardware to play with please test this in the meantime.

Emmanuel Grumbach provided helpful hints, and genua gmbh provided funding.

Index: if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.82
diff -u -p -r1.82 if_iwm.c
--- if_iwm.c25 May 2016 13:35:12 -  1.82
+++ if_iwm.c25 May 2016 14:05:56 -
@@ -168,6 +168,16 @@ const uint8_t iwm_nvm_channels[] = {
100, 104, 108, 112, 116, 120, 124, 128, 132, 136, 140, 144,
149, 153, 157, 161, 165
 };
+
+const uint8_t iwm_nvm_channels_8000[] = {
+   /* 2.4 GHz */
+   1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
+   /* 5 GHz */
+   36, 40, 44, 48, 52, 56, 60, 64, 68, 72, 76, 80, 84, 88, 92,
+   96, 100, 104, 108, 112, 116, 120, 124, 128, 132, 136, 140, 144,
+   149, 153, 157, 161, 165, 169, 173, 177, 181
+};
+
 #define IWM_NUM_2GHZ_CHANNELS  14
 
 const struct iwm_rate {
@@ -319,6 +329,8 @@ int iwm_parse_nvm_data(struct iwm_softc 
const uint16_t *, const uint16_t *,
const uint16_t *, const uint16_t *,
const uint16_t *);
+void   iwm_set_hw_address_8000(struct iwm_softc *, struct iwm_nvm_data *,
+   const uint16_t *, const uint16_t *);
 intiwm_parse_nvm_sections(struct iwm_softc *, struct iwm_nvm_section *);
 intiwm_nvm_init(struct iwm_softc *);
 intiwm_firmware_load_sect(struct iwm_softc *, uint32_t, const uint8_t *,
@@ -326,6 +338,9 @@ int iwm_firmware_load_sect(struct iwm_so
 intiwm_firmware_load_chunk(struct iwm_softc *, uint32_t, const uint8_t *,
uint32_t);
 intiwm_load_firmware_7000(struct iwm_softc *, enum iwm_ucode_type);
+intiwm_load_cpu_sections_8000(struct iwm_softc *, struct iwm_fw_sects *,
+   int , int *);
+intiwm_load_firmware_8000(struct iwm_softc *, enum iwm_ucode_type);
 intiwm_load_firmware(struct iwm_softc *, enum iwm_ucode_type);
 intiwm_start_fw(struct iwm_softc *, enum iwm_ucode_type);
 intiwm_send_tx_ant_cfg(struct iwm_softc *, uint8_t);
@@ -408,6 +423,8 @@ uint8_t iwm_mvm_lmac_scan_fill_channels(
struct iwm_scan_channel_cfg_lmac *, int);
 intiwm_mvm_fill_probe_req(struct iwm_softc *, struct iwm_scan_probe_req *);
 intiwm_mvm_lmac_scan(struct iwm_softc *);
+intiwm_mvm_config_umac_scan(struct iwm_softc *);
+intiwm_mvm_umac_scan(struct iwm_softc *);
 void   iwm_mvm_ack_rates(struct iwm_softc *, struct iwm_node *, int *, int *);
 void   iwm_mvm_mac_ctxt_cmd_common(struct iwm_softc *, struct iwm_node *,
struct iwm_mac_ctx_cmd *, uint32_t);
@@ -445,6 +462,7 @@ int iwm_ioctl(struct ifnet *, u_long, ca
 #ifdef IWM_DEBUG
 const char *iwm_desc_lookup(uint32_t);
 void   iwm_nic_error(struct iwm_softc *);
+void   iwm_nic_umac_error(struct iwm_softc *);
 #endif
 void   iwm_notif_intr(struct iwm_softc *);
 intiwm_intr(void *);
@@ -670,18 +688,19 @@ iwm_read_firmware(struct iwm_softc *sc, 
tlv_data, tlv_len)) != 0)
goto parse_out;
break;
-   case IWM_UCODE_TLV_NUM_OF_CPU:
+   case IWM_UCODE_TLV_NUM_OF_CPU: {
+   uint32_t num_cpu;
if (tlv_len != sizeof(uint32_t)) {
error = EINVAL;
goto parse_out;
}
-   if (le32toh(*(uint32_t*)tlv_data) != 1) {
-   DPRINTF(("%s: driver supports "
-   "only TLV_NUM_OF_CPU == 1", DEVNAME(sc)));
+   num_cpu = le32toh(*(uint32_t *)tlv_data);
+   if (num_cpu < 1 || num_cpu > 2) {
error = EINVAL;
goto parse_out;

Re: usbtv driver port (questions)

[Artturi Alm]

On Wed, May 25, 2016 at 08:45:29AM -0400, Ian Darwin wrote:
> On 2016-05-25 7:57 AM, Marcus Glocker wrote:
> > On Tue, May 24, 2016 at 04:46:57PM -0700, patrick keshishian wrote:
> > 
> > > Hi,
> > > 
> > > I have a "mostly working" driver port for Fushicai Audio-Video
> > > Grabber (vendor 0x1b71 product 0x3002).
> > > 
> > > I've had the video bit working for a few days on amd64 and macppc.
> > > I finally managed to get the audio bit working this morning on
> > > amd64, but still an encoding issue (LE vs BE) on macppc.
> > > 
> > > ...
> > > 2. Return values inside parenthesis or not?
> > The ongoing religious question :-)  Some people say return and sizeof
> > shouldn't be used with parenthesis because they are not functions.  I
> > still find it more readable with parenthesis.  We have both variations
> > in the tree.  Your choice.
> "Obviously without" :-)
> > 
> > > 3. Original source filenames were hyphenated (e.g., usbtv-video.c).
> > > OK to use underscore instead? I think it is preferred.
> > > (Next question may trump this one)
> > How's about something a bit shorter, like uvcap.c (USB Video Capture)
> > and then also call the driver uvcap?  Anyway just a proposal.  Otherwise
> > yes, replace the '-' by '_' in the filename.
> > 
> > 
> There are (at least) three other popular usb video capture devices with
> totally different chipsets (Syntek, Empia, Somagic, besides this Fushicai
> one, "usbtv007"), so calling one of them uvcap might be annoying, like if
> one of our many network drivers were called "network". See
> https://linuxtv.org/wiki/index.php/Easycap.
> Does it make sense to start a naming convention like: utvsyn, utvemp,
> utvsom, utvfu, ...?
> 
> Myself I own a Syntek one, which is why I'm aware of this issue:
>  port 4 addr 3: high speed, power 500 mA, config 1, USB 2.0 Video Capture
> Controller(0x0408), Syntek Semiconductor(0x05e1), rev 0.05
> 

fwiw. i still have a few "Somagic, Inc. SM-USB 007", for which i have planned
to revive a driver i came up with years ago, when iirc. usb stack was still
missing something needed to support it completely. would vote for utvsom,.. 

-Artturi

Re: usbtv driver port (questions)

[Ian Darwin]

On 2016-05-25 7:57 AM, Marcus Glocker wrote:

On Tue, May 24, 2016 at 04:46:57PM -0700, patrick keshishian wrote:


Hi,

I have a "mostly working" driver port for Fushicai Audio-Video
Grabber (vendor 0x1b71 product 0x3002).

I've had the video bit working for a few days on amd64 and macppc.
I finally managed to get the audio bit working this morning on
amd64, but still an encoding issue (LE vs BE) on macppc.

...
2. Return values inside parenthesis or not?

The ongoing religious question :-)  Some people say return and sizeof
shouldn't be used with parenthesis because they are not functions.  I
still find it more readable with parenthesis.  We have both variations
in the tree.  Your choice.

"Obviously without" :-)



3. Original source filenames were hyphenated (e.g., usbtv-video.c).
OK to use underscore instead? I think it is preferred.
(Next question may trump this one)

How's about something a bit shorter, like uvcap.c (USB Video Capture)
and then also call the driver uvcap?  Anyway just a proposal.  Otherwise
yes, replace the '-' by '_' in the filename.


There are (at least) three other popular usb video capture devices with 
totally different chipsets (Syntek, Empia, Somagic, besides this 
Fushicai one, "usbtv007"), so calling one of them uvcap might be 
annoying, like if one of our many network drivers were called "network". 
See https://linuxtv.org/wiki/index.php/Easycap.
Does it make sense to start a naming convention like: utvsyn, utvemp, 
utvsom, utvfu, ...?


Myself I own a Syntek one, which is why I'm aware of this issue:
 port 4 addr 3: high speed, power 500 mA, config 1, USB 2.0 Video 
Capture Controller(0x0408), Syntek Semiconductor(0x05e1), rev 0.05

Re: usbtv driver port (questions)

[Marcus Glocker]

On Tue, May 24, 2016 at 04:46:57PM -0700, patrick keshishian wrote:

> Hi,
> 
> I have a "mostly working" driver port for Fushicai Audio-Video
> Grabber (vendor 0x1b71 product 0x3002).
> 
> I've had the video bit working for a few days on amd64 and macppc.
> I finally managed to get the audio bit working this morning on
> amd64, but still an encoding issue (LE vs BE) on macppc.
> 
> I'd like to "polish up" the code for a post here, or off-list if
> that is preferred, for review and ridicule. So I have some
> questions which I don't think style(9) answers.
> 
> 1. Types: Original source used a few instances of "u16" types,
>but I think OpenBSD prefers "u_int16_t" and "uint16_t".
>Which to pick?

I vote for uint*_t.
 
> 2. Return values inside parenthesis or not?

The ongoing religious question :-)  Some people say return and sizeof
shouldn't be used with parenthesis because they are no functions.  I
still find it more readable with parenthesis.  We have both variations
in the tree.  Your choice.

> 3. Original source filenames were hyphenated (e.g., usbtv-video.c).
>OK to use underscore instead? I think it is preferred.
>(Next question may trump this one)

How's about something a bit shorter, like uvcap.c (USB Video Capture)
and then also call the driver uvcap?  Anyway just a proposal.  Otherwise
yes, replace the '-' by '_' in the filename.

> 4. Original source has three .c and one .h file. They are dual-
>licensed Linux sources: BSD and GPL.
> 
>I've borrowed from uvideo.c to get this driver working,
>introducing a fourth .c.
> 
>I'm pretty sure OBSD doesn't want so many files introduced.
>Should I combine the original .c files into one .c file, keep
>the .h and the new .c file I introduced? Maintaining copyright
>notices as-is (the original and one from uvideo.c).

For such a kind of driver I also would perfer one .c and one .h file.
 
> 5. If tech@ is not the place to post an initial port effort,
>especially from a first-timer, do let me know.

Well, I think tech@ is the right place.  Maybe we can take some stuff
off-list.  Though myself has no such device yet to test further.

smu(4) new style fans support

[Marcus Glocker]

Currently we just support fan commands to drive old style fans
with smu(4).  I got some reports from users with new style fans
getting wrong sysctl values therefore.

This diff also adds the commands for new style fans.  Some testing
would be welcome in the way of a) regression testing and b) see if
it fixes broken values on systems with new style fans.


Index: smu.c
===
RCS file: /cvs/src/sys/arch/macppc/dev/smu.c,v
retrieving revision 1.32
diff -u -p -u -p -r1.32 smu.c
--- smu.c   20 May 2016 21:56:00 -  1.32
+++ smu.c   25 May 2016 09:13:49 -
@@ -47,6 +47,7 @@ voidsmu_attach(struct device *, stru
 struct smu_fan {
struct thermal_fan fan;
u_int8_treg;
+   u_int8_told_style;
u_int16_t   min_rpm;
u_int16_t   max_rpm;
u_int16_t   unmanaged_rpm;
@@ -153,6 +154,7 @@ int smu_do_cmd(struct smu_softc *, int);
 intsmu_time_read(time_t *);
 intsmu_time_write(time_t);
 intsmu_get_datablock(struct smu_softc *sc, u_int8_t, u_int8_t *, size_t);
+void   smu_fan_check_style(struct smu_softc *, struct smu_fan *);
 intsmu_fan_set_rpm(struct smu_softc *, struct smu_fan *, u_int16_t);
 intsmu_fan_set_pwm(struct smu_softc *, struct smu_fan *, u_int16_t);
 intsmu_fan_read_rpm(struct smu_softc *, struct smu_fan *, u_int16_t *);
@@ -194,7 +196,7 @@ smu_attach(struct device *parent, struct
struct i2cbus_attach_args iba;
struct smu_fan *fan;
struct smu_sensor *sensor;
-   int nseg, node;
+   int nseg, node, i;
char type[32], loc[32];
u_int32_t reg, intr, gpio, val;
 #ifndef SMALL_KERNEL
@@ -393,6 +395,10 @@ smu_attach(struct device *parent, struct
return;
}
 
+   /* Check if we have old or new style fans */
+   for (i = 0; i < sc->sc_num_fans; i++)
+   smu_fan_check_style(sc, >sc_fans[i]);
+
 #ifndef SMALL_KERNEL
/* Sensors */
node = OF_getnodebyname(ca->ca_node, "sensors");
@@ -634,6 +640,25 @@ smu_get_datablock(struct smu_softc *sc, 
return (0);
 }
 
+void
+smu_fan_check_style(struct smu_softc *sc, struct smu_fan *fan)
+{
+   struct smu_cmd *cmd = (struct smu_cmd *)sc->sc_cmd;
+   int error;
+
+   /*
+* Check if we can read the fan rpm with the new style command.
+* If not we assume this is an old style fan.
+*/
+   cmd->cmd = SMU_FAN;
+   cmd->len = 2;
+   cmd->data[0] = 0x31;
+   cmd->data[1] = fan->reg;
+   error = smu_do_cmd(sc, 800);
+   if (error)
+   fan->old_style = 1;
+}
+
 int
 smu_fan_set_rpm(struct smu_softc *sc, struct smu_fan *fan, u_int16_t rpm)
 {
@@ -645,12 +670,21 @@ smu_fan_set_rpm(struct smu_softc *sc, st
 * the PowerMac8,1.  We simply store the value at both
 * locations.
 */
-   cmd->cmd = SMU_FAN;
-   cmd->len = 14;
-   cmd->data[0] = 0x00;/* fan-rpm-control */
-   cmd->data[1] = 0x01 << fan->reg;
-   cmd->data[2] = cmd->data[2 + fan->reg * 2] = (rpm >> 8) & 0xff;
-   cmd->data[3] = cmd->data[3 + fan->reg * 2] = (rpm & 0xff);
+   if (fan->old_style) {
+   cmd->cmd = SMU_FAN;
+   cmd->len = 14;
+   cmd->data[0] = 0x00;/* fan-rpm-control */
+   cmd->data[1] = 0x01 << fan->reg;
+   cmd->data[2] = cmd->data[2 + fan->reg * 2] = (rpm >> 8) & 0xff;
+   cmd->data[3] = cmd->data[3 + fan->reg * 2] = (rpm & 0xff);
+   } else {
+   cmd->cmd = SMU_FAN;
+   cmd->len = 4;
+   cmd->data[0] = 0x30;
+   cmd->data[1] = fan->reg;
+   cmd->data[2] = (rpm >> 8) & 0xff;
+   cmd->data[3] = rpm & 0xff;
+   }
return smu_do_cmd(sc, 800);
 }
 
@@ -659,12 +693,21 @@ smu_fan_set_pwm(struct smu_softc *sc, st
 {
struct smu_cmd *cmd = (struct smu_cmd *)sc->sc_cmd;
 
-   cmd->cmd = SMU_FAN;
-   cmd->len = 14;
-   cmd->data[0] = 0x10;/* fan-pwm-control */
-   cmd->data[1] = 0x01 << fan->reg;
-   cmd->data[2] = cmd->data[2 + fan->reg * 2] = (pwm >> 8) & 0xff;
-   cmd->data[3] = cmd->data[3 + fan->reg * 2] = (pwm & 0xff);
+   if (fan->old_style) {
+   cmd->cmd = SMU_FAN;
+   cmd->len = 14;
+   cmd->data[0] = 0x10;/* fan-pwm-control */
+   cmd->data[1] = 0x01 << fan->reg;
+   cmd->data[2] = cmd->data[2 + fan->reg * 2] = (pwm >> 8) & 0xff;
+   cmd->data[3] = cmd->data[3 + fan->reg * 2] = (pwm & 0xff);
+   } else {
+   cmd->cmd = SMU_FAN;
+   cmd->len = 4;
+   cmd->data[0] = 0x30;
+   cmd->data[1] = fan->reg;
+   cmd->data[2] = (pwm >> 8) & 0xff;
+   cmd->data[3] = pwm & 0xff;
+   }
return smu_do_cmd(sc, 800);
 }
 
@@ -674,13 +717,25 @@ smu_fan_read_rpm(struct