mbox series

[v3,0/3] Allow for rpmpd/rpmh/rpmhpd drivers to be loaded as permenent modules

Message ID 20200326224459.105170-1-john.stultz@linaro.org
Headers show
Series Allow for rpmpd/rpmh/rpmhpd drivers to be loaded as permenent modules | expand

Message

John Stultz March 26, 2020, 10:44 p.m. UTC 
This series simply allows the qcom rpmpd, rpmh and rpmhpd
drivers to be configured and loaded as permement modules.

This means the modules can be loaded, but not unloaded.

While maybe not ideal, this is an improvement over requiring the
drivers to be built in.

Feedback on this series would be welcome!

thanks
-john

New in v3:
* Added similar change to rpmh and rpmhpd drivers.

Cc: Todd Kjos <tkjos@google.com>
Cc: Saravana Kannan <saravanak@google.com>
Cc: Andy Gross <agross@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Rajendra Nayak <rnayak@codeaurora.org>
Cc: linux-arm-msm@vger.kernel.org

John Stultz (3):
  soc: qcom: rpmpd: Allow RPMPD driver to be loaded as a module
  soc: qcom: rpmh: Allow RPMH driver to be loaded as a module
  soc: qcom: rpmhpd: Allow RPMHPD driver to be loaded as a module

 drivers/soc/qcom/Kconfig    | 8 ++++----
 drivers/soc/qcom/rpmh-rsc.c | 6 ++++++
 drivers/soc/qcom/rpmhpd.c   | 5 +++++
 drivers/soc/qcom/rpmpd.c    | 6 ++++++
 4 files changed, 21 insertions(+), 4 deletions(-)

Comments

Bjorn Andersson April 14, 2020, 10:21 p.m. UTC | #1 
On Thu 26 Mar 15:44 PDT 2020, John Stultz wrote:

> This patch allow the rpmpd driver to be loaded as a permenent
> module. Meaning it can be loaded from a module, but then cannot
> be unloaded.
> 
> Ideally, it would include a remove hook and related logic, but
> apparently the genpd code isn't able to track usage and cleaning
> things up? (See: https://lkml.org/lkml/2019/1/24/38)
> 
> So making it a permenent module at least improves things slightly
> over requiring it to be a built in driver.
> 
> Feedback would be appreciated!
> 
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Andy Gross <agross@kernel.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Rajendra Nayak <rnayak@codeaurora.org>
> Cc: linux-arm-msm@vger.kernel.org
> Acked-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2:
> * Fix MODULE_LICENSE to be GPL v2 as suggested by Bjorn
> * Leave initcall as core_initcall, since that switches to module_initcall
>   only when built as a module, also suggested by Bjorn
> * Add module tags taken from Rajendra's earlier patch
> ---
>  drivers/soc/qcom/Kconfig | 4 ++--
>  drivers/soc/qcom/rpmpd.c | 6 ++++++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index d0a73e76d563..af774555b9d2 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -123,8 +123,8 @@ config QCOM_RPMHPD
>  	  for the voltage rail.
>  
>  config QCOM_RPMPD
> -	bool "Qualcomm RPM Power domain driver"
> -	depends on QCOM_SMD_RPM=y
> +	tristate "Qualcomm RPM Power domain driver"
> +	depends on QCOM_SMD_RPM
>  	help
>  	  QCOM RPM Power domain driver to support power-domains with
>  	  performance states. The driver communicates a performance state
> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
> index 2b1834c5609a..22fe94c03e79 100644
> --- a/drivers/soc/qcom/rpmpd.c
> +++ b/drivers/soc/qcom/rpmpd.c
> @@ -5,6 +5,7 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/mutex.h>
> +#include <linux/module.h>

module comes before mutex in the alphabet.

>  #include <linux/pm_domain.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -226,6 +227,7 @@ static const struct of_device_id rpmpd_match_table[] = {
>  	{ .compatible = "qcom,qcs404-rpmpd", .data = &qcs404_desc },
>  	{ }
>  };
> +MODULE_DEVICE_TABLE(of, rpmpd_match_table);
>  
>  static int rpmpd_send_enable(struct rpmpd *pd, bool enable)
>  {
> @@ -422,3 +424,7 @@ static int __init rpmpd_init(void)
>  	return platform_driver_register(&rpmpd_driver);
>  }
>  core_initcall(rpmpd_init);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPM Power Domain Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-rpmpd");

Is there any reason for this alias?

The module will be automatically loaded based on compatible and the
MODULE_DEVICE_TABLE() information above, and for ACPI would need a
similar acpi_device_id table.

Regards,
Bjorn

> -- 
> 2.17.1
>
John Stultz April 14, 2020, 10:24 p.m. UTC | #2 
On Tue, Apr 14, 2020 at 3:21 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 26 Mar 15:44 PDT 2020, John Stultz wrote:
>
> > This patch allow the rpmpd driver to be loaded as a permenent
> > module. Meaning it can be loaded from a module, but then cannot
> > be unloaded.
> >
> > Ideally, it would include a remove hook and related logic, but
> > apparently the genpd code isn't able to track usage and cleaning
> > things up? (See: https://lkml.org/lkml/2019/1/24/38)
> >
> > So making it a permenent module at least improves things slightly
> > over requiring it to be a built in driver.
> >
> > Feedback would be appreciated!
> >
> > Cc: Todd Kjos <tkjos@google.com>
> > Cc: Saravana Kannan <saravanak@google.com>
> > Cc: Andy Gross <agross@kernel.org>
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Rajendra Nayak <rnayak@codeaurora.org>
> > Cc: linux-arm-msm@vger.kernel.org
> > Acked-by: Saravana Kannan <saravanak@google.com>
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> > v2:
> > * Fix MODULE_LICENSE to be GPL v2 as suggested by Bjorn
> > * Leave initcall as core_initcall, since that switches to module_initcall
> >   only when built as a module, also suggested by Bjorn
> > * Add module tags taken from Rajendra's earlier patch
> > ---
> >  drivers/soc/qcom/Kconfig | 4 ++--
> >  drivers/soc/qcom/rpmpd.c | 6 ++++++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index d0a73e76d563..af774555b9d2 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -123,8 +123,8 @@ config QCOM_RPMHPD
> >         for the voltage rail.
> >
> >  config QCOM_RPMPD
> > -     bool "Qualcomm RPM Power domain driver"
> > -     depends on QCOM_SMD_RPM=y
> > +     tristate "Qualcomm RPM Power domain driver"
> > +     depends on QCOM_SMD_RPM
> >       help
> >         QCOM RPM Power domain driver to support power-domains with
> >         performance states. The driver communicates a performance state
> > diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
> > index 2b1834c5609a..22fe94c03e79 100644
> > --- a/drivers/soc/qcom/rpmpd.c
> > +++ b/drivers/soc/qcom/rpmpd.c
> > @@ -5,6 +5,7 @@
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> >  #include <linux/mutex.h>
> > +#include <linux/module.h>
>
> module comes before mutex in the alphabet.

:) Thanks for catching that.

> >  #include <linux/pm_domain.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > @@ -226,6 +227,7 @@ static const struct of_device_id rpmpd_match_table[] = {
> >       { .compatible = "qcom,qcs404-rpmpd", .data = &qcs404_desc },
> >       { }
> >  };
> > +MODULE_DEVICE_TABLE(of, rpmpd_match_table);
> >
> >  static int rpmpd_send_enable(struct rpmpd *pd, bool enable)
> >  {
> > @@ -422,3 +424,7 @@ static int __init rpmpd_init(void)
> >       return platform_driver_register(&rpmpd_driver);
> >  }
> >  core_initcall(rpmpd_init);
> > +
> > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPM Power Domain Driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:qcom-rpmpd");
>
> Is there any reason for this alias?
>
> The module will be automatically loaded based on compatible and the
> MODULE_DEVICE_TABLE() information above, and for ACPI would need a
> similar acpi_device_id table.

I pulled it in from Rajendra's earlier patch. I'm ok to drop it though.

I'll fix these up and respin. Thanks for the review!

thanks
-john
John Stultz April 15, 2020, 7:47 p.m. UTC | #3 
On Wed, Apr 15, 2020 at 11:25 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi John,
>
> with commit efde2659b0fe ("drivers: qcom: rpmh-rsc: Use rcuidle
> tracepoints for rpmh") the rpmh-rsc driver fails to build as a
> module:
>
> drivers/soc/qcom/rpmh-rsc.c:281:3: error: implicit declaration of function 'trace_rpmh_send_msg_rcuidle' [-Werror,-Wimplicit-function-decr]
>                 trace_rpmh_send_msg_rcuidle(drv, tcs_id, j, msgid, cmd);
>
>
> The problem is that the _rcuidle() functions are not generated for modules:
>
> #ifndef MODULE
> #define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
>         static inline void trace_##name##_rcuidle(proto)                \
>         {                                                               \
>                 if (static_key_false(&__tracepoint_##name.key))         \
>                         __DO_TRACE(&__tracepoint_##name,                \
>                                 TP_PROTO(data_proto),                   \
>                                 TP_ARGS(data_args),                     \
>                                 TP_CONDITION(cond), 1);                 \
>         }
> #else
> #define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)
> #endif
>
> Not sure what the best solution would be in this case. Having the macro
> define a dummy function for modules would fix the build error, however it
> would be confusing that the event is traced when the driver is built-in,
> but not when it is built as a module.
>
> I imagine the goal behind making this driver a module is to have a single
> kernel image for multiple SoC platforms, without too much platform
> specific code in the kernel image itself.
>
> I guess the question is whether there any options for keeping the driver
> modular and having consistent tracing behavior, short of removing the
> tracepoint.

Yea.  Stephen found that issue in -next last night once Bjorn added
the patches to his tree yesterday.

I've reached out to see if the restrictions on the trace_*_rcuidle
calls on modules is still necessary in this thread:
  https://lore.kernel.org/lkml/CALAqxLV4rM74wuzuZ+BkUi+keccxkAxv30N4vrFO7CVQ5vnT1A@mail.gmail.com/

For now, I suggested Bjorn revert the patch in his tree, and I'll try
to figure out an alternative solution to the trace call.

thanks
-john