Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1208251 > unrolled thread

Re: [PATCH] Input: psmouse - add small delay for IBM trackpoint pass-through mode

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH] Input: psmouse - add small delay for IBM trackpoint  pass-through mode Andreas Mohr <andi@lisas.de> - 2015-08-16 17:50 +0200
    Re: [PATCH] Input: psmouse - add small delay for IBM trackpoint  pass-through mode Stefan Assmann <sassmann@kpanic.de> - 2015-08-17 09:40 +0200

#1208251 — Re: [PATCH] Input: psmouse - add small delay for IBM trackpoint pass-through mode

Hi,

[rogue out-of-band reply, sorry - lkml.org mail info is broken]

> There are trackpoint devices that fail to respond to the PS2 command
> PSMOUSE_CMD_GETID if immediately queried after the parent device is
> deactivated. Add a small delay for the hardware to get in a sane state
> before sending any PS2 commands.

Hmm, "deactivated"?
Probably a parent needs to be "activated" for a passthrough device
(child device?)
to be able to communicate? (I don't know much about these things though...)


> +		usleep_range(10000, 15000);

Ah, used _range() API - strong bonus points
for caring about wakeup minimization! :)

(and I take it you surely cared to check proper device operation
at both cases of doing usleep()
with either upper or lower delay amount specified... ;)




In general it's somewhat sad
to see an unconditional implementation
via woefully imprecise delay-only operation here -
is there a way to have it implemented
as a properly *handshaked* protocol,
i.e. try (re-)doing some other query type
which would fail until init is ok or timeout?
OTOH in that case PSMOUSE_CMD_GETID probably happens to be
just that kind of "handshaked success" query type to be used here...


Or, IOW (pseudo code):

delay;
if (!query()) fail;

sounds rather worse from a handshaked-protocol POV than

while (retries_remaining)
    if (query()) break;
    delay;


This reasoning would probably suggest
that such a loop should be added *within* psmouse_probe(), at the first
check (i.e., PSMOUSE_CMD_GETID).

OTOH that handshaked loop is only feasible
if doing repeated PSMOUSE_CMD_GETID query attempts
is actually supported (tolerated) by devices
(vs. no-op delaying and *then* trying one time only),
i.e. that repeated PSMOUSE_CMD_GETID queries will succeed
rather than fail or even block...



BTW, to make it obvious why non-handshaked operation may easily end up worse:
certain devices (out of a couple thousands of different
China-made human interface device models
which will be relevant here ;)
might perhaps *require* getting queried immediately *without* any prior delay,
in which case the first (AND LAST!) PSMOUSE_CMD_GETID query
after the (your) delay
would now already fail for those devices...



HTH,

Andreas Mohr

-- 
GNU/Linux. It's not the software that's free, it's you.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1208433

On 16.08.2015 17:33, Andreas Mohr wrote:
> Hi,
> 
> [rogue out-of-band reply, sorry - lkml.org mail info is broken]

Hi Andreas,

> 
>> There are trackpoint devices that fail to respond to the PS2 command
>> PSMOUSE_CMD_GETID if immediately queried after the parent device is
>> deactivated. Add a small delay for the hardware to get in a sane state
>> before sending any PS2 commands.
> 
> Hmm, "deactivated"?
> Probably a parent needs to be "activated" for a passthrough device
> (child device?)
> to be able to communicate? (I don't know much about these things though...)

Comment from few lines above where I put the code.
/*
 * If this is a pass-through port deactivate parent so the device
 * connected to this port can be successfully identified
 */

> 
> 
>> +		usleep_range(10000, 15000);
> 
> Ah, used _range() API - strong bonus points
> for caring about wakeup minimization! :)
> 
> (and I take it you surely cared to check proper device operation
> at both cases of doing usleep()
> with either upper or lower delay amount specified... ;)

Yes, I went as low as 10ms and figured it still works. Then I added
another 5ms if the kernel wants to wake up at a later time. If that's
too much we can decrease the upper limit, although this should only
happen once per boot.

> 
> 
> 
> 
> In general it's somewhat sad
> to see an unconditional implementation
> via woefully imprecise delay-only operation here -
> is there a way to have it implemented
> as a properly *handshaked* protocol,
> i.e. try (re-)doing some other query type
> which would fail until init is ok or timeout?
> OTOH in that case PSMOUSE_CMD_GETID probably happens to be
> just that kind of "handshaked success" query type to be used here...

Thought about that too, but I know too little about the hardware to give
you a proper answer. I'm not even sure why PSMOUSE_CMD_GETID sometimes
fails at all. I assume that hardware needs some time to settle down
after the parent gets deactivated to accept commands.

> 
> 
> Or, IOW (pseudo code):
> 
> delay;
> if (!query()) fail;
> 
> sounds rather worse from a handshaked-protocol POV than
> 
> while (retries_remaining)
>     if (query()) break;
>     delay;
> 
> 
> This reasoning would probably suggest
> that such a loop should be added *within* psmouse_probe(), at the first
> check (i.e., PSMOUSE_CMD_GETID).
> 
> OTOH that handshaked loop is only feasible
> if doing repeated PSMOUSE_CMD_GETID query attempts
> is actually supported (tolerated) by devices
> (vs. no-op delaying and *then* trying one time only),
> i.e. that repeated PSMOUSE_CMD_GETID queries will succeed
> rather than fail or even block...
> 
> 
> 
> BTW, to make it obvious why non-handshaked operation may easily end up worse:
> certain devices (out of a couple thousands of different
> China-made human interface device models
> which will be relevant here ;)
> might perhaps *require* getting queried immediately *without* any prior delay,
> in which case the first (AND LAST!) PSMOUSE_CMD_GETID query
> after the (your) delay
> would now already fail for those devices...

All you're saying is correct. Please note that I carefully limited the
sleep to only SERIO_PS_PSTHRU devices, so the majority of devices out
there is not going to see this at all. IIRC, PSMOUSE_CMD_GETID is the
initial command to be sent to a PS2 device, so a delay before that
should not do any harm. Of course I cannot prove that this will not have
any side-effect on some random hardware, but I'm really trying to narrow
down the number of affected devices to those who may profit from the
change.

Thanks for the feedback.

  Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web