Re: [PATCH 1/2] gpio/davinci: fix oops on unbanked gpio irq request

Re: [PATCH 1/2] gpio/davinci: fix oops on unbanked gpio irq request

[Grant Likely]

On Sun, 11 Mar 2012 18:16:11 +0530, Sekhar Nori <nsekhar@ti.com> wrote:
> Unbanked GPIO irq setup code was overwriting chip_data leading
> to the following oops on request_irq()
> 
> Unable to handle kernel paging request at virtual address febfffff
[...]
> Fix the issue.

Applied, but for next time "Fix the issue" isn't the most helpful commit text.
Please describe *how* you fixed the issue so that future commit readers have
more clues as to what this patch is doing.

g.

RE: [PATCH 1/2] gpio/davinci: fix oops on unbanked gpio irq request

[Nori, Sekhar]

Hi Grant,

On Mon, Mar 12, 2012 at 23:21:35, Grant Likely wrote:
> On Sun, 11 Mar 2012 18:16:11 +0530, Sekhar Nori <nsekhar@ti.com> wrote:
> > Unbanked GPIO irq setup code was overwriting chip_data leading
> > to the following oops on request_irq()
> > 
> > Unable to handle kernel paging request at virtual address febfffff
> [...]
> > Fix the issue.
> 
> Applied, but for next time "Fix the issue" isn't the most helpful commit text.
> Please describe *how* you fixed the issue so that future commit readers have
> more clues as to what this patch is doing.

Thanks for the feedback and noted for future. If it's not too much trouble, 
can you change the last line in commit message to:

Fix the issue by making unbanked GPIO code not overwrite chip_data because 
generic irq chip code uses it for its data structures. Instead use 
handler_data to pass around GPIO controller information.

Regards,
Sekhar

RE: [PATCH 1/2] gpio/davinci: fix oops on unbanked gpio irq request

[Grant Likely]

On Mon, 12 Mar 2012 18:54:53 +0000, "Nori, Sekhar" <nsekhar@ti.com> wrote:
> Hi Grant,
> 
> On Mon, Mar 12, 2012 at 23:21:35, Grant Likely wrote:
> > On Sun, 11 Mar 2012 18:16:11 +0530, Sekhar Nori <nsekhar@ti.com> wrote:
> > > Unbanked GPIO irq setup code was overwriting chip_data leading
> > > to the following oops on request_irq()
> > > 
> > > Unable to handle kernel paging request at virtual address febfffff
> > [...]
> > > Fix the issue.
> > 
> > Applied, but for next time "Fix the issue" isn't the most helpful commit text.
> > Please describe *how* you fixed the issue so that future commit readers have
> > more clues as to what this patch is doing.
> 
> Thanks for the feedback and noted for future. If it's not too much trouble, 
> can you change the last line in commit message to:
> 
> Fix the issue by making unbanked GPIO code not overwrite chip_data because 
> generic irq chip code uses it for its data structures. Instead use 
> handler_data to pass around GPIO controller information.

I've already pushed it out, so I cannot rebase it, but thanks for
taking me seriously.  :-D

g.

Re: [PATCH 2/2] gpio/davinci: fix enabling unbanked GPIO IRQs

[Grant Likely]

On Sun, 11 Mar 2012 18:16:12 +0530, Sekhar Nori <nsekhar@ti.com> wrote:
> Unbanked GPIO IRQ handling code made a copy of just
> the irq_chip structure for GPIO IRQ lines which caused
> problems after the generic IRQ chip conversion because
> there was no valid irq_chip_type structure with the
> right "regs" populated. irq_gc_mask_set_bit() was
> therefore accessing random addresses.
> 
> Fix it by making a copy of irq_chip_type structure
> instead. This will ensure sane register offsets.
> 
> Cc: <stable@vger.kernel.org> # v3.0.x+
> Reported-by: Jon Povey <Jon.Povey@racelogic.co.uk>
> Tested-by: Jon Povey <Jon.Povey@racelogic.co.uk>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>

Applied, thx.

g.

> ---
>  drivers/gpio/gpio-davinci.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index a6777e5..3d00016 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -386,7 +386,7 @@ static int __init davinci_gpio_irq_setup(void)
>  	 * IRQ mux conflicts; gpio_irq_type_unbanked() is only for GPIOs.
>  	 */
>  	if (soc_info->gpio_unbanked) {
> -		static struct irq_chip gpio_irqchip_unbanked;
> +		static struct irq_chip_type gpio_unbanked;
>  
>  		/* pass "bank 0" GPIO IRQs to AINTC */
>  		chips[0].chip.to_irq = gpio_to_irq_unbanked;
> @@ -394,9 +394,10 @@ static int __init davinci_gpio_irq_setup(void)
>  
>  		/* AINTC handles mask/unmask; GPIO handles triggering */
>  		irq = bank_irq;
> -		gpio_irqchip_unbanked = *irq_get_chip(irq);
> -		gpio_irqchip_unbanked.name = "GPIO-AINTC";
> -		gpio_irqchip_unbanked.irq_set_type = gpio_irq_type_unbanked;
> +		gpio_unbanked = *container_of(irq_get_chip(irq),
> +					      struct irq_chip_type, chip);
> +		gpio_unbanked.chip.name = "GPIO-AINTC";
> +		gpio_unbanked.chip.irq_set_type = gpio_irq_type_unbanked;
>  
>  		/* default trigger: both edges */
>  		g = gpio2regs(0);
> @@ -405,7 +406,7 @@ static int __init davinci_gpio_irq_setup(void)
>  
>  		/* set the direct IRQs up to use that irqchip */
>  		for (gpio = 0; gpio < soc_info->gpio_unbanked; gpio++, irq++) {
> -			irq_set_chip(irq, &gpio_irqchip_unbanked);
> +			irq_set_chip(irq, &gpio_unbanked.chip);
>  			irq_set_handler_data(irq, &chips[gpio / 32]);
>  			irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH);
>  		}
> -- 
> 1.7.0.4
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.