gpioc user-space interrupt handling

[bobf_mrp3.com]

Summary

based on the GSoC submission for

https://wiki.freebsd.org/SummerOfCode2018Projects/UserSpaceGPIOinterrupts

github site:

https://github.com/ckraemer/freebsd/tree/gsoc2018

generated from diff at:

https://github.com/freebsd/freebsd/compare/master...ckraemer:gsoc2018.diff

Test Plan

review the code changes (as submitted) for functionality and completeness

test the implementation as-designed, correcting any problems noted

[test using an interrupt driven application that handles a rotating quadrature encoder]

verify and/or create documentation, as needed

consider additional enhancements (such as kernel messages, gpioctl support)

[bobf_mrp3.com]

applied diff to most recently obtained revision (r338415), added as-is. No build nor startup problems noted.

[bobf_mrp3.com]

./sys/dev/gpio/gpioc.c

60

gpioc_softc was moved from around line 66 in the original source. 'sc_npins' and 'sc_pin_intr' were added.

142

gpioc_softc was moved to line ~60

601

I'm not convinced that returning ENXIO here is the best idea. If no interrupts were configured, is it REALLY that bad of an error condition? As a character device it won't return an EOF but it could return a zero byte transfer like /dev/null probably does.

[bobf_mrp3.com]

./sys/dev/gpio/gpioc.c

776

what happens if 2 interrupt pins trigger simultaneously, or very close to that

[bobf_mrp3.com]

./sys/dev/gpio/gpioc.c

82

if more than one interrupt happens before the first one is processed, is there some way to hold off processing so you don't miss one? Or at least there should be a way of indicating multiple interrupts, and managing their individual states instead of causing an error later on by 'last_intr_pin' having an actual pin number in it for the last interrupt pin. Realistically I could configure many interrupt pins, something reasonable if I have a rotating quadrature encoder [which would have 4 of them].

90

obviously there's a collection of gpioc_cdevpriv structures being referred to here, but other functions later on make it look like there should be only one... so how many are there? TODO: check to see if the SLIST_ENTRY is something that's common for all devices or just this one... [as far as I can tell, only this one]

157

worth point out, this function releases the mutex, calls some stuff, then re-locks the mutex. I don't like it when functions act this way, so I need to make sure that this is really 'best practices' here, and fix it if it is not.

226

this part confuses me, why there are two separate structures, why the need for a consistency check in the first place. I'll need to see if anything similar to this is being done anyplace else in the kernel, but I doubt it.

824

why is this assigning 'sizeof()' and not the value of last_intr_pin ?

[bobf_mrp3.com]

./sys/dev/gpio/gpioc.c

349

I do not believe that this function will even work, other than allocating gpioc specific structures. Specifically, it's not calling PIC_SETUP_INTR or anything similar to actually set up a pin as an interrupt source. Is it necessary to use overlays for pins that can generate an interrupt for a user-space handler?

359

a potentially infinite wait loop while the first mutex is locked - I think that could potentially deadlock. need to check best practices for having sequential mutex locking, if a wait loop is necessary, etc. etc. and whether or not deadlocking is likely.

680

unlike using GPIO_PIN_SETFLAGS the call to gpioc_set_intr_config doesn't do a call to PIC_SETUP_INTR or anything similar that's located within the actual hardware driver itself - so how can it enable inerrupts? How can I even get an interrupt generated? Maybe if the OFW/FDT info has an interrupt configured already, will THAT work? In any case there doesn't seem to be a way to toggle the interrupt state 'on the fly' at all... and requiring overlays to use user-space interrupt handling seems a bit too much.

[bobf_mrp3.com]

./sys/dev/gpio/gpioc.c

359

maybe not a problem, this function is only called from gpioc_ioctl and not with a mutex locked. it's using mutex_sleep to be woken up later once config_locked is false. This may not be the best practice, however it's being used elsewhere including gpiobus.c . the name 'gpicfg' may not be ideal, however.

[bobf_mrp3.com]

re-did diff to include all lines

[bobf_mrp3.com]

./sys/dev/gpio/gpioc.c

162

I just noticed, the lock state isn't being restored on this error return. oops.

[bobf_mrp3.com]

./sys/dev/gpio/gpioc.c

90

additionally, does this whole mod need '#ifdef INTRNG' around it? It seems to me that INTRNG is needed for this... especially since gpio_alloc_intr_resource() relies on it [otherwise it's really just a stub returning NULL].

[bobf_mrp3.com]

./sys/dev/gpio/gpiobus.c

86

one thing I've observed is that the changes to gpiobus.c should be applied to ofw_gpiobus.c as well; otherwise, things using FDT won't work!!!

[bobf_mrp3.com]

./sys/dev/gpio/gpiobus.c

86

after a careful check, it looks like there aren't any that apply to ofw_gpiobus (yet).

[bobf_mrp3.com]

./sys/dev/gpio/gpioc.c

471

it occurs to me that all of these mallocs (in a loop, see below) are a waste of kernel resources. It would be a _LOT_ better to do a single malloc, then point the pointers at the apropriate place within the malloc'd block. This is a TODO item to improve the thing, once it's actually working.

[imp]

./sys/dev/gpio/gpioc.c

822

After an unconditional return, it's the style of FreeBSD to not use else {} but just have that code there.