On Wed, Jan 11, 2012 at 10:36:16PM +0100, Chase Douglas wrote:
> On 01/11/2012 06:18 PM, Dmitry Torokhov wrote:
> > On Wed, Jan 11, 2012 at 11:09:18AM +0100, Chase Douglas wrote:
> >> On 01/11/2012 11:04 AM, Henrik Rydberg wrote:
> >>>>> Applied, however I removed stable notation as this change IMO does not
> >>>>> qualify for the stable since it does not address a regression.
> >>>>
> >>>> It's not a regression per-se, but we want to fix buttonpad support, and
> >>>> we can't do that without this patch. It's a clear bug that when the
> >>>> property was added we did not set the property in the devices that need it.
> >>>
> >>> The current behavior depends on userspace and is not a kernel bug,
> >>> please stop the nonsense already.
> >>>
> >>> For bcm5974 devices, extracting buttonpad properties has been possible
> >>> since early 2009 (158e9287). The mechanism, predating the input
> >>> properties interface by nearly two years, has been used in the
> >>> multitouch and mtrack X drivers ever since. To those users, the
> >>> present patch has no effect at all.
> >>
> >> Just because an alternative interface has existed does not mean there
> >> isn't a bug.
> >
> > No, there isn't a bug. The driver simply does not support new interface
> > yet. In all our discussions, AFAIR, property bits were always supposed
> > to carry only advisory role, i.e. if a driver sets them up then
> > userspace has it easy and can act upon them. Absence of a property does
> > not guarantee anything.
>
> The problem is that we can't determine "absence" from "false". If
> EVIOCGPROP returned an error when the driver did not support property
> bits, then that would be different. However, if a driver does not set
> property bits it appears to userspace as though all properties are false.
Here is what I said earlier when we discussed Henrik's patch:
'As long as we document that !pointer && !direct means "unknown" or
"unspecified" so as to avoid having to update all devices at once that
should be OK.'
The same is applicable to other properties as well. If device is telling
you that it is a "buttonpad" you can trust it, but if it does not you
need to decide for yourself how to treat it.
>
> Henrik's proposal of using the bcm5974-specific driver capabilities
> interface would require code like:
>
> Get props
> if (props == 0) {
> try bcm5974-specific interface
> try ...-specific interface
> try ...-specific interface
> }
Right, except that there are no more "xxx-specific" interfaces, only bcm
one.
>
> This is not a reasonable solution for userspace. If you're going to
> argue that we need a thread-safe ioctl to get the current touch slot
> state, you can't honestly ask userspace to do this :).
Yes, I honestly can.
>
> Because we can't determine "absence" separate from "falseness", the
> interface can't really be "advisory", which I don't remember being the
> case anyways. To illustrate the point, proper click and drag support in
> X will require knowing whether a trackpad is a buttonpad. It may be
> required for multitouch support (we're still working out the details for
> trackpads). We will be filing more patches to add buttonpad properties
> to hid-magicmouse, synaptics, and maybe more.
>
> Any distro that will ship a new X.org with multitouch support and a
> kernel that is 3.2 or earlier will require these patches to function
> properly.
So are you trying to make an argument that we need to backport this
patch all the way to 2.6.30? Along with the EVIOCGPROP itself since it
was only added to 2.6.38 and therefore your new userspace would fail
miserably on kernels earlier than .38?
> Think of all the android devices out there with older kernels.
> People may want to flash distros with latest X.org on top of android
> kernels. Locked bootloaders means we can't be sure we can replace the
> kernel, even when we actually get the sources :).
Exactly, so here you give the perfect argument why the patch is not
needed for stable. Design your userspace as if kernel isn't upgradable.
>
> > Hmm, speaking of properties, shouldn't we set INPUT_PROP_POINTER on
> > bcm5974 while we are at it?
>
> I'm still confused on what INPUT_PROP_POINTER means, and how it is
> different from INPUT_PROP_DIRECT. I *think* we all agree on what
> INPUT_PROP_DIRECT means, but I don't want to set INPUT_PROP_POINTER on
> any devices before we have a clear definition.
I'll make it a bit easier - synaptics is already marked as pointer
device (which it is), so making other touchpads report
INPUT_PROP_POINTER makes things not only right but consistent.
>
> Henrik, can you comment on the documentation patches? You wrote the
> patch, so you hopefully know what's going on :).
>
> >> A device that has a physical property, but does not set the
> >> property bit in the driver is a real bug that needs to be fixed.
> >> Userspace should not have to quirk around broken implementations.
> >>
> >> It's true that userspace can quirk around things in a kernel that
> >> predates the property bits, but where the property bits are available
> >> the devices *must* set them or else things will break.
> >
> > If you are already supporting older kernels that do not support property
> > bits then you should already be set.
> >
> > Anyway, this is not a regression and not even new hardware enablement:
> >
> > " - It must fix a problem that causes a build error (but not for things
> > marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
> > security issue, or some "oh, that's not good" issue. In short,
> > something critical.
> > - New device IDs and quirks are also accepted."
> >
> > Therefore I do not feel that stable nomination is warranted. You may
> > still send it to stable, I'll add similar comment to that request and
> > leave it up to Greg to decide if he still wants to put it into stable.
>
> It doesn't fit exactly any of the categories, but stable isn't really
> cut-and-dry. What do you think should happen if a patch is not
> "critical" according to the above definitions, but would nonetheless be
> required by many distros because without it things would not work
> properly. Should every distro apply the patch individually, after
> hitting bugs and spending valuable developer time tracking down the fix,
> or should the patch be sent through the stable queue so everyone can
> benefit?
>
> Obviously, I think it makes sense to send a safe patch like this through
> the stable queue so everyone benefits (and I still believe this is a
> "bug fix" due to the inability to determine "absence" of a property). If
> you disagree, we can easily distro-patch this and every other patch we
> send for property bits.
You are doing a disservice to your users by producing userspace that
makes EVIOCGPROP mandatory (disservice because they can not easily
downgrade the kernel) but it is your choice.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
