Re: [PATCH v2 09/11] misc: throttler: Add core support for non-thermal throttling

[Matthias Kaehlcke <mka@chromium.org>]

Hi,

On Mon, Jun 11, 2018 at 06:49:13PM -0700, Brian Norris wrote:
> Hi!
> 
> A few comments, but I didn't get to finish a thorough review yet.
> 
> 
> On Thu, Jun 07, 2018 at 11:12:12AM -0700, Matthias Kaehlcke wrote:
> > The purpose of the throttler is to provide support for non-thermal
> > throttling. Throttling is triggered by external event, e.g. the
> > detection of a high battery discharge current, close to the OCP limit
> > of the battery. The throttler is only in charge of the throttling, not
> > the monitoring, which is done by another (possibly platform specific)
> > driver.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v2:
> > - completely reworked the driver to support configuration through OPPs, which
> >   requires a more dynamic handling
> > - added sysfs attribute to set the level for debugging and testing
> > - Makefile: depend on Kconfig option to traverse throttler directory
> > - Kconfig: removed 'default n'
> > - added SPDX line instead of license boiler-plate
> > - added entry to MAINTAINERS file
> > 
> > 
> >  MAINTAINERS                     |   7 +
> >  drivers/misc/Kconfig            |   1 +
> >  drivers/misc/Makefile           |   1 +
> >  drivers/misc/throttler/Kconfig  |  14 +
> >  drivers/misc/throttler/Makefile |   1 +
> >  drivers/misc/throttler/core.c   | 642 ++++++++++++++++++++++++++++++++
> >  include/linux/throttler.h       |  11 +
> >  7 files changed, 677 insertions(+)
> >  create mode 100644 drivers/misc/throttler/Kconfig
> >  create mode 100644 drivers/misc/throttler/Makefile
> >  create mode 100644 drivers/misc/throttler/core.c
> >  create mode 100644 include/linux/throttler.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 92e47b5b0480..f9550e5680ce 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13938,6 +13938,13 @@ T:	git git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git
> >  S:	Maintained
> >  F:	drivers/platform/x86/thinkpad_acpi.c
> >  
> > +THROTTLER DRIVERS
> > +M:	Matthias Kaehlcke <mka@chromium.org>
> > +L:	linux-pm@vger.kernel.org
> > +S:	Maintained
> > +F:	drivers/misc/throttler/
> > +F:	include/linux/throttler.h
> > +
> >  THUNDERBOLT DRIVER
> >  M:	Andreas Noever <andreas.noever@gmail.com>
> >  M:	Michael Jamet <michael.jamet@intel.com>
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 5d713008749b..691d9625d83c 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -513,4 +513,5 @@ source "drivers/misc/echo/Kconfig"
> >  source "drivers/misc/cxl/Kconfig"
> >  source "drivers/misc/ocxl/Kconfig"
> >  source "drivers/misc/cardreader/Kconfig"
> > +source "drivers/misc/throttler/Kconfig"
> >  endmenu
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index 20be70c3f118..b549ccad5215 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -57,3 +57,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)	+= aspeed-lpc-snoop.o
> >  obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
> >  obj-$(CONFIG_OCXL)		+= ocxl/
> >  obj-$(CONFIG_MISC_RTSX)		+= cardreader/
> > +obj-$(CONFIG_THROTTLER)		+= throttler/
> > diff --git a/drivers/misc/throttler/Kconfig b/drivers/misc/throttler/Kconfig
> > new file mode 100644
> > index 000000000000..e561f1df5085
> > --- /dev/null
> > +++ b/drivers/misc/throttler/Kconfig
> > @@ -0,0 +1,14 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +menuconfig THROTTLER
> > +	bool "Throttler support"
> > +	depends on OF
> > +	select CPU_FREQ
> > +	select PM_DEVFREQ
> 
> I'm curious whether we're actually truly compile-time dependent on
> {CPU,DEV}FREQ? It seems like those subsystems mostly stub out stuff so
> they fall back to no-ops if not built in.
>
> I know that's not very useful for your existing throttler, since it
> wouldn't be very effective if one or both were disabled.

The idea is not to depend on both options being enabled, since
throttling of one type might be all that is needed.

As the build bot pointed out cpufreq doesn't stub out all functions.
Probably the cleanest way to work around this is to define a stub for
cpufreq_update_policy() in the throttler when CONFIG_CPU_FREQ is not
defined.

> > diff --git a/drivers/misc/throttler/core.c b/drivers/misc/throttler/core.c
> > new file mode 100644
> > index 000000000000..15b50c111032
> > --- /dev/null
> > +++ b/drivers/misc/throttler/core.c
> > ...
> > +// #define DEBUG_THROTTLER
> 
> Did you mean to leave your debug code in? Seems like you have some
> related dead code under #ifdefs.

Yes, I left it in intentionally.

> (If you do want this, maybe it'd be better under debugfs, until somebody
> really wants to formalize and document it.)

Since it is code that is never enabled in an official kernel build I
found it simpler to use sysfs with a direct association with the
device, instead of having <debugfs>/throttler/<throttler_name>/level.

If folks have strong opinions about using debugfs or not having the
debug code at all I'm fine with changing/dropping it.

> > +static int thr_init_freq_table(struct throttler *thr, struct device *opp_dev,
> > +			       struct thr_freq_table *ft)
> > +{
> > +	struct device_node *np_opp_desc, *np_opp;
> > +	int nchilds;
> > +	uint32_t *freqs;
> > +	int nfreqs = 0;
> > +	int err = 0;
> > +
> > +	np_opp_desc = dev_pm_opp_of_get_opp_desc_node(opp_dev);
> > +	if (!np_opp_desc)
> > +		return -EINVAL;
> > +
> > +	nchilds = of_get_child_count(np_opp_desc);
> > +	if (!nchilds) {
> > +		err = -EINVAL;
> > +		goto out_node_put;
> > +	}
> > +
> > +	freqs = kzalloc(nchilds * sizeof(uint32_t), GFP_KERNEL);
> > +	if (!freqs) {
> > +		err = -ENOMEM;
> > +		goto out_node_put;
> > +	}
> > +
> > +	/* determine which OPPs are used by this throttler (if any) */
> > +	for_each_child_of_node(np_opp_desc, np_opp) {
> > +		int num_thr;
> > +		int i;
> > +
> > +		num_thr = of_property_count_u32_elems(np_opp, "opp-throttlers");
> > +		if (num_thr < 0)
> > +			continue;
> > +
> > +		for (i = 0; i < num_thr; i++) {
> > +			struct device_node *np_thr;
> > +			struct platform_device *pdev;
> > +
> > +			np_thr = of_parse_phandle(np_opp, "opp-throttlers", i);
> > +			if (!np_thr) {
> > +				dev_err(thr->dev,
> > +					"failed to parse phandle %d: %s\n", i,
> > +					np_opp->full_name);
> > +				continue;
> > +			}
> > +
> > +			pdev = of_find_device_by_node(np_thr);
> > +			if (!pdev) {
> > +				dev_err(thr->dev,
> > +					"could not find throttler dev: %s\n",
> > +					 np_thr->full_name);
> > +				of_node_put(np_thr);
> > +				continue;
> > +			}
> > +
> > +			/* OPP is used by this throttler */
> > +			if (&pdev->dev == thr->dev) {
> 
> So you're assuming that all throttlers are platform devices? Seems
> slightly shaky; I could easily imagine a similar device that's a SPI or
> I2C device.

As of now that's the only existing throttler. Adding handling for
throttlers on other buses that might never be implemented seems
premature. If throttlers with other bus types are added the core
code can be extended to support this using
of_find_i2c_device_by_node(), of_find_spi_device_by_node(), etc

Thanks

Matthias