Re: [PATCH] x86: check for valid irq_cfg pointer insmp_irq_move_cleanup_interrupt

[Thomas Gleixner]

On Mon, 21 May 2012, Dimitri Sivanich wrote:

> On Mon, May 21, 2012 at 11:07:04PM +0200, Thomas Gleixner wrote:
> > On Mon, 21 May 2012, Dimitri Sivanich wrote:
> > 
> > > The smp_irq_move_cleanup_interrupt routine should be checking for a valid
> > > irq_cfg pointer prior to accessing it.  It also seems that this should be
> > > done after taking the desc lock.
> > 
> > It seems that you either missed or failed to explain why it should be
> > done _after_ taking the lock.
> > 
> > Changelogs matter, really.
> >
> How about this?
>
> The smp_irq_move_cleanup_interrupt routine should be checking for a valid
> irq_cfg pointer prior to accessing it.

Why should it?
 
> Follow the same protocol shown in irq_set_chip_data(), by taking the desc
> lock before accessing this location.

This is not a proper explanation. irq_set_chip_data() might be wrong
as well and aside of that it might be correct to ignore that protocol
in that particular situation.

What's wrong with adding the actual wreckage scenario _AND_ the
solution to the changelog so that a casual reader, who is not
completely familiar with the code can understand what you are trying
to solve?

Don't misunderstand me. The patch is correct, just the explanation
sucks.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/