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

[Artem Bityutskiy <dedekind1@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

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?
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
we do like this:

if (!dev_ready()) {
	msleep(400);
	return
}

do {
} while

Why should we iterate if "dev_ready" is NULL?

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]