factor out ipv4 and ipv6 initial packet sanity checks for bridges

2021-05-30 Thread David Gwynne
if you're looking at an ip header, it makes sense to do some checks to
make sure that the values and addresses make some sense. the canonical
versions of these checks are in the ipv4 and ipv6 input paths, which
makes sense. when bridge(4) is about to run packets through pf it makes
sure the ip headers are sane before first, which i think also makes
sense. veb and tpmr don't do these checks before they run pf, but i
think they should. however, duplicating the code again doesn't appeal to
me.

this factors the ip checks out in the ip_input path, and uses that code
from bridge, veb, and tpmr.

this is mostly shuffling the deck chairs, but ipv6 is moved around a bit
more than ipv4, so some eyes and tests would be appreciated.

in the future i think the ipv6 code should do length checks like the
ipv4 code does too. this diff is big enough as it is though.

ok?

Index: net/if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.354
diff -u -p -r1.354 if_bridge.c
--- net/if_bridge.c 5 Mar 2021 06:44:09 -   1.354
+++ net/if_bridge.c 31 May 2021 04:21:51 -
@@ -1674,61 +1674,12 @@ bridge_ip(struct ifnet *brifp, int dir, 
switch (etype) {
 
case ETHERTYPE_IP:
-   if (m->m_pkthdr.len < sizeof(struct ip))
-   goto dropit;
-
-   /* Copy minimal header, and drop invalids */
-   if (m->m_len < sizeof(struct ip) &&
-   (m = m_pullup(m, sizeof(struct ip))) == NULL) {
-   ipstat_inc(ips_toosmall);
+   m = ipv4_check(ifp, m);
+   if (m == NULL)
return (NULL);
-   }
-   ip = mtod(m, struct ip *);
-
-   if (ip->ip_v != IPVERSION) {
-   ipstat_inc(ips_badvers);
-   goto dropit;
-   }
-
-   hlen = ip->ip_hl << 2;  /* get whole header length */
-   if (hlen < sizeof(struct ip)) {
-   ipstat_inc(ips_badhlen);
-   goto dropit;
-   }
-
-   if (hlen > m->m_len) {
-   if ((m = m_pullup(m, hlen)) == NULL) {
-   ipstat_inc(ips_badhlen);
-   return (NULL);
-   }
-   ip = mtod(m, struct ip *);
-   }
-
-   if ((m->m_pkthdr.csum_flags & M_IPV4_CSUM_IN_OK) == 0) {
-   if (m->m_pkthdr.csum_flags & M_IPV4_CSUM_IN_BAD) {
-   ipstat_inc(ips_badsum);
-   goto dropit;
-   }
-
-   ipstat_inc(ips_inswcsum);
-   if (in_cksum(m, hlen) != 0) {
-   ipstat_inc(ips_badsum);
-   goto dropit;
-   }
-   }
-
-   if (ntohs(ip->ip_len) < hlen)
-   goto dropit;
 
-   if (m->m_pkthdr.len < ntohs(ip->ip_len))
-   goto dropit;
-   if (m->m_pkthdr.len > ntohs(ip->ip_len)) {
-   if (m->m_len == m->m_pkthdr.len) {
-   m->m_len = ntohs(ip->ip_len);
-   m->m_pkthdr.len = ntohs(ip->ip_len);
-   } else
-   m_adj(m, ntohs(ip->ip_len) - m->m_pkthdr.len);
-   }
+   ip = mtod(m, struct ip *);
+   hlen = ip->ip_hl << 2;
 
 #ifdef IPSEC
if ((brifp->if_flags & IFF_LINK2) == IFF_LINK2 &&
@@ -1772,23 +1723,10 @@ bridge_ip(struct ifnet *brifp, int dir, 
break;
 
 #ifdef INET6
-   case ETHERTYPE_IPV6: {
-   struct ip6_hdr *ip6;
-
-   if (m->m_len < sizeof(struct ip6_hdr)) {
-   if ((m = m_pullup(m, sizeof(struct ip6_hdr)))
-   == NULL) {
-   ip6stat_inc(ip6s_toosmall);
-   return (NULL);
-   }
-   }
-
-   ip6 = mtod(m, struct ip6_hdr *);
-
-   if ((ip6->ip6_vfc & IPV6_VERSION_MASK) != IPV6_VERSION) {
-   ip6stat_inc(ip6s_badvers);
-   goto dropit;
-   }
+   case ETHERTYPE_IPV6:
+   m = ipv6_check(ifp, m);
+   if (m == NULL)
+   return (NULL);
 
 #ifdef IPSEC
hlen = sizeof(struct ip6_hdr);
@@ -1819,7 +1757,6 @@ bridge_ip(struct ifnet *brifp, int dir, 
 #endif /* NPF > 0 */
 
break;
-   }
 #endif /* INET6 */
 
default:
Index: net/if_tpmr.c
===
RCS file: /cvs/src/sys/net/if_tpmr.c,v
retrieving revision 1.26
diff -u -p -r1.26 if_tpmr.c
--- net/if_tpmr.c

Re: usbhidctl: add -R flag to dump raw report descriptor bytes

2021-05-30 Thread joshua stein
On Fri, 28 May 2021 at 21:52:57 -0500, joshua stein wrote:
> > Another approach which keeps the structure opaque is the extend the
> > usbhid(3) API with something like:
> > 
> > void
> > hid_get_report_desc_data(report_desc_t d, uint8_t **data, uint32_t 
> > *size)
> > {
> > *data = d->data;
> > *size = d->size;
> > }

Here's a new version of the usbhidctl diff using this new usbhid 
API, with man page help from jmc.


diff --git usr.bin/usbhidctl/usbhid.c usr.bin/usbhidctl/usbhid.c
index 921f211a280..910263d1392 100644
--- usr.bin/usbhidctl/usbhid.c
+++ usr.bin/usbhidctl/usbhid.c
@@ -755,6 +755,7 @@ usage(void)
fprintf(stderr,
"   %s -f device [-t table] -w name=value ...\n",
__progname);
+   fprintf(stderr, "   %s -f device -R\n", __progname);
exit(1);
 }
 
@@ -764,16 +765,18 @@ main(int argc, char **argv)
char const *dev;
char const *table;
size_t varnum;
-   int aflag, lflag, nflag, rflag, wflag;
-   int ch, hidfd;
+   uint32_t repsize;
+   int aflag, lflag, nflag, rflag, Rflag, wflag;
+   int ch, hidfd, x;
+   uint8_t *repdata;
report_desc_t repdesc;
char devnamebuf[PATH_MAX];
struct Susbvar variables[128];
 
-   wflag = aflag = nflag = verbose = rflag = lflag = 0;
+   wflag = aflag = nflag = verbose = rflag = Rflag = lflag = 0;
dev = NULL;
table = NULL;
-   while ((ch = getopt(argc, argv, "?af:lnrt:vw")) != -1) {
+   while ((ch = getopt(argc, argv, "?af:lnRrt:vw")) != -1) {
switch (ch) {
case 'a':
aflag = 1;
@@ -790,6 +793,9 @@ main(int argc, char **argv)
case 'r':
rflag = 1;
break;
+   case 'R':
+   Rflag = 1;
+   break;
case 't':
table = optarg;
break;
@@ -807,7 +813,8 @@ main(int argc, char **argv)
}
argc -= optind;
argv += optind;
-   if (dev == NULL || (lflag && (wflag || rflag))) {
+   if (dev == NULL || (lflag && (wflag || rflag || Rflag)) ||
+   (rflag && Rflag)) {
/*
 * No device specified, or attempting to loop and set
 * or dump report at the same time
@@ -942,6 +949,14 @@ main(int argc, char **argv)
if (repdesc == 0)
errx(1, "USB_GET_REPORT_DESC");
 
+   if (Rflag) {
+   hid_get_report_desc_data(repdesc, &repdata, &repsize);
+
+   for (x = 0; x < repsize; x++)
+   printf("%s0x%02x", x > 0 ? " " : "", repdata[x]);
+   printf("\n");
+   }
+
if (lflag) {
devloop(hidfd, repdesc, variables, varnum);
/* NOTREACHED */
@@ -951,10 +966,11 @@ main(int argc, char **argv)
/* Report mode header */
printf("Report descriptor:\n");
 
-   devshow(hidfd, repdesc, variables, varnum,
-   1 << hid_input |
-   1 << hid_output |
-   1 << hid_feature);
+   if (!Rflag)
+   devshow(hidfd, repdesc, variables, varnum,
+   1 << hid_input |
+   1 << hid_output |
+   1 << hid_feature);
 
if (rflag) {
/* Report mode trailer */
diff --git usr.bin/usbhidctl/usbhidctl.1 usr.bin/usbhidctl/usbhidctl.1
index 5b8e59f7bd7..0aa5b54f6f9 100644
--- usr.bin/usbhidctl/usbhidctl.1
+++ usr.bin/usbhidctl/usbhidctl.1
@@ -53,6 +53,9 @@
 .Fl f Ar device
 .Op Fl t Ar table
 .Fl w Ar name Ns = Ns Ar value ...
+.Nm
+.Fl f Ar device
+.Fl R
 .Sh DESCRIPTION
 .Nm
 can be used to output or modify the state of a USB HID (Human Interface 
Device).
@@ -88,6 +91,8 @@ Only 'input' items are displayed in this mode.
 .It Fl n
 Suppress printing of the item name when querying specific items.
 Only output the current value.
+.It Fl R
+Dump the raw USB HID report descriptor data as hexadecimal bytes.
 .It Fl r
 Dump the USB HID report descriptor.
 .It Fl t Ar table



Re: ftpd(8): Convert K&R function definitions to modern C

2021-05-30 Thread Todd C . Miller
On Sun, 30 May 2021 17:47:34 +0200, Jan Klemkow wrote:

> Convert K&R function definitions to modern C.

OK millert@

 - todd



Re: iwm: avoid 'mac clock not ready' panic

2021-05-30 Thread Mark Kettenis
> Date: Sun, 30 May 2021 22:26:09 +0200
> From: Stefan Sperling 
> 
> Steven observed a panic ("iwm0: mac clock not ready") while testing
> the iwm firmware update patch on a 9560 device.
> I've also seen this happen one time, at some point during development.
> 
> In hindsight it is a bad idea to look at hardware register state here.
> The point of iwm_nic_assert_locked() is to verify that iwm_nic_lock() has
> been called somewhere up in the call stack. Checking our own lock counter
> is sufficient for this purpose.
> 
> If locking the device worked then these registers had the expected state at
> that time and our lock counter was incremented. Apparently if the device runs
> into some issue later the state of these registers may change and trigger
> these panics. Instead we want to handle such failures gracefully and reset
> the device.
> 
> ok?

ok kettenis@

> (For the curious: iwm_nic_lock() is supposed to avoid a situation where the
> device enters some low power state while the driver expects the device to
> quickly respond to certain I/O requests. That's all I know.)
> 
> diff 385a08f3e862586df8f1803dfa09fc765a5c3610 /usr/src
> blob - 4b502468fea796f103fb84237146879e8c4df267
> file + sys/dev/pci/if_iwm.c
> --- sys/dev/pci/if_iwm.c
> +++ sys/dev/pci/if_iwm.c
> @@ -1069,11 +1069,6 @@ iwm_nic_lock(struct iwm_softc *sc)
>  void
>  iwm_nic_assert_locked(struct iwm_softc *sc)
>  {
> - uint32_t reg = IWM_READ(sc, IWM_CSR_GP_CNTRL);
> - if ((reg & IWM_CSR_GP_CNTRL_REG_FLAG_MAC_CLOCK_READY) == 0)
> - panic("%s: mac clock not ready", DEVNAME(sc));
> - if (reg & IWM_CSR_GP_CNTRL_REG_FLAG_GOING_TO_SLEEP)
> - panic("%s: mac gone to sleep", DEVNAME(sc));
>   if (sc->sc_nic_locks <= 0)
>   panic("%s: nic locks counter %d", DEVNAME(sc), 
> sc->sc_nic_locks);
>  }
> 
> 



iwm: avoid 'mac clock not ready' panic

2021-05-30 Thread Stefan Sperling
Steven observed a panic ("iwm0: mac clock not ready") while testing
the iwm firmware update patch on a 9560 device.
I've also seen this happen one time, at some point during development.

In hindsight it is a bad idea to look at hardware register state here.
The point of iwm_nic_assert_locked() is to verify that iwm_nic_lock() has
been called somewhere up in the call stack. Checking our own lock counter
is sufficient for this purpose.

If locking the device worked then these registers had the expected state at
that time and our lock counter was incremented. Apparently if the device runs
into some issue later the state of these registers may change and trigger
these panics. Instead we want to handle such failures gracefully and reset
the device.

ok?

(For the curious: iwm_nic_lock() is supposed to avoid a situation where the
device enters some low power state while the driver expects the device to
quickly respond to certain I/O requests. That's all I know.)

diff 385a08f3e862586df8f1803dfa09fc765a5c3610 /usr/src
blob - 4b502468fea796f103fb84237146879e8c4df267
file + sys/dev/pci/if_iwm.c
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -1069,11 +1069,6 @@ iwm_nic_lock(struct iwm_softc *sc)
 void
 iwm_nic_assert_locked(struct iwm_softc *sc)
 {
-   uint32_t reg = IWM_READ(sc, IWM_CSR_GP_CNTRL);
-   if ((reg & IWM_CSR_GP_CNTRL_REG_FLAG_MAC_CLOCK_READY) == 0)
-   panic("%s: mac clock not ready", DEVNAME(sc));
-   if (reg & IWM_CSR_GP_CNTRL_REG_FLAG_GOING_TO_SLEEP)
-   panic("%s: mac gone to sleep", DEVNAME(sc));
if (sc->sc_nic_locks <= 0)
panic("%s: nic locks counter %d", DEVNAME(sc), 
sc->sc_nic_locks);
 }



ftpd(8): Convert K&R function definitions to modern C

2021-05-30 Thread Jan Klemkow
Hi,

Convert K&R function definitions to modern C.

OK?

bye,
Jan

Index: ftpcmd.y
===
RCS file: /cvs/src/libexec/ftpd/ftpcmd.y,v
retrieving revision 1.72
diff -u -p -r1.72 ftpcmd.y
--- ftpcmd.y23 May 2021 17:01:21 -  1.72
+++ ftpcmd.y30 May 2021 15:32:50 -
@@ -1072,9 +1072,7 @@ static int yylex(void);
 extern int epsvall;
 
 static struct tab *
-lookup(p, cmd)
-   struct tab *p;
-   const char *cmd;
+lookup(struct tab *p, const char *cmd)
 {
 
for (; p->name != NULL; p++)
@@ -1089,9 +1087,7 @@ lookup(p, cmd)
  * get_line - a hacked up version of fgets to ignore TELNET escape codes.
  */
 int
-get_line(s, n)
-   char *s;
-   int n;
+get_line(char *s, int n)
 {
int c;
char *cs;
@@ -1176,8 +1172,7 @@ get_line(s, n)
 
 /*ARGSUSED*/
 void
-toolong(signo)
-   int signo;
+toolong(int signo)
 {
struct syslog_data sdata = SYSLOG_DATA_INIT;
 
@@ -1190,7 +1185,7 @@ toolong(signo)
 }
 
 static int
-yylex()
+yylex(void)
 {
static int cpos;
char *cp, *cp2;
@@ -1429,8 +1424,7 @@ yylex()
 }
 
 void
-upper(s)
-   char *s;
+upper(char *s)
 {
char *p;
 
@@ -1439,9 +1433,7 @@ upper(s)
 }
 
 static void
-help(ctab, s)
-   struct tab *ctab;
-   char *s;
+help(struct tab *ctab, char *s)
 {
struct tab *c;
int width, NCMDS;
@@ -1504,8 +1496,7 @@ help(ctab, s)
 }
 
 static void
-sizecmd(filename)
-   const char *filename;
+sizecmd(const char *filename)
 {
switch (type) {
case TYPE_L:
Index: monitor.c
===
RCS file: /cvs/src/libexec/ftpd/monitor.c,v
retrieving revision 1.28
diff -u -p -r1.28 monitor.c
--- monitor.c   20 May 2021 15:21:03 -  1.28
+++ monitor.c   30 May 2021 15:38:52 -
@@ -206,7 +206,7 @@ monitor_init(void)
  * for the user-privileged slave process and 1 for the monitor process.
  */
 int
-monitor_post_auth()
+monitor_post_auth(void)
 {
slave_pid = fork();
if (slave_pid == -1)



couple devices found on a intel nuc 10i7FNH

2021-05-30 Thread Felix Kronlage-Dammers
ahoi,

recognize couple devices found in a Intel Nuc 10i7FNH.
Since the JHL7540 exists with a few pci ids, this suffixes the existing
one so it matches with the new addition.

felix



Index: sys/dev/pci/pcidevs
===
RCS file: /cvs/src/sys/dev/pci/pcidevs,v
retrieving revision 1.1970
diff -u -p -u -r1.1970 pcidevs
--- sys/dev/pci/pcidevs 19 May 2021 05:20:48 -  1.1970
+++ sys/dev/pci/pcidevs 30 May 2021 19:24:05 -
@@ -302,6 +302,7 @@ vendor  ALTIMA  0x173b  Altima
 vendor ANTARES 0x1754  Antares Microsystems
 vendor CAVIUM  0x177d  Cavium
 vendor BELKIN2 0x1799  Belkin
+vendor GENESYS 0x17a0  Genesys Logic
 vendor LENOVO  0x17aa  Lenovo
 vendor HAWKING 0x17b3  Hawking Technology
 vendor NETCHIP 0x17cc  NetChip Technology
@@ -3034,6 +3035,9 @@ product FUSIONIO IOXTREME_PRO 0x1007  ioX
 /* Future Domain products */
 product FUTUREDOMAIN TMC_18C30 0x  TMC-18C30
 
+/* Genesys Logic products */
+product GENESYS GLI_9755   0x9755  SDHC
+
 /* Guillemot products */
 product GEMTEK PR103   0x1001  PR103
 
@@ -3977,9 +3981,12 @@ product INTEL I219_V90x15e2  I219-V
 product INTEL I219_LM5 0x15e3  I219-LM
 product INTEL X550EM_A_1G_T0x15e4  X553 SGMII
 product INTEL X550EM_A_1G_T_L  0x15e5  X553 SGMII
-product INTEL JHL7540_PCIE 0x15ea  JHL7540 Thunderbolt 3
-product INTEL JHL7540  0x15eb  JHL7540 Thunderbolt 3
-product INTEL JHL7540_XHCI 0x15ec  JHL7540 Thunderbolt 3
+product INTEL JHL7540_PCIE_1   0x15e7  JHL7540 Thunderbolt 3
+product INTEL JHL7540_10x15e8  JHL7540 Thunderbolt 3
+product INTEL JHL7540_XHCI_1   0x15e9  JHL7540 Thunderbolt 3
+product INTEL JHL7540_PCIE_2   0x15ea  JHL7540 Thunderbolt 3
+product INTEL JHL7540_20x15eb  JHL7540 Thunderbolt 3
+product INTEL JHL7540_XHCI_2   0x15ec  JHL7540 Thunderbolt 3
 product INTEL I225_LM  0x15f2  I225-LM
 product INTEL I225_V   0x15f3  I225-V
 product INTEL I219_LM150x15f4  I219-LM


-- 
GPG/PGP:   7A0B612C /  5F4D 9B06 C240 3250 35BF  66ED 1AD3 A9B8 7A0B 612C



Re: Avoid shifting of a negative value in sys_adjfreq()

2021-05-30 Thread Christian Weisgerber
Visa Hankala:

> However, wouldn't it be better if the code avoided the
> situation, for example by defining ADJFREQ_MIN as the negative
> of ADJFREQ_MAX?

Indeed it would.  ok naddy@

> --- kern/kern_time.c  23 Dec 2020 20:45:02 -  1.151
> +++ kern/kern_time.c  30 May 2021 15:38:09 -
> @@ -396,7 +396,7 @@ sys_settimeofday(struct proc *p, void *v
>  }
>  
>  #define ADJFREQ_MAX (5LL << 32)
> -#define ADJFREQ_MIN (-5LL << 32)
> +#define ADJFREQ_MIN (-ADJFREQ_MAX)
>  
>  int
>  sys_adjfreq(struct proc *p, void *v, register_t *retval)
> 

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



Avoid shifting of a negative value in sys_adjfreq()

2021-05-30 Thread Visa Hankala
When cross-compiling the kernel using ports clang (tsk tsk), the
compiler reports the following:

src/sys/kern/kern_time.c:418:11: error: shifting a
  negative signed value is undefined [-Werror,-Wshift-negative-value]
if (f < ADJFREQ_MIN || f > ADJFREQ_MAX)
^
src/sys/kern/kern_time.c:399:35: note: expanded from
  macro 'ADJFREQ_MIN'
#define ADJFREQ_MIN (-5LL << 32)
  ^

Normally this does not occur because of the implicit -fwrapv option
with base clang. However, wouldn't it be better if the code avoided the
situation, for example by defining ADJFREQ_MIN as the negative
of ADJFREQ_MAX?

Index: kern/kern_time.c
===
RCS file: src/sys/kern/kern_time.c,v
retrieving revision 1.151
diff -u -p -r1.151 kern_time.c
--- kern/kern_time.c23 Dec 2020 20:45:02 -  1.151
+++ kern/kern_time.c30 May 2021 15:38:09 -
@@ -396,7 +396,7 @@ sys_settimeofday(struct proc *p, void *v
 }
 
 #define ADJFREQ_MAX (5LL << 32)
-#define ADJFREQ_MIN (-5LL << 32)
+#define ADJFREQ_MIN (-ADJFREQ_MAX)
 
 int
 sys_adjfreq(struct proc *p, void *v, register_t *retval)



Re: uvideo(4) new quirk flag UVIDEO_FLAG_NOATTACH

2021-05-30 Thread Marcus Glocker
On Mon, 5 Apr 2021 23:54:31 +0200
Marcus Glocker  wrote:

> On Mon, Apr 05, 2021 at 11:27:10PM +0200, Mark Kettenis wrote:
> 
> [...]
> 
> > > > > How common is it to explain the system behavior in cases like
> > > > > this? Would it be less surprising (generate less misc@
> > > > > traffic) if we printed a note explaining why the camera was
> > > > > skipped?  
> > > > 
> > > > I wouldn't print a specific message per unsupported device, but
> > > > I think a generic message that the video device isn't supported
> > > > would make sense.  Something like that maybe?  Obviously this
> > > > can print more than once, but I'm not sure if it's worth to
> > > > make that a unique print.
> > > > 
> > > > E.g.:
> > > > 
> > > > vmm0 at mainbus0: VMX/EPT
> > > > uvideo: device 13d3:56b2 isn't supported
> > > > uvideo: device 13d3:56b2 isn't supported
> > > > ugen0 at uhub0 port 8 "SunplusIT Inc Integrated Camera" rev
> > > > 2.01/17.11 addr 2  
> > > 
> > > No; match functions shouldn't print stuff.  
> > 
> > If you really wanted to print a message, you'll need to let
> > uvideo(4) attach and then print a "not supported" message early on
> > in the attach function and return.  
> 
> Right.  Despite the print line, maybe this way is anyway better than
> doing this check in the match function.

While cleaning up my diffs I just found that this one hasn't been
committed.  OK to get this in?


Index: sys/dev/usb/uvideo.c
===
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.212
diff -u -p -u -p -r1.212 uvideo.c
--- sys/dev/usb/uvideo.c5 Apr 2021 20:45:49 -   1.212
+++ sys/dev/usb/uvideo.c5 Apr 2021 21:51:11 -
@@ -490,9 +490,6 @@ uvideo_match(struct device *parent, void
/* quirk devices */
quirk = uvideo_lookup(uaa->vendor, uaa->product);
if (quirk != NULL) {
-   if (quirk->flags & UVIDEO_FLAG_NOATTACH)
-   return (UMATCH_NONE);
-
if (quirk->flags & UVIDEO_FLAG_REATTACH)
return (UMATCH_VENDOR_PRODUCT_CONF_IFACE);
 
@@ -577,6 +574,11 @@ uvideo_attach(struct device *parent, str
 
/* maybe the device has quirks */
sc->sc_quirk = uvideo_lookup(uaa->vendor, uaa->product);
+
+   if (sc->sc_quirk && sc->sc_quirk->flags & UVIDEO_FLAG_NOATTACH) {
+   printf("%s: device not supported\n", DEVNAME(sc));
+   return;
+   }
 
if (sc->sc_quirk && sc->sc_quirk->ucode_name)
config_mountroot(self, uvideo_attach_hook);



Re: usbhidctl: add -R flag to dump raw report descriptor bytes

2021-05-30 Thread Theo Buehler
On Fri, May 28, 2021 at 09:52:57PM -0500, joshua stein wrote:
> On Wed, 26 May 2021 at 08:13:52 +0200, Anton Lindqvist wrote:
> > On Tue, May 25, 2021 at 08:31:14AM +0200, Anton Lindqvist wrote:
> > > On Mon, May 24, 2021 at 09:17:26AM -0500, joshua stein wrote:
> > > > This is useful for parsing the report descriptor with a different 
> > > > tool to find issues with our HID parser.
> > > > 
> > > > I've found https://eleccelerator.com/usbdescreqparser/ to be 
> > > > helpful.
> > > > 
> > > > 
> > > > diff --git usr.bin/usbhidctl/usbhid.c usr.bin/usbhidctl/usbhid.c
> > > > index 921f211a280..bd0b5da0222 100644
> > > > --- usr.bin/usbhidctl/usbhid.c
> > > > +++ usr.bin/usbhidctl/usbhid.c
> > > > @@ -106,6 +106,11 @@ static struct {
> > > >  #define REPORT_MAXVAL 2
> > > >  };
> > > >  
> > > > +struct report_desc {
> > > > +   uint32_t size;
> > > > +   uint8_t data[1];
> > > > +};
> > > 
> > > This structure is defined in lib/libusbhid/usbvar.h which is not
> > > installed. Maybe it should and avoid this repetition and potential ABI
> > > breaks (quite unlikely but still)?
> > 
> > Another approach which keeps the structure opaque is the extend the
> > usbhid(3) API with something like:
> > 
> > void
> > hid_get_report_desc_data(report_desc_t d, uint8_t **data, uint32_t 
> > *size)
> > {
> > *data = d->data;
> > *size = d->size;
> > }
> 
> Here's a version of that:

libusbhid needs a minor bump:

Index: lib/libusbhid/shlib_version
===
RCS file: /cvs/src/lib/libusbhid/shlib_version,v
retrieving revision 1.8
diff -u -p -r1.8 shlib_version
--- lib/libusbhid/shlib_version 12 May 2014 17:03:28 -  1.8
+++ lib/libusbhid/shlib_version 30 May 2021 07:04:33 -
@@ -1,2 +1,2 @@
 major=7
-minor=0
+minor=1
Index: distrib/sets/lists/base/mi
===
RCS file: /cvs/src/distrib/sets/lists/base/mi,v
retrieving revision 1.1022
diff -u -p -r1.1022 mi
--- distrib/sets/lists/base/mi  27 May 2021 05:51:50 -  1.1022
+++ distrib/sets/lists/base/mi  30 May 2021 07:04:51 -
@@ -675,7 +675,7 @@
 ./usr/lib/libtermcap.so.14.0
 ./usr/lib/libtermlib.so.14.0
 ./usr/lib/libtls.so.21.0
-./usr/lib/libusbhid.so.7.0
+./usr/lib/libusbhid.so.7.1
 ./usr/lib/libutil.so.15.0
 ./usr/lib/libz.so.5.0
 ./usr/lib/locate