* [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)
@ 2019-03-08 0:15 Joe Perches
2019-03-22 12:43 ` Herbert Xu
2019-03-22 12:54 ` Christophe Leroy
0 siblings, 2 replies; 8+ messages in thread
From: Joe Perches @ 2019-03-08 0:15 UTC (permalink / raw)
To: Herbert Xu, David S. Miller; +Cc: linux-crypto, LKML
Normal use of IS_ENABLED is with a CONFIG_<SYMBOL> and
there is no -DDEBUG in the Makefile here.
Replace the IS_ENABLED(DEBUG) with #ifdef DEBUG/#endif
blocks.
Miscellanea:
o Move the sahara_state array into the function that uses it.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/crypto/sahara.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/crypto/sahara.c b/drivers/crypto/sahara.c
index 8c32a3059b4a..d2b4bd483507 100644
--- a/drivers/crypto/sahara.c
+++ b/drivers/crypto/sahara.c
@@ -348,14 +348,13 @@ static void sahara_decode_error(struct sahara_dev *dev, unsigned int error)
dev_err(dev->device, "\n");
}
-static const char *sahara_state[4] = { "Idle", "Busy", "Error", "HW Fault" };
-
static void sahara_decode_status(struct sahara_dev *dev, unsigned int status)
{
+#ifdef DEBUG
u8 state;
-
- if (!IS_ENABLED(DEBUG))
- return;
+ static const char *sahara_state[4] = {
+ "Idle", "Busy", "Error", "HW Fault"
+ };
state = SAHARA_STATUS_GET_STATE(status);
@@ -400,15 +399,14 @@ static void sahara_decode_status(struct sahara_dev *dev, unsigned int status)
sahara_read(dev, SAHARA_REG_CDAR));
dev_dbg(dev->device, "Initial DAR: 0x%08x\n\n",
sahara_read(dev, SAHARA_REG_IDAR));
+#endif
}
static void sahara_dump_descriptors(struct sahara_dev *dev)
{
+#ifdef DEBUG
int i;
- if (!IS_ENABLED(DEBUG))
- return;
-
for (i = 0; i < SAHARA_MAX_HW_DESC; i++) {
dev_dbg(dev->device, "Descriptor (%d) (%pad):\n",
i, &dev->hw_phys_desc[i]);
@@ -421,15 +419,14 @@ static void sahara_dump_descriptors(struct sahara_dev *dev)
dev->hw_desc[i]->next);
}
dev_dbg(dev->device, "\n");
+#endif
}
static void sahara_dump_links(struct sahara_dev *dev)
{
+#ifdef DEBUG
int i;
- if (!IS_ENABLED(DEBUG))
- return;
-
for (i = 0; i < SAHARA_MAX_HW_LINK; i++) {
dev_dbg(dev->device, "Link (%d) (%pad):\n",
i, &dev->hw_phys_link[i]);
@@ -439,6 +436,7 @@ static void sahara_dump_links(struct sahara_dev *dev)
dev->hw_link[i]->next);
}
dev_dbg(dev->device, "\n");
+#endif
}
static int sahara_hw_descriptor_create(struct sahara_dev *dev)
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)
2019-03-08 0:15 [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG) Joe Perches
@ 2019-03-22 12:43 ` Herbert Xu
2019-03-22 14:29 ` Ard Biesheuvel
2019-03-22 12:54 ` Christophe Leroy
1 sibling, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2019-03-22 12:43 UTC (permalink / raw)
To: Joe Perches; +Cc: David S. Miller, linux-crypto, LKML
On Thu, Mar 07, 2019 at 04:15:55PM -0800, Joe Perches wrote:
> Normal use of IS_ENABLED is with a CONFIG_<SYMBOL> and
> there is no -DDEBUG in the Makefile here.
>
> Replace the IS_ENABLED(DEBUG) with #ifdef DEBUG/#endif
> blocks.
>
> Miscellanea:
>
> o Move the sahara_state array into the function that uses it.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> drivers/crypto/sahara.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
Even if this is correct this is way too ugly. The original code
at least compiled everything regardless of macros. Your new code
won't detect compile errors in debugging code unless debugging is
enabled.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)
2019-03-22 12:43 ` Herbert Xu
@ 2019-03-22 14:29 ` Ard Biesheuvel
2019-03-22 15:07 ` Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2019-03-22 14:29 UTC (permalink / raw)
To: Herbert Xu
Cc: Joe Perches, David S. Miller,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML
On Fri, 22 Mar 2019 at 13:43, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Mar 07, 2019 at 04:15:55PM -0800, Joe Perches wrote:
> > Normal use of IS_ENABLED is with a CONFIG_<SYMBOL> and
> > there is no -DDEBUG in the Makefile here.
> >
> > Replace the IS_ENABLED(DEBUG) with #ifdef DEBUG/#endif
> > blocks.
> >
> > Miscellanea:
> >
> > o Move the sahara_state array into the function that uses it.
> >
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> > drivers/crypto/sahara.c | 20 +++++++++-----------
> > 1 file changed, 9 insertions(+), 11 deletions(-)
>
> Even if this is correct this is way too ugly. The original code
> at least compiled everything regardless of macros. Your new code
> won't detect compile errors in debugging code unless debugging is
> enabled.
>
What's wrong with IS_ENABLED(DEBUG) anyway? It may not be 'normal use'
but it works fine.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)
2019-03-22 14:29 ` Ard Biesheuvel
@ 2019-03-22 15:07 ` Joe Perches
2019-03-22 16:21 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2019-03-22 15:07 UTC (permalink / raw)
To: Ard Biesheuvel, Herbert Xu
Cc: David S. Miller, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML
On Fri, 2019-03-22 at 15:29 +0100, Ard Biesheuvel wrote:
> On Fri, 22 Mar 2019 at 13:43, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Thu, Mar 07, 2019 at 04:15:55PM -0800, Joe Perches wrote:
> > > Normal use of IS_ENABLED is with a CONFIG_<SYMBOL> and
> > > there is no -DDEBUG in the Makefile here.
> > >
> > > Replace the IS_ENABLED(DEBUG) with #ifdef DEBUG/#endif
> > > blocks.
> > >
> > > Miscellanea:
> > >
> > > o Move the sahara_state array into the function that uses it.
> > >
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > > ---
> > > drivers/crypto/sahara.c | 20 +++++++++-----------
> > > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > Even if this is correct this is way too ugly. The original code
> > at least compiled everything regardless of macros. Your new code
> > won't detect compile errors in debugging code unless debugging is
> > enabled.
> >
>
> What's wrong with IS_ENABLED(DEBUG) anyway? It may not be 'normal use'
> but it works fine.
drivers/crypto/sahara.c is the only user in the kernel tree.
So only it's abnormal use. I rather like the concept actually.
IS_ENABLED is almost exclusively used with CONFIG_<FOO> symbols
and it could be useful to require it to be used with CONFIG_<FOO>
symbols and use some other similar mechanism for DEBUG use.
Maybe just adding a global #define in kernel.h like
#define IS_DEBUG_ENABLED IS_ENABLED(DEBUG)
to isolate this in one place might be better.
A good thing about using IS_ENABLED or the suggested IS_DEBUG_ENABLED
would be that least gcc 5+ seems to automatically elide the uses of any
unreferenced static const char * arrays like the sahara_state uses here.
(I don't have gcc4 anymore so I couldn't check that version)
So using 'if (IS_DEBUG_ENABLED)' could simplify some existing code like
the many uses of
#ifdef DEBUG
...
#endif
or equivalent
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)
2019-03-22 15:07 ` Joe Perches
@ 2019-03-22 16:21 ` Ard Biesheuvel
2019-03-22 16:29 ` Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2019-03-22 16:21 UTC (permalink / raw)
To: Joe Perches
Cc: Herbert Xu, David S. Miller,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML
On Fri, 22 Mar 2019 at 16:07, Joe Perches <joe@perches.com> wrote:
>
> On Fri, 2019-03-22 at 15:29 +0100, Ard Biesheuvel wrote:
> > On Fri, 22 Mar 2019 at 13:43, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > On Thu, Mar 07, 2019 at 04:15:55PM -0800, Joe Perches wrote:
> > > > Normal use of IS_ENABLED is with a CONFIG_<SYMBOL> and
> > > > there is no -DDEBUG in the Makefile here.
> > > >
> > > > Replace the IS_ENABLED(DEBUG) with #ifdef DEBUG/#endif
> > > > blocks.
> > > >
> > > > Miscellanea:
> > > >
> > > > o Move the sahara_state array into the function that uses it.
> > > >
> > > > Signed-off-by: Joe Perches <joe@perches.com>
> > > > ---
> > > > drivers/crypto/sahara.c | 20 +++++++++-----------
> > > > 1 file changed, 9 insertions(+), 11 deletions(-)
> > >
> > > Even if this is correct this is way too ugly. The original code
> > > at least compiled everything regardless of macros. Your new code
> > > won't detect compile errors in debugging code unless debugging is
> > > enabled.
> > >
> >
> > What's wrong with IS_ENABLED(DEBUG) anyway? It may not be 'normal use'
> > but it works fine.
>
> drivers/crypto/sahara.c is the only user in the kernel tree.
> So only it's abnormal use. I rather like the concept actually.
>
> IS_ENABLED is almost exclusively used with CONFIG_<FOO> symbols
> and it could be useful to require it to be used with CONFIG_<FOO>
> symbols and use some other similar mechanism for DEBUG use.
>
> Maybe just adding a global #define in kernel.h like
>
> #define IS_DEBUG_ENABLED IS_ENABLED(DEBUG)
>
__is_defined(DEBUG) seems to be the most appropriate here. I don't
feel strongly about whether we or not should wrap it in another macro.
> to isolate this in one place might be better.
>
> A good thing about using IS_ENABLED or the suggested IS_DEBUG_ENABLED
> would be that least gcc 5+ seems to automatically elide the uses of any
> unreferenced static const char * arrays like the sahara_state uses here.
>
Yes, that is kind of the point. If you use #ifdef, the compiler will
complain about unused static variables in this case.
> (I don't have gcc4 anymore so I couldn't check that version)
>
> So using 'if (IS_DEBUG_ENABLED)' could simplify some existing code like
> the many uses of
>
> #ifdef DEBUG
> ...
> #endif
>
> or equivalent
>
>
My vote would be to use __is_defined(DEBUG) in place, but if others
prefer wrapping it in a macro, that is also fine with me.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)
2019-03-22 16:21 ` Ard Biesheuvel
@ 2019-03-22 16:29 ` Joe Perches
0 siblings, 0 replies; 8+ messages in thread
From: Joe Perches @ 2019-03-22 16:29 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Herbert Xu, David S. Miller,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML
On Fri, 2019-03-22 at 17:21 +0100, Ard Biesheuvel wrote:
> On Fri, 22 Mar 2019 at 16:07, Joe Perches <joe@perches.com> wrote:
> > Maybe just adding a global #define in kernel.h like
> > #define IS_DEBUG_ENABLED IS_ENABLED(DEBUG)
[]
> __is_defined(DEBUG) seems to be the most appropriate here. I don't
> feel strongly about whether we or not should wrap it in another macro.
[]
> > A good thing about using IS_ENABLED or the suggested IS_DEBUG_ENABLED
> > would be that least gcc 5+ seems to automatically elide the uses of any
> > unreferenced static const char * arrays like the sahara_state uses here.
[]
> My vote would be to use __is_defined(DEBUG) in place, but if others
> prefer wrapping it in a macro, that is also fine with me.
I think __is_defined is fine too.
Either works for me.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)
2019-03-08 0:15 [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG) Joe Perches
2019-03-22 12:43 ` Herbert Xu
@ 2019-03-22 12:54 ` Christophe Leroy
2019-03-22 14:13 ` Daniel Thompson
1 sibling, 1 reply; 8+ messages in thread
From: Christophe Leroy @ 2019-03-22 12:54 UTC (permalink / raw)
To: Joe Perches, Herbert Xu, David S. Miller; +Cc: linux-crypto, LKML
Le 08/03/2019 à 01:15, Joe Perches a écrit :
> Normal use of IS_ENABLED is with a CONFIG_<SYMBOL> and
> there is no -DDEBUG in the Makefile here.
>
> Replace the IS_ENABLED(DEBUG) with #ifdef DEBUG/#endif
> blocks.
By doing this you hide a big part of the code which cannot be verified
unless DEBUG is defined.
I suggest to add the following helper:
static bool is_debug_enabled(void)
{
#ifdef DEBUG
return true;
#else
return false;
#endif
}
then replace
if (IS_ENABLED(DEBUG))
by
if (is_debug_enabled())
Christophe
>
> Miscellanea:
>
> o Move the sahara_state array into the function that uses it.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> drivers/crypto/sahara.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/crypto/sahara.c b/drivers/crypto/sahara.c
> index 8c32a3059b4a..d2b4bd483507 100644
> --- a/drivers/crypto/sahara.c
> +++ b/drivers/crypto/sahara.c
> @@ -348,14 +348,13 @@ static void sahara_decode_error(struct sahara_dev *dev, unsigned int error)
> dev_err(dev->device, "\n");
> }
>
> -static const char *sahara_state[4] = { "Idle", "Busy", "Error", "HW Fault" };
> -
> static void sahara_decode_status(struct sahara_dev *dev, unsigned int status)
> {
> +#ifdef DEBUG
> u8 state;
> -
> - if (!IS_ENABLED(DEBUG))
> - return;
> + static const char *sahara_state[4] = {
> + "Idle", "Busy", "Error", "HW Fault"
> + };
>
> state = SAHARA_STATUS_GET_STATE(status);
>
> @@ -400,15 +399,14 @@ static void sahara_decode_status(struct sahara_dev *dev, unsigned int status)
> sahara_read(dev, SAHARA_REG_CDAR));
> dev_dbg(dev->device, "Initial DAR: 0x%08x\n\n",
> sahara_read(dev, SAHARA_REG_IDAR));
> +#endif
> }
>
> static void sahara_dump_descriptors(struct sahara_dev *dev)
> {
> +#ifdef DEBUG
> int i;
>
> - if (!IS_ENABLED(DEBUG))
> - return;
> -
> for (i = 0; i < SAHARA_MAX_HW_DESC; i++) {
> dev_dbg(dev->device, "Descriptor (%d) (%pad):\n",
> i, &dev->hw_phys_desc[i]);
> @@ -421,15 +419,14 @@ static void sahara_dump_descriptors(struct sahara_dev *dev)
> dev->hw_desc[i]->next);
> }
> dev_dbg(dev->device, "\n");
> +#endif
> }
>
> static void sahara_dump_links(struct sahara_dev *dev)
> {
> +#ifdef DEBUG
> int i;
>
> - if (!IS_ENABLED(DEBUG))
> - return;
> -
> for (i = 0; i < SAHARA_MAX_HW_LINK; i++) {
> dev_dbg(dev->device, "Link (%d) (%pad):\n",
> i, &dev->hw_phys_link[i]);
> @@ -439,6 +436,7 @@ static void sahara_dump_links(struct sahara_dev *dev)
> dev->hw_link[i]->next);
> }
> dev_dbg(dev->device, "\n");
> +#endif
> }
>
> static int sahara_hw_descriptor_create(struct sahara_dev *dev)
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)
2019-03-22 12:54 ` Christophe Leroy
@ 2019-03-22 14:13 ` Daniel Thompson
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Thompson @ 2019-03-22 14:13 UTC (permalink / raw)
To: Christophe Leroy
Cc: Joe Perches, Herbert Xu, David S. Miller, linux-crypto, LKML
On Fri, Mar 22, 2019 at 01:54:47PM +0100, Christophe Leroy wrote:
>
>
> Le 08/03/2019 à 01:15, Joe Perches a écrit :
> > Normal use of IS_ENABLED is with a CONFIG_<SYMBOL> and
> > there is no -DDEBUG in the Makefile here.
> >
> > Replace the IS_ENABLED(DEBUG) with #ifdef DEBUG/#endif
> > blocks.
>
> By doing this you hide a big part of the code which cannot be verified
> unless DEBUG is defined.
The dump functions already use dev_dbg() throughout and appear not to
have any side effects (only local variables are modified). Can we not simply
*remove* the if(IS_ENABLED_DEBUG) and rely on dev_dbg() to hide things
from the code generater when the debug messages are disabled?
Or put another way, is there any harm in allowing a system that enables
CONFIG_DYNAMIC_DEBUG to observe the status dumps?
Daniel.
> static bool is_debug_enabled(void)
> {
> #ifdef DEBUG
> return true;
> #else
> return false;
> #endif
> }
>
> then replace
>
> if (IS_ENABLED(DEBUG))
>
> by
>
> if (is_debug_enabled())
>
> Christophe
>
> >
> > Miscellanea:
> >
> > o Move the sahara_state array into the function that uses it.
> >
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> > drivers/crypto/sahara.c | 20 +++++++++-----------
> > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/crypto/sahara.c b/drivers/crypto/sahara.c
> > index 8c32a3059b4a..d2b4bd483507 100644
> > --- a/drivers/crypto/sahara.c
> > +++ b/drivers/crypto/sahara.c
> > @@ -348,14 +348,13 @@ static void sahara_decode_error(struct sahara_dev *dev, unsigned int error)
> > dev_err(dev->device, "\n");
> > }
> > -static const char *sahara_state[4] = { "Idle", "Busy", "Error", "HW Fault" };
> > -
> > static void sahara_decode_status(struct sahara_dev *dev, unsigned int status)
> > {
> > +#ifdef DEBUG
> > u8 state;
> > -
> > - if (!IS_ENABLED(DEBUG))
> > - return;
> > + static const char *sahara_state[4] = {
> > + "Idle", "Busy", "Error", "HW Fault"
> > + };
> > state = SAHARA_STATUS_GET_STATE(status);
> > @@ -400,15 +399,14 @@ static void sahara_decode_status(struct sahara_dev *dev, unsigned int status)
> > sahara_read(dev, SAHARA_REG_CDAR));
> > dev_dbg(dev->device, "Initial DAR: 0x%08x\n\n",
> > sahara_read(dev, SAHARA_REG_IDAR));
> > +#endif
> > }
> > static void sahara_dump_descriptors(struct sahara_dev *dev)
> > {
> > +#ifdef DEBUG
> > int i;
> > - if (!IS_ENABLED(DEBUG))
> > - return;
> > -
> > for (i = 0; i < SAHARA_MAX_HW_DESC; i++) {
> > dev_dbg(dev->device, "Descriptor (%d) (%pad):\n",
> > i, &dev->hw_phys_desc[i]);
> > @@ -421,15 +419,14 @@ static void sahara_dump_descriptors(struct sahara_dev *dev)
> > dev->hw_desc[i]->next);
> > }
> > dev_dbg(dev->device, "\n");
> > +#endif
> > }
> > static void sahara_dump_links(struct sahara_dev *dev)
> > {
> > +#ifdef DEBUG
> > int i;
> > - if (!IS_ENABLED(DEBUG))
> > - return;
> > -
> > for (i = 0; i < SAHARA_MAX_HW_LINK; i++) {
> > dev_dbg(dev->device, "Link (%d) (%pad):\n",
> > i, &dev->hw_phys_link[i]);
> > @@ -439,6 +436,7 @@ static void sahara_dump_links(struct sahara_dev *dev)
> > dev->hw_link[i]->next);
> > }
> > dev_dbg(dev->device, "\n");
> > +#endif
> > }
> > static int sahara_hw_descriptor_create(struct sahara_dev *dev)
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-03-22 16:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-08 0:15 [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG) Joe Perches
2019-03-22 12:43 ` Herbert Xu
2019-03-22 14:29 ` Ard Biesheuvel
2019-03-22 15:07 ` Joe Perches
2019-03-22 16:21 ` Ard Biesheuvel
2019-03-22 16:29 ` Joe Perches
2019-03-22 12:54 ` Christophe Leroy
2019-03-22 14:13 ` Daniel Thompson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).