Re: Adding pulse support to gpio(4), gpioctl(8)

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Marc Balmer <mbalmer%NetBSD.org@localhost> wrote:
> Here comes the third iteration of the gpio(4) patch.  In addition to
> whjat I already described, u_int_XXX types have been replaced by
> uint_XXX and gpio(4) is made MPSAFE.  Comments?
> 
> Furthermore I am thinking if it would be useful if more than one process
> could open a gpio device, as a long as they use different pins, e.g. one
> process controlls a stepper motor using some of the pins while another
> process drives some LEDs.

There is nothing what prevents from multiple threads calling gpioioctl(),
which is obviously not MP-safe.  As soon as you will start fixing this, it
will bring you back to the point I have already stated - the design needs
to be MP-safe in general.

> +     mutex_enter(&sc->sc_mtx);
> +     if (sc->sc_opened)
> +             return EBUSY;

This leaks the lock.

>       int                      sc_opened;
> +     kmutex_t                 sc_mtx;

Preferred suffix is _lock, rather than _mtx, so please use sc_lock.

> +             sc->sc_pins[pin].pin_ticks_on = tvtohz(&pulse->gp_pulse_on);
> +             sc->sc_pins[pin].pin_ticks_off = tvtohz(&pulse->gp_pulse_off);
> +             if (sc->sc_pins[pin].pin_ticks_on == 0
> +                 || sc->sc_pins[pin].pin_ticks_off == 0) {
> +                     sc->sc_pins[pin].pin_ticks_on = hz / 2;
> +                     sc->sc_pins[pin].pin_ticks_off = hz / 2;
> +             }

Use gpio_pin_t *gpin = &sc->sc_pins[pin]; and gpin variable instead of
sc->sc_pins[pin] each time.  Apart from better readability, GCC will most
likely generate a better code.

-- 
Mindaugas