* Re: [PATCH] driver core: Remove device link creation limitation
From: Rafael J. Wysocki @ 2019-07-11 11:39 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rafael J. Wysocki, Greg Kroah-Hartman, LKML, Linux PM,
Lukas Wunner, Jon Hunter, Ulf Hansson, Marek Szyprowski
In-Reply-To: <CAGETcx-FgohUahM+aLOBUQGgJKyPW6ptFAqRdcPJG2FkE6CN5w@mail.gmail.com>
On Thu, Jul 11, 2019 at 12:06 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jul 10, 2019 at 3:19 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If device_link_add() is called for a consumer/supplier pair with an
> > existing device link between them and the existing link's type is
> > not in agreement with the flags passed to that function by its
> > caller, NULL will be returned. That is seriously inconvenient,
> > because it forces the callers of device_link_add() to worry about
> > what others may or may not do even if that is not relevant to them
> > for any other reasons.
> >
> > It turns out, however, that this limitation can be made go away
> > relatively easily.
> >
> > The underlying observation is that if DL_FLAG_STATELESS has been
> > passed to device_link_add() in flags for the given consumer/supplier
> > pair at least once, calling either device_link_del() or
> > device_link_remove() to release the link returned by it should work,
> > but there are no other requirements associated with that flag. In
> > turn, if at least one of the callers of device_link_add() for the
> > given consumer/supplier pair has not passed DL_FLAG_STATELESS to it
> > in flags, the driver core should track the status of the link and act
> > on it as appropriate (ie. the link should be treated as "managed").
> > This means that DL_FLAG_STATELESS needs to be set for managed device
> > links and it should be valid to call device_link_del() or
> > device_link_remove() to drop references to them in certain
> > sutiations.
> >
> > To allow that to happen, introduce a new (internal) device link flag
> > called DL_FLAG_MANAGED and make device_link_add() set it automatically
> > whenever DL_FLAG_STATELESS is not passed to it. Also make it take
> > additional references to existing device links that were previously
> > stateless (that is, with DL_FLAG_STATELESS set and DL_FLAG_MANAGED
> > unset) and will need to be managed going forward and initialize
> > their status (which has been DL_STATE_NONE so far).
> >
> > Accordingly, when a managed device link is dropped automatically
> > by the driver core, make it clear DL_FLAG_MANAGED, reset the link's
> > status back to DL_STATE_NONE and drop the reference to it associated
> > with DL_FLAG_MANAGED instead of just deleting it right away (to
> > allow it to stay around in case it still needs to be released
> > explicitly by someone).
> >
> > With that, since setting DL_FLAG_STATELESS doesn't mean that the
> > device link in question is not managed any more, replace all of the
> > status-tracking checks against DL_FLAG_STATELESS with analogous
> > checks against DL_FLAG_MANAGED and update the documentation to
> > reflect these changes.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > Documentation/driver-api/device_link.rst | 4
> > drivers/base/core.c | 169 +++++++++++++++++--------------
> > drivers/base/power/runtime.c | 4
> > include/linux/device.h | 4
> > 4 files changed, 102 insertions(+), 79 deletions(-)
> >
> > Index: linux-pm/drivers/base/core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/core.c
> > +++ linux-pm/drivers/base/core.c
> > @@ -124,6 +124,50 @@ static int device_is_dependent(struct de
> > return ret;
> > }
> >
> > +static void device_link_init_status(struct device_link *link,
> > + struct device *consumer,
> > + struct device *supplier)
> > +{
> > + switch (supplier->links.status) {
> > + case DL_DEV_PROBING:
> > + switch (consumer->links.status) {
> > + case DL_DEV_PROBING:
> > + /*
> > + * A consumer driver can create a link to a supplier
> > + * that has not completed its probing yet as long as it
> > + * knows that the supplier is already functional (for
> > + * example, it has just acquired some resources from the
> > + * supplier).
> > + */
> > + link->status = DL_STATE_CONSUMER_PROBE;
> > + break;
> > + default:
> > + link->status = DL_STATE_DORMANT;
> > + break;
> > + }
> > + break;
> > + case DL_DEV_DRIVER_BOUND:
> > + switch (consumer->links.status) {
> > + case DL_DEV_PROBING:
> > + link->status = DL_STATE_CONSUMER_PROBE;
> > + break;
> > + case DL_DEV_DRIVER_BOUND:
> > + link->status = DL_STATE_ACTIVE;
> > + break;
> > + default:
> > + link->status = DL_STATE_AVAILABLE;
> > + break;
> > + }
> > + break;
> > + case DL_DEV_UNBINDING:
> > + link->status = DL_STATE_SUPPLIER_UNBIND;
> > + break;
> > + default:
> > + link->status = DL_STATE_DORMANT;
> > + break;
> > + }
> > +}
> > +
> > static int device_reorder_to_tail(struct device *dev, void *not_used)
> > {
> > struct device_link *link;
> > @@ -179,9 +223,9 @@ void device_pm_move_to_tail(struct devic
> > * of the link. If DL_FLAG_PM_RUNTIME is not set, DL_FLAG_RPM_ACTIVE will be
> > * ignored.
> > *
> > - * If DL_FLAG_STATELESS is set in @flags, the link is not going to be managed by
> > - * the driver core and, in particular, the caller of this function is expected
> > - * to drop the reference to the link acquired by it directly.
> > + * If DL_FLAG_STATELESS is set in @flags, the caller of this function is
> > + * expected to release the link returned by it directly with the help of either
> > + * device_link_del() or device_link_remove().
> > *
> > * If that flag is not set, however, the caller of this function is handing the
> > * management of the link over to the driver core entirely and its return value
> > @@ -201,9 +245,16 @@ void device_pm_move_to_tail(struct devic
> > * be used to request the driver core to automaticall probe for a consmer
> > * driver after successfully binding a driver to the supplier device.
> > *
> > - * The combination of DL_FLAG_STATELESS and either DL_FLAG_AUTOREMOVE_CONSUMER
> > - * or DL_FLAG_AUTOREMOVE_SUPPLIER set in @flags at the same time is invalid and
> > - * will cause NULL to be returned upfront.
> > + * The combination of DL_FLAG_STATELESS and one of DL_FLAG_AUTOREMOVE_CONSUMER,
> > + * DL_FLAG_AUTOREMOVE_SUPPLIER, or DL_FLAG_AUTOPROBE_CONSUMER set in @flags at
> > + * the same time is invalid and will cause NULL to be returned upfront.
> > + * However, if a device link between the given @consumer and @supplier pair
> > + * exists already when this function is called for them, the existing link will
> > + * be returned regardless of its current type and status (the link's flags may
> > + * be modified then). The caller of this function is then expected to treat
> > + * the link as though it has just been created, so (in particular) if
> > + * DL_FLAG_STATELESS was passed in @flags, the link needs to be released
> > + * explicitly when not needed any more (as stated above).
> > *
> > * A side effect of the link creation is re-ordering of dpm_list and the
> > * devices_kset list by moving the consumer device and all devices depending
> > @@ -223,7 +274,8 @@ struct device_link *device_link_add(stru
> > (flags & DL_FLAG_STATELESS &&
> > flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
> > DL_FLAG_AUTOREMOVE_SUPPLIER |
> > - DL_FLAG_AUTOPROBE_CONSUMER)) ||
> > + DL_FLAG_AUTOPROBE_CONSUMER |
> > + DL_FLAG_MANAGED)) ||
> > (flags & DL_FLAG_AUTOPROBE_CONSUMER &&
> > flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
> > DL_FLAG_AUTOREMOVE_SUPPLIER)))
>
> If DL_FLAG_MANAGED is meant to be an internal flag (as in caller
> shouldn't use it), maybe it'll be better to just check for it in flags
> and reject it? Because looks like you are setting it anyway if
> STATELESS isn't set.
Well, to be honest, I was kind of divided here, because passing
MANAGED if STATELESS is not passed is technically not a bug and
returning NULL in that case seemed a bit awkward to me.
That said, the function doesn't reject flags with unused bits set
anyway and arguably it should, so I'll add that check and that'll
cover MANAGED too.
>
> > @@ -236,6 +288,9 @@ struct device_link *device_link_add(stru
> > }
> > }
> >
> > + if (!(flags & DL_FLAG_STATELESS))
> > + flags |= DL_FLAG_MANAGED;
> > +
> > device_links_write_lock();
> > device_pm_lock();
> >
> > @@ -262,15 +317,6 @@ struct device_link *device_link_add(stru
> > if (link->consumer != consumer)
> > continue;
> >
> > - /*
> > - * Don't return a stateless link if the caller wants a stateful
> > - * one and vice versa.
> > - */
> > - if (WARN_ON((flags & DL_FLAG_STATELESS) != (link->flags & DL_FLAG_STATELESS))) {
> > - link = NULL;
> > - goto out;
> > - }
> > -
> > if (flags & DL_FLAG_PM_RUNTIME) {
> > if (!(link->flags & DL_FLAG_PM_RUNTIME)) {
> > pm_runtime_new_link(consumer);
> > @@ -281,6 +327,7 @@ struct device_link *device_link_add(stru
> > }
> >
> > if (flags & DL_FLAG_STATELESS) {
> > + link->flags |= DL_FLAG_STATELESS;
> > kref_get(&link->kref);
> > goto out;
> > }
> > @@ -299,6 +346,11 @@ struct device_link *device_link_add(stru
> > link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER |
> > DL_FLAG_AUTOREMOVE_SUPPLIER);
> > }
> > + if (!(link->flags & DL_FLAG_MANAGED)) {
> > + kref_get(&link->kref);
> > + link->flags |= DL_FLAG_MANAGED;
> > + device_link_init_status(link, consumer, supplier);
> > + }
> > goto out;
> > }
> >
> > @@ -325,48 +377,10 @@ struct device_link *device_link_add(stru
> > kref_init(&link->kref);
> >
> > /* Determine the initial link state. */
> > - if (flags & DL_FLAG_STATELESS) {
> > + if (flags & DL_FLAG_STATELESS)
> > link->status = DL_STATE_NONE;
> > - } else {
> > - switch (supplier->links.status) {
> > - case DL_DEV_PROBING:
> > - switch (consumer->links.status) {
> > - case DL_DEV_PROBING:
> > - /*
> > - * A consumer driver can create a link to a
> > - * supplier that has not completed its probing
> > - * yet as long as it knows that the supplier is
> > - * already functional (for example, it has just
> > - * acquired some resources from the supplier).
> > - */
> > - link->status = DL_STATE_CONSUMER_PROBE;
> > - break;
> > - default:
> > - link->status = DL_STATE_DORMANT;
> > - break;
> > - }
> > - break;
> > - case DL_DEV_DRIVER_BOUND:
> > - switch (consumer->links.status) {
> > - case DL_DEV_PROBING:
> > - link->status = DL_STATE_CONSUMER_PROBE;
> > - break;
> > - case DL_DEV_DRIVER_BOUND:
> > - link->status = DL_STATE_ACTIVE;
> > - break;
> > - default:
> > - link->status = DL_STATE_AVAILABLE;
> > - break;
> > - }
> > - break;
> > - case DL_DEV_UNBINDING:
> > - link->status = DL_STATE_SUPPLIER_UNBIND;
> > - break;
> > - default:
> > - link->status = DL_STATE_DORMANT;
> > - break;
> > - }
> > - }
> > + else
> > + device_link_init_status(link, consumer, supplier);
> >
> > /*
> > * Some callers expect the link creation during consumer driver probe to
> > @@ -528,7 +542,7 @@ static void device_links_missing_supplie
> > * mark the link as "consumer probe in progress" to make the supplier removal
> > * wait for us to complete (or bad things may happen).
> > *
> > - * Links with the DL_FLAG_STATELESS flag set are ignored.
> > + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> > */
>
> Nitpick. It might be easier to read if you removed the double
> negative. So something like:
> Links without the DL_FLAG_MANAGED flag set are ignored.
Fair enough.
> > int device_links_check_suppliers(struct device *dev)
> > {
> > @@ -538,7 +552,7 @@ int device_links_check_suppliers(struct
> > device_links_write_lock();
> >
> > list_for_each_entry(link, &dev->links.suppliers, c_node) {
> > - if (link->flags & DL_FLAG_STATELESS)
> > + if (!(link->flags & DL_FLAG_MANAGED))
> > continue;
> >
> > if (link->status != DL_STATE_AVAILABLE) {
> > @@ -563,7 +577,7 @@ int device_links_check_suppliers(struct
> > *
> > * Also change the status of @dev's links to suppliers to "active".
> > *
> > - * Links with the DL_FLAG_STATELESS flag set are ignored.
> > + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> > */
> > void device_links_driver_bound(struct device *dev)
> > {
> > @@ -572,7 +586,7 @@ void device_links_driver_bound(struct de
> > device_links_write_lock();
> >
> > list_for_each_entry(link, &dev->links.consumers, s_node) {
> > - if (link->flags & DL_FLAG_STATELESS)
> > + if (!(link->flags & DL_FLAG_MANAGED))
> > continue;
> >
> > /*
> > @@ -593,7 +607,7 @@ void device_links_driver_bound(struct de
> > }
> >
> > list_for_each_entry(link, &dev->links.suppliers, c_node) {
> > - if (link->flags & DL_FLAG_STATELESS)
> > + if (!(link->flags & DL_FLAG_MANAGED))
> > continue;
> >
> > WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
> > @@ -605,6 +619,13 @@ void device_links_driver_bound(struct de
> > device_links_write_unlock();
> > }
> >
> > +static void device_link_drop_managed(struct device_link *link)
> > +{
> > + link->flags &= ~DL_FLAG_MANAGED;
> > + WRITE_ONCE(link->status, DL_STATE_NONE);
> > + kref_put(&link->kref, __device_link_del);
> > +}
> > +
> > /**
> > * __device_links_no_driver - Update links of a device without a driver.
> > * @dev: Device without a drvier.
> > @@ -615,18 +636,18 @@ void device_links_driver_bound(struct de
> > * unless they already are in the "supplier unbind in progress" state in which
> > * case they need not be updated.
> > *
> > - * Links with the DL_FLAG_STATELESS flag set are ignored.
> > + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> > */
> > static void __device_links_no_driver(struct device *dev)
> > {
> > struct device_link *link, *ln;
> >
> > list_for_each_entry_safe_reverse(link, ln, &dev->links.suppliers, c_node) {
> > - if (link->flags & DL_FLAG_STATELESS)
> > + if (!(link->flags & DL_FLAG_MANAGED))
> > continue;
> >
> > if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER)
> > - __device_link_del(&link->kref);
> > + device_link_drop_managed(link);
> > else if (link->status == DL_STATE_CONSUMER_PROBE ||
> > link->status == DL_STATE_ACTIVE)
> > WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
> > @@ -643,7 +664,7 @@ static void __device_links_no_driver(str
> > * %__device_links_no_driver() to update links to suppliers for it as
> > * appropriate.
> > *
> > - * Links with the DL_FLAG_STATELESS flag set are ignored.
> > + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> > */
> > void device_links_no_driver(struct device *dev)
> > {
> > @@ -652,7 +673,7 @@ void device_links_no_driver(struct devic
> > device_links_write_lock();
> >
> > list_for_each_entry(link, &dev->links.consumers, s_node) {
> > - if (link->flags & DL_FLAG_STATELESS)
> > + if (!(link->flags & DL_FLAG_MANAGED))
> > continue;
> >
> > /*
> > @@ -680,7 +701,7 @@ void device_links_no_driver(struct devic
> > * invoke %__device_links_no_driver() to update links to suppliers for it as
> > * appropriate.
> > *
> > - * Links with the DL_FLAG_STATELESS flag set are ignored.
> > + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> > */
> > void device_links_driver_cleanup(struct device *dev)
> > {
> > @@ -689,7 +710,7 @@ void device_links_driver_cleanup(struct
> > device_links_write_lock();
> >
> > list_for_each_entry_safe(link, ln, &dev->links.consumers, s_node) {
> > - if (link->flags & DL_FLAG_STATELESS)
> > + if (!(link->flags & DL_FLAG_MANAGED))
> > continue;
> >
> > WARN_ON(link->flags & DL_FLAG_AUTOREMOVE_CONSUMER);
> > @@ -702,7 +723,7 @@ void device_links_driver_cleanup(struct
> > */
> > if (link->status == DL_STATE_SUPPLIER_UNBIND &&
> > link->flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
> > - __device_link_del(&link->kref);
> > + device_link_drop_managed(link);
> >
> > WRITE_ONCE(link->status, DL_STATE_DORMANT);
> > }
> > @@ -724,7 +745,7 @@ void device_links_driver_cleanup(struct
> > *
> > * Return 'false' if there are no probing or active consumers.
> > *
> > - * Links with the DL_FLAG_STATELESS flag set are ignored.
> > + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> > */
> > bool device_links_busy(struct device *dev)
> > {
> > @@ -734,7 +755,7 @@ bool device_links_busy(struct device *de
> > device_links_write_lock();
> >
> > list_for_each_entry(link, &dev->links.consumers, s_node) {
> > - if (link->flags & DL_FLAG_STATELESS)
> > + if (!(link->flags & DL_FLAG_MANAGED))
> > continue;
> >
> > if (link->status == DL_STATE_CONSUMER_PROBE
> > @@ -764,7 +785,7 @@ bool device_links_busy(struct device *de
> > * driver to unbind and start over (the consumer will not re-probe as we have
> > * changed the state of the link already).
> > *
> > - * Links with the DL_FLAG_STATELESS flag set are ignored.
> > + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> > */
> > void device_links_unbind_consumers(struct device *dev)
> > {
> > @@ -776,7 +797,7 @@ void device_links_unbind_consumers(struc
> > list_for_each_entry(link, &dev->links.consumers, s_node) {
> > enum device_link_state status;
> >
> > - if (link->flags & DL_FLAG_STATELESS)
> > + if (!(link->flags & DL_FLAG_MANAGED))
> > continue;
> >
> > status = link->status;
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -1624,7 +1624,7 @@ void pm_runtime_remove(struct device *de
> > * runtime PM references to the device, drop the usage counter of the device
> > * (as many times as needed).
> > *
> > - * Links with the DL_FLAG_STATELESS flag set are ignored.
> > + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> > *
> > * Since the device is guaranteed to be runtime-active at the point this is
> > * called, nothing else needs to be done here.
> > @@ -1641,7 +1641,7 @@ void pm_runtime_clean_up_links(struct de
> > idx = device_links_read_lock();
> >
> > list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
> > - if (link->flags & DL_FLAG_STATELESS)
> > + if (!(link->flags & DL_FLAG_MANAGED))
> > continue;
> >
> > while (refcount_dec_not_one(&link->rpm_active))
> > Index: linux-pm/include/linux/device.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/device.h
> > +++ linux-pm/include/linux/device.h
> > @@ -829,12 +829,13 @@ enum device_link_state {
> > /*
> > * Device link flags.
> > *
> > - * STATELESS: The core won't track the presence of supplier/consumer drivers.
> > + * STATELESS: The core will not remove this link automatically.
> > * AUTOREMOVE_CONSUMER: Remove the link automatically on consumer driver unbind.
> > * PM_RUNTIME: If set, the runtime PM framework will use this link.
> > * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
> > * AUTOREMOVE_SUPPLIER: Remove the link automatically on supplier driver unbind.
> > * AUTOPROBE_CONSUMER: Probe consumer driver automatically after supplier binds.
> > + * MANAGED: The core tracks presence of supplier/consumer drivers (internal).
> > */
> > #define DL_FLAG_STATELESS BIT(0)
> > #define DL_FLAG_AUTOREMOVE_CONSUMER BIT(1)
> > @@ -842,6 +843,7 @@ enum device_link_state {
> > #define DL_FLAG_RPM_ACTIVE BIT(3)
> > #define DL_FLAG_AUTOREMOVE_SUPPLIER BIT(4)
> > #define DL_FLAG_AUTOPROBE_CONSUMER BIT(5)
> > +#define DL_FLAG_MANAGED BIT(6)
> >
> > /**
> > * struct device_link - Device link representation.
> > Index: linux-pm/Documentation/driver-api/device_link.rst
> > ===================================================================
> > --- linux-pm.orig/Documentation/driver-api/device_link.rst
> > +++ linux-pm/Documentation/driver-api/device_link.rst
> > @@ -78,8 +78,8 @@ typically deleted in its ``->remove`` ca
> > driver is compiled as a module, the device link is added on module load and
> > orderly deleted on unload. The same restrictions that apply to device link
> > addition (e.g. exclusion of a parallel suspend/resume transition) apply equally
> > -to deletion. Device links with ``DL_FLAG_STATELESS`` unset (i.e. managed
> > -device links) are deleted automatically by the driver core.
> > +to deletion. Device links managed by the driver core are deleted automatically
> > +by it.
> >
> > Several flags may be specified on device link addition, two of which
> > have already been mentioned above: ``DL_FLAG_STATELESS`` to express that no
> >
>
> Other than those 2 minor comments, this looks good to me.
>
> Reviewed-by: Saravana Kannan <saravanak@google.com>
Thanks!
^ permalink raw reply
* Re: [RFC PATCH] PM: QoS: Get rid of unused flags
From: Rafael J. Wysocki @ 2019-07-11 12:15 UTC (permalink / raw)
To: Amit Kucheria
Cc: Linux Kernel Mailing List, Rafael J. Wysocki, Len Brown,
Pavel Machek, Linux PM
In-Reply-To: <ed318d093fde671dc0bae0aeb8cf07e56a72cf27.1562751312.git.amit.kucheria@linaro.org>
On Wed, Jul 10, 2019 at 12:12 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>
> The network_latency and network_throughput flags for PM-QoS have not
> found much use in drivers or in userspace since they were introduced.
>
> Commit 4a733ef1bea7 ("mac80211: remove PM-QoS listener") removed the
> only user PM_QOS_NETWORK_LATENCY in the kernel a while ago and there
> don't seem to be any userspace tools using the character device files
> either.
>
> PM_QOS_MEMORY_BANDWIDTH was never even added to the trace events.
>
> Remove all the flags except cpu_dma_latency.
>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
I agree with the rationale, but the patch will clash with some PM QoS
changes from Viresh already in linux-next AFAICS. Can you please
rebase it on top of that material?
> ---
> I've looked around for use of /dev/network_throughput and
> /dev/network_bandwidth) and not found any userspace programs that seem to
> use this currently. So this shouldn't be breaking our ABI contract with
> userspace.
>
> Documentation/power/pm_qos_interface.txt | 5 +--
> include/linux/pm_qos.h | 6 ---
> include/trace/events/power.h | 8 +---
> kernel/power/qos.c | 48 ------------------------
> 4 files changed, 4 insertions(+), 63 deletions(-)
>
> diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
> index 19c5f7b1a7bab..ac53ca4ae24e1 100644
> --- a/Documentation/power/pm_qos_interface.txt
> +++ b/Documentation/power/pm_qos_interface.txt
> @@ -5,8 +5,7 @@ performance expectations by drivers, subsystems and user space applications on
> one of the parameters.
>
> Two different PM QoS frameworks are available:
> -1. PM QoS classes for cpu_dma_latency, network_latency, network_throughput,
> -memory_bandwidth.
> +1. PM QoS classes for cpu_dma_latency
> 2. the per-device PM QoS framework provides the API to manage the per-device latency
> constraints and PM QoS flags.
>
> @@ -74,7 +73,7 @@ cleanup of a process, the interface requires the process to register its
> parameter requests in the following way:
>
> To register the default pm_qos target for the specific parameter, the process
> -must open one of /dev/[cpu_dma_latency, network_latency, network_throughput]
> +must open /dev/cpu_dma_latency
>
> As long as the device node is held open that process has a registered
> request on the parameter.
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 6ea1ae373d77c..2a3c237b1910d 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -13,9 +13,6 @@
> enum {
> PM_QOS_RESERVED = 0,
> PM_QOS_CPU_DMA_LATENCY,
> - PM_QOS_NETWORK_LATENCY,
> - PM_QOS_NETWORK_THROUGHPUT,
> - PM_QOS_MEMORY_BANDWIDTH,
>
> /* insert new class ID */
> PM_QOS_NUM_CLASSES,
> @@ -33,9 +30,6 @@ enum pm_qos_flags_status {
> #define PM_QOS_LATENCY_ANY_NS ((s64)PM_QOS_LATENCY_ANY * NSEC_PER_USEC)
>
> #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> -#define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> -#define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
> -#define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE 0
> #define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE PM_QOS_LATENCY_ANY
> #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT PM_QOS_LATENCY_ANY
> #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS PM_QOS_LATENCY_ANY_NS
> diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> index f7aece721aed7..7457e238e1b74 100644
> --- a/include/trace/events/power.h
> +++ b/include/trace/events/power.h
> @@ -379,9 +379,7 @@ DECLARE_EVENT_CLASS(pm_qos_request,
>
> TP_printk("pm_qos_class=%s value=%d",
> __print_symbolic(__entry->pm_qos_class,
> - { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" },
> - { PM_QOS_NETWORK_LATENCY, "NETWORK_LATENCY" },
> - { PM_QOS_NETWORK_THROUGHPUT, "NETWORK_THROUGHPUT" }),
> + { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" }),
> __entry->value)
> );
>
> @@ -426,9 +424,7 @@ TRACE_EVENT(pm_qos_update_request_timeout,
>
> TP_printk("pm_qos_class=%s value=%d, timeout_us=%ld",
> __print_symbolic(__entry->pm_qos_class,
> - { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" },
> - { PM_QOS_NETWORK_LATENCY, "NETWORK_LATENCY" },
> - { PM_QOS_NETWORK_THROUGHPUT, "NETWORK_THROUGHPUT" }),
> + { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" }),
> __entry->value, __entry->timeout_us)
> );
>
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 33e3febaba53f..9568a2fe7c116 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -78,57 +78,9 @@ static struct pm_qos_object cpu_dma_pm_qos = {
> .name = "cpu_dma_latency",
> };
>
> -static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> -static struct pm_qos_constraints network_lat_constraints = {
> - .list = PLIST_HEAD_INIT(network_lat_constraints.list),
> - .target_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> - .default_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> - .no_constraint_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> - .type = PM_QOS_MIN,
> - .notifiers = &network_lat_notifier,
> -};
> -static struct pm_qos_object network_lat_pm_qos = {
> - .constraints = &network_lat_constraints,
> - .name = "network_latency",
> -};
> -
> -
> -static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> -static struct pm_qos_constraints network_tput_constraints = {
> - .list = PLIST_HEAD_INIT(network_tput_constraints.list),
> - .target_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> - .default_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> - .no_constraint_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> - .type = PM_QOS_MAX,
> - .notifiers = &network_throughput_notifier,
> -};
> -static struct pm_qos_object network_throughput_pm_qos = {
> - .constraints = &network_tput_constraints,
> - .name = "network_throughput",
> -};
> -
> -
> -static BLOCKING_NOTIFIER_HEAD(memory_bandwidth_notifier);
> -static struct pm_qos_constraints memory_bw_constraints = {
> - .list = PLIST_HEAD_INIT(memory_bw_constraints.list),
> - .target_value = PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE,
> - .default_value = PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE,
> - .no_constraint_value = PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE,
> - .type = PM_QOS_SUM,
> - .notifiers = &memory_bandwidth_notifier,
> -};
> -static struct pm_qos_object memory_bandwidth_pm_qos = {
> - .constraints = &memory_bw_constraints,
> - .name = "memory_bandwidth",
> -};
> -
> -
> static struct pm_qos_object *pm_qos_array[] = {
> &null_pm_qos,
> &cpu_dma_pm_qos,
> - &network_lat_pm_qos,
> - &network_throughput_pm_qos,
> - &memory_bandwidth_pm_qos,
> };
>
> static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v2 2/6] ARM: tegra: Expose functions required for cpuidle driver
From: Jon Hunter @ 2019-07-11 12:42 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, Peter De Schrijver,
Rafael J. Wysocki, Daniel Lezcano
Cc: linux-pm, linux-tegra, linux-arm-kernel, linux-kernel
In-Reply-To: <20190711031312.10038-3-digetx@gmail.com>
On 11/07/2019 04:13, Dmitry Osipenko wrote:
> The upcoming unified CPUIDLE driver will be added to the drivers/cpuidle/
> directory and it will require all these Tegra PM-core functions.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> arch/arm/mach-tegra/Makefile | 2 +-
> arch/arm/mach-tegra/platsmp.c | 2 --
> arch/arm/mach-tegra/pm.c | 16 +++++++---------
> arch/arm/mach-tegra/pm.h | 3 ---
> arch/arm/mach-tegra/sleep.h | 1 -
> include/linux/clk/tegra.h | 13 +++++++++++++
> include/soc/tegra/pm.h | 28 ++++++++++++++++++++++++++++
> 7 files changed, 49 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
> index 5d93a0b36866..27bd5d9865e3 100644
> --- a/arch/arm/mach-tegra/Makefile
> +++ b/arch/arm/mach-tegra/Makefile
> @@ -13,7 +13,7 @@ obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += pm-tegra20.o
> obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += sleep-tegra30.o
> obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += pm-tegra30.o
> obj-$(CONFIG_SMP) += platsmp.o
> -obj-$(CONFIG_HOTPLUG_CPU) += hotplug.o
> +obj-y += hotplug.o
>
> obj-$(CONFIG_ARCH_TEGRA_114_SOC) += sleep-tegra30.o
> obj-$(CONFIG_ARCH_TEGRA_114_SOC) += pm-tegra30.o
> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
> index e6911a14c096..c8a63719a143 100644
> --- a/arch/arm/mach-tegra/platsmp.c
> +++ b/arch/arm/mach-tegra/platsmp.c
> @@ -183,8 +183,6 @@ const struct smp_operations tegra_smp_ops __initconst = {
> .smp_prepare_cpus = tegra_smp_prepare_cpus,
> .smp_secondary_init = tegra_secondary_init,
> .smp_boot_secondary = tegra_boot_secondary,
> -#ifdef CONFIG_HOTPLUG_CPU
> .cpu_kill = tegra_cpu_kill,
> .cpu_die = tegra_cpu_die,
> -#endif
> };
> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
> index 6aaacb5757e1..f9c9bce9e15d 100644
> --- a/arch/arm/mach-tegra/pm.c
> +++ b/arch/arm/mach-tegra/pm.c
> @@ -123,11 +123,9 @@ void tegra_clear_cpu_in_lp2(void)
> spin_unlock(&tegra_lp2_lock);
> }
>
> -bool tegra_set_cpu_in_lp2(void)
> +void tegra_set_cpu_in_lp2(void)
> {
> int phy_cpu_id = cpu_logical_map(smp_processor_id());
> - bool last_cpu = false;
> - cpumask_t *cpu_lp2_mask = tegra_cpu_lp2_mask;
> u32 *cpu_in_lp2 = tegra_cpu_lp2_mask;
>
> spin_lock(&tegra_lp2_lock);
> @@ -135,11 +133,7 @@ bool tegra_set_cpu_in_lp2(void)
> BUG_ON((*cpu_in_lp2 & BIT(phy_cpu_id)));
> *cpu_in_lp2 |= BIT(phy_cpu_id);
>
> - if ((phy_cpu_id == 0) && cpumask_equal(cpu_lp2_mask, cpu_online_mask))
> - last_cpu = true;
> -
> spin_unlock(&tegra_lp2_lock);
> - return last_cpu;
> }
I think that the commit message should describe what is going on here or
this should be a separate change.
>
> static int tegra_sleep_cpu(unsigned long v2p)
> @@ -195,14 +189,16 @@ static void tegra_pm_set(enum tegra_suspend_mode mode)
> tegra_pmc_enter_suspend_mode(mode);
> }
>
> -void tegra_idle_lp2_last(void)
> +int tegra_idle_lp2_last(void)
> {
> + int err;
> +
> tegra_pm_set(TEGRA_SUSPEND_LP2);
>
> cpu_cluster_pm_enter();
> suspend_cpu_complex();
>
> - cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
> + err = cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
>
> /*
> * Resume L2 cache if it wasn't re-enabled early during resume,
> @@ -214,6 +210,8 @@ void tegra_idle_lp2_last(void)
>
> restore_cpu_complex();
> cpu_cluster_pm_exit();
> +
> + return err;
> }
>
> enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
> diff --git a/arch/arm/mach-tegra/pm.h b/arch/arm/mach-tegra/pm.h
> index 1e51a9b636eb..81525f5f4a44 100644
> --- a/arch/arm/mach-tegra/pm.h
> +++ b/arch/arm/mach-tegra/pm.h
> @@ -23,9 +23,6 @@ void tegra20_sleep_core_init(void);
> void tegra30_lp1_iram_hook(void);
> void tegra30_sleep_core_init(void);
>
> -void tegra_clear_cpu_in_lp2(void);
> -bool tegra_set_cpu_in_lp2(void);
> -void tegra_idle_lp2_last(void);
> extern void (*tegra_tear_down_cpu)(void);
>
> #ifdef CONFIG_PM_SLEEP
> diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h
> index d219872b7546..0d9956e9a8ea 100644
> --- a/arch/arm/mach-tegra/sleep.h
> +++ b/arch/arm/mach-tegra/sleep.h
> @@ -124,7 +124,6 @@ void tegra30_hotplug_shutdown(void);
> #endif
>
> void tegra20_tear_down_cpu(void);
> -int tegra30_sleep_cpu_secondary_finish(unsigned long);
> void tegra30_tear_down_cpu(void);
>
> #endif
> diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h
> index b8aef62cc3f5..cf0f2cb5e109 100644
> --- a/include/linux/clk/tegra.h
> +++ b/include/linux/clk/tegra.h
> @@ -108,6 +108,19 @@ static inline void tegra_cpu_clock_resume(void)
>
> tegra_cpu_car_ops->resume();
> }
> +#else
> +static inline bool tegra_cpu_rail_off_ready(void)
> +{
> + return false;
> +}
> +
> +static inline void tegra_cpu_clock_suspend(void)
> +{
> +}
> +
> +static inline void tegra_cpu_clock_resume(void)
> +{
> +}
> #endif
>
> extern void tegra210_xusb_pll_hw_control_enable(void);
> diff --git a/include/soc/tegra/pm.h b/include/soc/tegra/pm.h
> index 951fcd738d55..fa18c2df5028 100644
> --- a/include/soc/tegra/pm.h
> +++ b/include/soc/tegra/pm.h
> @@ -20,6 +20,12 @@ tegra_pm_validate_suspend_mode(enum tegra_suspend_mode mode);
>
> /* low-level resume entry point */
> void tegra_resume(void);
> +
> +int tegra30_sleep_cpu_secondary_finish(unsigned long arg);
> +void tegra_clear_cpu_in_lp2(void);
> +void tegra_set_cpu_in_lp2(void);
> +int tegra_idle_lp2_last(void);
> +void tegra_cpu_die(unsigned int cpu);
> #else
> static inline enum tegra_suspend_mode
> tegra_pm_validate_suspend_mode(enum tegra_suspend_mode mode)
> @@ -30,6 +36,28 @@ tegra_pm_validate_suspend_mode(enum tegra_suspend_mode mode)
> static inline void tegra_resume(void)
> {
> }
> +
> +static inline int tegra30_sleep_cpu_secondary_finish(unsigned long arg)
> +{
> + return -1;
> +}
-ENOTSUPP?
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 1/3] opp: core: add regulators enable and disable
From: Kamil Konieczny @ 2019-07-11 12:58 UTC (permalink / raw)
To: Viresh Kumar
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <20190711062226.4i4bvbsyczshdlyr@vireshk-i7>
On 11.07.2019 08:22, Viresh Kumar wrote:
> On 08-07-19, 16:11, k.konieczny@partner.samsung.com wrote:
>> From: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>
>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>> regulators to dev_pm_opp_put_regulators(). This prepares for
>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>> ---
>> drivers/opp/core.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..947cac452854 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>> if (ret)
>> goto free_regulators;
>>
>> + for (i = 0; i < opp_table->regulator_count; i++) {
>> + ret = regulator_enable(opp_table->regulators[i]);
>> + if (ret < 0)
>> + goto disable;
>> + }
>
> What about doing this in the same loop of regulator_get_optional() ?
yes, this is good, it will simplify code
>> +
>> return opp_table;
>>
>> +disable:
>> + while (i != 0)
>> + regulator_disable(opp_table->regulators[--i]);
>> +
>> + i = opp_table->regulator_count;
>
> You also need to call _free_set_opp_data() here.
good catch
if I move enable in loop above, then this will not be needed
>> free_regulators:
>> while (i != 0)
>> regulator_put(opp_table->regulators[--i]);
>> @@ -1609,6 +1620,8 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>>
>> /* Make sure there are no concurrent readers while updating opp_table */
>> WARN_ON(!list_empty(&opp_table->opp_list));
>
> Preserve the blank line here.
>
>> + for (i = opp_table->regulator_count - 1; i >= 0; i--)
>> + regulator_disable(opp_table->regulators[i]);
>>
>> for (i = opp_table->regulator_count - 1; i >= 0; i--)
>> regulator_put(opp_table->regulators[i]);
>
> Only single loop should be sufficient for this.
you are right, I will do this in single loop
I will send v2
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [RFC PATCH] PM: QoS: Get rid of unused flags
From: Amit Kucheria @ 2019-07-11 12:58 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux Kernel Mailing List, Rafael J. Wysocki, Len Brown,
Pavel Machek, Linux PM
In-Reply-To: <CAJZ5v0hq2yLBOp1OVswS0u-PGOLjq65wfjUqWZB2Tf5oPhOQpQ@mail.gmail.com>
On Thu, Jul 11, 2019 at 5:45 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jul 10, 2019 at 12:12 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> >
> > The network_latency and network_throughput flags for PM-QoS have not
> > found much use in drivers or in userspace since they were introduced.
> >
> > Commit 4a733ef1bea7 ("mac80211: remove PM-QoS listener") removed the
> > only user PM_QOS_NETWORK_LATENCY in the kernel a while ago and there
> > don't seem to be any userspace tools using the character device files
> > either.
> >
> > PM_QOS_MEMORY_BANDWIDTH was never even added to the trace events.
> >
> > Remove all the flags except cpu_dma_latency.
> >
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
>
> I agree with the rationale, but the patch will clash with some PM QoS
> changes from Viresh already in linux-next AFAICS. Can you please
> rebase it on top of that material?
Sure, I'll rebase on linux-next and resend.
> > ---
> > I've looked around for use of /dev/network_throughput and
> > /dev/network_bandwidth) and not found any userspace programs that seem to
> > use this currently. So this shouldn't be breaking our ABI contract with
> > userspace.
> >
> > Documentation/power/pm_qos_interface.txt | 5 +--
> > include/linux/pm_qos.h | 6 ---
> > include/trace/events/power.h | 8 +---
> > kernel/power/qos.c | 48 ------------------------
> > 4 files changed, 4 insertions(+), 63 deletions(-)
> >
> > diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
> > index 19c5f7b1a7bab..ac53ca4ae24e1 100644
> > --- a/Documentation/power/pm_qos_interface.txt
> > +++ b/Documentation/power/pm_qos_interface.txt
> > @@ -5,8 +5,7 @@ performance expectations by drivers, subsystems and user space applications on
> > one of the parameters.
> >
> > Two different PM QoS frameworks are available:
> > -1. PM QoS classes for cpu_dma_latency, network_latency, network_throughput,
> > -memory_bandwidth.
> > +1. PM QoS classes for cpu_dma_latency
> > 2. the per-device PM QoS framework provides the API to manage the per-device latency
> > constraints and PM QoS flags.
> >
> > @@ -74,7 +73,7 @@ cleanup of a process, the interface requires the process to register its
> > parameter requests in the following way:
> >
> > To register the default pm_qos target for the specific parameter, the process
> > -must open one of /dev/[cpu_dma_latency, network_latency, network_throughput]
> > +must open /dev/cpu_dma_latency
> >
> > As long as the device node is held open that process has a registered
> > request on the parameter.
> > diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> > index 6ea1ae373d77c..2a3c237b1910d 100644
> > --- a/include/linux/pm_qos.h
> > +++ b/include/linux/pm_qos.h
> > @@ -13,9 +13,6 @@
> > enum {
> > PM_QOS_RESERVED = 0,
> > PM_QOS_CPU_DMA_LATENCY,
> > - PM_QOS_NETWORK_LATENCY,
> > - PM_QOS_NETWORK_THROUGHPUT,
> > - PM_QOS_MEMORY_BANDWIDTH,
> >
> > /* insert new class ID */
> > PM_QOS_NUM_CLASSES,
> > @@ -33,9 +30,6 @@ enum pm_qos_flags_status {
> > #define PM_QOS_LATENCY_ANY_NS ((s64)PM_QOS_LATENCY_ANY * NSEC_PER_USEC)
> >
> > #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> > -#define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
> > -#define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
> > -#define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE 0
> > #define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE PM_QOS_LATENCY_ANY
> > #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT PM_QOS_LATENCY_ANY
> > #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS PM_QOS_LATENCY_ANY_NS
> > diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> > index f7aece721aed7..7457e238e1b74 100644
> > --- a/include/trace/events/power.h
> > +++ b/include/trace/events/power.h
> > @@ -379,9 +379,7 @@ DECLARE_EVENT_CLASS(pm_qos_request,
> >
> > TP_printk("pm_qos_class=%s value=%d",
> > __print_symbolic(__entry->pm_qos_class,
> > - { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" },
> > - { PM_QOS_NETWORK_LATENCY, "NETWORK_LATENCY" },
> > - { PM_QOS_NETWORK_THROUGHPUT, "NETWORK_THROUGHPUT" }),
> > + { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" }),
> > __entry->value)
> > );
> >
> > @@ -426,9 +424,7 @@ TRACE_EVENT(pm_qos_update_request_timeout,
> >
> > TP_printk("pm_qos_class=%s value=%d, timeout_us=%ld",
> > __print_symbolic(__entry->pm_qos_class,
> > - { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" },
> > - { PM_QOS_NETWORK_LATENCY, "NETWORK_LATENCY" },
> > - { PM_QOS_NETWORK_THROUGHPUT, "NETWORK_THROUGHPUT" }),
> > + { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" }),
> > __entry->value, __entry->timeout_us)
> > );
> >
> > diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> > index 33e3febaba53f..9568a2fe7c116 100644
> > --- a/kernel/power/qos.c
> > +++ b/kernel/power/qos.c
> > @@ -78,57 +78,9 @@ static struct pm_qos_object cpu_dma_pm_qos = {
> > .name = "cpu_dma_latency",
> > };
> >
> > -static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> > -static struct pm_qos_constraints network_lat_constraints = {
> > - .list = PLIST_HEAD_INIT(network_lat_constraints.list),
> > - .target_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> > - .default_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> > - .no_constraint_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
> > - .type = PM_QOS_MIN,
> > - .notifiers = &network_lat_notifier,
> > -};
> > -static struct pm_qos_object network_lat_pm_qos = {
> > - .constraints = &network_lat_constraints,
> > - .name = "network_latency",
> > -};
> > -
> > -
> > -static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> > -static struct pm_qos_constraints network_tput_constraints = {
> > - .list = PLIST_HEAD_INIT(network_tput_constraints.list),
> > - .target_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> > - .default_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> > - .no_constraint_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
> > - .type = PM_QOS_MAX,
> > - .notifiers = &network_throughput_notifier,
> > -};
> > -static struct pm_qos_object network_throughput_pm_qos = {
> > - .constraints = &network_tput_constraints,
> > - .name = "network_throughput",
> > -};
> > -
> > -
> > -static BLOCKING_NOTIFIER_HEAD(memory_bandwidth_notifier);
> > -static struct pm_qos_constraints memory_bw_constraints = {
> > - .list = PLIST_HEAD_INIT(memory_bw_constraints.list),
> > - .target_value = PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE,
> > - .default_value = PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE,
> > - .no_constraint_value = PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE,
> > - .type = PM_QOS_SUM,
> > - .notifiers = &memory_bandwidth_notifier,
> > -};
> > -static struct pm_qos_object memory_bandwidth_pm_qos = {
> > - .constraints = &memory_bw_constraints,
> > - .name = "memory_bandwidth",
> > -};
> > -
> > -
> > static struct pm_qos_object *pm_qos_array[] = {
> > &null_pm_qos,
> > &cpu_dma_pm_qos,
> > - &network_lat_pm_qos,
> > - &network_throughput_pm_qos,
> > - &memory_bandwidth_pm_qos,
> > };
> >
> > static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [PATCH 2/3] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Kamil Konieczny @ 2019-07-11 13:36 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc@vger.kernel.org
In-Reply-To: <CAJKOXPfWr-2t_e3f6oi7E6KLLRAbskzgEKz26XyK5n_9C8wV1w@mail.gmail.com>
On 10.07.2019 19:04, Krzysztof Kozlowski wrote:
> On Mon, 8 Jul 2019 at 16:12, <k.konieczny@partner.samsung.com> wrote:
>>
>> From: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>
>> Reuse opp core code for setting bus clock and voltage. As a side
>> effect this allow useage of coupled regulators feature (required
>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>> uses regulator_set_voltage_triplet() for setting regulator voltage
>> while the old code used regulator_set_voltage_tol() with fixed
>> tolerance. This patch also removes no longer needed parsing of DT
>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>
> Please also update the bindings in such case. Both with removal of
> unused property and with example/recommended regulator couplings.
Right, I will remove it.
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
^ permalink raw reply
* [PATCH v2] PM: QoS: Get rid of unused flags
From: Amit Kucheria @ 2019-07-11 14:21 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, Rafael J. Wysocki, Len Brown,
Pavel Machek
Cc: linux-pm
In-Reply-To: <cover.1562854650.git.amit.kucheria@linaro.org>
The network_latency and network_throughput flags for PM-QoS have not
found much use in drivers or in userspace since they were introduced.
Commit 4a733ef1bea7 ("mac80211: remove PM-QoS listener") removed the
only user PM_QOS_NETWORK_LATENCY in the kernel a while ago and there
don't seem to be any userspace tools using the character device files
either.
PM_QOS_MEMORY_BANDWIDTH was never even added to the trace events.
Remove all the flags except cpu_dma_latency.
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
Changes from v1:
- Rebased on linux-next to deal with .rst conversion of docs
I've looked around for use of /dev/network_throughput and
/dev/network_bandwidth) and not found any userspace programs that seem to
use this currently. So this shouldn't be breaking our ABI contract with
userspace.
Documentation/power/pm_qos_interface.rst | 5 +--
include/linux/pm_qos.h | 6 ---
include/trace/events/power.h | 8 +---
kernel/power/qos.c | 48 ------------------------
4 files changed, 4 insertions(+), 63 deletions(-)
diff --git a/Documentation/power/pm_qos_interface.rst b/Documentation/power/pm_qos_interface.rst
index 945fc6d760c9..a00d607107ec 100644
--- a/Documentation/power/pm_qos_interface.rst
+++ b/Documentation/power/pm_qos_interface.rst
@@ -7,8 +7,7 @@ performance expectations by drivers, subsystems and user space applications on
one of the parameters.
Two different PM QoS frameworks are available:
-1. PM QoS classes for cpu_dma_latency, network_latency, network_throughput,
-memory_bandwidth.
+1. PM QoS classes for cpu_dma_latency
2. the per-device PM QoS framework provides the API to manage the per-device latency
constraints and PM QoS flags.
@@ -79,7 +78,7 @@ cleanup of a process, the interface requires the process to register its
parameter requests in the following way:
To register the default pm_qos target for the specific parameter, the process
-must open one of /dev/[cpu_dma_latency, network_latency, network_throughput]
+must open /dev/cpu_dma_latency
As long as the device node is held open that process has a registered
request on the parameter.
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 6ea1ae373d77..2a3c237b1910 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -13,9 +13,6 @@
enum {
PM_QOS_RESERVED = 0,
PM_QOS_CPU_DMA_LATENCY,
- PM_QOS_NETWORK_LATENCY,
- PM_QOS_NETWORK_THROUGHPUT,
- PM_QOS_MEMORY_BANDWIDTH,
/* insert new class ID */
PM_QOS_NUM_CLASSES,
@@ -33,9 +30,6 @@ enum pm_qos_flags_status {
#define PM_QOS_LATENCY_ANY_NS ((s64)PM_QOS_LATENCY_ANY * NSEC_PER_USEC)
#define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
-#define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
-#define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
-#define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE 0
#define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE PM_QOS_LATENCY_ANY
#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT PM_QOS_LATENCY_ANY
#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS PM_QOS_LATENCY_ANY_NS
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index f7aece721aed..7457e238e1b7 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -379,9 +379,7 @@ DECLARE_EVENT_CLASS(pm_qos_request,
TP_printk("pm_qos_class=%s value=%d",
__print_symbolic(__entry->pm_qos_class,
- { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" },
- { PM_QOS_NETWORK_LATENCY, "NETWORK_LATENCY" },
- { PM_QOS_NETWORK_THROUGHPUT, "NETWORK_THROUGHPUT" }),
+ { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" }),
__entry->value)
);
@@ -426,9 +424,7 @@ TRACE_EVENT(pm_qos_update_request_timeout,
TP_printk("pm_qos_class=%s value=%d, timeout_us=%ld",
__print_symbolic(__entry->pm_qos_class,
- { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" },
- { PM_QOS_NETWORK_LATENCY, "NETWORK_LATENCY" },
- { PM_QOS_NETWORK_THROUGHPUT, "NETWORK_THROUGHPUT" }),
+ { PM_QOS_CPU_DMA_LATENCY, "CPU_DMA_LATENCY" }),
__entry->value, __entry->timeout_us)
);
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 33e3febaba53..9568a2fe7c11 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -78,57 +78,9 @@ static struct pm_qos_object cpu_dma_pm_qos = {
.name = "cpu_dma_latency",
};
-static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
-static struct pm_qos_constraints network_lat_constraints = {
- .list = PLIST_HEAD_INIT(network_lat_constraints.list),
- .target_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
- .default_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
- .no_constraint_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
- .type = PM_QOS_MIN,
- .notifiers = &network_lat_notifier,
-};
-static struct pm_qos_object network_lat_pm_qos = {
- .constraints = &network_lat_constraints,
- .name = "network_latency",
-};
-
-
-static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
-static struct pm_qos_constraints network_tput_constraints = {
- .list = PLIST_HEAD_INIT(network_tput_constraints.list),
- .target_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
- .default_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
- .no_constraint_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
- .type = PM_QOS_MAX,
- .notifiers = &network_throughput_notifier,
-};
-static struct pm_qos_object network_throughput_pm_qos = {
- .constraints = &network_tput_constraints,
- .name = "network_throughput",
-};
-
-
-static BLOCKING_NOTIFIER_HEAD(memory_bandwidth_notifier);
-static struct pm_qos_constraints memory_bw_constraints = {
- .list = PLIST_HEAD_INIT(memory_bw_constraints.list),
- .target_value = PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE,
- .default_value = PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE,
- .no_constraint_value = PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE,
- .type = PM_QOS_SUM,
- .notifiers = &memory_bandwidth_notifier,
-};
-static struct pm_qos_object memory_bandwidth_pm_qos = {
- .constraints = &memory_bw_constraints,
- .name = "memory_bandwidth",
-};
-
-
static struct pm_qos_object *pm_qos_array[] = {
&null_pm_qos,
&cpu_dma_pm_qos,
- &network_lat_pm_qos,
- &network_throughput_pm_qos,
- &memory_bandwidth_pm_qos,
};
static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
--
2.17.1
^ permalink raw reply related
* Re: Coccinelle: Checking of_node_put() calls with SmPL
From: Tyrel Datwyler @ 2019-07-11 14:41 UTC (permalink / raw)
To: wen.yang99, Markus.Elfring, julia.lawall
Cc: wang.yi59, linux-pm, rjw, daniel.lezcano, kernel-janitors,
linux-kernel, oss, paulus, xue.zhihong, linuxppc-dev,
cheng.shengyu
In-Reply-To: <201907111435459627761@zte.com.cn>
On 07/10/2019 11:35 PM, wen.yang99@zte.com.cn wrote:
>>> we developed a coccinelle script to detect such problems.
>>
>> Would you find the implementation of the function “dt_init_idle_driver”
>> suspicious according to discussed source code search patterns?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpuidle/dt_idle_states.c?id=e9a83bd2322035ed9d7dcf35753d3f984d76c6a5#n208
>> https://elixir.bootlin.com/linux/v5.2/source/drivers/cpuidle/dt_idle_states.c#L208
>>
>>
>>> This script is still being improved.
>>
>> Will corresponding software development challenges become more interesting?
>
> Hello Markus,
> This is the simplified code pattern for it:
>
> 172 for (i = 0; ; i++) {
This loop can only be exited on a break.
> 173 state_node = of_parse_phandle(...); ---> Obtain here
> ...
> 177 match_id = of_match_node(matches, state_node);
> 178 if (!match_id) {
> 179 err = -ENODEV;
> 180 break; ---> Jump out of the loop without releasing it
> 181 }
> 182
> 183 if (!of_device_is_available(state_node)) {
> 184 of_node_put(state_node);
> 185 continue; ---> Release the object references within a loop
> 186 }
> ...
> 208 of_node_put(state_node); --> Release the object references within a loop
This is required at the end of every loop or continue to free the reference.
Only a break will exit the loop where we hit the below of_node_put().
> 209 }
> 210
> 211 of_node_put(state_node); --> There may be double free here.
None of the break conditions call of_node_put(), so it needs to be called here.
-Tyrel
>
> This code pattern is very interesting and the coccinelle software should also recognize this pattern.
>
> Regards,
> Wen
>
^ permalink raw reply
* Re: [IMX] [DRM]: help in analyzing clock_summary rate difference
From: Pintu Agarwal @ 2019-07-11 15:29 UTC (permalink / raw)
To: Fabio Estevam, s.hauer, p.zabel
Cc: open list,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Kernelnewbies, linux-pm
In-Reply-To: <CAOuPNLg5A1bB-Tmndm4PvsJ40tj0yn-bJ2mfifEpjAF-t84wiQ@mail.gmail.com>
Hi,
I need one general help in analyzing difference in clock_summary rate
before and after the system resume.
I am using custom IMX7 board with 4.9 Kernel.
With this I am trying to support some functionality during suspend/resume.
I am trying to analyze clk_summary from:
# cat /sys/kernel/debug/clk/clk_summary
I observed that there are difference in "clock rate" after system
resume and module install. However the enable/prepare count remains
the same.
Since I am not much familiar with clock framework, I am looking for
some help to analyze this issue. It's an internal issue.
May be someone which is familiar with clock analysis or fixed the
similar issue earlier can give me some guidance.
What does the difference in clock rate indicates?
What analysis can be done to narrow down the root cause?
Any example of reference could be helpful to understand.
Thank You!
Regards,
Pintu
^ permalink raw reply
* Re: The tick is active on idle adaptive-tick CPUs when /dev/cpu_dma_latency is used
From: Thomas Lindroth @ 2019-07-11 16:24 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, Linux Kernel Mailing List, Peter Zijlstra,
Frederic Weisbecker
In-Reply-To: <312565511.gEFFlSTcEG@kreacher>
On 7/10/19 12:22 PM, Rafael J. Wysocki wrote:
> On Saturday, July 6, 2019 3:02:11 PM CEST Thomas Lindroth wrote:
>> On 7/6/19 1:06 PM, Rafael J. Wysocki wrote:
>>> The patch is below, but note that it adds the tick stopping overhead to the idle loop
>>> for CPUs that are not adaptive-tick and when PM QoS latency constraints are used
>>> which is not desirable in general.
>>>
>>> Please test it, but as I said above, the real solution appears to be to treat adaptive-tick
>>> CPUs in a special way in the idle loop.
>>>
>>> ---
>>> drivers/cpuidle/governors/menu.c | 16 +++++-----------
>>> 1 file changed, 5 insertions(+), 11 deletions(-)
>>>
>>> Index: linux-pm/drivers/cpuidle/governors/menu.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
>>> +++ linux-pm/drivers/cpuidle/governors/menu.c
>>> @@ -302,9 +302,10 @@ static int menu_select(struct cpuidle_dr
>>> !drv->states[0].disabled && !dev->states_usage[0].disable)) {
>>> /*
>>> * In this case state[0] will be used no matter what, so return
>>> - * it right away and keep the tick running.
>>> + * it right away and keep the tick running if state[0] is a
>>> + * polling one.
>>> */
>>> - *stop_tick = false;
>>> + *stop_tick = !!(drv->states[0].flags & CPUIDLE_FLAG_POLLING);
>>> return 0;
>>> }
>>>
>>> @@ -395,16 +396,9 @@ static int menu_select(struct cpuidle_dr
>>>
>>> return idx;
>>> }
>>> - if (s->exit_latency > latency_req) {
>>> - /*
>>> - * If we break out of the loop for latency reasons, use
>>> - * the target residency of the selected state as the
>>> - * expected idle duration so that the tick is retained
>>> - * as long as that target residency is low enough.
>>> - */
>>> - predicted_us = drv->states[idx].target_residency;
>>> + if (s->exit_latency > latency_req)
>>> break;
>>> - }
>>> +
>>> idx = i;
>>> }
>>
>> I tested the patch and it appears to work. Idle CPUs now have ticks disabled even
>> when /dev/cpu_dma_latency is used.
>
> OK, thanks, but as I said previously, you'd see the problem again with the PM QoS
> latency constraint set to 0, which is somewhat inconsistent. Also, this fix is
> specific to the menu governor and the behavior should not depend on the
> governor here IMO, so I have another patch to try (appended).
>
> Please test it (instead of the previous one) and report back.
>
> ---
> kernel/sched/idle.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -191,7 +191,8 @@ static void cpuidle_idle_call(void)
> */
> next_state = cpuidle_select(drv, dev, &stop_tick);
>
> - if (stop_tick || tick_nohz_tick_stopped())
> + if (stop_tick || tick_nohz_tick_stopped() ||
> + !housekeeping_cpu(dev->cpu, HK_FLAG_TICK))
> tick_nohz_idle_stop_tick();
> else
> tick_nohz_idle_retain_tick();
>
I tested this patch and it seems to work fine using the menu governor
and PM QoS latency constraints matching each C-state including 0.
I didn't test the TEO governor.
^ permalink raw reply
* Re: [PATCHv3 6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)
From: Janakarajan Natarajan @ 2019-07-11 16:58 UTC (permalink / raw)
To: Viresh Kumar, Natarajan, Janakarajan
Cc: linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-pm@vger.kernel.org, devel@acpica.org, Rafael J . Wysocki,
Len Brown, Robert Moore, Erik Schmauss, Ghannam, Yazen
In-Reply-To: <20190711061208.yqxt4ps67vmsy7sp@vireshk-i7>
On 7/11/2019 1:12 AM, Viresh Kumar wrote:
> On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
>> diff --git a/drivers/cpufreq/amd-cpufreq.c b/drivers/cpufreq/amd-cpufreq.c
>> +#define pr_fmt(fmt) "AMD Cpufreq: " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/cpu.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/acpi.h>
>> +#include <linux/delay.h>
> Please keep them in alphabetical order.
Ok.
>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <acpi/cppc_acpi.h>
>> +
>> +struct amd_desc {
>> + int cpu_id;
>> + struct cppc_ctrls ctrls;
>> + struct kobject kobj;
>> +};
>> +
>> +struct amd_desc **all_cpu_data;
>> +
>> +static unsigned int cppc_enable;
>> +module_param(cppc_enable, uint, 0644);
>> +MODULE_PARM_DESC(cppc_enable,
>> + "1 - enable AMD CpuFreq, create CPPC sysfs entries.");
>> +
>> +#define to_amd_desc(a) container_of(a, struct amd_desc, kobj)
>> +
>> +#define show_func(access_fn, struct_name, member_name) \
>> + static ssize_t show_##member_name(struct kobject *kobj, \
>> + struct kobj_attribute *attr, \
>> + char *buf) \
>> + { \
>> + struct amd_desc *desc = to_amd_desc(kobj); \
>> + struct struct_name st_name = {0}; \
>> + int ret; \
>> + \
>> + ret = access_fn(desc->cpu_id, &st_name); \
>> + if (ret) \
>> + return ret; \
>> + \
>> + return scnprintf(buf, PAGE_SIZE, "%llu\n", \
>> + (u64)st_name.member_name); \
>> + } \
>> +
>> +#define store_func(struct_name, member_name, reg_idx) \
>> + static ssize_t store_##member_name(struct kobject *kobj, \
>> + struct kobj_attribute *attr, \
>> + const char *buf, size_t count)\
>> + { \
>> + struct amd_desc *desc = to_amd_desc(kobj); \
>> + struct struct_name st_name = {0}; \
>> + u32 val; \
>> + int ret; \
>> + \
>> + ret = kstrtou32(buf, 0, &val); \
>> + if (ret) \
>> + return ret; \
>> + \
>> + st_name.member_name = val; \
>> + \
>> + ret = cppc_set_reg(desc->cpu_id, &st_name, reg_idx); \
>> + if (ret) \
>> + return ret; \
>> + \
>> + return count; \
>> + } \
>> +
>> +#define define_one_rw(struct_name, access_fn, member_name, reg_idx) \
>> + show_func(access_fn, struct_name, member_name) \
>> + store_func(struct_name, member_name, reg_idx) \
>> + define_one_global_rw(member_name)
>> +
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, enable, ENABLE);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, max_perf, MAX_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, min_perf, MIN_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, desired_perf, DESIRED_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, auto_sel_enable, AUTO_SEL_ENABLE);
>> +
>> +static struct attribute *amd_cpufreq_attributes[] = {
>> + &enable.attr,
>> + &max_perf.attr,
>> + &min_perf.attr,
>> + &desired_perf.attr,
>> + &auto_sel_enable.attr,
>> + NULL
>> +};
>> +
>> +static const struct attribute_group amd_cpufreq_attr_group = {
>> + .attrs = amd_cpufreq_attributes,
>> +};
>> +
>> +static struct kobj_type amd_cpufreq_type = {
>> + .sysfs_ops = &kobj_sysfs_ops,
>> + .default_attrs = amd_cpufreq_attributes,
>> +};
>> +
>> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> +{
>> + return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>> +{
>> + return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
>> +{
>> + return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
>> + unsigned int index)
>> +{
>> + return 0;
>> +}
> All empty helpers ? There is nothing you need to do ?
When we posted v2 of this patchset, Rafael let us know that he was
uncomfortable
going behind the (acpi-cpufreq) drivers back by letting the user
communicate directly
with the platform. That's the reason we have an empty driver whose
primary purpose
is to expose sysfs entries for the user.
>
>> +
>> +static struct cpufreq_driver amd_cpufreq_driver = {
>> + .name = "amd_cpufreq",
>> + .init = amd_cpufreq_cpu_init,
>> + .exit = amd_cpufreq_cpu_exit,
>> + .verify = amd_cpufreq_cpu_verify,
>> + .target_index = amd_cpufreq_cpu_target_index,
>> +};
>> +
>> +static void amd_cpufreq_sysfs_delete_params(void)
>> +{
>> + int i;
>> +
>> + for_each_possible_cpu(i) {
>> + if (all_cpu_data[i]) {
>> + kobject_del(&all_cpu_data[i]->kobj);
> Shouldn't you use kobject_put() instead of this ?
Ok.
>
>> + kfree(all_cpu_data[i]);
>> + }
>> + }
>> +
>> + kfree(all_cpu_data);
>> +}
>> +
>> +static int __init amd_cpufreq_sysfs_expose_params(void)
>> +{
>> + struct device *cpu_dev;
>> + int i, ret;
>> +
>> + all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
>> + GFP_KERNEL);
>> +
>> + if (!all_cpu_data)
>> + return -ENOMEM;
>> +
>> + for_each_possible_cpu(i) {
>> + all_cpu_data[i] = kzalloc(sizeof(struct amd_desc), GFP_KERNEL);
>> + if (!all_cpu_data[i]) {
>> + ret = -ENOMEM;
>> + goto free;
>> + }
>> +
>> + all_cpu_data[i]->cpu_id = i;
>> + cpu_dev = get_cpu_device(i);
>> + ret = kobject_init_and_add(&all_cpu_data[i]->kobj, &amd_cpufreq_type,
>> + &cpu_dev->kobj, "amd_cpufreq");
>> + if (ret)
>> + goto free;
>> + }
>> +
>> + return 0;
>> +free:
>> + amd_cpufreq_sysfs_delete_params();
>> + return ret;
>> +}
> Instead of doing this once for all CPUs, it would be better to do it
> every time the ->init() callback of the driver gets called. If you
> have one cpufreq policy for each CPU (i.e. no CPUs share clock lines),
> then the init() callback will get called once for each CPU.
We are trying to avoid any use of the cpufreq mechanism.
Thanks,
Janakarajan
>
>> +
>> +static int __init amd_cpufreq_init(void)
>> +{
>> + int ret = 0;
>> +
>> + /*
>> + * Use only if:
>> + * - AMD,
>> + * - Family 17h (or) newer and,
>> + * - Explicitly enabled
>> + */
>> + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
>> + boot_cpu_data.x86 < 0x17 || !cppc_enable)
>> + return -ENODEV;
>> +
>> + ret = cpufreq_register_driver(&amd_cpufreq_driver);
>> + if (ret) {
>> + pr_info("Failed to register driver\n");
>> + goto out;
>> + }
>> +
>> + ret = amd_cpufreq_sysfs_expose_params();
>> + if (ret) {
>> + pr_info("Could not create sysfs entries\n");
>> + cpufreq_unregister_driver(&amd_cpufreq_driver);
>> + goto out;
>> + }
>> +
>> + pr_info("Using amd-cpufreq driver\n");
>> + return ret;
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +static void __exit amd_cpufreq_exit(void)
>> +{
>> + amd_cpufreq_sysfs_delete_params();
>> + cpufreq_unregister_driver(&amd_cpufreq_driver);
>> +}
>> +
>> +static const struct acpi_device_id amd_acpi_ids[] __used = {
>> + {ACPI_PROCESSOR_DEVICE_HID, },
>> + {}
>> +};
>> +
>> +device_initcall(amd_cpufreq_init);
>> +module_exit(amd_cpufreq_exit);
>> +MODULE_DEVICE_TABLE(acpi, amd_acpi_ids);
> All three should be placed directly below the struct/function they
> represent without any blank lines in between. As suggested in
> kernel documentation.
>
>> +
>> +MODULE_AUTHOR("Janakarajan Natarajan");
>> +MODULE_DESCRIPTION("AMD CPUFreq driver based on ACPI CPPC v6.1 spec");
>> +MODULE_LICENSE("GPL");
> Should this be "GPL v2" ?
>
>> --
>> 2.17.1
^ permalink raw reply
* Re: [PATCH v2 1/6] ARM: tegra: Remove cpuidle drivers
From: Dmitry Osipenko @ 2019-07-11 17:03 UTC (permalink / raw)
To: Jon Hunter, Thierry Reding, Peter De Schrijver, Rafael J. Wysocki,
Daniel Lezcano
Cc: linux-pm, linux-tegra, linux-arm-kernel, linux-kernel
In-Reply-To: <c087a5cb-2ffa-1cf6-f0bf-631234759a22@nvidia.com>
11.07.2019 12:26, Jon Hunter пишет:
>
> On 11/07/2019 04:13, Dmitry Osipenko wrote:
>> Remove the old drivers to replace them cleanly with a new one later on.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> arch/arm/mach-tegra/Makefile | 13 --
>> arch/arm/mach-tegra/cpuidle-tegra114.c | 89 -----------
>> arch/arm/mach-tegra/cpuidle-tegra20.c | 212 -------------------------
>> arch/arm/mach-tegra/cpuidle-tegra30.c | 132 ---------------
>> arch/arm/mach-tegra/cpuidle.c | 50 ------
>> arch/arm/mach-tegra/cpuidle.h | 21 ---
>> arch/arm/mach-tegra/irq.c | 18 ---
>> arch/arm/mach-tegra/irq.h | 11 --
>> arch/arm/mach-tegra/pm.c | 7 -
>> arch/arm/mach-tegra/pm.h | 1 -
>> arch/arm/mach-tegra/reset-handler.S | 11 --
>> arch/arm/mach-tegra/reset.h | 9 +-
>> arch/arm/mach-tegra/sleep-tegra20.S | 190 +---------------------
>> arch/arm/mach-tegra/sleep.h | 12 --
>> arch/arm/mach-tegra/tegra.c | 3 -
>> drivers/soc/tegra/Kconfig | 1 -
>> include/soc/tegra/cpuidle.h | 4 -
>> 17 files changed, 5 insertions(+), 779 deletions(-)
>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra114.c
>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra20.c
>> delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra30.c
>> delete mode 100644 arch/arm/mach-tegra/cpuidle.c
>> delete mode 100644 arch/arm/mach-tegra/cpuidle.h
>> delete mode 100644 arch/arm/mach-tegra/irq.h
>
> By removing all the above, it is really hard to review the diff. Is
> there any way you could first consolidate the cpuidle drivers into say
> the existing arch/arm/mach-tegra/cpuidle-tegra20.c and then move to
> drivers/cpuidle?
I'm afraid that it will make reviewing even more difficult because
everything that is removed here is not returned in the further patches.
The new driver is based on the older ones, but I wrote it from scratch
and it's not only looks different, but also works a bit different as you
may see.
Could you please clarify what exactly makes it hard to review? The diff
looks pretty clean to me, while squashing everything into existing
driver should be quite a mess.
^ permalink raw reply
* Re: [PATCH v2 1/2] interconnect: Add support for path tags
From: Evan Green @ 2019-07-11 17:06 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-pm, David Dai, Vincent Guittot, Bjorn Andersson,
amit.kucheria, Doug Anderson, Sean Sweeney, LKML,
linux-arm Mailing List, linux-arm-msm
In-Reply-To: <20190618091724.28232-2-georgi.djakov@linaro.org>
Hi Georgi and David,
On Tue, Jun 18, 2019 at 2:17 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
>
> Consumers may have use cases with different bandwidth requirements based
> on the system or driver state. The consumer driver can append a specific
> tag to the path and pass this information to the interconnect platform
> driver to do the aggregation based on this state.
>
> Introduce icc_set_tag() function that will allow the consumers to append
> an optional tag to each path. The aggregation of these tagged paths is
> platform specific.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
> drivers/interconnect/core.c | 24 +++++++++++++++++++++++-
> drivers/interconnect/qcom/sdm845.c | 2 +-
> include/linux/interconnect-provider.h | 4 ++--
> include/linux/interconnect.h | 5 +++++
> 4 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 871eb4bc4efc..251354bb7fdc 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -29,6 +29,7 @@ static struct dentry *icc_debugfs_dir;
> * @req_node: entry in list of requests for the particular @node
> * @node: the interconnect node to which this constraint applies
> * @dev: reference to the device that sets the constraints
> + * @tag: path tag (optional)
> * @avg_bw: an integer describing the average bandwidth in kBps
> * @peak_bw: an integer describing the peak bandwidth in kBps
> */
> @@ -36,6 +37,7 @@ struct icc_req {
> struct hlist_node req_node;
> struct icc_node *node;
> struct device *dev;
> + u32 tag;
> u32 avg_bw;
> u32 peak_bw;
> };
> @@ -204,7 +206,7 @@ static int aggregate_requests(struct icc_node *node)
> node->peak_bw = 0;
>
> hlist_for_each_entry(r, &node->req_list, req_node)
> - p->aggregate(node, r->avg_bw, r->peak_bw,
> + p->aggregate(node, r->tag, r->avg_bw, r->peak_bw,
> &node->avg_bw, &node->peak_bw);
>
> return 0;
> @@ -385,6 +387,26 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> }
> EXPORT_SYMBOL_GPL(of_icc_get);
>
> +/**
> + * icc_set_tag() - set an optional tag on a path
> + * @path: the path we want to tag
> + * @tag: the tag value
> + *
> + * This function allows consumers to append a tag to the requests associated
> + * with a path, so that a different aggregation could be done based on this tag.
> + */
> +void icc_set_tag(struct icc_path *path, u32 tag)
> +{
> + int i;
> +
> + if (!path)
> + return;
> +
> + for (i = 0; i < path->num_nodes; i++)
> + path->reqs[i].tag = tag;
It's a little unfortunate to have this tag sprayed across all the
request nodes in the path, even though it's really a single value. If
we thought there were likely to be more attributes common to a path
that a provider might want access to, we could add a pointer to the
path in icc_req, then stick the tag in the path. But if the tag ends
up being the only thing worth looking at, then I guess what you have
now is slightly better.
^ permalink raw reply
* Re: [PATCH v2 2/2] interconnect: qcom: Add tagging and wake/sleep support for sdm845
From: Evan Green @ 2019-07-11 17:06 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-pm, David Dai, Vincent Guittot, Bjorn Andersson,
amit.kucheria, Doug Anderson, Sean Sweeney, LKML,
linux-arm Mailing List, linux-arm-msm
In-Reply-To: <20190618091724.28232-3-georgi.djakov@linaro.org>
Hi Georgi and David,
On Tue, Jun 18, 2019 at 2:17 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
>
> From: David Dai <daidavid1@codeaurora.org>
>
> Add support for wake and sleep commands by using a tag to indicate
> whether or not the aggregate and set requests fall into execution
> state specific bucket.
>
> Signed-off-by: David Dai <daidavid1@codeaurora.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
> drivers/interconnect/qcom/sdm845.c | 129 ++++++++++++++++++++++-------
> 1 file changed, 98 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
> index fb526004c82e..c100aab39415 100644
> --- a/drivers/interconnect/qcom/sdm845.c
> +++ b/drivers/interconnect/qcom/sdm845.c
> @@ -66,6 +66,17 @@ struct bcm_db {
> #define SDM845_MAX_BCM_PER_NODE 2
> #define SDM845_MAX_VCD 10
>
> +#define QCOM_ICC_BUCKET_AMC 0
What is AMC again? Is it the "right now" bucket? Maybe a comment on
the meaning of this bucket would be helpful.
> +#define QCOM_ICC_BUCKET_WAKE 1
> +#define QCOM_ICC_BUCKET_SLEEP 2
> +#define QCOM_ICC_NUM_BUCKETS 3
> +#define QCOM_ICC_TAG_AMC BIT(QCOM_ICC_BUCKET_AMC)
> +#define QCOM_ICC_TAG_WAKE BIT(QCOM_ICC_BUCKET_WAKE)
> +#define QCOM_ICC_TAG_SLEEP BIT(QCOM_ICC_BUCKET_SLEEP)
> +#define QCOM_ICC_TAG_ACTIVE_ONLY (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE)
> +#define QCOM_ICC_TAG_ALWAYS (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE |\
> + QCOM_ICC_TAG_SLEEP)
> +
> /**
> * struct qcom_icc_node - Qualcomm specific interconnect nodes
> * @name: the node name used in debugfs
> @@ -75,7 +86,9 @@ struct bcm_db {
> * @channels: num of channels at this node
> * @buswidth: width of the interconnect between a node and the bus
> * @sum_avg: current sum aggregate value of all avg bw requests
> + * @sum_avg_cached: previous sum aggregate value of all avg bw requests
> * @max_peak: current max aggregate value of all peak bw requests
> + * @max_peak_cached: previous max aggregate value of all peak bw requests
> * @bcms: list of bcms associated with this logical node
> * @num_bcms: num of @bcms
> */
> @@ -86,8 +99,10 @@ struct qcom_icc_node {
> u16 num_links;
> u16 channels;
> u16 buswidth;
> - u64 sum_avg;
> - u64 max_peak;
> + u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
> + u64 sum_avg_cached[QCOM_ICC_NUM_BUCKETS];
> + u64 max_peak[QCOM_ICC_NUM_BUCKETS];
> + u64 max_peak_cached[QCOM_ICC_NUM_BUCKETS];
> struct qcom_icc_bcm *bcms[SDM845_MAX_BCM_PER_NODE];
> size_t num_bcms;
> };
> @@ -112,8 +127,8 @@ struct qcom_icc_bcm {
> const char *name;
> u32 type;
> u32 addr;
> - u64 vote_x;
> - u64 vote_y;
> + u64 vote_x[QCOM_ICC_NUM_BUCKETS];
> + u64 vote_y[QCOM_ICC_NUM_BUCKETS];
> bool dirty;
> bool keepalive;
> struct bcm_db aux_data;
> @@ -555,7 +570,7 @@ inline void tcs_cmd_gen(struct tcs_cmd *cmd, u64 vote_x, u64 vote_y,
> cmd->wait = true;
> }
>
> -static void tcs_list_gen(struct list_head *bcm_list,
> +static void tcs_list_gen(struct list_head *bcm_list, int bucket,
> struct tcs_cmd tcs_list[SDM845_MAX_VCD],
> int n[SDM845_MAX_VCD])
> {
> @@ -573,8 +588,8 @@ static void tcs_list_gen(struct list_head *bcm_list,
> commit = true;
> cur_vcd_size = 0;
> }
> - tcs_cmd_gen(&tcs_list[idx], bcm->vote_x, bcm->vote_y,
> - bcm->addr, commit);
> + tcs_cmd_gen(&tcs_list[idx], bcm->vote_x[bucket],
> + bcm->vote_y[bucket], bcm->addr, commit);
> idx++;
> n[batch]++;
> /*
> @@ -595,32 +610,39 @@ static void tcs_list_gen(struct list_head *bcm_list,
>
> static void bcm_aggregate(struct qcom_icc_bcm *bcm)
> {
> - size_t i;
> - u64 agg_avg = 0;
> - u64 agg_peak = 0;
> + size_t i, bucket;
> + u64 agg_avg[QCOM_ICC_NUM_BUCKETS] = {0};
> + u64 agg_peak[QCOM_ICC_NUM_BUCKETS] = {0};
> u64 temp;
>
> - for (i = 0; i < bcm->num_nodes; i++) {
> - temp = bcm->nodes[i]->sum_avg * bcm->aux_data.width;
> - do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
> - agg_avg = max(agg_avg, temp);
> + for (bucket = 0; bucket < QCOM_ICC_NUM_BUCKETS; bucket++) {
> + for (i = 0; i < bcm->num_nodes; i++) {
> + temp = bcm->nodes[i]->sum_avg_cached[bucket] * bcm->aux_data.width;
> + do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
> + agg_avg[bucket] = max(agg_avg[bucket], temp);
>
> - temp = bcm->nodes[i]->max_peak * bcm->aux_data.width;
> - do_div(temp, bcm->nodes[i]->buswidth);
Why is it that this one doesn't have the multiply by
bcm->nodes[i]->channels again? I can't recall if there was a reason.
If it's correct maybe it deserves a comment.
> - agg_peak = max(agg_peak, temp);
> - }
> + temp = bcm->nodes[i]->max_peak_cached[bucket] * bcm->aux_data.width;
> + do_div(temp, bcm->nodes[i]->buswidth);
> + agg_peak[bucket] = max(agg_peak[bucket], temp);
>
> - temp = agg_avg * 1000ULL;
> - do_div(temp, bcm->aux_data.unit);
> - bcm->vote_x = temp;
> + bcm->nodes[i]->sum_avg[bucket] = 0;
> + bcm->nodes[i]->max_peak[bucket] = 0;
I don't understand the sum_avg vs sum_avg_cached. Here's what I understand:
1. qcom_icc_aggregate() does the math from the incoming values on
sum_avg, and then clobbers sum_avg_cached with those values.
2. bcm_aggregate() uses sum_avg_cached in its calculations, then clears sum_avg.
But I don't get why that's needed. Why not just have sum_avg? Wouldn't
it work the same? Ok, it wouldn't if you ended up calling
bcm_aggregate() multiple times on the same bcm. But you have a dirty
flag that prevents this from happening. So I think it's safe to remove
the cached arrays, and just clear out the sum_avg when you aggregate.
> + }
>
> - temp = agg_peak * 1000ULL;
> - do_div(temp, bcm->aux_data.unit);
> - bcm->vote_y = temp;
> + temp = agg_avg[bucket] * 1000ULL;
> + do_div(temp, bcm->aux_data.unit);
> + bcm->vote_x[bucket] = temp;
>
> - if (bcm->keepalive && bcm->vote_x == 0 && bcm->vote_y == 0) {
> - bcm->vote_x = 1;
> - bcm->vote_y = 1;
> + temp = agg_peak[bucket] * 1000ULL;
> + do_div(temp, bcm->aux_data.unit);
> + bcm->vote_y[bucket] = temp;
> + }
> +
> + if (bcm->keepalive && bcm->vote_x[0] == 0 && bcm->vote_y[0] == 0) {
> + bcm->vote_x[QCOM_ICC_BUCKET_AMC] = 1;
> + bcm->vote_x[QCOM_ICC_BUCKET_WAKE] = 1;
> + bcm->vote_y[QCOM_ICC_BUCKET_AMC] = 1;
> + bcm->vote_y[QCOM_ICC_BUCKET_WAKE] = 1;
> }
>
> bcm->dirty = false;
> @@ -631,15 +653,25 @@ static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> {
> size_t i;
> struct qcom_icc_node *qn;
> + unsigned long tag_word = (unsigned long)tag;
>
> qn = node->data;
>
> + if (!tag)
> + tag_word = QCOM_ICC_TAG_ALWAYS;
> +
> + for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
> + if (test_bit(i, &tag_word)) {
I guess all this extra business with tag_word and casting is so that
you can use test_bit, which is presumably a tiny bit faster? Does this
actually make a measurable difference? Maybe in the name of simplicity
we just do if (tag & BIT(i)), and then optimize if we find that
conditional to be a hotspot?
> + qn->sum_avg[i] += avg_bw;
> + qn->max_peak[i] = max_t(u32, qn->max_peak[i], peak_bw);
> + qn->sum_avg_cached[i] = qn->sum_avg[i];
> + qn->max_peak_cached[i] = qn->max_peak[i];
> + }
> + }
> +
> *agg_avg += avg_bw;
> *agg_peak = max_t(u32, *agg_peak, peak_bw);
>
> - qn->sum_avg = *agg_avg;
> - qn->max_peak = *agg_peak;
> -
> for (i = 0; i < qn->num_bcms; i++)
> qn->bcms[i]->dirty = true;
>
> @@ -675,7 +707,7 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
> * Construct the command list based on a pre ordered list of BCMs
> * based on VCD.
> */
> - tcs_list_gen(&commit_list, cmds, commit_idx);
> + tcs_list_gen(&commit_list, QCOM_ICC_BUCKET_AMC, cmds, commit_idx);
>
> if (!commit_idx[0])
> return ret;
> @@ -693,6 +725,41 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
> return ret;
> }
>
> + INIT_LIST_HEAD(&commit_list);
> +
> + for (i = 0; i < qp->num_bcms; i++) {
> + /*
> + * Only generate WAKE and SLEEP commands if a resource's
> + * requirements change as the execution environment transitions
> + * between different power states.
> + */
> + if (qp->bcms[i]->vote_x[QCOM_ICC_BUCKET_WAKE] !=
> + qp->bcms[i]->vote_x[QCOM_ICC_BUCKET_SLEEP] ||
> + qp->bcms[i]->vote_y[QCOM_ICC_BUCKET_WAKE] !=
> + qp->bcms[i]->vote_y[QCOM_ICC_BUCKET_SLEEP]) {
> + list_add_tail(&qp->bcms[i]->list, &commit_list);
> + }
> + }
> +
> + if (list_empty(&commit_list))
> + return ret;
> +
> + tcs_list_gen(&commit_list, QCOM_ICC_BUCKET_WAKE, cmds, commit_idx);
> +
> + ret = rpmh_write_batch(qp->dev, RPMH_WAKE_ONLY_STATE, cmds, commit_idx);
> + if (ret) {
> + pr_err("Error sending WAKE RPMH requests (%d)\n", ret);
> + return ret;
> + }
> +
> + tcs_list_gen(&commit_list, QCOM_ICC_BUCKET_SLEEP, cmds, commit_idx);
> +
> + ret = rpmh_write_batch(qp->dev, RPMH_SLEEP_STATE, cmds, commit_idx);
> + if (ret) {
> + pr_err("Error sending SLEEP RPMH requests (%d)\n", ret);
> + return ret;
> + }
> +
> return ret;
> }
>
^ permalink raw reply
* Re: [PATCH v2 2/6] ARM: tegra: Expose functions required for cpuidle driver
From: Dmitry Osipenko @ 2019-07-11 17:25 UTC (permalink / raw)
To: Jon Hunter, Thierry Reding, Peter De Schrijver, Rafael J. Wysocki,
Daniel Lezcano
Cc: linux-pm, linux-tegra, linux-arm-kernel, linux-kernel
In-Reply-To: <bc6e96be-91ee-5d94-cbc2-46d2e2f25bce@nvidia.com>
11.07.2019 15:42, Jon Hunter пишет:
>
> On 11/07/2019 04:13, Dmitry Osipenko wrote:
>> The upcoming unified CPUIDLE driver will be added to the drivers/cpuidle/
>> directory and it will require all these Tegra PM-core functions.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> arch/arm/mach-tegra/Makefile | 2 +-
>> arch/arm/mach-tegra/platsmp.c | 2 --
>> arch/arm/mach-tegra/pm.c | 16 +++++++---------
>> arch/arm/mach-tegra/pm.h | 3 ---
>> arch/arm/mach-tegra/sleep.h | 1 -
>> include/linux/clk/tegra.h | 13 +++++++++++++
>> include/soc/tegra/pm.h | 28 ++++++++++++++++++++++++++++
>> 7 files changed, 49 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
>> index 5d93a0b36866..27bd5d9865e3 100644
>> --- a/arch/arm/mach-tegra/Makefile
>> +++ b/arch/arm/mach-tegra/Makefile
>> @@ -13,7 +13,7 @@ obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += pm-tegra20.o
>> obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += sleep-tegra30.o
>> obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += pm-tegra30.o
>> obj-$(CONFIG_SMP) += platsmp.o
>> -obj-$(CONFIG_HOTPLUG_CPU) += hotplug.o
>> +obj-y += hotplug.o
>>
>> obj-$(CONFIG_ARCH_TEGRA_114_SOC) += sleep-tegra30.o
>> obj-$(CONFIG_ARCH_TEGRA_114_SOC) += pm-tegra30.o
>> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
>> index e6911a14c096..c8a63719a143 100644
>> --- a/arch/arm/mach-tegra/platsmp.c
>> +++ b/arch/arm/mach-tegra/platsmp.c
>> @@ -183,8 +183,6 @@ const struct smp_operations tegra_smp_ops __initconst = {
>> .smp_prepare_cpus = tegra_smp_prepare_cpus,
>> .smp_secondary_init = tegra_secondary_init,
>> .smp_boot_secondary = tegra_boot_secondary,
>> -#ifdef CONFIG_HOTPLUG_CPU
>> .cpu_kill = tegra_cpu_kill,
>> .cpu_die = tegra_cpu_die,
>> -#endif
>> };
>> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
>> index 6aaacb5757e1..f9c9bce9e15d 100644
>> --- a/arch/arm/mach-tegra/pm.c
>> +++ b/arch/arm/mach-tegra/pm.c
>> @@ -123,11 +123,9 @@ void tegra_clear_cpu_in_lp2(void)
>> spin_unlock(&tegra_lp2_lock);
>> }
>>
>> -bool tegra_set_cpu_in_lp2(void)
>> +void tegra_set_cpu_in_lp2(void)
>> {
>> int phy_cpu_id = cpu_logical_map(smp_processor_id());
>> - bool last_cpu = false;
>> - cpumask_t *cpu_lp2_mask = tegra_cpu_lp2_mask;
>> u32 *cpu_in_lp2 = tegra_cpu_lp2_mask;
>>
>> spin_lock(&tegra_lp2_lock);
>> @@ -135,11 +133,7 @@ bool tegra_set_cpu_in_lp2(void)
>> BUG_ON((*cpu_in_lp2 & BIT(phy_cpu_id)));
>> *cpu_in_lp2 |= BIT(phy_cpu_id);
>>
>> - if ((phy_cpu_id == 0) && cpumask_equal(cpu_lp2_mask, cpu_online_mask))
>> - last_cpu = true;
>> -
>> spin_unlock(&tegra_lp2_lock);
>> - return last_cpu;
>> }
>
> I think that the commit message should describe what is going on here or
> this should be a separate change.
Indeed, it could be not very obvious what's going on here without a
thorough review. I'll factor out all these minor changes into separate
commits.
In particular there is no need to know whether CPU is the "last_cpu" for
the new driver because CPU0 is always the "last" since it awaits for the
secondaries in the coupled state.
>> static int tegra_sleep_cpu(unsigned long v2p)
>> @@ -195,14 +189,16 @@ static void tegra_pm_set(enum tegra_suspend_mode mode)
>> tegra_pmc_enter_suspend_mode(mode);
>> }
>>
>> -void tegra_idle_lp2_last(void)
>> +int tegra_idle_lp2_last(void)
>> {
>> + int err;
>> +
>> tegra_pm_set(TEGRA_SUSPEND_LP2);
>>
>> cpu_cluster_pm_enter();
>> suspend_cpu_complex();
>>
>> - cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
>> + err = cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
>>
>> /*
>> * Resume L2 cache if it wasn't re-enabled early during resume,
>> @@ -214,6 +210,8 @@ void tegra_idle_lp2_last(void)
>>
>> restore_cpu_complex();
>> cpu_cluster_pm_exit();
>> +
>> + return err;
>> }
>>
>> enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
>> diff --git a/arch/arm/mach-tegra/pm.h b/arch/arm/mach-tegra/pm.h
>> index 1e51a9b636eb..81525f5f4a44 100644
>> --- a/arch/arm/mach-tegra/pm.h
>> +++ b/arch/arm/mach-tegra/pm.h
>> @@ -23,9 +23,6 @@ void tegra20_sleep_core_init(void);
>> void tegra30_lp1_iram_hook(void);
>> void tegra30_sleep_core_init(void);
>>
>> -void tegra_clear_cpu_in_lp2(void);
>> -bool tegra_set_cpu_in_lp2(void);
>> -void tegra_idle_lp2_last(void);
>> extern void (*tegra_tear_down_cpu)(void);
>>
>> #ifdef CONFIG_PM_SLEEP
>> diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h
>> index d219872b7546..0d9956e9a8ea 100644
>> --- a/arch/arm/mach-tegra/sleep.h
>> +++ b/arch/arm/mach-tegra/sleep.h
>> @@ -124,7 +124,6 @@ void tegra30_hotplug_shutdown(void);
>> #endif
>>
>> void tegra20_tear_down_cpu(void);
>> -int tegra30_sleep_cpu_secondary_finish(unsigned long);
>> void tegra30_tear_down_cpu(void);
>>
>> #endif
>> diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h
>> index b8aef62cc3f5..cf0f2cb5e109 100644
>> --- a/include/linux/clk/tegra.h
>> +++ b/include/linux/clk/tegra.h
>> @@ -108,6 +108,19 @@ static inline void tegra_cpu_clock_resume(void)
>>
>> tegra_cpu_car_ops->resume();
>> }
>> +#else
>> +static inline bool tegra_cpu_rail_off_ready(void)
>> +{
>> + return false;
>> +}
>> +
>> +static inline void tegra_cpu_clock_suspend(void)
>> +{
>> +}
>> +
>> +static inline void tegra_cpu_clock_resume(void)
>> +{
>> +}
>> #endif
>>
>> extern void tegra210_xusb_pll_hw_control_enable(void);
>> diff --git a/include/soc/tegra/pm.h b/include/soc/tegra/pm.h
>> index 951fcd738d55..fa18c2df5028 100644
>> --- a/include/soc/tegra/pm.h
>> +++ b/include/soc/tegra/pm.h
>> @@ -20,6 +20,12 @@ tegra_pm_validate_suspend_mode(enum tegra_suspend_mode mode);
>>
>> /* low-level resume entry point */
>> void tegra_resume(void);
>> +
>> +int tegra30_sleep_cpu_secondary_finish(unsigned long arg);
>> +void tegra_clear_cpu_in_lp2(void);
>> +void tegra_set_cpu_in_lp2(void);
>> +int tegra_idle_lp2_last(void);
>> +void tegra_cpu_die(unsigned int cpu);
>> #else
>> static inline enum tegra_suspend_mode
>> tegra_pm_validate_suspend_mode(enum tegra_suspend_mode mode)
>> @@ -30,6 +36,28 @@ tegra_pm_validate_suspend_mode(enum tegra_suspend_mode mode)
>> static inline void tegra_resume(void)
>> {
>> }
>> +
>> +static inline int tegra30_sleep_cpu_secondary_finish(unsigned long arg)
>> +{
>> + return -1;
>> +}
>
> -ENOTSUPP?
Good point, thanks!
^ permalink raw reply
* Re: [PATCH RFC 2/4] OPP: Add and export helper to set bandwidth
From: Bjorn Andersson @ 2019-07-11 17:40 UTC (permalink / raw)
To: Sibi Sankar
Cc: viresh.kumar, nm, sboyd, georgi.djakov, agross, david.brown,
robh+dt, mark.rutland, rjw, linux-arm-msm, devicetree,
linux-kernel, linux-pm, saravanak
In-Reply-To: <20190627133424.4980-3-sibis@codeaurora.org>
On Thu 27 Jun 06:34 PDT 2019, Sibi Sankar wrote:
> Add and export 'dev_pm_opp_set_bw' to set the bandwidth
> levels associated with an OPP for a given frequency.
>
While this looks quite reasonable I'm uncertain about the overall OPP
API.
With the profiling based (bwmon/llcc) approach we would acquire the peak
bandwidth from the OPP table and calculate the average dynamically,
based on measurements and heuristics.
For that I think we will have a struct dev_pm_opp at hand (e.g. from
devfreq_recommended_opp() or similar), from which we want to read the
peak value and then apply the icc vote. Or would we want to update the
avg bw and then apply the opp using a method like this? (In which case
we probably don't want to pass a freq, but a struct dev_pm_opp *, to
avoid the additional lookup)
Regards,
Bjorn
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
> drivers/opp/core.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_opp.h | 6 ++++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index c85c04dc2c7de..78f42960860d1 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -746,6 +746,52 @@ static int _set_required_opps(struct device *dev,
> return ret;
> }
>
> +/**
> + * dev_pm_opp_set_bw() - Configures OPP bandwidth levels
> + * @dev: device for which we do this operation
> + * @freq: bandwidth values to set with matching 'freq'
> + *
> + * This configures the bandwidth to the levels specified
> + * by the OPP corresponding to the given frequency.
> + *
> + * Return: 0 on success or a negative error value.
> + */
> +int dev_pm_opp_set_bw(struct device *dev, unsigned long freq)
> +{
> + struct opp_table *opp_table;
> + struct dev_pm_opp *opp;
> + int ret = 0;
> + int i;
> +
> + opp = dev_pm_opp_find_freq_exact(dev, freq, true);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + opp_table = _find_opp_table(dev);
> + if (IS_ERR(opp_table)) {
> + dev_err(dev, "%s: device opp table doesn't exist\n", __func__);
> + ret = PTR_ERR(opp_table);
> + goto put_opp;
> + }
> +
> + if (IS_ERR_OR_NULL(opp_table->paths)) {
> + ret = -ENODEV;
> + goto put_opp_table;
> + }
> +
> + for (i = 0; i < opp_table->path_count; i++) {
> + ret = icc_set_bw(opp_table->paths[i], opp->bandwidth[i].avg,
> + opp->bandwidth[i].peak);
> + }
> +
> +put_opp_table:
> + dev_pm_opp_put_opp_table(opp_table);
> +put_opp:
> + dev_pm_opp_put(opp);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
> +
> /**
> * dev_pm_opp_set_rate() - Configure new OPP based on frequency
> * @dev: device for which we do this operation
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index a17c462974851..1cdc2d0a2b20e 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -152,6 +152,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char **names
> void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
> int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
> int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
> +int dev_pm_opp_set_bw(struct device *dev, unsigned long freq);
> int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
> int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
> void dev_pm_opp_remove_table(struct device *dev);
> @@ -336,6 +337,11 @@ static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_f
> return -ENOTSUPP;
> }
>
> +static inline int dev_pm_opp_set_bw(struct device *dev, unsigned long freq)
> +{
> + return -ENOTSUPP;
> +}
> +
> static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask)
> {
> return -ENOTSUPP;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
^ permalink raw reply
* Re: [PATCH v2 3/6] cpuidle: Add unified driver for NVIDIA Tegra SoCs
From: Dmitry Osipenko @ 2019-07-11 18:35 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Peter De Schrijver,
Rafael J. Wysocki, Daniel Lezcano
Cc: linux-pm, linux-tegra, linux-arm-kernel, linux-kernel
In-Reply-To: <20190711031312.10038-4-digetx@gmail.com>
11.07.2019 6:13, Dmitry Osipenko пишет:
> The new driver is based on the old CPU Idle drivers that are removed now
> from arm/arch/mach-tegra/ directory. Those removed drivers were reworked
> and squashed into a single unified driver that covers multiple hardware
> generations, starting from Tegra20 and ending with Tegra124.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> arch/arm/mach-tegra/tegra.c | 4 +
> drivers/cpuidle/Kconfig.arm | 8 +
> drivers/cpuidle/Makefile | 1 +
> drivers/cpuidle/cpuidle-tegra.c | 304 ++++++++++++++++++++++++++++++++
> include/soc/tegra/cpuidle.h | 4 +
> 5 files changed, 321 insertions(+)
> create mode 100644 drivers/cpuidle/cpuidle-tegra.c
>
> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
> index d9237769a37c..f1ce2857a251 100644
> --- a/arch/arm/mach-tegra/tegra.c
> +++ b/arch/arm/mach-tegra/tegra.c
> @@ -36,6 +36,7 @@
> #include <asm/mach/arch.h>
> #include <asm/mach/time.h>
> #include <asm/mach-types.h>
> +#include <asm/psci.h>
> #include <asm/setup.h>
>
> #include "board.h"
> @@ -92,6 +93,9 @@ static void __init tegra_dt_init_late(void)
> if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) &&
> of_machine_is_compatible("nvidia,tegra20"))
> platform_device_register_simple("tegra20-cpufreq", -1, NULL, 0);
> +
> + if (IS_ENABLED(CONFIG_ARM_TEGRA_CPUIDLE) && !psci_smp_available())
> + platform_device_register_simple("tegra-cpuidle", -1, NULL, 0);
> }
>
> static const char * const tegra_dt_board_compat[] = {
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 48cb3d4bb7d1..d90861361f1d 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -76,3 +76,11 @@ config ARM_MVEBU_V7_CPUIDLE
> depends on ARCH_MVEBU && !ARM64
> help
> Select this to enable cpuidle on Armada 370, 38x and XP processors.
> +
> +config ARM_TEGRA_CPUIDLE
> + bool "CPU Idle Driver for NVIDIA Tegra SoCs"
> + depends on ARCH_TEGRA && !ARM64
> + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP
> + select ARM_CPU_SUSPEND
> + help
> + Select this to enable cpuidle for NVIDIA Tegra20/30/114/124 SoCs.
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 9d7176cee3d3..470d17fa8746 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_ARM_U8500_CPUIDLE) += cpuidle-ux500.o
> obj-$(CONFIG_ARM_AT91_CPUIDLE) += cpuidle-at91.o
> obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += cpuidle-exynos.o
> obj-$(CONFIG_ARM_CPUIDLE) += cpuidle-arm.o
> +obj-$(CONFIG_ARM_TEGRA_CPUIDLE) += cpuidle-tegra.o
>
> ###############################################################################
> # MIPS drivers
> diff --git a/drivers/cpuidle/cpuidle-tegra.c b/drivers/cpuidle/cpuidle-tegra.c
> new file mode 100644
> index 000000000000..54974cd2255f
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-tegra.c
> @@ -0,0 +1,304 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * CPU idle driver for Tegra CPUs
> + *
> + * Copyright (c) 2010-2013, NVIDIA Corporation.
> + * Copyright (c) 2011 Google, Inc.
> + * Author: Colin Cross <ccross@android.com>
> + * Gary King <gking@nvidia.com>
> + *
> + * Rework for 3.3 by Peter De Schrijver <pdeschrijver@nvidia.com>
> + *
> + * Tegra20/124 driver unification by Dmitry Osipenko <digetx@gmail.com>
> + */
> +
> +#include <linux/cpuidle.h>
> +#include <linux/cpumask.h>
> +#include <linux/cpu_pm.h>
> +#include <linux/errno.h>
> +#include <linux/ktime.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#include <linux/clk/tegra.h>
> +#include <linux/firmware/trusted_foundations.h>
> +
> +#include <soc/tegra/cpuidle.h>
> +#include <soc/tegra/flowctrl.h>
> +#include <soc/tegra/fuse.h>
> +#include <soc/tegra/pm.h>
> +
> +#include <asm/cpuidle.h>
> +#include <asm/firmware.h>
> +#include <asm/smp_plat.h>
> +#include <asm/suspend.h>
> +
> +#define TEGRA_C1 0
> +#define TEGRA_C7 1
> +#define TEGRA_CC6 2
> +
> +static atomic_t tegra_idle_barrier;
> +
> +static inline bool tegra_cpuidle_using_firmware(void)
> +{
> + return firmware_ops->prepare_idle && firmware_ops->do_idle;
> +}
> +
> +static int tegra_shutdown_secondary_cpu(unsigned long cpu)
> +{
> + tegra_cpu_die(cpu);
> +
> + return -EINVAL;
> +}
> +
> +static int tegra_await_secondary_cpus_shutdown(void)
> +{
> + ktime_t timeout = ktime_add_ms(ktime_get(), 1);
> +
> + /*
> + * The boot CPU shall await for the secondaries shutdown in order
> + * to power-off CPU's cluster safely.
> + */
> + do {
> + if (tegra_cpu_rail_off_ready())
> + return 0;
> +
> + } while (ktime_compare(ktime_get(), timeout) < 0);
> +
> + return -ETIMEDOUT;
> +}
> +
> +static void tegra_wake_up_secondary_cpus(void)
> +{
> + unsigned int cpu, lcpu;
> +
> + for_each_cpu(lcpu, cpu_online_mask) {
> + cpu = cpu_logical_map(lcpu);
> +
> + if (cpu > 0) {
> + tegra_enable_cpu_clock(cpu);
> + tegra_cpu_out_of_reset(cpu);
> + flowctrl_write_cpu_halt(cpu, 0);
> + }
> + }
> +}
> +
> +static int tegra_cpuidle_cc6_enter(void)
> +{
> + int err;
> +
> + err = tegra_await_secondary_cpus_shutdown();
> + if (err)
> + return err;
> +
> + err = tegra_idle_lp2_last();
> +
> + tegra_wake_up_secondary_cpus();
> +
> + return err;
> +}
> +
> +static int tegra_cpuidle_c7_enter(void)
> +{
> + int err;
> +
> + if (tegra_cpuidle_using_firmware()) {
> + err = call_firmware_op(prepare_idle, TF_PM_MODE_LP2_NOFLUSH_L2);
> + if (err)
> + return err;
> +
> + return call_firmware_op(do_idle, 0);
> + }
> +
> + return cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
> +}
> +
> +static int tegra_cpuidle_enter(struct cpuidle_device *dev,
> + int index, unsigned int cpu)
> +{
> + int err;
> +
> + local_fiq_disable();
> + tegra_set_cpu_in_lp2();
> + cpu_pm_enter();
> +
> + switch (index) {
> + case TEGRA_C7:
> + err = tegra_cpuidle_c7_enter();
> + break;
> + case TEGRA_CC6:
> + cpuidle_coupled_parallel_barrier(dev, &tegra_idle_barrier);
I realized that this is not very correct. We still need to do a proper
barrier with SGI checking in order to bail out if other CPU sent IPI
during of the awaiting for a coupled barrier to avoid the overhead of
unnecessary power-gating. Will correct that in the next revision.
^ permalink raw reply
* [PATCH v2] thermal: Add some error messages
From: Amit Kucheria @ 2019-07-11 20:31 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, joe, Zhang Rui, Eduardo Valentin,
Daniel Lezcano
Cc: linux-pm
In-Reply-To: <cover.1562876950.git.amit.kucheria@linaro.org>
When registering a thermal zone device, we currently return -EINVAL in
four cases. This makes it a little hard to debug the real cause of the
failure.
Print some error messages to make it easier for developer to figure out
what happened.
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
drivers/thermal/thermal_core.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 46cfb7de4eb2..e1c2c2638219 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1238,17 +1238,26 @@ thermal_zone_device_register(const char *type, int trips, int mask,
int count;
struct thermal_governor *governor;
- if (!type || strlen(type) == 0)
+ if (!type || strlen(type) == 0) {
+ pr_err("Error: No thermal zone type defined\n");
return ERR_PTR(-EINVAL);
+ }
- if (type && strlen(type) >= THERMAL_NAME_LENGTH)
+ if (type && strlen(type) >= THERMAL_NAME_LENGTH) {
+ pr_err("Error: Thermal zone name (%s) too long, should be under %d chars\n",
+ type, THERMAL_NAME_LENGTH);
return ERR_PTR(-EINVAL);
+ }
- if (trips > THERMAL_MAX_TRIPS || trips < 0 || mask >> trips)
+ if (trips > THERMAL_MAX_TRIPS || trips < 0 || mask >> trips) {
+ pr_err("Error: Incorrect number of thermal trips\n");
return ERR_PTR(-EINVAL);
+ }
- if (!ops)
+ if (!ops) {
+ pr_err("Error: Thermal zone device ops not defined\n");
return ERR_PTR(-EINVAL);
+ }
if (trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp))
return ERR_PTR(-EINVAL);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v1 0/3] Add required-opps support to devfreq passive gov
From: Saravana Kannan @ 2019-07-11 23:16 UTC (permalink / raw)
To: Viresh Kumar
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki,
Android Kernel Team, Linux PM, LKML
In-Reply-To: <CAGETcx-gSQLtbM+7WgqNvhCypmwoMNjkAkGDqRnK=PUGUOxjkQ@mail.gmail.com>
Bump. I saw your email in Sibi's patch series. His patch series is
just one use case/user of this feature. This patch series is useful in
general. Do plan to Ack this? Or thoughts on my earlier response?
Thanks,
Saravana
On Fri, Jun 28, 2019 at 1:26 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Jun 27, 2019 at 11:49 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 26-06-19, 11:10, Saravana Kannan wrote:
> > > On Tue, Jun 25, 2019 at 11:32 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > > > So, when a CPU changes frequency, we must change the performance state
> > > > of PM domain and change frequency/bw of the cache synchronously.
> > >
> > > I mean, it's going to be changed when we get the CPUfreq transition
> > > notifiers. From a correctness point of view, setting it inside the OPP
> > > framework is not any better than doing it when we get the notifiers.
> >
> > That's what the problem is. All maintainers now a days ask people to
> > stay away from notifiers and we are making that base of another new
> > thing we are starting.
>
> In that case we can just add direct calls in cpufreq.c to let devfreq
> know about the frequency changes. But then again, CPU is just one
> example for this use case. I'm just using that because people are more
> familiar with that.
>
> > Over that, with many cpufreq drivers we have fast switching enabled
> > and notifiers disabled. How will they make these things work ? We
> > still want to scale L3 in those cases as well.
>
> Nothing is preventing them from using the xlate OPP API I added to
> figure out all the CPU to L3 frequency mapping and then set the L3
> frequency directly from the CPUfreq driver.
>
> Also, whether we use OPP framework or devfreq to set the L3 frequency,
> it's going to block fast switching because both these frameworks have
> APIs that can sleep.
>
> But really, most mobile use cases don't want to permanently tie L3
> freq to CPU freq. Having it go through devfreq and being able to
> switch governors is a very important need for mobile products.
>
> Keep in mind that nothing in this series does any of the cpufreq stuff
> yet. That'll need a few more changes. I was just using CPUfreq as an
> example.
>
> > > I see this as "required for good performance". So I don't see it as
> > > redefining required-opps. If someone wants good performance/power
> > > balance they follow the "required-opps". Technically even the PM
> > > pstates are required for good power. Otherwise, the system could leave
> > > the voltage at max and stuff would still work.
> > >
> > > Also, the slave device might need to get input from multiple master
> > > devices and aggregate the request before setting the slave device
> > > frequency. So I don't think OPP framework would be the right place to
> > > deal with those things. For example, L3 might (will) have different
> > > mappings for big vs little cores. So that needs to be aggregated and
> > > set properly by the slave device driver. Also, GPU might have a
> > > mapping for L3 too. In which case the L3 slave driver needs to take
> > > input from even more masters before it decides its frequency. But most
> > > importantly, we still need the ability to change governors for L3.
> > > Again these are just examples with L3 and it can get more complicated
> > > based on the situation.
> > >
> > > Most importantly, instead of always going by mapping, one might decide
> > > to scale the L3 based on some other governor (that looks at some HW
> > > counter). Or just set it to performance governor for a use case for
> > > which performance is more important. All of this comes for free with
> > > devfreq and if we always set it from OPP framework we don't give this
> > > required control to userspace.
> > >
> > > I think going through devfreq is the right approach for this. And we
> > > can always rewrite the software if we find problems in the future. But
> > > as it stands today, this will help cases like exynos without the need
> > > for a lot of changes. Hope I've convinced you.
> >
> > I understand the aggregation thing and fully support that the
> > aggregation can't happen in OPP core and must be done somewhere else.
> > But the input can go from OPP core while the frequency is changing,
> > isn't it ?
>
> I'm not opposed to OPP sending input to devfreq to let it know that a
> master device frequency change is happening. But I think this is kinda
> orthogonal to this patch series.
>
> Today the passive governor looks at the master device's devfreq
> frequency changes to trigger the frequency change of the slave
> devfreq. It neither supports tracking OPP frequency change nor CPUfreq
> frequency change. If that's something we want to add, we can look into
> that separately as passive governor (or a new governor) changes.
>
> But then not all devices (CPUfreq or otherwise) use OPP to set the
> frequencies. So it's beneficial to have all of these frameworks as
> inputs for devfreq passive (like) governor. CPUfreq is actually a bit
> more tricky because we'll also have to track hotplug, etc. So direct
> calls from CPUfreq to devfreq (similar to cpufreq stats tracking)
> would be good.
>
> -Saravana
^ permalink raw reply
* [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
From: Joel Fernandes (Google) @ 2019-07-11 23:43 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Josh Triplett, keescook, kernel-hardening,
Lai Jiangshan, Len Brown, linux-acpi, linux-pci, linux-pm,
Mathieu Desnoyers, neilb, netdev, oleg, Paul E. McKenney,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190711234401.220336-1-joel@joelfernandes.org>
This patch adds support for checking RCU reader sections in list
traversal macros. Optionally, if the list macro is called under SRCU or
other lock/mutex protection, then appropriate lockdep expressions can be
passed to make the checks pass.
Existing list_for_each_entry_rcu() invocations don't need to pass the
optional fourth argument (cond) unless they are under some non-RCU
protection and needs to make lockdep check pass.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
include/linux/rculist.h | 29 ++++++++++++++++++++++++-----
include/linux/rcupdate.h | 7 +++++++
kernel/rcu/Kconfig.debug | 11 +++++++++++
kernel/rcu/update.c | 26 ++++++++++++++++++++++++++
4 files changed, 68 insertions(+), 5 deletions(-)
diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index e91ec9ddcd30..78c15ec6b2c9 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -40,6 +40,23 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
*/
#define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
+/*
+ * Check during list traversal that we are within an RCU reader
+ */
+
+#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
+#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
+
+#ifdef CONFIG_PROVE_RCU_LIST
+#define __list_check_rcu(dummy, cond, ...) \
+ ({ \
+ RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
+ "RCU-list traversed in non-reader section!"); \
+ })
+#else
+#define __list_check_rcu(dummy, cond, ...) ({})
+#endif
+
/*
* Insert a new entry between two known consecutive entries.
*
@@ -348,9 +365,10 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
* the _rcu list-mutation primitives such as list_add_rcu()
* as long as the traversal is guarded by rcu_read_lock().
*/
-#define list_for_each_entry_rcu(pos, head, member) \
- for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
- &pos->member != (head); \
+#define list_for_each_entry_rcu(pos, head, member, cond...) \
+ for (__list_check_rcu(dummy, ## cond, 0), \
+ pos = list_entry_rcu((head)->next, typeof(*pos), member); \
+ &pos->member != (head); \
pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
/**
@@ -621,8 +639,9 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
* the _rcu list-mutation primitives such as hlist_add_head_rcu()
* as long as the traversal is guarded by rcu_read_lock().
*/
-#define hlist_for_each_entry_rcu(pos, head, member) \
- for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
+#define hlist_for_each_entry_rcu(pos, head, member, cond...) \
+ for (__list_check_rcu(dummy, ## cond, 0), \
+ pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
typeof(*(pos)), member); \
pos; \
pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 922bb6848813..712b464ab960 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -223,6 +223,7 @@ int debug_lockdep_rcu_enabled(void);
int rcu_read_lock_held(void);
int rcu_read_lock_bh_held(void);
int rcu_read_lock_sched_held(void);
+int rcu_read_lock_any_held(void);
#else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
@@ -243,6 +244,12 @@ static inline int rcu_read_lock_sched_held(void)
{
return !preemptible();
}
+
+static inline int rcu_read_lock_any_held(void)
+{
+ return !preemptible();
+}
+
#endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
#ifdef CONFIG_PROVE_RCU
diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
index 0ec7d1d33a14..b20d0e2903d1 100644
--- a/kernel/rcu/Kconfig.debug
+++ b/kernel/rcu/Kconfig.debug
@@ -7,6 +7,17 @@ menu "RCU Debugging"
config PROVE_RCU
def_bool PROVE_LOCKING
+config PROVE_RCU_LIST
+ bool "RCU list lockdep debugging"
+ depends on PROVE_RCU
+ default n
+ help
+ Enable RCU lockdep checking for list usages. By default it is
+ turned off since there are several list RCU users that still
+ need to be converted to pass a lockdep expression. To prevent
+ false-positive splats, we keep it default disabled but once all
+ users are converted, we can remove this config option.
+
config TORTURE_TEST
tristate
default n
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index c3bf44ba42e5..9cb30006a5e1 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -298,6 +298,32 @@ int rcu_read_lock_bh_held(void)
}
EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
+int rcu_read_lock_any_held(void)
+{
+ int lockdep_opinion = 0;
+
+ if (!debug_lockdep_rcu_enabled())
+ return 1;
+ if (!rcu_is_watching())
+ return 0;
+ if (!rcu_lockdep_current_cpu_online())
+ return 0;
+
+ /* Preemptible RCU flavor */
+ if (lock_is_held(&rcu_lock_map))
+ return 1;
+
+ /* BH flavor */
+ if (in_softirq() || irqs_disabled())
+ return 1;
+
+ /* Sched flavor */
+ if (debug_locks)
+ lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
+ return lockdep_opinion || !preemptible();
+}
+EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
+
#endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
/**
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v1 2/6] ipv4: add lockdep condition to fix for_each_entry
From: Joel Fernandes (Google) @ 2019-07-11 23:43 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Josh Triplett, keescook, kernel-hardening,
Lai Jiangshan, Len Brown, linux-acpi, linux-pci, linux-pm,
Mathieu Desnoyers, neilb, netdev, oleg, Paul E. McKenney,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190711234401.220336-1-joel@joelfernandes.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
net/ipv4/fib_frontend.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index b298255f6fdb..ef7c9f8e8682 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -127,7 +127,8 @@ struct fib_table *fib_get_table(struct net *net, u32 id)
h = id & (FIB_TABLE_HASHSZ - 1);
head = &net->ipv4.fib_table_hash[h];
- hlist_for_each_entry_rcu(tb, head, tb_hlist) {
+ hlist_for_each_entry_rcu(tb, head, tb_hlist,
+ lockdep_rtnl_is_held()) {
if (tb->tb_id == id)
return tb;
}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v1 6/6] acpi: Use built-in RCU list checking for acpi_ioremaps list
From: Joel Fernandes (Google) @ 2019-07-11 23:44 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Josh Triplett, keescook, kernel-hardening,
Lai Jiangshan, Len Brown, linux-acpi, linux-pci, linux-pm,
Mathieu Desnoyers, neilb, netdev, oleg, Paul E. McKenney,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190711234401.220336-1-joel@joelfernandes.org>
list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
it for acpi_ioremaps list traversal.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
drivers/acpi/osl.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index f29e427d0d1d..c8b5d712c7ae 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -28,6 +28,7 @@
#include <linux/slab.h>
#include <linux/mm.h>
#include <linux/highmem.h>
+#include <linux/lockdep.h>
#include <linux/pci.h>
#include <linux/interrupt.h>
#include <linux/kmod.h>
@@ -94,6 +95,7 @@ struct acpi_ioremap {
static LIST_HEAD(acpi_ioremaps);
static DEFINE_MUTEX(acpi_ioremap_lock);
+#define acpi_ioremap_lock_held() lock_is_held(&acpi_ioremap_lock.dep_map)
static void __init acpi_request_region (struct acpi_generic_address *gas,
unsigned int length, char *desc)
@@ -220,7 +222,7 @@ acpi_map_lookup(acpi_physical_address phys, acpi_size size)
{
struct acpi_ioremap *map;
- list_for_each_entry_rcu(map, &acpi_ioremaps, list)
+ list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
if (map->phys <= phys &&
phys + size <= map->phys + map->size)
return map;
@@ -263,7 +265,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
{
struct acpi_ioremap *map;
- list_for_each_entry_rcu(map, &acpi_ioremaps, list)
+ list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
if (map->virt <= virt &&
virt + size <= map->virt + map->size)
return map;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v1 5/6] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator
From: Joel Fernandes (Google) @ 2019-07-11 23:44 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Josh Triplett, keescook, kernel-hardening,
Lai Jiangshan, Len Brown, linux-acpi, linux-pci, linux-pm,
Mathieu Desnoyers, neilb, netdev, oleg, Paul E. McKenney,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190711234401.220336-1-joel@joelfernandes.org>
The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
reader-lock held, because the pci_mmcfg_lock is already held. Make this
known to the list macro so that it fixes new lockdep warnings that
trigger due to lockdep checks added to list_for_each_entry_rcu().
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
arch/x86/pci/mmconfig-shared.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 7389db538c30..6fa42e9c4e6f 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -29,6 +29,7 @@
static bool pci_mmcfg_running_state;
static bool pci_mmcfg_arch_init_failed;
static DEFINE_MUTEX(pci_mmcfg_lock);
+#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
LIST_HEAD(pci_mmcfg_list);
@@ -54,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
struct pci_mmcfg_region *cfg;
/* keep list sorted by segment and starting bus number */
- list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
+ list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) {
if (cfg->segment > new->segment ||
(cfg->segment == new->segment &&
cfg->start_bus >= new->start_bus)) {
@@ -118,7 +119,7 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
{
struct pci_mmcfg_region *cfg;
- list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
+ list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held())
if (cfg->segment == segment &&
cfg->start_bus <= bus && bus <= cfg->end_bus)
return cfg;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v1 3/6] driver/core: Convert to use built-in RCU list checking
From: Joel Fernandes (Google) @ 2019-07-11 23:43 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Josh Triplett, keescook, kernel-hardening,
Lai Jiangshan, Len Brown, linux-acpi, linux-pci, linux-pm,
Mathieu Desnoyers, neilb, netdev, oleg, Paul E. McKenney,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190711234401.220336-1-joel@joelfernandes.org>
list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
it in driver core.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
drivers/base/base.h | 1 +
drivers/base/core.c | 10 ++++++++++
drivers/base/power/runtime.c | 15 ++++++++++-----
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index b405436ee28e..0d32544b6f91 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -165,6 +165,7 @@ static inline int devtmpfs_init(void) { return 0; }
/* Device links support */
extern int device_links_read_lock(void);
extern void device_links_read_unlock(int idx);
+extern int device_links_read_lock_held(void);
extern int device_links_check_suppliers(struct device *dev);
extern void device_links_driver_bound(struct device *dev);
extern void device_links_driver_cleanup(struct device *dev);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index fd7511e04e62..6c5ca9685647 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -68,6 +68,11 @@ void device_links_read_unlock(int idx)
{
srcu_read_unlock(&device_links_srcu, idx);
}
+
+int device_links_read_lock_held(void)
+{
+ return srcu_read_lock_held(&device_links_srcu);
+}
#else /* !CONFIG_SRCU */
static DECLARE_RWSEM(device_links_lock);
@@ -91,6 +96,11 @@ void device_links_read_unlock(int not_used)
{
up_read(&device_links_lock);
}
+
+int device_links_read_lock_held(void)
+{
+ return lock_is_held(&device_links_lock);
+}
#endif /* !CONFIG_SRCU */
/**
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 952a1e7057c7..7a10e8379a70 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -287,7 +287,8 @@ static int rpm_get_suppliers(struct device *dev)
{
struct device_link *link;
- list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+ device_links_read_lock_held()) {
int retval;
if (!(link->flags & DL_FLAG_PM_RUNTIME) ||
@@ -309,7 +310,8 @@ static void rpm_put_suppliers(struct device *dev)
{
struct device_link *link;
- list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+ device_links_read_lock_held()) {
if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND)
continue;
@@ -1640,7 +1642,8 @@ void pm_runtime_clean_up_links(struct device *dev)
idx = device_links_read_lock();
- list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
+ list_for_each_entry_rcu(link, &dev->links.consumers, s_node,
+ device_links_read_lock_held()) {
if (link->flags & DL_FLAG_STATELESS)
continue;
@@ -1662,7 +1665,8 @@ void pm_runtime_get_suppliers(struct device *dev)
idx = device_links_read_lock();
- list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+ device_links_read_lock_held())
if (link->flags & DL_FLAG_PM_RUNTIME) {
link->supplier_preactivated = true;
refcount_inc(&link->rpm_active);
@@ -1683,7 +1687,8 @@ void pm_runtime_put_suppliers(struct device *dev)
idx = device_links_read_lock();
- list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+ device_links_read_lock_held())
if (link->supplier_preactivated) {
link->supplier_preactivated = false;
if (refcount_dec_not_one(&link->rpm_active))
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v1 4/6] workqueue: Convert for_each_wq to use built-in list check
From: Joel Fernandes (Google) @ 2019-07-11 23:43 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Josh Triplett, keescook, kernel-hardening,
Lai Jiangshan, Len Brown, linux-acpi, linux-pci, linux-pm,
Mathieu Desnoyers, neilb, netdev, oleg, Paul E. McKenney,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190711234401.220336-1-joel@joelfernandes.org>
list_for_each_entry_rcu now has support to check for RCU reader sections
as well as lock. Just use the support in it, instead of explictly
checking in the caller.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/workqueue.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 9657315405de..91ed7aca16e5 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -424,9 +424,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
* ignored.
*/
#define for_each_pwq(pwq, wq) \
- list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
- if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
- else
+ list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
+ lock_is_held(&(wq->mutex).dep_map))
#ifdef CONFIG_DEBUG_OBJECTS_WORK
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox