On Fri, 15 Jan 2021 22:41:13 +0100 Marcus Glocker <[email protected]> wrote:
> On Fri, 15 Jan 2021 11:37:47 -0500 > Bryan Steele <[email protected]> wrote: > > > On Fri, Jan 15, 2021 at 06:23:01AM -0700, Thomas Frohwein wrote: > > > On Sat, Jan 09, 2021 at 10:16:16AM +0100, Marcus Glocker wrote: > > > > > > > On Thu, Jan 07, 2021 at 08:20:35PM +0100, Marcus Glocker wrote: > > > > > > > > > > I have heard from others who tried the diff that the PS4 > > > > > > controller is causing problems with the way it attaches. I > > > > > > ordered one to trial-and- error this myself at home. Could > > > > > > you share output of lsusb -vv? Thanks for giving it a try! > > > > > > > > > > > > > > > > Sure, here we go. > > > > > If I can find anything myself in the meantime I let you know. > > > > > > > > > > > > > So the problem doesn't seem to be in your new ujoy(4) code, but > > > > how the dev/hid/hid.c:hid_is_collection() function tries to cope > > > > with the PS4 controller. > > > > > > So with the hid_is_collection() problem not easy to mitigate [1], > > > should we table the ujoy(4) proposal for now pending a fix for the > > > problems with the PS4 controller? Or is this interesting enough > > > for some to work on moving forward despite this issue and finding > > > a solution for this specific (and in some ways unusual) device > > > later? > > > > Considering the hid_is_collection() fix from NetBSD proposed by > > Marcus fixes the issue with ujoy(4) but breaks some other drivers > > (imt(4) and ims(4)), could we not inline it into ujoy(4) for now? > > It's fairly small helper function. Like hid_is_joy_collection()? > > I think that could be an XXX workaround until somebody finds a proper, > generic fix for hid_is_collection() ... That's what you meant, yes? Can this been updated in the overall diff and re-verified if it still works fine with the other controllers? It (obviously) works with my PS4 controller. Index: sys/dev/usb/ujoy.c =================================================================== RCS file: sys/dev/usb/ujoy.c diff -N sys/dev/usb/ujoy.c --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ sys/dev/usb/ujoy.c 22 Jan 2021 17:54:46 -0000 @@ -0,0 +1,149 @@ +/* $OpenBSD$ */ + +/* + * Copyright (c) 2020 Thomas Frohwein <[email protected]> + * Copyright (c) 2020 Bryan Steele <[email protected]> + * + * 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 <sys/param.h> +#include <sys/systm.h> +#include <sys/kernel.h> +#include <sys/malloc.h> +#include <sys/signalvar.h> +#include <sys/device.h> +#include <sys/ioctl.h> +#include <sys/conf.h> +#include <sys/tty.h> +#include <sys/selinfo.h> +#include <sys/proc.h> +#include <sys/vnode.h> +#include <sys/poll.h> +#include <sys/fcntl.h> + +#include <dev/usb/usb.h> +#include <dev/usb/usbhid.h> + +#include <dev/usb/usbdevs.h> +#include <dev/usb/usbdi.h> +#include <dev/usb/usbdi_util.h> + +#include <dev/usb/uhidev.h> +#include <dev/usb/uhid.h> + +int ujoy_match(struct device *, void *, void *); + +struct cfdriver ujoy_cd = { + NULL, "ujoy", DV_DULL +}; + +const struct cfattach ujoy_ca = { + sizeof(struct uhid_softc), + ujoy_match, + uhid_attach, + uhid_detach, +}; + +/* + * XXX workaround: + * + * This is a copy of sys/dev/hid/hid.c:hid_is_collection(), synced up to the + * NetBSD version. Our current hid_is_collection() is not playing nice with + * all HID devices like the PS4 controller. But applying this version + * globally breaks other HID devices like ims(4) and imt(4). Until our global + * hid_is_collection() can't be fixed to play nice with all HID devices, we + * go for this dedicated ujoy(4) version. + */ +int +ujoy_hid_is_collection(const void *desc, int size, uint8_t id, int32_t usage) +{ + struct hid_data *hd; + struct hid_item hi; + uint32_t coll_usage = ~0; + + hd = hid_start_parse(desc, size, hid_input); + if (hd == NULL) + return (0); + + while (hid_get_item(hd, &hi)) { + if (hi.kind == hid_collection && + hi.collection == HCOLL_APPLICATION) + coll_usage = hi.usage; + + if (hi.kind == hid_endcollection) + coll_usage = ~0; + + if (hi.kind == hid_input && + coll_usage == usage && + hi.report_ID == id) { + hid_end_parse(hd); + return (1); + } + } + hid_end_parse(hd); + + return (0); +} + +int +ujoy_match(struct device *parent, void *match, void *aux) +{ + struct uhidev_attach_arg *uha = (struct uhidev_attach_arg *)aux; + int size; + void *desc; + int ret = UMATCH_NONE; + + if (uha->reportid == UHIDEV_CLAIM_ALLREPORTID) + return (ret); + + /* Find the general usage page and gamecontroller collections */ + uhidev_get_report_desc(uha->parent, &desc, &size); + + if (ujoy_hid_is_collection(desc, size, uha->reportid, + HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_JOYSTICK))) + ret = UMATCH_IFACECLASS; + + if (ujoy_hid_is_collection(desc, size, uha->reportid, + HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_GAME_PAD))) + ret = UMATCH_IFACECLASS; + + return (ret); +} + +int +ujoyopen(dev_t dev, int flag, int mode, struct proc *p) +{ + /* Restrict ujoy devices to read operations */ + if ((flag & FWRITE)) + return (EPERM); + return (uhid_do_open(dev, flag, mode, p)); +} + +int +ujoyioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p) +{ + switch (cmd) { + case FIONBIO: + case FIOASYNC: + case USB_GET_DEVICEINFO: + case USB_GET_REPORT: + case USB_GET_REPORT_DESC: + case USB_GET_REPORT_ID: + break; + default: + return (EPERM); + } + + return (uhidioctl(dev, cmd, addr, flag, p)); +}
