[possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)

[possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)

[Joe Perches]

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)

Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)

[Herbert Xu]

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

Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)

[Christophe Leroy]

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)
> 

Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)

[Daniel Thompson]

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)
> > 

Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)

[Ard Biesheuvel]

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.

Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)

[Joe Perches]

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

Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)

[Ard Biesheuvel]

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.

Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG)

[Joe Perches]

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.