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-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
