On Fri, Jan 22, 2021 at 07:00:57PM +0100, Marcus Glocker wrote:
> On Fri, 15 Jan 2021 22:41:13 +0100
> Marcus Glocker <mar...@nazgul.ch> wrote:
>
> > On Fri, 15 Jan 2021 11:37:47 -0500
> > Bryan Steele <bry...@gmail.com> 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?
Yep, this is what I meant. Hopefully it won't have to stay for long, but
t would be nice to get ujoy(4) working as many controllers as possible
so that it can go in.
Can you send a new full diff thfr@? I still need to test with my USB
SNES controller, but tentatively this is still ok brynet@
-Bryan.
> 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 <t...@openbsd.org>
> + * Copyright (c) 2020 Bryan Steele <bry...@openbsd.org>
> + *
> + * 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));
> +}
>
