Re: [PATCH] gpio: mxs: implement get_direction callback

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

Hello,

On Mon, Nov 17, 2014 at 04:58:15PM +0100, Janusz Użycki wrote:
> 
> W dniu 2014-11-17 o 16:53, Uwe Kleine-König pisze:
> >Hello,
> >
> >On Mon, Nov 17, 2014 at 11:05:51AM +0100, Richard Genoud wrote:
> >>2014-11-17 10:59 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>:
> >>>Hello Richard,
> >>>
> >>>>>>>>So finally the prototypes would be:
> >>>>>>>>int mctrl_gpio_request_irqs(struct mctrl_gpios*, struct
> >>>>>>>>uart_port*, irqhandler_t);
> >>>>>>>>void mctrl_gpio_free_irqs(struct mctrl_gpios*);
> >>>>>>I think:
> >>>>>>
> >>>>>>         struct mctrl_gpios {
> >>>>>>                 struct uart_port *port;
> >>>>>>                 struct {
> >>>>>>                         gpio_desc *gpio;
> >>>>>>                         unsigned int irq;
> >>>>I think it's just "int irq;" there
> >>>irqs are unsigned. Some functions returning an irq use "int", but
> >>>depending on who you ask this only for error reporting or a relict.
> >>>Use 0 for invalid/unused in mctrl_gpio*.
> >>>
> >>>>>Yes. I tried to assign irq value in mctrl_gpio_init() only.
> >>>>>There was another issue if CONFIG_GPIOLIB is not defined but it looks mctrl_
> >>>>>disable/enable_ms()
> >>>>>and mctrl_ irq handler solve the problem.
> >>>>>
> >>>>>>   Not sure there is a corresponding request_irq variant for that.
> >>>>>
> >>>>>What would you propose?
> >>>>In atmel_request_gpio_irq(), the function irq_set_status_flags(irq,
> >>>>IRQ_NOAUTOEN); is used before request_irq to prevent the irq from
> >>>>being enabled when requested.
> >>>I'm not sure this is allowed. How do you handle request_irq failing? (I
> >>>just checked: you don't.) Consider another thread just doing
> >>>request_irq($yourirq, ...) between
> >>>
> >>>         irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
> >>>
> >>>and
> >>>
> >>>         err = request_irq(irq[i], ...
> >>well, in this case, request_irq() will fail and all the previously
> >>requested irqs will be freed:
> >>     /*
> >>      * If something went wrong, rollback.
> >>      */
> >>     while (err && (--i >= 0))
> >>         if (irq[i] >= 0)
> >>             free_irq(irq[i], port);
> >Just in case you didn't notice: Your statement is right, but for the
> >other caller to request_irq there is something fishy. He gets
> >IRQ_NOAUTOEN without being able to notice ...
> 
> Likely the gpio interrupts will never shared. We can say mctrl_gpio
> is the only owner
> of a gpio after a request.
Right. The important part of your sentence is: "after a request". So at
the time irq_set_status_flags(..., IRQ_NOAUTOEN) is called, you're not
yet owning it. At a minimum you must clear the flag in the error path.

I'd like to have a statement from Thomas here if this is considered OK.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html