K750 fixes in hidpp-rework branch

[Peter Wu]

On Monday 02 September 2013 11:34:47 Martin Pitt wrote:
> 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?

Oh actually, I thought it was about the read()s. I assumed that the write 
would be very fast, not having a time to get interrupted. This has never shown 
an issue so far, so I did not bother checking for it.

> > 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.

Correct.

> > >    + 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.

The reason it does not happen anymore is because some fields are passed as 
separate parameters instead of one struct, not because the request pointer is 
const. In all callers, the request pointer equals the response pointer.

> > 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 :)

Fixed and pushed!

> > 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.

For the future we might consider using polling instead, but right now the hid-
logitech-dj has some issues (XHCI-related?) on some systems and there is a new 
patch set that also changes enumeration behavior[1][2]. That seems to be an 
unstable base for trying to drop this 30ms sleep.

Regards,
Peter

 [1]: https://code.google.com/p/chromium/issues/detail?id=221455
 [2]: https://git.lekensteyn.nl/peter/linux/log/?h=logitech-wtp
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/devkit-devel/attachments/20130902/820c9b58/attachment.pgp>