| Message ID | 20200326224459.105170-3-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow for rpmpd/rpmh/rpmhpd drivers to be loaded as permenent modules | expand |
On Thu 26 Mar 15:44 PDT 2020, John Stultz wrote: > This patch allow the rpmh 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 > the rpmh driver is fairly core to the system, so once its loaded > with almost anythign else to get the system to go, the dependencies > are not likely to ever also be removed. > > 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 > Signed-off-by: John Stultz <john.stultz@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> and applied. Regards, Bjorn > --- > drivers/soc/qcom/Kconfig | 2 +- > drivers/soc/qcom/rpmh-rsc.c | 6 ++++++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index af774555b9d2..ac91eaf810f7 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -104,7 +104,7 @@ config QCOM_RMTFS_MEM > Say y here if you intend to boot the modem remoteproc. > > config QCOM_RPMH > - bool "Qualcomm RPM-Hardened (RPMH) Communication" > + tristate "Qualcomm RPM-Hardened (RPMH) Communication" > depends on ARCH_QCOM && ARM64 || COMPILE_TEST > help > Support for communication with the hardened-RPM blocks in > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index e278fc11fe5c..30585d98fdf1 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -11,6 +11,7 @@ > #include <linux/io.h> > #include <linux/kernel.h> > #include <linux/list.h> > +#include <linux/module.h> > #include <linux/of.h> > #include <linux/of_irq.h> > #include <linux/of_platform.h> > @@ -679,6 +680,8 @@ static const struct of_device_id rpmh_drv_match[] = { > { .compatible = "qcom,rpmh-rsc", }, > { } > }; > +MODULE_DEVICE_TABLE(of, rpmh_drv_match); > + > > static struct platform_driver rpmh_driver = { > .probe = rpmh_rsc_probe, > @@ -693,3 +696,6 @@ static int __init rpmh_driver_init(void) > return platform_driver_register(&rpmh_driver); > } > arch_initcall(rpmh_driver_init); > + > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.17.1 >
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.
On Thu, Mar 26, 2020 at 10:44:58PM +0000, John Stultz wrote:
> This patch allow the rpmh 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
> the rpmh driver is fairly core to the system, so once its loaded
> with almost anythign else to get the system to go, the dependencies
> are not likely to ever also be removed.
>
> 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
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> drivers/soc/qcom/Kconfig | 2 +-
> drivers/soc/qcom/rpmh-rsc.c | 6 ++++++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index af774555b9d2..ac91eaf810f7 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -104,7 +104,7 @@ config QCOM_RMTFS_MEM
> Say y here if you intend to boot the modem remoteproc.
>
> config QCOM_RPMH
> - bool "Qualcomm RPM-Hardened (RPMH) Communication"
> + tristate "Qualcomm RPM-Hardened (RPMH) Communication"
> depends on ARCH_QCOM && ARM64 || COMPILE_TEST
> help
> Support for communication with the hardened-RPM blocks in
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index e278fc11fe5c..30585d98fdf1 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -11,6 +11,7 @@
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> +#include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_irq.h>
> #include <linux/of_platform.h>
> @@ -679,6 +680,8 @@ static const struct of_device_id rpmh_drv_match[] = {
> { .compatible = "qcom,rpmh-rsc", },
> { }
> };
> +MODULE_DEVICE_TABLE(of, rpmh_drv_match);
> +
>
> static struct platform_driver rpmh_driver = {
> .probe = rpmh_rsc_probe,
> @@ -693,3 +696,6 @@ static int __init rpmh_driver_init(void)
> return platform_driver_register(&rpmh_driver);
> }
> arch_initcall(rpmh_driver_init);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
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
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index af774555b9d2..ac91eaf810f7 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -104,7 +104,7 @@ config QCOM_RMTFS_MEM Say y here if you intend to boot the modem remoteproc. config QCOM_RPMH - bool "Qualcomm RPM-Hardened (RPMH) Communication" + tristate "Qualcomm RPM-Hardened (RPMH) Communication" depends on ARCH_QCOM && ARM64 || COMPILE_TEST help Support for communication with the hardened-RPM blocks in diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index e278fc11fe5c..30585d98fdf1 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -11,6 +11,7 @@ #include <linux/io.h> #include <linux/kernel.h> #include <linux/list.h> +#include <linux/module.h> #include <linux/of.h> #include <linux/of_irq.h> #include <linux/of_platform.h> @@ -679,6 +680,8 @@ static const struct of_device_id rpmh_drv_match[] = { { .compatible = "qcom,rpmh-rsc", }, { } }; +MODULE_DEVICE_TABLE(of, rpmh_drv_match); + static struct platform_driver rpmh_driver = { .probe = rpmh_rsc_probe, @@ -693,3 +696,6 @@ static int __init rpmh_driver_init(void) return platform_driver_register(&rpmh_driver); } arch_initcall(rpmh_driver_init); + +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Driver"); +MODULE_LICENSE("GPL v2");
This patch allow the rpmh 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 the rpmh driver is fairly core to the system, so once its loaded with almost anythign else to get the system to go, the dependencies are not likely to ever also be removed. 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 Signed-off-by: John Stultz <john.stultz@linaro.org> --- drivers/soc/qcom/Kconfig | 2 +- drivers/soc/qcom/rpmh-rsc.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)