linux-kernel - Re: [PATCH v2 1/2] mtd: rawnand: Provide helper for polling GPIO R/B pin

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181013075537.291e145f@bbrezillon>
Date:   Sat, 13 Oct 2018 07:55:37 +0200
From:   Boris Brezillon <boris.brezillon@...tlin.com>
To:     Janusz Krzysztofik <jmkrzyszt@...il.com>
Cc:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Marek Vasut <marek.vasut@...il.com>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] mtd: rawnand: Provide helper for polling GPIO
 R/B pin

Hi Janusz,

On Fri, 12 Oct 2018 22:41:00 +0200
Janusz Krzysztofik <jmkrzyszt@...il.com> wrote:

> Each controller driver with access to NAND R/B pin over GPIO would have 
> to reimplement the polling loop otherwise.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@...il.com>
> ---
> Changelog:
> v2:
> New patch - v1 consisted of only one patch (the followning one)
> 
> 
>  drivers/mtd/nand/raw/nand_base.c | 38 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/rawnand.h      | 10 ++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 05bd0779fe9b..ff1ac4a3c647 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -45,6 +45,9 @@
>  #include <linux/io.h>
>  #include <linux/mtd/partitions.h>
>  #include <linux/of.h>
> +#ifdef CONFIG_GPIOLIB
> +#include <linux/gpio/consumer.h>
> +#endif

The ifdef is not needed here, linux/gpio/consumer.h already has dummy
wrappers when CONFIG_GPIOLIB is not enabled.

>  
>  #include "internals.h"
>  
> @@ -531,6 +534,41 @@ int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms)
>  };
>  EXPORT_SYMBOL_GPL(nand_soft_waitrdy);
>  
> +#ifdef CONFIG_GPIOLIB
> +/**
> + * nand_gpio_waitrdy - Poll R/B GPIO pin until ready
> + * @chip: NAND chip structure
> + * @gpiod: GPIO descriptor of R/B pin
> + * @timeout_ms: Timeout in ms
> + *
> + * Poll the R/B GPIO pin until it becomes ready. If that does not happen
> + * whitin the specified timeout, -ETIMEDOUT is returned.
> + *
> + * This helper is intended to be used when the controller has access to the
> + * NAND R/B pin over GPIO.
> + *
> + * Be aware that calling this helper from an ->exec_op() implementation means
> + * ->exec_op() must be re-entrant.

This is not true for this function: it does not call nand_exec_op().

> + *
> + * Return 0 if the R/B pin indicates chip is ready, a negative error otherwise.
> + */
> +int nand_gpio_waitrdy(struct nand_chip *chip, struct gpio_desc *gpiod,
> +		      unsigned long timeout_ms)
> +{
> +	/* Wait until command is processed or timeout occurs */
> +	timeout_ms = jiffies + msecs_to_jiffies(timeout_ms);
> +	do {
> +		if (gpiod_get_value_cansleep(gpiod))
> +			return 0;
> +
> +		cond_resched();
> +	} while	(time_before(jiffies, timeout_ms));

> +
> +	return gpiod_get_value_cansleep(gpiod) ? 0 : -ETIMEDOUT;
> +};
> +EXPORT_SYMBOL_GPL(nand_gpio_waitrdy);
> +#endif

Hm, I don't see any other helpers defined in #ifdef blocks though most
of them are optionals and are most of the time not used by drivers.
Let's keep things consistent (at the expense of embedding unused code
in nand.o) and remove the #ifdef here. If someone starts complaining
about the size of the rawnand core, we'll consider doing that.

> +
>  /**
>   * panic_nand_get_device - [GENERIC] Get chip for selected access
>   * @chip: the nand chip descriptor
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index e10b126e148f..09f0ed1345b1 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -1346,4 +1346,14 @@ void nand_release(struct nand_chip *chip);
>   */
>  int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms);
>  
> +#ifdef CONFIG_GPIOLIB
> +struct gpio_desc;
> +/*
> + * External helper for controller drivers that have to implement the WAITRDY
> + * instruction and do have GPIO pin to check it.
> + */

You can drop this comment, this is already explained in the kerneldoc
header above the function def.

> +int nand_gpio_waitrdy(struct nand_chip *chip, struct gpio_desc *gpiod,
> +		      unsigned long timeout_ms);
> +#endif
> +
>  #endif /* __LINUX_MTD_RAWNAND_H */

Regards,

Boris