[1/3] mxc_nand: set spare size and pages per block

On 2010-08-10, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Sorry, last time I sent only up to 09/12, so the patches I explicitely
> mentioned to solve the things from your previous series were missing.
> I just sent them. My versions of the patches differ slightly.

Your version allows a small window between request_irq() and
irq_control() where on the i.MX21 there is a possibility of the
interrupts being disabled twice. Namely, if an interrupt occurs before
irq_control() has had a chance to disable it. IMHO it would be better to
call:

    set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);

for the i.MX21 before requesting the irq. This closes the window.

For non-i.MX21 this window also exists, but since in that situation the
irq hander simply unnecessarily sets a bit, it is not so dramatic. By
masking the interrupt before requesting the irq, the windows is also
closed for non-i.MX21.

> For this patch I decided to initialize every bit in NFC_V1_V2_CONFIG1
> from scratch so that we do not depend on any reset or bootloader
> values.  I think this is cleaner so I propose that we use my version
> of the patch.

I agree that initializing all the bits is better. But you need to set
the mask as well. Your latest patches clear the mask when initializing
V1_V2_CONFIG1 and V3_CONFIG2. For non-i.MX21 the mask should always be
set except when explicitly waiting for an interrupt in wait_op_done().

All the patches (01-12) were tested on an i.MX35 with 16-bit NAND and
work as expected. My only recommendations would be to close the window
at request_irq() and also include the following patch to set the mask
during preset().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/mxc_nand.c |    3 +++
 1 file changed, 3 insertions(+)

On Tue, Aug 10, 2010 at 04:31:40PM +0200, John Ogness wrote:
> On 2010-08-10, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > Sorry, last time I sent only up to 09/12, so the patches I explicitely
> > mentioned to solve the things from your previous series were missing.
> > I just sent them. My versions of the patches differ slightly.
> 
> Your version allows a small window between request_irq() and
> irq_control() where on the i.MX21 there is a possibility of the
> interrupts being disabled twice. Namely, if an interrupt occurs before
> irq_control() has had a chance to disable it. IMHO it would be better to
> call:
> 
>     set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);
> 
> for the i.MX21 before requesting the irq. This closes the window.

IIRC it is not allowed to call set_irq_flags before request_irq. We are
changing a resource we do not own yet.
I think the worst thing that could happen without this change is that we
get an interrupt after request_irq.
Alternatively we could set the interrupt mask bit before requesting the
irq.

Sascha

On 2010-08-11, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> Your version allows a small window between request_irq() and
>> irq_control() where on the i.MX21 there is a possibility of the
>> interrupts being disabled twice. Namely, if an interrupt occurs
>> before irq_control() has had a chance to disable it. IMHO it would be
>> better to call:
>> 
>>     set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);
>> 
>> for the i.MX21 before requesting the irq. This closes the window.
>
> IIRC it is not allowed to call set_irq_flags before request_irq. We
> are changing a resource we do not own yet.

Normally, yes. But the only way the IRQF_NOAUTOEN flag can ever be
useful is if set_irq_flags() is called before request_irq().

> I think the worst thing that could happen without this change is that
> we get an interrupt after request_irq.

Yes. And this causes a problem on the i.MX21 because the interrupt
handler will disable the interrupts. The irq_control() after
request_irq() will _also_ disable the interrupts. This means the
interrupts are disabled twice, which causes some issues on an RT kernel.

> Alternatively we could set the interrupt mask bit before requesting
> the irq.

Yes. This would be best. But for the i.MX21 it is important that the
interrupts are unmasked after irq_control() has disabled the interrupts.

John Ogness

On Wed, Aug 11, 2010 at 03:16:34PM +0200, John Ogness wrote:
> On 2010-08-11, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> Your version allows a small window between request_irq() and
> >> irq_control() where on the i.MX21 there is a possibility of the
> >> interrupts being disabled twice. Namely, if an interrupt occurs
> >> before irq_control() has had a chance to disable it. IMHO it would be
> >> better to call:
> >> 
> >>     set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);
> >> 
> >> for the i.MX21 before requesting the irq. This closes the window.
> >
> > IIRC it is not allowed to call set_irq_flags before request_irq. We
> > are changing a resource we do not own yet.
> 
> Normally, yes. But the only way the IRQF_NOAUTOEN flag can ever be
> useful is if set_irq_flags() is called before request_irq().
> 
> > I think the worst thing that could happen without this change is that
> > we get an interrupt after request_irq.
> 
> Yes. And this causes a problem on the i.MX21 because the interrupt
> handler will disable the interrupts. The irq_control() after
> request_irq() will _also_ disable the interrupts. This means the
> interrupts are disabled twice, which causes some issues on an RT kernel.
> 
> > Alternatively we could set the interrupt mask bit before requesting
> > the irq.
> 
> Yes. This would be best. But for the i.MX21 it is important that the
> interrupts are unmasked after irq_control() has disabled the interrupts.

Ok, I'll update the patches to use this approach.

Sascha