Re: opendev(3) tweak

[Sebastien Marie]

On Thu, Jun 09, 2016 at 09:19:30PM +0200, Theo Buehler wrote:
> On Tue, Mar 15, 2016 at 12:32:16PM -0600, Theo de Raadt wrote:
> > I am simply saying that pledge before opendev() makes no sense,
> > because opendev() does not gaurantee the type of descriptor it is
> > opening.
> 
> I noticed that this patch is still uncommitted since nobody ok'd it.
> Sorry about that. Freshly generated patch below.
> 
> ok tb@

ok semarie@ too

> $ ktrace fdisk /dev/tty
> Abort trap (core dumped)
> $ kdump | tail
>  28663 fdiskCALL  open(0x17b1f512f220,0)
>  28663 fdiskNAMI  "/dev/tty"
>  28663 fdiskRET   open 3
>  28663 fdiskCALL  fstat(3,0x7f7f07f0)
>  28663 fdiskSTRU  struct stat { dev=1040, ino=1280, mode=crw-rw-rw- , 
> nlink=1, uid=0<"root">, gid=0<"wheel">, rdev=256, atime=1465498384<"Jun  9 
> 20:53:04 2016">.697276353, mtime=1465498384<"Jun  9 20:53:04 
> 2016">.697276353, ctime=1465498384<"Jun  9 20:53:04 2016">.697276353, size=0, 
> blocks=0, blksize=65536, flags=0x0, gen=0x0 }
>  28663 fdiskRET   fstat 0
>  28663 fdiskCALL  ioctl(3,DIOCGPDINFO,0x17b1f5135160)
>  28663 fdiskPLDG  ioctl, "ioctl", errno 1 Operation not permitted
>  28663 fdiskPSIG  SIGABRT SIG_DFL code <-538976289>
>  28663 fdiskNAMI  "fdisk.core"
> 
> Index: fdisk.c
> ===
> RCS file: /var/cvs/src/sbin/fdisk/fdisk.c,v
> retrieving revision 1.100
> diff -u -p -r1.100 fdisk.c
> --- fdisk.c   28 Mar 2016 16:55:09 -  1.100
> +++ fdisk.c   28 Apr 2016 08:05:27 -
> @@ -85,10 +85,6 @@ main(int argc, char *argv[])
>   struct dos_mbr dos_mbr;
>   struct mbr mbr;
>  
> - /* "proc exec" for man page display */
> - if (pledge("stdio rpath wpath disklabel proc exec", NULL) == -1)
> - err(1, "pledge");
> -
>   while ((ch = getopt(argc, argv, "iegpuvf:c:h:s:l:b:y")) != -1) {
>   const char *errstr;
>  
> @@ -168,6 +164,10 @@ main(int argc, char *argv[])
>  
>   disk.name = argv[0];
>   DISK_open(i_flag || u_flag || e_flag);
> +
> + /* "proc exec" for man page display */
> + if (pledge("stdio rpath wpath disklabel proc exec", NULL) == -1)
> + err(1, "pledge");
>  
>   error = MBR_read(0, _mbr);
>   if (error)
> 
> 

-- 
Sebastien Marie

Re: cbfb: coreboot framebuffer console

[Ted Unangst]

joshua stein wrote:
> +int
> +cb_parse_table(paddr_t addr)
> +{
> + int i, j;
> +
> + for (i = 0; i < (4 * 1024); i += 16) {
> + struct cb_header *cbh;
> + struct cb_entry *cbe;
> + void *cbtable;
> +
> + cbh = (struct cb_header *)(PMAP_DIRECT_MAP(addr + i));
> + if (memcmp(cbh->signature, "LBIO", 4) != 0)
> + continue;
> +
> + if (!cbh->header_bytes)
> + continue;
> +
> + if (cb_checksum(cbh, sizeof(*cbh)) != 0)
> + return (-1);
> +
> + cbtable = (void *)PMAP_DIRECT_MAP(addr + i + cbh->header_bytes);
> +
> + for (j = 0; j < cbh->table_bytes; j += cbe->size) {
> + cbe = (struct cb_entry *)((char *)cbtable + j);

A small matter, but you can avoid a few casts here by making cbtable vaddr_t.
That is the type returned by PMAP_DIRECT_MAP and supports arithmetic.

Re: cbfb: coreboot framebuffer console

[Mark Kettenis]

> Date: Fri, 10 Jun 2016 14:38:05 -0500
> From: joshua stein 
> 
> This implements a new framebuffer console (based on efifb) for amd64
> devices running Coreboot, which do not have otherwise working EFI,
> VGA text, inteldrm, or serial console.  So mostly just Skylake
> Chromebooks.
> 
> This is required for my HP Chromebook 13 (and would have previously
> been required for the Broadwell Chromebook Pixel 2015) since SeaBIOS
> only handles printing text on Coreboot's framebuffer while at the
> bootloader.  Once the kernel loads and tries to find a vga device,
> it would fail and print nothing on the screen.  With this driver I
> get a fully working console and working X11 via the wsfb driver.
> 
> I've tested this on a couple non-Chromebook machines, including my
> APU which is also using Coreboot, but since that has a serial
> console, this driver does not try to attach.

The logic to decide whether this should attach looks good to me.

Mst stuff in here is duplicated from efifb though, so I wonder if code
could be shared.

> Index: dev/wscons/wsconsio.h
> ===
> RCS file: /cvs/src/sys/dev/wscons/wsconsio.h,v
> retrieving revision 1.74
> diff -u -p -u -p -r1.74 wsconsio.h
> --- dev/wscons/wsconsio.h 30 Mar 2016 23:34:12 -  1.74
> +++ dev/wscons/wsconsio.h 10 Jun 2016 17:35:38 -
> @@ -348,6 +348,7 @@ struct wsmouse_calibcoords {
>  #define  WSDISPLAY_TYPE_INTELDRM 69  /* Intel KMS 
> framebuffer */
>  #define  WSDISPLAY_TYPE_RADEONDRM 70 /* ATI Radeon KMS 
> framebuffer */
>  #define  WSDISPLAY_TYPE_EFIFB71  /* EFI framebuffer */
> +#define  WSDISPLAY_TYPE_CBFB 72  /* Coreboot framebuffer 
> */
>  
>  /* Basic display information.  Not applicable to all display types. */
>  struct wsdisplay_fbinfo {
> Index: arch/amd64/amd64/cbfb.c
> ===
> RCS file: arch/amd64/amd64/cbfb.c
> diff -N arch/amd64/amd64/cbfb.c
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ arch/amd64/amd64/cbfb.c   10 Jun 2016 17:35:38 -
> @@ -0,0 +1,447 @@
> +/*   $OpenBSD$ */
> +
> +/*
> + * Coreboot framebuffer console driver
> + *
> + * Copyright (c) 2016 joshua stein 
> + * Copyright (c) 2015 YASUOKA Masahiko 
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/* coreboot tables */
> +
> +struct cb_header {
> + union {
> + uint8_t signature[4]; /* "LBIO" */
> + uint32_t signature32;
> + };
> + uint32_theader_bytes;
> + uint32_theader_checksum;
> + uint32_ttable_bytes;
> + uint32_ttable_checksum;
> + uint32_ttable_entries;
> +};
> +
> +struct cb_framebuffer {
> + uint64_tphysical_address;
> + uint32_tx_resolution;
> + uint32_ty_resolution;
> + uint32_tbytes_per_line;
> + uint8_t bits_per_pixel;
> + uint8_t red_mask_pos;
> + uint8_t red_mask_size;
> + uint8_t green_mask_pos;
> + uint8_t green_mask_size;
> + uint8_t blue_mask_pos;
> + uint8_t blue_mask_size;
> + uint8_t reserved_mask_pos;
> + uint8_t reserved_mask_size;
> +};
> +
> +struct cb_entry {
> + uint32_ttag;
> +#define CB_TAG_VERSION  0x0004
> +#define CB_TAG_FORWARD  0x0011
> +#define CB_TAG_FRAMEBUFFER  0x0012
> + uint32_tsize;
> + union {
> + charstring[0];
> + uint64_t forward;
> + struct cb_framebuffer fb;
> + } u;
> +};
> +
> +struct cbfb {
> + struct rasops_info rinfo;
> + int  depth;
> + paddr_t  paddr;
> + psize_t  psize;
> +
> + struct cb_framebuffertable_cbfb;
> +};
> +
> +struct cbfb_softc {
> + struct devicesc_dev;
> + struct cbfb *sc_fb;
> +};
> +
> +int   cbfb_match(struct device *, void *, void *);
> +void  cbfb_attach(struct device *, struct 

Re: MBIM Patch (Round 3)

[Mark Kettenis]

> Date: Fri, 10 Jun 2016 17:20:18 +0100
> From: Stuart Henderson 
> 
> On 2016/06/10 16:05, Mark Kettenis wrote:
> > In any case this is something we can figure out once the code hits the
> > tree.  Unless mpi@ is still unhappy with the way the driver performs
> > ioctls, I think we should get the driver bits into the tree such that
> > we can work on fixing the remaining issues with it.
> 
> Agreed.
> 
> Did people notice the uhub.c part? Any concerns with that?

Yes, that is a separate issue as well.  And should therefore be a
separate commit.  It doesn't seem unreasonable to me to retry once
before giving up.  But this is another area where mpi@ should have the
final say.

The printf doesn't trigger on my Sierra Wireless EM7345 (which really
is an Intel XMM 7160 in disguise).

Fix typos in comments in amd64/stand

[Tom Cosgrove]

Thanks

Tom


Index: sys/arch/amd64/stand/efiboot/efiboot.c
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/amd64/stand/efiboot/efiboot.c,v
retrieving revision 1.12
diff -u -p -u -r1.12 efiboot.c
--- sys/arch/amd64/stand/efiboot/efiboot.c  15 May 2016 22:48:02 -  
1.12
+++ sys/arch/amd64/stand/efiboot/efiboot.c  10 Jun 2016 18:14:14 -
@@ -466,7 +466,7 @@ efi_cons_getshifts(dev_t dev)
return (0);
 }
 
-/* XXX: serial console is not supporte yet */
+/* XXX: serial console is not supported yet */
 int comspeed = 9600;
 int com_addr = -1;
 int com_speed = -1;
@@ -477,7 +477,7 @@ int com_speed = -1;
 /*
  * ACPI GUID is confusing in UEFI spec.
  * {EFI_,}_ACPI_20_TABLE_GUID or EFI_ACPI_TABLE_GUID means
- * ACPI 2.0 or abobe.
+ * ACPI 2.0 or above.
  */
 static EFI_GUID acpi_guid = ACPI_20_TABLE_GUID;
 static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
Index: sys/arch/amd64/stand/libsa/cmd_i386.c
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/amd64/stand/libsa/cmd_i386.c,v
retrieving revision 1.9
diff -u -p -u -r1.9 cmd_i386.c
--- sys/arch/amd64/stand/libsa/cmd_i386.c   8 Nov 2015 00:42:39 -   
1.9
+++ sys/arch/amd64/stand/libsa/cmd_i386.c   10 Jun 2016 18:14:14 -
@@ -183,7 +183,7 @@ Xmemory(void)
p++;
}
 
-   /* Handle (possibly non-existant) address part */
+   /* Handle (possibly non-existent) address part */
switch (*p) {
case '@':
addr = strtoll(p + 1, NULL, 0);
Index: sys/arch/amd64/stand/libsa/memprobe.c
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/amd64/stand/libsa/memprobe.c,v
retrieving revision 1.16
diff -u -p -u -r1.16 memprobe.c
--- sys/arch/amd64/stand/libsa/memprobe.c   8 Oct 2015 14:46:05 -   
1.16
+++ sys/arch/amd64/stand/libsa/memprobe.c   10 Jun 2016 18:14:14 -
@@ -281,7 +281,7 @@ memprobe(void)
 * Compute compatibility values:
 * cnvmem -- is the upper boundary of conventional
 *  memory (below IOM_BEGIN (=640k))
-* extmem -- is the size of the contignous extended
+* extmem -- is the size of the contiguous extended
 *  memory segment starting at 1M
 *
 * We ignore "good" memory in the 640K-1M hole.
@@ -346,7 +346,7 @@ mem_limit(long long ml)
if (p->type != BIOS_MAP_FREE)
continue;
 
-   /* Wholy above limit, nuke it */
+   /* Wholly above limit, nuke it */
if ((sp >= ml) && (ep >= ml)) {
bcopy (p + 1, p, (char *)bios_memmap +
   sizeof(bios_memmap) - (char *)p);
Index: sys/arch/amd64/stand/libsa/pxe.h
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/amd64/stand/libsa/pxe.h,v
retrieving revision 1.6
diff -u -p -u -r1.6 pxe.h
--- sys/arch/amd64/stand/libsa/pxe.h2 Mar 2011 07:15:45 -   1.6
+++ sys/arch/amd64/stand/libsa/pxe.h10 Jun 2016 18:14:14 -
@@ -346,7 +346,7 @@ typedef struct {
 #  define PXENV_UNDI_ISR_OUT_NOT_OUTS  1
 
/*
-* one of these will bre returnd for PXEND_UNDI_ISR_IN_PROCESS
+* one of these will be returned for PXEND_UNDI_ISR_IN_PROCESS
 * and PXENV_UNDI_ISR_IN_GET_NEXT
 */
 #  define PXENV_UNDI_ISR_OUT_DONE  0

Fix typos in comments in i386/stand

[Tom Cosgrove]

Thanks

Tom


Index: sys/arch/i386/stand/libsa/apmprobe.c
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/i386/stand/libsa/apmprobe.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 apmprobe.c
--- sys/arch/i386/stand/libsa/apmprobe.c8 Jul 2014 13:31:30 -   
1.18
+++ sys/arch/i386/stand/libsa/apmprobe.c10 Jun 2016 18:06:23 -
@@ -27,7 +27,7 @@
  */
 /*
  * APM derived from: apm_init.S, LP (Laptop Package)
- * wich contained this:
+ * which contained this:
  * Copyright (C) 1994 by HOSOKAWA, Tatsumi 
  *
  */
Index: sys/arch/i386/stand/libsa/cmd_i386.c
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/i386/stand/libsa/cmd_i386.c,v
retrieving revision 1.35
diff -u -p -u -r1.35 cmd_i386.c
--- sys/arch/i386/stand/libsa/cmd_i386.c18 Sep 2015 13:30:56 -  
1.35
+++ sys/arch/i386/stand/libsa/cmd_i386.c10 Jun 2016 18:06:23 -
@@ -179,7 +179,7 @@ Xmemory(void)
p++;
}
 
-   /* Handle (possibly non-existant) address part */
+   /* Handle (possibly non-existent) address part */
switch (*p) {
case '@':
addr = strtoll(p + 1, NULL, 0);
Index: sys/arch/i386/stand/libsa/memprobe.c
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/i386/stand/libsa/memprobe.c,v
retrieving revision 1.56
diff -u -p -u -r1.56 memprobe.c
--- sys/arch/i386/stand/libsa/memprobe.c8 Oct 2015 14:46:05 -   
1.56
+++ sys/arch/i386/stand/libsa/memprobe.c10 Jun 2016 18:06:23 -
@@ -286,7 +286,7 @@ memprobe(void)
 * Compute compatibility values:
 * cnvmem -- is the upper boundary of conventional
 *  memory (below IOM_BEGIN (=640k))
-* extmem -- is the size of the contignous extended
+* extmem -- is the size of the contiguous extended
 *  memory segment starting at 1M
 *
 * We ignore "good" memory in the 640K-1M hole.
@@ -338,7 +338,7 @@ mem_limit(long long ml)
if (p->type != BIOS_MAP_FREE)
continue;
 
-   /* Wholy above limit, nuke it */
+   /* Wholly above limit, nuke it */
if ((sp >= ml) && (ep >= ml)) {
bcopy (p + 1, p, (char *)bios_memmap +
   sizeof(bios_memmap) - (char *)p);
Index: sys/arch/i386/stand/libsa/pxe.h
===
RCS file: /home/OpenBSD/cvs/src/sys/arch/i386/stand/libsa/pxe.h,v
retrieving revision 1.6
diff -u -p -u -r1.6 pxe.h
--- sys/arch/i386/stand/libsa/pxe.h 2 Mar 2011 07:15:45 -   1.6
+++ sys/arch/i386/stand/libsa/pxe.h 10 Jun 2016 18:06:23 -
@@ -346,7 +346,7 @@ typedef struct {
 #  define PXENV_UNDI_ISR_OUT_NOT_OUTS  1
 
/*
-* one of these will bre returnd for PXEND_UNDI_ISR_IN_PROCESS
+* one of these will be returned for PXEND_UNDI_ISR_IN_PROCESS
 * and PXENV_UNDI_ISR_IN_GET_NEXT
 */
 #  define PXENV_UNDI_ISR_OUT_DONE  0

Re: MBIM Patch (Round 3)

[Stuart Henderson]

On 2016/06/10 16:05, Mark Kettenis wrote:
> In any case this is something we can figure out once the code hits the
> tree.  Unless mpi@ is still unhappy with the way the driver performs
> ioctls, I think we should get the driver bits into the tree such that
> we can work on fixing the remaining issues with it.

Agreed.

Did people notice the uhub.c part? Any concerns with that?

> The ifconfig bits seem to be a bit more controversial.  And we
> defenitely need to test those in a RAMDISK context as well.

yes.

Re: MBIM Patch (Round 3)

[Stuart Henderson]

I found 
https://lists.freedesktop.org/archives/libmbim-devel/2016-April/000704.html
which suggests that the Lenovo firmware build of the 7455 is definitely one
that needs the fccauth command.

I don't think fccauth or device support are things that should stop the driver
going in though. It doesn't matter if it works everywhere.

Re: tetris(6): -Wshadow cleanup

[Todd C. Miller]

On Fri, 10 Jun 2016 16:32:43 +0200, Theo Buehler wrote:

> Get rid of the two gcc -Wshadow warnings:
> 
> numnames is public in 
> i shadows the local loop indexing variable of scr_update, so no need to
> rename it.

OK millert@

 - todd

tetris(6): -Wshadow cleanup

[Theo Buehler]

Get rid of the two gcc -Wshadow warnings:

numnames is public in 
i shadows the local loop indexing variable of scr_update, so no need to
rename it.

Index: scores.c
===
RCS file: /var/cvs/src/games/tetris/scores.c,v
retrieving revision 1.20
diff -u -p -r1.20 scores.c
--- scores.c16 Mar 2016 15:00:35 -  1.20
+++ scores.c10 Jun 2016 14:21:50 -
@@ -264,7 +264,7 @@ static int
 checkscores(struct highscore *hs, int num)
 {
struct highscore *sp;
-   int i, j, k, numnames;
+   int i, j, k, nrnames;
int levelfound[NLEVELS];
struct peruser {
char *name;
@@ -281,21 +281,21 @@ checkscores(struct highscore *hs, int nu
qsort((void *)hs, nscores, sizeof(*hs), cmpscores);
for (i = MINLEVEL; i < NLEVELS; i++)
levelfound[i] = 0;
-   numnames = 0;
+   nrnames = 0;
for (i = 0, sp = hs; i < num;) {
/*
 * This is O(n^2), but do you think we care?
 */
-   for (j = 0, pu = count; j < numnames; j++, pu++)
+   for (j = 0, pu = count; j < nrnames; j++, pu++)
if (strcmp(sp->hs_name, pu->name) == 0)
break;
-   if (j == numnames) {
+   if (j == nrnames) {
/*
 * Add new user, set per-user count to 1.
 */
pu->name = sp->hs_name;
pu->times = 1;
-   numnames++;
+   nrnames++;
} else {
/*
 * Two ways to keep this score:
Index: screen.c
===
RCS file: /var/cvs/src/games/tetris/screen.c,v
retrieving revision 1.16
diff -u -p -r1.16 screen.c
--- screen.c4 Jan 2016 17:33:24 -   1.16
+++ screen.c10 Jun 2016 14:21:50 -
@@ -389,7 +389,6 @@ scr_update(void)
 
/* draw preview of next pattern */
if (showpreview && (nextshape != lastshape)) {
-   int i;
static int r=5, c=2;
int tr, tc, t;
 

Re: MBIM Patch (Round 3)

[Mark Kettenis]

> From: Gerhard Roth 
> Date: Fri, 10 Jun 2016 00:31:44 +0200
> 
> On 10.06.2016 00:22, Stuart Henderson wrote:
> > On 2016/06/10 00:10, Mark Kettenis wrote:
> >>> From: Gerhard Roth 
> >>> Date: Thu, 9 Jun 2016 23:48:23 +0200
> >>>
> >>> On 09.06.2016 23:42, Mark Kettenis wrote:
> > Date: Thu, 9 Jun 2016 22:59:28 +0200 (CEST)
> > From: Mark Kettenis 
> >
> >> Date: Wed, 8 Jun 2016 15:08:52 +0200
> >> From: Gerhard Roth 
> >>
> >> I would be glad to hear from some people trying this with a real MBIM
> >> device.
> >
> > Sierra Wireless EM7345 4G LTE here.  This devices currently attached
> > as umodem(4).  But I did add its vendor id and device id to umb_devs,
> > which makes it partially attach:
> >
> > umb0 at uhub0 port 4 "Sierra Wireless Inc. Sierra Wireless EM7345 4G 
> > LTE" rev 2.00/17.29 addr 2
> > umb0: switching to config #1
> > umb0: ignoring invalid segment size 1500
> > umb0: ctrl_len=512, maxpktlen=8192, cap=0x4
> > umb0: no control interface found
> >
> > (this is with UMB_DEBUG enabled)
> >
> > It seems this device needs some additional poking to select alternate
> > interface settings.
> 
>  With the appropriate alternate settings for the communication
>  interface and data interface (1 and 2) hardcoded in the driver, this
>  works!
> >>>
> >>> Great!
> >>>
> >>> Although another example of a device violating the MBIM spec which
> >>> clearly states that alternate settings 0 and 1 have to be used.
> >>
> >> Which version of the spec are you looking at?  Because my copy
> >> (Revision 1.0 Errata-1) clearly status alternate settings 1 and 2 have
> >> to be used ;).
> >
> > There are different sections, 3.2 for dual-mode (NCM1.0 / MBIM)
> > devices talks about various alternate settings for NCM1.0 and
> > for MBIM, then 6.6 which is only dealing with MBIM just talks
> > about 0 and 1 ..
> >
> 
> That's how I looked at it, too. But it seems that some vendors look
> at it differently ;)
> 
> So we should find a solution where we don't need to use the
> currently hard-coded MBIM_INTERFACE_ALTSETTING (== 1) anymore.

I don't think that is too difficult.  The alternate settings are
encoded in the bAlternateSetting of the Interface Descriptor.  So we
just need to remmeber those when we iterate over the full set of
interface descriptors at the start of umb_attach().

In any case this is something we can figure out once the code hits the
tree.  Unless mpi@ is still unhappy with the way the driver performs
ioctls, I think we should get the driver bits into the tree such that
we can work on fixing the remaining issues with it.

The ifconfig bits seem to be a bit more controversial.  And we
defenitely need to test those in a RAMDISK context as well.

Re: MBIM Patch (Round 3)

[Bryan Vyhmeister]

On Fri, Jun 10, 2016 at 09:43:36AM +0200, Gerhard Roth wrote:
> Hmm, I don't see the missing break. It is still stuck in the same
> state trying to turn on the radio and always getting non-confirmative
> resonses.
> 
> If the break before "case UMB_S_RADIO:" is missing, I would expect
> to see a debug printf "umb0: init: checking SIM state ...".
> Could you please check once again?

>From a previous email you said to comment out the break after "case
UMB_S_RADIO" which did not have any effect. Commenting out the break
after "case UMB_S_OPEN" as I believe you are saying above did have an
effect.


case UMB_S_OPEN:
DPRINTF("%s: init: turning radio on ...\n", DEVNAM(sc));
umb_radio(sc, 1);
// break;
case UMB_S_RADIO:
DPRINTF("%s: init: checking SIM state ...\n", DEVNAM(sc));
umb_cmd(sc, MBIM_CID_SUBSCRIBER_READY_STATUS, MBIM_CMDOP_QRY,
NULL, 0);
break;
case UMB_S_SIMREADY:
DPRINTF("%s: init: attaching ...\n", DEVNAM(sc));
umb_packet_service(sc, 1);
break;

> The error code is rather generic and gives us no help to
> determine why exactly the device refuses to turn on the
> radio. But it reports that the hw-state is on, which means
> we can at least exclude any rfkill switch ;)
> 
> So why does the firmware refuse to turn it on? Could still
> be a problem of the PIN-less SIM card you're using.

With the break commented out above I now see this output from ifconfig
umb0 (sensitive info with XXX):

umb0: flags=8811 mtu 1500
index 5 priority 0
roaming disabled registration not registered
state SIM is ready cell-class none
SIM initialized PIN valid (3 attempts left)
subscriber-id  ICC-id XX
device EM7455 IMEI 014582000 firmware SWI9X30C_02.08.
phone# ++XX APN broadband
status: down

I have not seen the first ten digits of the SIM card's phone number, the
subscriber-id, or the ICC-id in ifconfig output before so that seems
promising. Could this be something like needing the FCC auth? The debug
output follows. Thank you!

Bryan



OpenBSD 6.0-beta (GENERIC.MP) #1: Fri Jun 10 04:25:48 PDT 2016
r...@x.brycv.com:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 17024274432 (16235MB)
avail mem = 16503758848 (15739MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.8 @ 0xb7c01000 (66 entries)
bios0: vendor LENOVO version "R02ET44W (1.17 )" date 01/25/2016
bios0: LENOVO 20F6CTO1WW
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP UEFI SSDT SSDT ECDT HPET APIC MCFG SSDT SSDT DBGP DBG2 
BOOT BATB SSDT SSDT MSDM DMAR ASF! FPDT UEFI
acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) PXSX(S4) PXSX(S4) PXSX(S4) 
PXSX(S4) EXP8(S4) PXSX(S4) XHCI(S3)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiec0 at acpi0
acpihpet0 at acpi0: 2399 Hz
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz, 2485.32 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,SGX,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SENSOR,ARAT
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges
cpu0: apic clock running at 24MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE
cpu1 at mainbus0: apid 2 (application processor)
cpu1: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz, 2494.21 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,SGX,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SENSOR,ARAT
cpu1: 256KB 64b/line 8-way L2 cache
cpu1: smt 0, core 1, package 0
cpu2 at mainbus0: apid 1 (application processor)
cpu2: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz, 2494.21 MHz
cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,SGX,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SENSOR,ARAT
cpu2: 256KB 64b/line 8-way L2 

Re: httpd(8) fix incorrect comment

[Florian Obser]

On Tue, Jun 07, 2016 at 12:18:48PM +0200, Frank Schoep wrote:
> Came across an incorrect comment in httpd(8) explaining memory
> allocation. Comment claims that 5 times the source memory needs to
> be allocated if source consists solely of "<" and ">", but those
> characters expand to four bytes ("&[g/l]t;"). "&" is the reason that
> 5 times the memory is required ("");

Fixed (but kept a one line comment), thanks!

> 
> 
> Index: httpd.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/httpd.c,v
> retrieving revision 1.55
> diff -u -p -r1.55 httpd.c
> --- httpd.c   22 May 2016 19:19:21 -  1.55
> +++ httpd.c   7 Jun 2016 09:18:47 -
> @@ -744,7 +744,10 @@ escape_html(const char* src)
> {
>   char*dp, *dst;
> 
> - /* We need 5 times the memory if every letter is "<" or ">". */
> + /*
> +  * We need 5 times the memory if every source character is
> +  * "&" (escaped to "").
> +  */
>   if ((dst = calloc(5, strlen(src) + 1)) == NULL)
>   return NULL;
> 
> 

-- 
I'm not entirely sure you are real.

iwm(4): Add missing iwm_free_rx_ring(). Free nvm_sections[i].data buffers.

[Imre Vadasz]

Hi,

After parsing the nvm_sections data in iwm_nvm_init(), we can free the
nvm_sections[i].data allocations again.

In the iwm_attach() failure path, the iwm_free_rx_ring() call for freeing
the rx ring memory was missing.

Index: if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.86
diff -u -r1.86 if_iwm.c
--- if_iwm.c3 Jun 2016 16:16:25 -   1.86
+++ if_iwm.c10 Jun 2016 11:47:29 -
@@ -3085,10 +3085,16 @@
nvm_sections[section].length = len;
}
free(buf, M_DEVBUF, bufsz);
-   if (error)
-   return error;
+   if (error == 0)
+   error = iwm_parse_nvm_sections(sc, nvm_sections);
 
-   return iwm_parse_nvm_sections(sc, nvm_sections);
+   for (i = 0; i < IWM_NVM_NUM_OF_SECTIONS; i++) {
+   if (nvm_sections[i].data != NULL)
+   free(nvm_sections[i].data, M_DEVBUF,
+   nvm_sections[i].length);
+   }
+
+   return error;
 }
 
 /*
@@ -8055,6 +8061,7 @@
/* Free allocated memory if something failed during attachment. */
 fail4: while (--txq_i >= 0)
iwm_free_tx_ring(sc, >txq[txq_i]);
+   iwm_free_rx_ring(sc, >rxq);
iwm_free_sched(sc);
 fail3: if (sc->ict_dma.vaddr != NULL)
iwm_free_ict(sc);

Re: `rt_addr' or the end of `rt_ifa'

[Florian Obser]

Any reason you are not adding the free(9) sizes in rtrequest(), too?

OK florian@

On Wed, Jun 08, 2016 at 04:23:33PM +0200, Martin Pieuchot wrote:
> Being able to remove the requirement of an configured address for every
> route entry would have multiple benefit:
> 
>   . We could add route before the interface gets an address (useful in
> some p2p configurations)
>   . The kernel wouldn't have to manage stale ifas
>   . The network data structures would be less coupled, making it easier
> to write SMP safe code (yes I found new bugs!)
> 
> In order to achieve such goal we should preserve the "source routing"
> feature.  Which mean that route entry MUST still carry an address.
> 
> So the diff below adds a `rt_addr' field that will for the moment be a
> copy of `rt_ifa->ifa_addr'.  Once all the `rt_ifa' occurrences are
> converted to `rt_addr' we can relax the logic for adding a route.
> 
> This is the first of 14 steps.
> 
> Index: net/route.c
> ===
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.307
> diff -u -p -r1.307 route.c
> --- net/route.c   8 Jun 2016 13:26:06 -   1.307
> +++ net/route.c   8 Jun 2016 14:19:48 -
> @@ -139,6 +139,8 @@
>  #include 
>  #endif
>  
> +#define ROUNDUP(a) (a>0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : 
> sizeof(long))
> +
>  /* Give some jitter to hash, to avoid synchronization between routers. */
>  static uint32_t  rt_hashjitter;
>  
> @@ -151,6 +153,7 @@ struct pool   rtentry_pool;   /* pool for r
>  struct pool  rttimer_pool;   /* pool for rttimer structures */
>  
>  void rt_timer_init(void);
> +int  rt_setaddr(struct rtentry *, struct sockaddr *);
>  int  rtflushclone1(struct rtentry *, void *, u_int);
>  void rtflushclone(unsigned int, struct rtentry *);
>  int  rt_if_remove_rtdelete(struct rtentry *, void *, u_int);
> @@ -434,7 +437,6 @@ rtref(struct rtentry *rt)
>  void
>  rtfree(struct rtentry *rt)
>  {
> - struct ifaddr   *ifa;
>   int  refcnt;
>  
>   if (rt == NULL)
> @@ -452,16 +454,14 @@ rtfree(struct rtentry *rt)
>  
>   KERNEL_LOCK();
>   rt_timer_remove_all(rt);
> - ifa = rt->rt_ifa;
> - if (ifa)
> - ifafree(ifa);
> + ifafree(rt->rt_ifa);
>   rtlabel_unref(rt->rt_labelid);
>  #ifdef MPLS
>   if (rt->rt_flags & RTF_MPLS)
>   free(rt->rt_llinfo, M_TEMP, sizeof(struct rt_mpls));
>  #endif
> - if (rt->rt_gateway)
> - free(rt->rt_gateway, M_RTABLE, 0);
> + free(rt->rt_addr, M_RTABLE, ROUNDUP(rt->rt_addr->sa_len));
> + free(rt->rt_gateway, M_RTABLE, ROUNDUP(rt->rt_gateway->sa_len));
>   free(rt_key(rt), M_RTABLE, rt_key(rt)->sa_len);
>   KERNEL_UNLOCK();
>  
> @@ -486,7 +486,7 @@ rt_sendmsg(struct rtentry *rt, int cmd, 
>   ifp = if_get(rt->rt_ifidx);
>   if (ifp != NULL) {
>   info.rti_info[RTAX_IFP] = sdltosa(ifp->if_sadl);
> - info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
> + info.rti_info[RTAX_IFA] = rt->rt_addr;
>   }
>  
>   rt_missmsg(cmd, , rt->rt_flags, rt->rt_priority, rt->rt_ifidx, 0,
> @@ -767,8 +767,6 @@ ifa_ifwithroute(int flags, struct sockad
>   return (ifa);
>  }
>  
> -#define ROUNDUP(a) (a>0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : 
> sizeof(long))
> -
>  int
>  rt_getifa(struct rt_addrinfo *info, u_int rtid)
>  {
> @@ -958,7 +956,7 @@ rtrequest(int req, struct rt_addrinfo *i
>* will get the new address and interface later.
>*/
>   info->rti_ifa = NULL;
> - info->rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
> + info->rti_info[RTAX_IFA] = rt->rt_addr;
>   }
>  
>   info->rti_flags = rt->rt_flags | (RTF_CLONED|RTF_HOST);
> @@ -1090,10 +1088,12 @@ rtrequest(int req, struct rt_addrinfo *i
>* the routing table because the radix MPATH code use
>* it to (re)order routes.
>*/
> - if ((error = rt_setgate(rt, info->rti_info[RTAX_GATEWAY]))) {
> + if ((error = rt_setaddr(rt, ifa->ifa_addr)) ||
> + (error = rt_setgate(rt, info->rti_info[RTAX_GATEWAY]))) {
>   ifafree(ifa);
>   rtfree(rt->rt_parent);
>   rtfree(rt->rt_gwroute);
> + free(rt->rt_addr, M_RTABLE, 0);
>   free(rt->rt_gateway, M_RTABLE, 0);
>   free(ndst, M_RTABLE, dlen);
>   pool_put(_pool, rt);
> @@ -1124,6 +1124,7 @@ rtrequest(int req, struct rt_addrinfo *i
>   ifafree(ifa);
>   rtfree(rt->rt_parent);
>   rtfree(rt->rt_gwroute);
> + 

Re: tetris(6): clear row zero when eliding a row

[Theo Buehler]

On Fri, Jun 10, 2016 at 06:00:48AM +0200, Theo Buehler wrote:
> The tetris playing field has an extra row on top of the visible rows.
> It is possible and not an error to place a tile in such a way that it
> occupies a square of this invisible row (the game ends when the next
> tile starts out on an already occupied square).
> 
> The problem with this is that there's a bug in the function eliding
> rows: it never clears the extra row.
> 
> This means that once a square in the extra row is occupied, it remains
> occupied.  In other words, the column becomes unusable for game play.
> This is helpful if one of the two columns at the border of the playing
> field is concerned (you will be able to can clear future rows quicker)
> and deadly otherwise (you soon won't be able to clear rows anymore).
> 
> So let's clear that extra row whenever a row is elided.

I just noticed that christos@netbsd made the exact same change almost
exactly a year ago:
http://cvsweb.netbsd.org/bsdweb.cgi/src/games/tetris/tetris.c.diff?r1=1.30=1.31_with_tag=MAIN

Sorry, I should have checked before sending this mail.

> 
> Index: tetris.c
> ===
> RCS file: /var/cvs/src/games/tetris/tetris.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 tetris.c
> --- tetris.c  7 Mar 2016 12:07:57 -   1.30
> +++ tetris.c  10 Jun 2016 03:09:10 -
> @@ -105,6 +105,7 @@ elide(void)
>   tsleep();
>   while (--base != 0)
>   board[base + B_COLS] = board[base];
> + memset([1], 0, B_COLS - 2);
>   scr_update();
>   tsleep();
>   break;
> 

Re: MBIM Patch (Round 3)

[Gerhard Roth]

On 10.06.2016 05:09, Bryan Vyhmeister wrote:

On Thu, Jun 09, 2016 at 10:31:58PM +0200, Gerhard Roth wrote:

If that doesn't help, please set UMB_DEBUG and set umb_debug to 5.


I left that break commented out which perhaps I shouldn't have but below
is the output when I set an apn and bring umb0 up.


Hmm, I don't see the missing break. It is still stuck in the same
state trying to turn on the radio and always getting non-confirmative
resonses.

If the break before "case UMB_S_RADIO:" is missing, I would expect
to see a debug printf "umb0: init: checking SIM state ...".
Could you please check once again?



[...]

umb0: init: turning radio on ...
umb0: set radio on
umb0: -> set MBIM_CID_RADIO_STATE (tid 7)
umb0: sent MBIM_COMMAND_MSG (tid 7)
   0:   03 00 00 00 34 00 00 00 07 00 00 00 01 00 00 00
  16:   00 00 00 00 a2 89 cc 33 bc bb 8b 4f b6 b0 13 3e
  32:   c2 aa e6 df 03 00 00 00 01 00 00 00 04 00 00 00
  48:   01 00 00 00
umb0: umb_intr: response available
umb0: got response: len 56
   0:   03 00 00 80 38 00 00 00 07 00 00 00 01 00 00 00
  16:   00 00 00 00 a2 89 cc 33 bc bb 8b 4f b6 b0 13 3e
  32:   c2 aa e6 df 03 00 00 00 02 00 00 00 08 00 00 00

  ^^^
  status == MBIM_STATUS_FAILURE

  48:   01 00 00 00 00 00 00 00

  ^^^ 
  hw_state on sw_state off

umb0: <- rcv MBIM_COMMAND_DONE (tid 7)
umb0: set/qry MBIM_CID_RADIO_STATE failed: FAILURE



The error code is rather generic and gives us no help to
determine why exactly the device refuses to turn on the
radio. But it reports that the hw-state is on, which means
we can at least exclude any rfkill switch ;)

So why does the firmware refuse to turn it on? Could still
be a problem of the PIN-less SIM card you're using.

Re: Uninitialised variable in sys/arch/armv7/exynos/crosec.c

[Jonathan Gray]

On Wed, Jun 08, 2016 at 08:51:28PM +0100, Tom Cosgrove wrote:
> Hi
> 
> I can't test this :) but it might bite someone who was trying to hack
> in this area.
> 
> Thanks
> 
> Tom

Thanks, committed.  I'm not aware of anyone with a working exynos setup
so this can't break anything.

> 
> 
> Index: sys/arch/armv7/exynos/crosec.c
> ===
> RCS file: /home/OpenBSD/cvs/src/sys/arch/armv7/exynos/crosec.c,v
> retrieving revision 1.1
> diff -u -p -u -r1.1 crosec.c
> --- sys/arch/armv7/exynos/crosec.c26 Jan 2015 02:48:24 -  1.1
> +++ sys/arch/armv7/exynos/crosec.c8 Jun 2016 19:52:58 -
> @@ -222,7 +222,7 @@ cros_ec_command_inptr(struct cros_ec_sof
>   int ret;
>  
>   delay(5);
> - cros_ec_send_command(sc, EC_CMD_GET_COMMS_STATUS, 0,
> + ret = cros_ec_send_command(sc, EC_CMD_GET_COMMS_STATUS, 
> 0,
>   NULL, 0,
>   (uint8_t **), sizeof(*resp));
>   if (ret < 0)
>