| Message ID | 5ef0cab4dee128058a43f43c723c13924662e80d.camel@perches.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [possible] crypto: sahara - Use #ifdef DEBUG not IS_ENABLED(DEBUG) | expand |
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,
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) >
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) > >
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.
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
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.
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.
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)
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(-)