diff mbox series

[05/10] hwrng: stm32 - rework error handling in stm32_rng_read()

Message ID 20230908165120.730867-6-gatien.chevallier@foss.st.com (mailing list archive)
State New, archived
Headers show
Series hwrng: stm32: support STM32MP13x platforms | expand

Commit Message

Gatien CHEVALLIER Sept. 8, 2023, 4:51 p.m. UTC
Try to conceal seed errors when possible. If, despite the error
concealing tries, a seed error is still present, then return an error.

A clock error does not compromise the hardware block and data can
still be read from RNG_DR. Just warn that the RNG clock is too slow
and clear RNG_SR.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
 drivers/char/hw_random/stm32-rng.c | 53 +++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 12 deletions(-)

Comments

kernel test robot Sept. 8, 2023, 9:48 p.m. UTC | #1
Hi Gatien,

kernel test robot noticed the following build warnings:

[auto build test WARNING on atorgue-stm32/stm32-next]
[also build test WARNING on robh/for-next herbert-crypto-2.6/master herbert-cryptodev-2.6/master linus/master v6.5 next-20230908]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Gatien-Chevallier/dt-bindings-rng-introduce-new-compatible-for-STM32MP13x/20230909-005611
base:   https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
patch link:    https://lore.kernel.org/r/20230908165120.730867-6-gatien.chevallier%40foss.st.com
patch subject: [PATCH 05/10] hwrng: stm32 - rework error handling in stm32_rng_read()
config: hexagon-randconfig-002-20230909 (https://download.01.org/0day-ci/archive/20230909/202309090508.CyBIfKeu-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230909/202309090508.CyBIfKeu-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309090508.CyBIfKeu-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/char/hw_random/stm32-rng.c:9:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/char/hw_random/stm32-rng.c:9:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/char/hw_random/stm32-rng.c:9:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/char/hw_random/stm32-rng.c:210:35: warning: left operand of comma operator has no effect [-Wunused-value]
     210 |                         if (WARN_ON(sr & RNG_SR_CEIS), "RNG clock too slow - %x\n", sr)
         |                                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:55:57: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                                         ^~~~~~~~~~~
   include/linux/compiler.h:57:52: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
>> drivers/char/hw_random/stm32-rng.c:210:35: warning: left operand of comma operator has no effect [-Wunused-value]
     210 |                         if (WARN_ON(sr & RNG_SR_CEIS), "RNG clock too slow - %x\n", sr)
         |                                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:55:57: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                                         ^~~~~~~~~~~
   include/linux/compiler.h:57:86: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                                                      ^~~~
   include/linux/compiler.h:68:3: note: expanded from macro '__trace_if_value'
      68 |         (cond) ?                                        \
         |          ^~~~
   8 warnings generated.


vim +210 drivers/char/hw_random/stm32-rng.c

   162	
   163	
   164	static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
   165	{
   166		struct stm32_rng_private *priv = container_of(rng, struct stm32_rng_private, rng);
   167		unsigned int i = 0;
   168		int retval = 0, err = 0;
   169		u32 sr;
   170	
   171		pm_runtime_get_sync((struct device *) priv->rng.priv);
   172	
   173		if (readl_relaxed(priv->base + RNG_SR) & RNG_SR_SEIS)
   174			stm32_rng_conceal_seed_error(rng);
   175	
   176		while (max >= sizeof(u32)) {
   177			sr = readl_relaxed(priv->base + RNG_SR);
   178			/*
   179			 * Manage timeout which is based on timer and take
   180			 * care of initial delay time when enabling the RNG.
   181			 */
   182			if (!sr && wait) {
   183				err = readl_relaxed_poll_timeout_atomic(priv->base
   184									   + RNG_SR,
   185									   sr, sr,
   186									   10, 50000);
   187				if (err) {
   188					dev_err((struct device *)priv->rng.priv,
   189						"%s: timeout %x!\n", __func__, sr);
   190					break;
   191				}
   192			} else if (!sr) {
   193				/* The FIFO is being filled up */
   194				break;
   195			}
   196	
   197			if (sr != RNG_SR_DRDY) {
   198				if (sr & RNG_SR_SEIS) {
   199					err = stm32_rng_conceal_seed_error(rng);
   200					i++;
   201					if (err && i > RNG_NB_RECOVER_TRIES) {
   202						dev_err((struct device *)priv->rng.priv,
   203							"Couldn't recover from seed error\n");
   204						return -ENOTRECOVERABLE;
   205					}
   206	
   207					continue;
   208				}
   209	
 > 210				if (WARN_ON(sr & RNG_SR_CEIS), "RNG clock too slow - %x\n", sr)
   211					writel_relaxed(0, priv->base + RNG_SR);
   212			}
   213	
   214			/* Late seed error case: DR being 0 is an error status */
   215			*(u32 *)data = readl_relaxed(priv->base + RNG_DR);
   216			if (!*(u32 *)data) {
   217				err = stm32_rng_conceal_seed_error(rng);
   218				i++;
   219				if (err && i > RNG_NB_RECOVER_TRIES) {
   220					dev_err((struct device *)priv->rng.priv,
   221						"Couldn't recover from seed error");
   222					return -ENOTRECOVERABLE;
   223				}
   224	
   225				continue;
   226			}
   227	
   228			i = 0;
   229			retval += sizeof(u32);
   230			data += sizeof(u32);
   231			max -= sizeof(u32);
   232		}
   233	
   234		pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
   235		pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);
   236	
   237		return retval || !wait ? retval : -EIO;
   238	}
   239
Herbert Xu Sept. 12, 2023, 4:02 a.m. UTC | #2
On Fri, Sep 08, 2023 at 06:51:15PM +0200, Gatien Chevallier wrote:
>
> +			if (WARN_ON(sr & RNG_SR_CEIS), "RNG clock too slow - %x\n", sr)

Introducing an unconditional WARN_ON is not acceptable.  If you
really need it, make it WARN_ON_ONCE.  But why does this need
to be a WARN instead of dev_err?

Cheers,
Gatien CHEVALLIER Sept. 12, 2023, 7:36 a.m. UTC | #3
Hello Herbert,

On 9/12/23 06:02, Herbert Xu wrote:
> On Fri, Sep 08, 2023 at 06:51:15PM +0200, Gatien Chevallier wrote:
>>
>> +			if (WARN_ON(sr & RNG_SR_CEIS), "RNG clock too slow - %x\n", sr)
> 
> Introducing an unconditional WARN_ON is not acceptable.  If you
> really need it, make it WARN_ON_ONCE.  But why does this need
> to be a WARN instead of dev_err?
> 
> Cheers,

It was my mistake and not my intention to change the WARN_ON_ONCE to a
WARN_ON. I've already sent a V2 correcting this.

It was already a WARN_ON_ONCE on the first implementation of the driver.
This is not an unrecoverable error as it doesn't affect the quality of
the entropy delivered by the RNG. The output is just too slow. It's here
to warn the developer of an incorrect clock source setting.

Best regards,
Gatien
diff mbox series

Patch

diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
index adefe8edfd07..3d32e0f4baef 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -43,6 +43,8 @@ 
 
 #define RNG_HTCR		0x10
 
+#define RNG_NB_RECOVER_TRIES	3
+
 struct stm32_rng_data {
 	u32	cr;
 	u32	nscr;
@@ -162,10 +164,10 @@  static int stm32_rng_conceal_seed_error(struct hwrng *rng)
 
 static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
-	struct stm32_rng_private *priv =
-	    container_of(rng, struct stm32_rng_private, rng);
+	struct stm32_rng_private *priv = container_of(rng, struct stm32_rng_private, rng);
+	unsigned int i = 0;
+	int retval = 0, err = 0;
 	u32 sr;
-	int retval = 0;
 
 	pm_runtime_get_sync((struct device *) priv->rng.priv);
 
@@ -174,30 +176,57 @@  static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 
 	while (max >= sizeof(u32)) {
 		sr = readl_relaxed(priv->base + RNG_SR);
-		/* Manage timeout which is based on timer and take */
-		/* care of initial delay time when enabling rng	*/
+		/*
+		 * Manage timeout which is based on timer and take
+		 * care of initial delay time when enabling the RNG.
+		 */
 		if (!sr && wait) {
-			int err;
-
 			err = readl_relaxed_poll_timeout_atomic(priv->base
 								   + RNG_SR,
 								   sr, sr,
 								   10, 50000);
-			if (err)
+			if (err) {
 				dev_err((struct device *)priv->rng.priv,
 					"%s: timeout %x!\n", __func__, sr);
+				break;
+			}
+		} else if (!sr) {
+			/* The FIFO is being filled up */
+			break;
 		}
 
-		/* If error detected or data not ready... */
 		if (sr != RNG_SR_DRDY) {
-			if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
-					"bad RNG status - %x\n", sr))
+			if (sr & RNG_SR_SEIS) {
+				err = stm32_rng_conceal_seed_error(rng);
+				i++;
+				if (err && i > RNG_NB_RECOVER_TRIES) {
+					dev_err((struct device *)priv->rng.priv,
+						"Couldn't recover from seed error\n");
+					return -ENOTRECOVERABLE;
+				}
+
+				continue;
+			}
+
+			if (WARN_ON(sr & RNG_SR_CEIS), "RNG clock too slow - %x\n", sr)
 				writel_relaxed(0, priv->base + RNG_SR);
-			break;
 		}
 
+		/* Late seed error case: DR being 0 is an error status */
 		*(u32 *)data = readl_relaxed(priv->base + RNG_DR);
+		if (!*(u32 *)data) {
+			err = stm32_rng_conceal_seed_error(rng);
+			i++;
+			if (err && i > RNG_NB_RECOVER_TRIES) {
+				dev_err((struct device *)priv->rng.priv,
+					"Couldn't recover from seed error");
+				return -ENOTRECOVERABLE;
+			}
+
+			continue;
+		}
 
+		i = 0;
 		retval += sizeof(u32);
 		data += sizeof(u32);
 		max -= sizeof(u32);