Re: [PATCH 08/12] iwlwifi: export DHC framework and add first public entry, twt_setup

[Luca Coelho <luca@coelho.fi>]

On Mon, 2021-10-18 at 10:51 +0300, Kalle Valo wrote:
> Luca Coelho <luca@coelho.fi> writes:
> 
> > On Sat, 2021-08-21 at 17:04 +0300, Kalle Valo wrote:
> > > Luca Coelho <luca@coelho.fi> writes:
> > > 
> > > > From: Luca Coelho <luciano.coelho@intel.com>
> > > > 
> > > > Export the debug host command framework and add the twt_setup entry.
> > > > This will allow external parties to use these debugging features.
> > > > More entries can be added later on.
> > > > 
> > > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > 
> > > [...]
> > > 
> > > > --- a/drivers/net/wireless/intel/iwlwifi/Kconfig
> > > > +++ b/drivers/net/wireless/intel/iwlwifi/Kconfig
> > > > @@ -92,6 +92,12 @@ config IWLWIFI_BCAST_FILTERING
> > > >  	  If unsure, don't enable this option, as some programs might
> > > >  	  expect incoming broadcasts for their normal operations.
> > > >  
> > > > 
> > > > +config IWLWIFI_DHC
> > > > +	bool "Enable debug host commands"
> > > > +	help
> > > > +	  This option enables the debug host command API.  It's used
> > > > +	  for debugging and validation purposes.
> > > > +
> > > 
> > > Why a new Kconfig option? Those should not be added lightly.
> > 
> > This is a debugging feature that is not really needed in production
> > kernels, so we prefer to allow it to be removed so we don't waste
> > resources.
> 
> What resources exactly? I would say if the admin or distro maintainer
> wants to save on resources he will disable IWLWIFI_DEBUGFS. Why do we
> need to have multiple Kconfig options for iwlwifi debugfs interface?
> 
> > We're publishing this for a few reasons:
> > 
> > 1. it will help prevent rebasing mistakes when sending patches upstream
> > from our internal tree, because a lot of this code is spread around the
> > driver;
> > 
> > 2. in some occasions, we may ask advanced users to enable it so we can
> > get more data and run more tests in case of tricky bugs;
> > 
> > 3. for the specific case of twt_setup, this allows running some TWT
> > test scenarios with our driver that wouldn't be easily available
> > otherwise.
> 
> Sure, I understand all that. The better debug features we have in
> upstream the better. But I don't understand why a new Kconfig option is
> needed for DHC feature.
> 
> > Is it okay to keep it?
> 
> In the past Linus has stated his dislike of adding pointless Kconfig
> options, with which I strongly agree, and to me it looks like
> IWLWIFI_DHC is exactly that. So I'm very hesitant about this.

Okay, fair enough.  I'll hold this back now, rework it without the
Kconfig option and send it again in the future.

Thanks for the comments!

--
Cheers,
Luca.