Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967056AbdDSQvJ (ORCPT ); Wed, 19 Apr 2017 12:51:09 -0400 Received: from mail-yw0-f175.google.com ([209.85.161.175]:34249 "EHLO mail-yw0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967019AbdDSQuu (ORCPT ); Wed, 19 Apr 2017 12:50:50 -0400 MIME-Version: 1.0 In-Reply-To: <20170419153045.GB23319@leoy-linaro> References: <1491485461-22800-1-git-send-email-leo.yan@linaro.org> <1491485461-22800-7-git-send-email-leo.yan@linaro.org> <20170418174050.GC22806@linaro.org> <20170419001807.GB8524@leoy-linaro> <20170419153045.GB23319@leoy-linaro> From: Mathieu Poirier Date: Wed, 19 Apr 2017 10:50:43 -0600 Message-ID: Subject: Re: [PATCH v6 6/8] coresight: add support for CPU debug module To: Leo Yan Cc: Jonathan Corbet , Rob Herring , Mark Rutland , Wei Xu , Catalin Marinas , Will Deacon , Andy Gross , David Brown , Suzuki K Poulose , Stephen Boyd , linux-doc@vger.kernel.org, "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, Mike Leach , Sudeep Holla Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2633 Lines: 61 On 19 April 2017 at 09:30, Leo Yan wrote: > On Wed, Apr 19, 2017 at 08:52:12AM -0600, Mathieu Poirier wrote: > > [...] > >> >> > +static bool debug_enable; >> >> > +module_param_named(enable, debug_enable, bool, 0600); >> >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality " >> >> > + "(default is 0, which means is disabled by default)"); >> >> >> >> For this driver we have a debugFS interface so I question the validity of a >> >> kernel module parameter. Other than adding complexity to the code it offers no >> >> real added value. If a user is to insmod a module, it is just as easy to switch >> >> on the functionality using debugFS in a second step. >> > >> > This module parameter can be used for kernel command line, so >> > it's useful when user wants to dynamically turn on/off the >> > functionality at boot up time. >> > >> > Does this make sense for you? Removing this parameter is okay for >> > me, but this means users need to decide if use it by Kernel config >> > with static building in. This is a bit contradictory with before's >> > discussion. >> >> My hope was to use the kernel command line and the debugFS interface, >> avoiding the module parameter. Look at what the baycom_par and >> blacklist drivers are doing with the "__setup()" function and see if >> we can void a module parameter. If not then let it be, unless someone >> else has a better idea. >> >> [1]. drivers/net/hamradio/baycom_par.c >> [2]. drivers/s390/cio/blacklist.c > > This driver supports module mode. So we can choose to use either module > parameter or __setup(). But as described in the file > Documentation/admin-guide/kernel-parameters.rst, the module parameter > is more flexible to be used at boot time or insmod the module: > > "Parameters for modules which are built into the kernel need to be > specified on the kernel command line. modprobe looks through the > kernel command line (/proc/cmdline) and collects module parameters > when it loads a module, so the kernel command line can be used for > loadable modules too." > > __setup() cannot support module mode, and when use __setup(), we need > register one callback function for it. The callback function is > friendly to parse complex parameters rather than module parameter. > but it's not necessary for this case. __setup() definitely supports module - the baycom driver is a good example of that. But as you pointed out kernel-parameters.rst is pretty clear on how to proceed. As such disregard my comment and proceed with a module parameter. > > So I'm bias to use module parameter :) > > Thanks, > Leo Yan