| 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 |
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
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,
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 --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);
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(-)