cyapa(4), driver for the Cypress APA I2C trackpad

[grembo]

Summary

This touchpad is found in the Acer c720 chromebook. The driver
has been originally ported from DragonFly BSD, but was changed
a lot both in terms of usability and technology. Those changes
include (but are not limited to):

• different touchpad layout
• two finger scrolling
• configurable thumb area
• configurable minimum pressure
• configurable tap to click
• kqueue support
• FreeBSD like device numbering
• improved PS/2 emulation, so that the driver works just fine using moused (see man page).

Unfortunately I don't have access to any data sheets,
so development depended on Matt Dillon's work (and some
trial).

The device shows up properly in devinfo:

ig4iic0
  smbus0
    cyapa0

Sysctl output:

$ sysctl dev.cyapa.0
dev.cyapa.0.%parent: smbus0
dev.cyapa.0.%pnpinfo: 
dev.cyapa.0.%location: addr=0x67
dev.cyapa.0.%driver: cyapa
dev.cyapa.0.%desc: Cypress APA I2C Trackpad

Test Plan

I tested the driver myself for about half a year and I know
of at least two more individuals using it on the same hardware.

Man pages have been checked using igor and mandoc -Tlint.

kldload ig4
kldload cyapa
igor cyapa.4
mandoc -Tlint cyapa.4

[kib]

sys/dev/cyapa/cyapa.c

127

cyapa.h is not generated, it lives in sources. Use the #include <path/to/driver/cyapa.h> construct.

280

I am not sure what deadlock do you mean. Is it because cyapa_sc->mutex is the lock for knotes ? Why not use KNOTE_LOCKED() and require the lock on the _notify() entry ?

287

Is it fine to have lost wakeup there ?

442

No initialization in the declaration.

446

if (bus != NULL)

Why this check is needed at all ?

459

what is this ? why ?

not to mention that we have pause(9)

461

if (error != 0)

478

How is it possible ?

481

This is not needed, sc is initialized by newbus.

489

Is it possible ?
I think this could be changed to KASSERT if you are so cautious.

Also, shouldn't the attach do cleanup, e.g. call mtx_fini, if it returns error ?

543

Do you need full kernel process, or could the kernel thread be enough ?

Kernel process creates the whole process structure, in particular, a separate address space. Threads eat less memory.

555

Why atomic ?

559

This is ugly and not robust. Look at other examples in kernel how to do this race-free.

561

How could devnode be NULL ?

581

How this could be ?

585

This is N, for N > 1000, attempt to make an exclusive open on the cdev. It does not work (e.g., file descriptor is duped on fork, or it could be passed over the unix domain socket).

If you need to track per-open data, use cdevpriv(9).

But from the quick look over the code, I do not see why do you need this as well. If user opens the device several times, he is stubbing himself into the eye, but kernel is completely fine with the arrangement. So why care ?

733

If driver did read any data from the device, and later an error occured, the driver must return the read data to userspace, and supress the error.

[grembo]

share/man/man4/cyapa.4

34

I won't fix this one, as it's part of the "standard" phrase used in those drivers. I'll change the other occurrences though.

94

Right now those ranges are not really limited and I can't tell for sure which values will cause which results, therefore I won't add those explanations right now, ok?

170

It's important to point out that tap to click is only enabled for left and right mouse button events (I'll leave out naming the middle button as well). The background is, that tap to click can be executed by accident quire easily and unintended pasting with the middle button in X can have quite disastrous consequences.

Please verify the new version is ok (it feels awkward style wise)

sys/dev/cyapa/cyapa.c

280

KNOTE is lockless and is therefore using the global knlist_lock right now. I have to agree that I don't see a reason not to reverse the logic here.

I changed it like you suggested and added sc->mutex as the lock for knotes.

287

Yes

459

this was a leftover, removed it

489

With the stable version of the driver this shouldn't happen, so I removed it. Cleanup is a good point, I moved initialization of the mutex below the last early return to get the same effect.

543

a thread should work as well, will change this.

559

I changed the code to use a thread and locking.

581

I removed all of these now from various functions, assuming that they can only be called will the device is still alive (a lot of defensive coding in this driver tbh).

585

good call

733

Entering the while loop, only mtx_sleep interrupted could cause an error, otherwise error will be 0 and data is still in the fifo. So the data is still in there and ready in case the user tries another blocking read (as it only applies to blocking). So this part shouldn't be an issue.

The error returned my uiomove might be a bigger issue (as data might have been read from the fifo) therefore I'll add a condition at the end of the code (basically: if didread error = 0)

1008

Are you sure this applies here? Imho there's nothing read here that could be returned to the user in a useful way.

[grembo]

Changes based on @wblock's review of the man page and
@kib's review of the driver code.

Tried to address all concerns, please see my comments for
cases where I didn't intentionally.

[kib]

sys/dev/cyapa/cyapa.c

353

Use pause(9), for all such tsleeps.

485

Note that by creating the cdev so early, you expose the devfs node to the user requests before the rest of the initialization is done. Currently there is no way to handle si_drv1 assignment race-free, but the rest could be improved.

534

No initialization at declaration, check all places.

561

Still, there is an attempt to enforce single-open. It does not work.

710

Is it safe to drop the lock this way, you are accessing the driver data unlocked afterward.

And, fifo_read() asserts the lock, from what I see.

[grembo]

More changes based on @kib's code review.

sys/dev/cyapa/cyapa.c

561

Nope, this is not the point here, at least not intentionally. The purpose of count is to keep track *if* the device is opened at all. If the device is closed, the driver reduces the polling frequency to save power.

I changed this now, so that it's locked properly and uses devfs_set_cdevpriv. The way it's written it assumes that for every open call, there will be a dtor call as well (increase/decrease the counter). If this assumption is wrong and the approach stupid, please point me to a different solution that allows the driver to know if the device is open (regardless of the number of consumers).

710

The call to fifo_read essentially just retrieves a pointer, I moved this up within the locked scope now.

The call to uiomove itself should be save, as the data it's copying can only be overwritten when fifo_read adjusted rindex etc., which happens in the call three lines below this and within the locked scope again. There are no other calls to fifo_read* on sc->rfifo outside of this function that would modify rindex.

[kib]

sys/dev/cyapa/cyapa.c

124

Headers should be ordered alphabetically, except param.h being first header, and some exceptions caused by dependencies.

155

struct cdev *devnode;

276

Empty line after '{' if no locals. Look for other places with the same style(9) bug.

294

Assert that the lock is owned.

452

I think that the comment means that you should check dmiinfo to ensure that you are on the right platform. Otherwise other machine might use 0x67 as the address of the incompatible device.

577

Put { on the same line as if and as else.

1053

spaces around '|', as should be around any binary op.

1178

This lock could be argued to have some uses, e.g. it ensures that the assignment is seen by other threads taking the sc lock. Also, I suggest to enter the loop locked. Then, you should not unlock after testing sc->detaching. This would also fix the bug of not locking the sc before poll_thread_running = 0 assignment (the unlock is unpaired).

1208

But this lock is useless. The result of the read could be invalidated immediately after the lock is dropped. This is not a serious thing, because all it would do is cause wrong poll frequency for one cycle. So you could just read unlocked and have the same race.

[grembo]

More changes based on @kib's review.

sys/dev/cyapa/cyapa.c

452

Other operating systems as well as other IIC drivers use the same mechanism, so I think it's ok. The driver has to be loaded explicitly anyway and worst case initialization will fail.

Should I simply remove the comment?

1178

I don't think holding the lock during an entire loop cycle is necessary.

Please note that the only way to leave the for loop is the break statement on line 1240, so the lock is always held in the poll_thread_running = 0 assignment:

        cyapa_lock(sc);
        if (sc->detaching)
                break;
        cyapa_unlock(sc);
}
sc->poll_thread_running = 0;
cyapa_unlock(sc);

[kib]

sys/dev/cyapa/cyapa.c

1179

No, I propose to unlock as the first action in the loop body. This + remove unlock in the last line of the loop body.

[grembo]

Change locking in for (now while) loop

[grembo]

sys/dev/cyapa/cyapa.c

1180

That makes a lot of sense. I also changed it to a while loop now, so that the condition is at the top.

[kib]

There are still small style(9) violations, but I do not want to waste time on them.

I read the patch mostly to correct errors I can see with the use of the FreeBSD KPI. I did not even tried to understand the logic of the device management. I am fine with the aspects of code I understand.

Also, I did not verified the build. Right now the driver is built on all arches, is it reasonable ?

sys/dev/cyapa/cyapa.c

453

No, I think comment must stay to at least give a hint what platform it is supposed to work on. But IMO it would be better to look at DMI data to not fiddle with unknown hardware.

[grembo]

Also, I did not verified the build. Right now the driver is built on all arches, is it reasonable ?

Based on a quick check on google that trackpad is also found in some ARM devices (there's some Samsung ARM Chromebook using) , not sure if thats arm or arm64 though. I can't test it on any of those, but leaving it in would at least allow users of these machines to test and file a bug report.

That said, would this look reasonable to you:

# $FreeBSD: head/sys/modules/i2c/Makefile 93023 2002-03-23 15:49:15Z nsouch $

SUBDIR =
SUBDIR += controllers if_ic smbus iicbus iicbb iicsmb iic smb
.if ${MACHINE_CPUARCH} == "i386" || ${MACHINE_CPUARCH} == "amd64" ||
    ${MACHINE_CPUARCH} == "arm" || ${MACHINE_CPUARCH} == "arm64"
SUBDIR += cyapa
.endif

(the same would be done to man/man4/Makefile of course)

sys/dev/cyapa/cyapa.c

453

I might look into that in a separate commit, it sounds interesting.

[kib]

Is the same smbus address used on other platforms ? Apparently the question of the arches and device_attach() could be related.

[grembo]

In D3068#63643, @kib wrote:

Is the same smbus address used on other platforms ? Apparently the question of the arches and device_attach() could be related.

According to ARM Chromebook XE303C12-A01UK Kernel dmesg + /proc/cpuinfo yes:

[    0.868998] cyapa 1-0067: Cypress APA Trackpad Information:
[    0.869002]     Product ID:  CYTRA-116002-00
[    0.869004]     Protocol Generation:  3
[    0.869007]     Firmware Version:  2.4
[    0.869009]     Hardware Version:  2.0
[    0.869012]     Max ABS X,Y:   1280,680
[    0.869014]     Physical Size X,Y:   98,55
[    0.869111] input: Cypress APA Trackpad (cyapa) as /devices/s3c2440-i2c.1/i2c-1/1-0067/input/input1

[imp]

In D3068#63620, @grembo wrote:

Also, I did not verified the build. Right now the driver is built on all arches, is it reasonable ?

Based on a quick check on google that trackpad is also found in some ARM devices (there's some Samsung ARM Chromebook using) , not sure if thats arm or arm64 though. I can't test it on any of those, but leaving it in would at least allow users of these machines to test and file a bug report.

That said, would this look reasonable to you:

# $FreeBSD: head/sys/modules/i2c/Makefile 93023 2002-03-23 15:49:15Z nsouch $

SUBDIR =
SUBDIR += controllers if_ic smbus iicbus iicbb iicsmb iic smb
.if ${MACHINE_CPUARCH} == "i386" || ${MACHINE_CPUARCH} == "amd64" ||
    ${MACHINE_CPUARCH} == "arm" || ${MACHINE_CPUARCH} == "arm64"
SUBDIR += cyapa
.endif

(the same would be done to man/man4/Makefile of course)

Is there some reason not to compile it on other architectures? It's a generic i2c device, from all appearances. The fewer crazy if statements like this that we have the better. We already have *WAY* too many of them.

[wblock]

share/man/man4/cyapa.4

150

I don't understand this. So level 1 is not supported, but still the default? Maybe "and" should be "through".

155

Needs a comma (after a space):
.Xr moused 8 ,

[grembo]

In D3068#63711, @imp wrote:

In D3068#63620, @grembo wrote:

Also, I did not verified the build. Right now the driver is built on all arches, is it reasonable ?

Based on a quick check on google that trackpad is also found in some ARM devices (there's some Samsung ARM Chromebook using) , not sure if thats arm or arm64 though. I can't test it on any of those, but leaving it in would at least allow users of these machines to test and file a bug report.

That said, would this look reasonable to you:

# $FreeBSD: head/sys/modules/i2c/Makefile 93023 2002-03-23 15:49:15Z nsouch $

SUBDIR =
SUBDIR += controllers if_ic smbus iicbus iicbb iicsmb iic smb
.if ${MACHINE_CPUARCH} == "i386" || ${MACHINE_CPUARCH} == "amd64" ||
    ${MACHINE_CPUARCH} == "arm" || ${MACHINE_CPUARCH} == "arm64"
SUBDIR += cyapa
.endif

(the same would be done to man/man4/Makefile of course)

Is there some reason not to compile it on other architectures? It's a generic i2c device, from all appearances. The fewer crazy if statements like this that we have the better. We already have *WAY* too many of them.

It should build on all platforms. Following @imp's advise I'll commit it without the if statement, if anyone dislikes this feel free to add it yourself.

share/man/man4/cyapa.4

150

They're all supported, so does this look better?

It supports
.Xr moused 8
levels 0 through 2, level 1 is used by default.

155

added

[kib]

In D3068#63711, @imp wrote:

Is there some reason not to compile it on other architectures? It's a generic i2c device, from all appearances. The fewer crazy if statements like this that we have the better. We already have *WAY* too many of them.

Note that the driver assumes that whatever device is found on the given iic bus at the address 0x67, it is his own. This might have quite unforeseen consequences.

Anyway, I do not plan to read this code more, feel free to commit.

[grembo]

In D3068#63930, @kib wrote:

In D3068#63711, @imp wrote:

Is there some reason not to compile it on other architectures? It's a generic i2c device, from all appearances. The fewer crazy if statements like this that we have the better. We already have *WAY* too many of them.

Note that the driver assumes that whatever device is found on the given iic bus at the address 0x67, it is his own. This might have quite unforeseen consequences.

I will look into improving this.

@kib Thank you so much for your review, it was extremely helpful.

[grembo]

This is committed now, thanks again to @kib and @wblock for their most helpful reviews, I hope it didn't consume too much of your time.

The commit differs in two ways from this last recorded version, I added "sc" as a parameter to a couple of fifo calls, so that mtx_assert actually catches on (turns out that I didn't test with INVARIANTS enabled, that's why I missed a problem kib pointed out during the review), and I added this BUG section to the man page:

.Sh BUGS
The
.Nm
driver detects the device based on its I2C address (0x67).
This might have unforeseen consequences if the initialization sequence
is sent to an unknown device at that address.

@wblock: Feel free to make this sound nicer.

[jhb]

A few minor nits/suggestions.

head/sys/dev/cyapa/cyapa.c

152 ↗

(On Diff #7304)

This doesn't appear to be used (it's initialized, but nothing ever reads its value).

466 ↗

(On Diff #7304)

This looks to be unused?

489 ↗

(On Diff #7304)

I think smbus_get_addr() here would be cleaner (cyapa_probe already uses it).

[grembo]

@jhb: Please see D3351