K750 fixes in hidpp-rework branch

[Martin Pitt]

Hey Peter,

Peter Wu [2013-09-02 11:22 +0200]:
> >  - hidpp_device_cmd():
> >    + Do we need to check for EINTR in the write()? Or are all
> >      commands very tiny and thus the chance of EINTRing is negligible?
> 
> When trying to debug, I encountered a few times that write/read fails due to 
> EINTR, therefore I added this check.

There are EINTR checks inhidpp_device_read_resp() and
hidpp_discard_messages(), but not in hidpp_device_cmd(), hence my
gut feeling that it should also be in hidpp_device_cmd(). Or did you
mean you already fixed it locally without pushing yet?

> >    + In this part:
> > 
> >               /* validate response */
> >               if (read_msg.type != HIDPP_MSG_TYPE_SHORT &&
> >                       read_msg.type != HIDPP_MSG_TYPE_LONG) {
> >                       /* ignore key presses, mouse motions, etc. */
> >                       continue;
> >               }
> > 
> >       This means we read an unknown message type and thus we probably
> >       did a short or too long (if there are more messages queued)
> >       read(), i. e. the next read() will be complete garbage as we
> >       start reading in the middle of some message. Can it actually
> >       happen that the next read() after a command write() is unrelated
> >       to that write()? I. e. do messages actually arrive out of band?
> 
> The hidraw API guaruantees that message boundaries are preserved. See also the 
> implementation of hidpp_discard_messages(), that only reads one byte and let 
> the kernel take care of the remainder.

Ah good, so we basically can never read beyond the current message in
a single read()? That's nice for re-syncing indeed.

> >    + After the "out:" label it unconditionally copies the read message
> >      from the local "read_msg" into the output argument "response"
> >      anyway. Any reason why it couldn't use "response" right away for
> >      the read() call and evaluation, and drop "read_msg"?
> 
> Before "hidpp: split request read/write functions", the response pointer could 
> be the same as the request. After that commit, I can now directly copy to 
> read_msg. I will fix this.

Ah, good point. But as the request pointer is const* now that
shouldn't happen any more.

> Should I name it 'tmp2' instead? No, you are right ;) The scope is limited to 
> if which is probably a better place for it. I do not want to add a new commit 
> for fixing such a tiny style issue, can I rebase it before merging?

Sure, I'm a promoter of --amend and rebase -i for having a clean
history instead of lots of little "fixup" commits as long as stuff
didn't land upstream yet :)

> I looked at the maximum "Default report interval" in the HID++ 1.0 spec (20 
> ms) and added a few ms extra. It might not make any sense at all to use that 
> value here, but it seems a safe boundary to allow the hidpp-logitech-dj driver 
> to complete initialisation.

Ack, thanks. It's not very friendly to use sleep() in D-BUS services,
but we can probably live with 30 ms.

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/devkit-devel/attachments/20130902/a1a427db/attachment.pgp>