Re: [PATCH kdrive/ephyr v7 3/9] kdrive: introduce input hot-plugging support for udev and hal backends (#33140)

2016-02-12 Thread Laércio de Sousa 
2016-02-11 19:35 GMT-02:00 Peter Hutterer :
>
> I just checked the kernel sources and there is no specific error case that
> this code can trigger, it's something to do with the fd itself (EBADF,
> EPERM, etc.). I guess fd is already closed/reset by the time you get here.
> What errno do you get?

I've got EBADF (Bad File Descriptor). Anyway, I realized that this error
message when trying to ungrab devices are unrelated to fd unregistering.

I've modified EvdevPtrDisable/EvdevKbdDisable functions, making them call
KdUnregisterFd *twice*. I don't know why, but it seems to solve my problem
(my "garbage collector" no longer detect orphan fds left behind and my
Xephyr no longer segfaults when I unplug/replug them).

Some more details about my issues:

1. This problem with incomplete fd unregistering only affects the device
with the *lowest* fd number (I've found it by swapping the keyboard and
mouse connections in my USB hub).

2. When I unplug the device with the lowest fd number (e.g. mouse), the one
with the highest fd number (e.g. keyboard) stops working. If I then unplug
this highest fd device, it does *not* disappear from "xinput list". When I
finally replug this device, it works again, but it appears "duplicated" in
"xinput list", with another fd number. Example:

⎡ Virtual core pointer id=2 [master pointer  (3)]
⎜   ↳ Virtual core XTEST pointer   id=4 [slave  pointer  (2)]
⎜   ↳ SIGMACHIP Usb Mouse  id=6 [slave  pointer  (2)]
⎣ Virtual core keyboardid=3 [master keyboard (2)]
↳ Virtual core XTEST keyboard  id=5 [slave  keyboard (3)]
↳ HID 04f3:0103id=7 [slave  keyboard (3)]
↳ HID 04f3:0103id=8 [slave  keyboard (3)]

Crazily, issue 2 above only seems to affect Debian/Ubuntu systems. I have
another openSUSE box with Xephyr-based multi-seat configured, and it's not
affected by issue 2. I'll do more tests with this openSUSE box to see if
it's affected by issue 1.
-- 
*Laércio de Sousa*
*Orientador de Informática*
*Escola Municipal "Professor Eulálio Gruppi"*
*Rua Ismael da Silva Mello, 559, Mogi Moderno*
*Mogi das Cruzes - SPCEP 08717-390*
Telefone: (11) 4726-8313
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH kdrive/ephyr v7 3/9] kdrive: introduce input hot-plugging support for udev and hal backends (#33140)

2016-02-12 Thread Laércio de Sousa 
I've just confirmed: my openSUSE box is *not* affected by fd unregistering
issue. So I won't treat it as an upstream bug anymore, and I'll drop all
additional verifications from this patch.

2016-02-12 10:13 GMT-02:00 Laércio de Sousa <
laercioso...@sme-mogidascruzes.sp.gov.br>:

>
> 2016-02-11 19:35 GMT-02:00 Peter Hutterer :
>>
>> I just checked the kernel sources and there is no specific error case that
>> this code can trigger, it's something to do with the fd itself (EBADF,
>> EPERM, etc.). I guess fd is already closed/reset by the time you get here.
>> What errno do you get?
>
> I've got EBADF (Bad File Descriptor). Anyway, I realized that this error
> message when trying to ungrab devices are unrelated to fd unregistering.
>
> I've modified EvdevPtrDisable/EvdevKbdDisable functions, making them call
> KdUnregisterFd *twice*. I don't know why, but it seems to solve my problem
> (my "garbage collector" no longer detect orphan fds left behind and my
> Xephyr no longer segfaults when I unplug/replug them).
>
> Some more details about my issues:
>
> 1. This problem with incomplete fd unregistering only affects the device
> with the *lowest* fd number (I've found it by swapping the keyboard and
> mouse connections in my USB hub).
>
> 2. When I unplug the device with the lowest fd number (e.g. mouse), the
> one with the highest fd number (e.g. keyboard) stops working. If I then
> unplug this highest fd device, it does *not* disappear from "xinput list".
> When I finally replug this device, it works again, but it appears
> "duplicated" in "xinput list", with another fd number. Example:
>
> ⎡ Virtual core pointer id=2 [master pointer  (3)]
> ⎜   ↳ Virtual core XTEST pointer   id=4 [slave  pointer  (2)]
> ⎜   ↳ SIGMACHIP Usb Mouse  id=6 [slave  pointer  (2)]
> ⎣ Virtual core keyboardid=3 [master keyboard (2)]
> ↳ Virtual core XTEST keyboard  id=5 [slave  keyboard (3)]
> ↳ HID 04f3:0103id=7 [slave  keyboard (3)]
> ↳ HID 04f3:0103id=8 [slave  keyboard (3)]
>
> Crazily, issue 2 above only seems to affect Debian/Ubuntu systems. I have
> another openSUSE box with Xephyr-based multi-seat configured, and it's not
> affected by issue 2. I'll do more tests with this openSUSE box to see if
> it's affected by issue 1.
> --
> *Laércio de Sousa*
> *Orientador de Informática*
> *Escola Municipal "Professor Eulálio Gruppi"*
> *Rua Ismael da Silva Mello, 559, Mogi Moderno*
> *Mogi das Cruzes - SPCEP 08717-390*
> Telefone: (11) 4726-8313
>



-- 
*Laércio de Sousa*
*Orientador de Informática*
*Escola Municipal "Professor Eulálio Gruppi"*
*Rua Ismael da Silva Mello, 559, Mogi Moderno*
*Mogi das Cruzes - SPCEP 08717-390*
Telefone: (11) 4726-8313
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH kdrive/ephyr v7 3/9] kdrive: introduce input hot-plugging support for udev and hal backends (#33140)

2016-02-11 Thread Laércio de Sousa 
2016-02-11 0:56 GMT-02:00 Peter Hutterer :

> we don't have a 1:1 mapping between devices and fd (e.g. wacom devices all
> hang off a single fd). Even the fd itself is a DDX concept, so RemoveDevice
> cannot close the fd.
>
> What should happen here though is that the pDev->public.devicePrivate
> points
> to something kdrive understands and that includes the fd.
>
Reading kdrive evdev sources, I've found that
EvdevPtrDisable/EvdevKbdDisable functions
do have a KdUnregisterFd() call. Example:

static void
EvdevPtrDisable(KdPointerInfo * pi)
{
Kevdev *ke;

ke = pi->driverPrivate;

if (!pi || !pi->driverPrivate)
return;

KdUnregisterFd(pi, ke->fd, TRUE);

if (ioctl(ke->fd, EVIOCGRAB, 0) < 0)
perror("Ungrabbing evdev mouse device failed");

free(ke);
pi->driverPrivate = 0;
}

However, it seems to fail in my system, because I always get
that "Ungrabbing evdev mouse device failed" error message.

-- 
*Laércio de Sousa*
*Orientador de Informática*
*Escola Municipal "Professor Eulálio Gruppi"*
*Rua Ismael da Silva Mello, 559, Mogi Moderno*
*Mogi das Cruzes - SPCEP 08717-390*
Telefone: (11) 4726-8313
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH kdrive/ephyr v7 3/9] kdrive: introduce input hot-plugging support for udev and hal backends (#33140)

2016-02-11 Thread Peter Hutterer 
On Thu, Feb 11, 2016 at 08:23:55AM -0200, Laércio de Sousa wrote:
> 2016-02-11 0:56 GMT-02:00 Peter Hutterer :
> 
> > we don't have a 1:1 mapping between devices and fd (e.g. wacom devices all
> > hang off a single fd). Even the fd itself is a DDX concept, so RemoveDevice
> > cannot close the fd.
> >
> > What should happen here though is that the pDev->public.devicePrivate
> > points
> > to something kdrive understands and that includes the fd.
> >
> Reading kdrive evdev sources, I've found that
> EvdevPtrDisable/EvdevKbdDisable functions
> do have a KdUnregisterFd() call. Example:
> 
> static void
> EvdevPtrDisable(KdPointerInfo * pi)
> {
> Kevdev *ke;
> 
> ke = pi->driverPrivate;
> 
> if (!pi || !pi->driverPrivate)
> return;
> 
> KdUnregisterFd(pi, ke->fd, TRUE);
> 
> if (ioctl(ke->fd, EVIOCGRAB, 0) < 0)
> perror("Ungrabbing evdev mouse device failed");
> 
> free(ke);
> pi->driverPrivate = 0;
> }
> 
> However, it seems to fail in my system, because I always get
> that "Ungrabbing evdev mouse device failed" error message.

I just checked the kernel sources and there is no specific error case that
this code can trigger, it's something to do with the fd itself (EBADF,
EPERM, etc.). I guess fd is already closed/reset by the time you get here.
What errno do you get?

Cheers,
   Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH kdrive/ephyr v7 3/9] kdrive: introduce input hot-plugging support for udev and hal backends (#33140)

2016-02-11 Thread Peter Hutterer 
On Thu, Feb 11, 2016 at 04:10:23PM -0200, Laércio de Sousa wrote:
> 2016-02-11 8:23 GMT-02:00 Laércio de Sousa <
> laercioso...@sme-mogidascruzes.sp.gov.br>:
> 
> > 2016-02-11 0:56 GMT-02:00 Peter Hutterer :
> >
> >> we don't have a 1:1 mapping between devices and fd (e.g. wacom devices all
> >> hang off a single fd). Even the fd itself is a DDX concept, so
> >> RemoveDevice
> >> cannot close the fd.
> >>
> >> What should happen here though is that the pDev->public.devicePrivate
> >> points
> >> to something kdrive understands and that includes the fd.
> >>
> > Reading kdrive evdev sources, I've found that
> > EvdevPtrDisable/EvdevKbdDisable functions
> > do have a KdUnregisterFd() call. Example:
> >
> > static void
> > EvdevPtrDisable(KdPointerInfo * pi)
> > {
> > Kevdev *ke;
> >
> > ke = pi->driverPrivate;
> >
> > if (!pi || !pi->driverPrivate)
> > return;
> >
> > KdUnregisterFd(pi, ke->fd, TRUE);
> >
> > if (ioctl(ke->fd, EVIOCGRAB, 0) < 0)
> > perror("Ungrabbing evdev mouse device failed");
> >
> > free(ke);
> > pi->driverPrivate = 0;
> > }
> >
> > However, it seems to fail in my system, because I always get
> > that "Ungrabbing evdev mouse device failed" error message.
> >
> I'm convincing myself this is more a ploblem with kdrive evdev driver than
> with kdrive itself, so I'm tempted to keep that code in
> DeleteInputDeviceRequest() as is, with a mention that it's a kind of
> "safety measure against buggy drivers". What do you think?

tbh, I think it's easier if we just fix buggy drivers. it's not like we have
that many of them :)

Cheers,
   Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH kdrive/ephyr v7 3/9] kdrive: introduce input hot-plugging support for udev and hal backends (#33140)

2016-02-11 Thread Laércio de Sousa 
2016-02-11 8:23 GMT-02:00 Laércio de Sousa <
laercioso...@sme-mogidascruzes.sp.gov.br>:

> 2016-02-11 0:56 GMT-02:00 Peter Hutterer :
>
>> we don't have a 1:1 mapping between devices and fd (e.g. wacom devices all
>> hang off a single fd). Even the fd itself is a DDX concept, so
>> RemoveDevice
>> cannot close the fd.
>>
>> What should happen here though is that the pDev->public.devicePrivate
>> points
>> to something kdrive understands and that includes the fd.
>>
> Reading kdrive evdev sources, I've found that
> EvdevPtrDisable/EvdevKbdDisable functions
> do have a KdUnregisterFd() call. Example:
>
> static void
> EvdevPtrDisable(KdPointerInfo * pi)
> {
> Kevdev *ke;
>
> ke = pi->driverPrivate;
>
> if (!pi || !pi->driverPrivate)
> return;
>
> KdUnregisterFd(pi, ke->fd, TRUE);
>
> if (ioctl(ke->fd, EVIOCGRAB, 0) < 0)
> perror("Ungrabbing evdev mouse device failed");
>
> free(ke);
> pi->driverPrivate = 0;
> }
>
> However, it seems to fail in my system, because I always get
> that "Ungrabbing evdev mouse device failed" error message.
>
I'm convincing myself this is more a ploblem with kdrive evdev driver than
with kdrive itself, so I'm tempted to keep that code in
DeleteInputDeviceRequest() as is, with a mention that it's a kind of
"safety measure against buggy drivers". What do you think?


-- 
*Laércio de Sousa*
*Orientador de Informática*
*Escola Municipal "Professor Eulálio Gruppi"*
*Rua Ismael da Silva Mello, 559, Mogi Moderno*
*Mogi das Cruzes - SPCEP 08717-390*
Telefone: (11) 4726-8313
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH kdrive/ephyr v7 3/9] kdrive: introduce input hot-plugging support for udev and hal backends (#33140)

2016-02-10 Thread Laércio de Sousa 
2016-02-08 17:45 GMT-02:00 Adam Jackson :
>
> This introduces a warning:
>
> kdrive.c:1217:1: warning: no previous prototype for
> 'systemd_logind_release_fd' [-Wmissing-prototypes]
>  systemd_logind_release_fd(int _major, int _minor, int fd)
>  ^
> kdrive.c:1222:1: warning: no previous prototype for
> 'systemd_logind_vtenter' [-Wmissing-prototypes]
>  systemd_logind_vtenter(void)

 ^
>
OK. I've just introduced the prototypes in kdrive.c itself (so I don't need
to #include ). These warnings are now gone.

This seems like the sort of comment where the act of writing it is a
> sign that the code must be wrong.  Why is RemoveDevice not closing the
> fd for you?
>
I'm not sure why it happens, but without this "redundant" check around
RemoveDevice(), if I unplug the mouse, replug it and start moving it,
Xephyr segfaults.
-- 
*Laércio de Sousa*
*Orientador de Informática*
*Escola Municipal "Professor Eulálio Gruppi"*
*Rua Ismael da Silva Mello, 559, Mogi Moderno*
*Mogi das Cruzes - SPCEP 08717-390*
Telefone: (11) 4726-8313
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH kdrive/ephyr v7 3/9] kdrive: introduce input hot-plugging support for udev and hal backends (#33140)

2016-02-10 Thread Peter Hutterer 
On Mon, Feb 08, 2016 at 02:45:43PM -0500, Adam Jackson wrote:
> On Fri, 2015-12-11 at 11:43 -0200, Laércio de Sousa wrote:
> 
> > +void
> > +systemd_logind_release_fd(int _major, int _minor, int fd)
> > +{
> > +}
> > +
> > +void
> > +systemd_logind_vtenter(void)
> > +{
> > +}
> 
> This introduces a warning:
> 
> kdrive.c:1217:1: warning: no previous prototype for 
> 'systemd_logind_release_fd' [-Wmissing-prototypes]
>  systemd_logind_release_fd(int _major, int _minor, int fd)
>  ^
> kdrive.c:1222:1: warning: no previous prototype for 'systemd_logind_vtenter' 
> [-Wmissing-prototypes]
>  systemd_logind_vtenter(void)
>  ^
>  
> > @@ -2230,5 +2348,55 @@ NewInputDeviceRequest(InputOption *options, 
> > InputAttributes * attrs,
> >  void
> >  DeleteInputDeviceRequest(DeviceIntPtr pDev)
> >  {
> > +#if defined(CONFIG_UDEV) || defined(CONFIG_HAL)
> > +OsBlockSIGIO();
> >  RemoveDevice(pDev, TRUE);
> > +
> > +/*
> > + * Search for fds that were not unregistered correctly
> > + * by RemoveDevice() call and unregister them.
> > + * Code taken from KdInputDisable() implementation.
> > + */
> 
> This seems like the sort of comment where the act of writing it is a
> sign that the code must be wrong.  Why is RemoveDevice not closing the
> fd for you?

we don't have a 1:1 mapping between devices and fd (e.g. wacom devices all
hang off a single fd). Even the fd itself is a DDX concept, so RemoveDevice
cannot close the fd.

What should happen here though is that the pDev->public.devicePrivate points
to something kdrive understands and that includes the fd.

Cheers,
   Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH kdrive/ephyr v7 3/9] kdrive: introduce input hot-plugging support for udev and hal backends (#33140)

2016-02-08 Thread Adam Jackson 
On Fri, 2015-12-11 at 11:43 -0200, Laércio de Sousa wrote:

> +void
> +systemd_logind_release_fd(int _major, int _minor, int fd)
> +{
> +}
> +
> +void
> +systemd_logind_vtenter(void)
> +{
> +}

This introduces a warning:

kdrive.c:1217:1: warning: no previous prototype for 'systemd_logind_release_fd' 
[-Wmissing-prototypes]
 systemd_logind_release_fd(int _major, int _minor, int fd)
 ^
kdrive.c:1222:1: warning: no previous prototype for 'systemd_logind_vtenter' 
[-Wmissing-prototypes]
 systemd_logind_vtenter(void)
 ^
 
> @@ -2230,5 +2348,55 @@ NewInputDeviceRequest(InputOption *options, 
> InputAttributes * attrs,
>  void
>  DeleteInputDeviceRequest(DeviceIntPtr pDev)
>  {
> +#if defined(CONFIG_UDEV) || defined(CONFIG_HAL)
> +OsBlockSIGIO();
>  RemoveDevice(pDev, TRUE);
> +
> +/*
> + * Search for fds that were not unregistered correctly
> + * by RemoveDevice() call and unregister them.
> + * Code taken from KdInputDisable() implementation.
> + */

This seems like the sort of comment where the act of writing it is a
sign that the code must be wrong.  Why is RemoveDevice not closing the
fd for you?

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel