RE: [PATCH 2/2] mtd: nand: use hrtimer to measure timeout in nand_wait{_ready, }

[Johan Gunnarsson <johan.gunnarsson@axis.com>]

> -----Original Message-----
> From: Artem Bityutskiy [mailto:dedekind1@gmail.com]
> Sent: den 22 maj 2012 09:53
> To: Johan Gunnarsson; Brian Norris
> Cc: linux-mtd@lists.infradead.org; Jesper Nilsson
> Subject: Re: [PATCH 2/2] mtd: nand: use hrtimer to measure timeout in
> nand_wait{_ready, }
> 
> On Mon, 2012-05-21 at 10:42 +0200, Johan Gunnarsson wrote:
> > @@ -533,19 +541,29 @@ static void panic_nand_wait_ready(struct
> mtd_info *mtd, unsigned long timeo)
> >  void nand_wait_ready(struct mtd_info *mtd)
> >  {
> >  	struct nand_chip *chip = mtd->priv;
> > -	unsigned long timeo = jiffies + 2;
> > +	struct hrtimer timer;
> >
> >  	/* 400ms timeout */
> >  	if (in_interrupt() || oops_in_progress)
> >  		return panic_nand_wait_ready(mtd, 400);
> >
> >  	led_trigger_event(nand_led_trigger, LED_FULL);
> > +
> > +	/* Arm timeout timer for 20ms timeout */
> > +	hrtimer_init(&timer, CLOCK_REALTIME, HRTIMER_MODE_REL);
> > +	timer.function = nand_wait_timeout_callback;
> > +	hrtimer_start(&timer, ns_to_ktime(20 * 1000 * 1000),
> > +	              HRTIMER_MODE_REL);
> > +
> >  	/* Wait until command is processed or timeout occurs */
> >  	do {
> >  		if (chip->dev_ready(mtd))
> >  			break;
> >  		touch_softlockup_watchdog();
> > -	} while (time_before(jiffies, timeo));
> 
> Si this function waits for the chip, but if we cannot access the
> "ready"
> pin - it waits for 400msecs? Where this number 400 comes from? Why not
> 500? How many chips we have which do not provide "dev_ready()"
> function?

Not an expert on the MTD framework -- but I believe the 400 comes from the comment above nand_wait(): "Wait for command done. This applies to erase and program only. Erase can take up to 400ms and program up to 20ms according to general NAND and SmartMedia specs."

> Can we simplify this function and assume everyone has "dev_ready()" and
> add BUG_ON(!chip->dev_ready) - could you make a small research ? Or can

The drivers/mtd/nand/h1910.c driver sets dev_ready to NULL on init. The others seem to have dev_ready implementations.

> we do like this:
> 
> if (!dev_ready()) {
> 	msleep(400);
> 	return
> }
> 
> do {
> } while
> 
> Why should we iterate if "dev_ready" is NULL?

Actually, it's worse than that. If dev_ready is NULL (as in the h1910.c case) we'll crash the kernel, right?

> 
> My point is that this stuff ancient horror which needs love and
> cleaning
> up. Your does not make it less scary. I'd prefer to first clean the
> whole thing up, and then fix it.
> 
> I'm CCing Brian who spent a lot of time digging nand_base.c recently,
> he
> has probably got some ideas.
> 
> --
> Best Regards,
> Artem Bityutskiy