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

[Marc Balmer <mbalmer%NetBSD.org@localhost>]

Am 24.08.11 00:00, schrieb Mindaugas Rasiukevicius:
> 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.

Well, you need to open it first, before you can to ioctl, and if only
one process can open it, only one process can ioctl it, right?

> 
>> +    mutex_enter(&sc->sc_mtx);
>> +    if (sc->sc_opened)
>> +            return EBUSY;
> 
> This leaks the lock.

Yes, this part is utterly wrong.

> 
>>      int                      sc_opened;
>> +    kmutex_t                 sc_mtx;
> 
> Preferred suffix is _lock, rather than _mtx, so please use sc_lock.

Yes, no problem.

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

Sure, no problem either.