Date:   Fri, 22 Mar 2019 14:13:00 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Christophe Leroy <christophe.leroy@c-s.fr>
Cc:     Joe Perches <joe@perches.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        linux-crypto@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [possible PATCH] crypto: sahara - Use #ifdef DEBUG not
 IS_ENABLED(DEBUG)
Message-ID: <20190322141300.foza2h2lx5aklchq@holly.lan>
References: <5ef0cab4dee128058a43f43c723c13924662e80d.camel@perches.com>
 <4ea24643-5832-981f-3d39-9a84692be6e7@c-s.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4ea24643-5832-981f-3d39-9a84692be6e7@c-s.fr>
User-Agent: NeoMutt/20180716
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

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