linux-kernel - Re: [RFC RESEND 1/3] PM / devfreq: Add watermark events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 9 Dec 2014 16:17:01 +0900
From:	Alexandre Courbot <gnurou@...il.com>
To:	Arto Merilainen <amerilainen@...dia.com>
Cc:	MyungJoo Ham <myungjoo.ham@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Tomeu Vizoso <tomeu.vizoso@...labora.com>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	Stephen Warren <swarren@...dotorg.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Grant Likely <grant.likely@...aro.org>, srasal@...dia.com
Subject: Re: [RFC RESEND 1/3] PM / devfreq: Add watermark events

On Fri, Dec 5, 2014 at 10:41 PM, Arto Merilainen <amerilainen@...dia.com> wrote:
> From: Shridhar Rasal <srasal@...dia.com>
>
> This patch adds support for watermark events. These events inform
> the governor that the device load has gone below (low watermark) or
> above (high watermark) certain load value.

This is definitely a useful feature to have, as it reflects what
efficient hardware does (raise an interrupt when load goes above/under
a certain threshold). In particular Tomeu's ACTMON driver would
probably greatly benefit from it.

A few comments below:

>
> Signed-off-by: Shridhar Rasal <srasal@...dia.com>
> Signed-off-by: Arto Merilainen <amerilainen@...dia.com>
> ---
>  drivers/devfreq/devfreq.c  | 19 +++++++++++++++++++
>  drivers/devfreq/governor.h |  1 +
>  include/linux/devfreq.h    | 26 ++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 30b538d8cc90..8333d024132a 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1050,6 +1050,25 @@ static struct attribute *devfreq_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(devfreq);
>
> +/**
> + * devfreq_watermark_event() - Handles watermark events
> + * @devfreq: the devfreq instance to be updated
> + * @type: type of watermark event
> + */
> +int devfreq_watermark_event(struct devfreq *devfreq, int type)

Here I suppose "type" is to be an enum watermark_type - therefore it
should probably be changed to that type instead of int.

> +{
> +       if (!devfreq)
> +               return -EINVAL;
> +
> +       if (!devfreq->governor)
> +               return -EINVAL;

Let's fold these two conditions into "if (!devfreq || !devfreq->governor)"

> +
> +       return devfreq->governor->event_handler(devfreq,
> +                               DEVFREQ_GOV_WMARK, &type);
> +}
> +EXPORT_SYMBOL(devfreq_watermark_event);
> +
> +

Extra whiteline here.

>  static int __init devfreq_init(void)
>  {
>         devfreq_class = class_create(THIS_MODULE, "devfreq");
> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> index fad7d6321978..fb9875388f3f 100644
> --- a/drivers/devfreq/governor.h
> +++ b/drivers/devfreq/governor.h
> @@ -24,6 +24,7 @@
>  #define DEVFREQ_GOV_INTERVAL                   0x3
>  #define DEVFREQ_GOV_SUSPEND                    0x4
>  #define DEVFREQ_GOV_RESUME                     0x5
> +#define DEVFREQ_GOV_WMARK                      0x6
>
>  /* Caution: devfreq->lock must be locked before calling update_devfreq */
>  extern int update_devfreq(struct devfreq *devfreq);
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index f1863dcd83ea..b5bf6c4fe286 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -21,6 +21,12 @@
>
>  struct devfreq;
>
> +enum watermark_type {

Change the name to devfreq_watermark_event maybe? We know it's a type
already from the context, and we should use the devfreq_ prefix to
avoid name collisions.

> +       NO_WATERMARK_EVENT = 0,
> +       HIGH_WATERMARK_EVENT = 1,
> +       LOW_WATERMARK_EVENT = 2

... and if the type is renamed to watermark_event, these could become
DEVFREQ_NO_WATERMARK, DEVFREQ_HIGH_WATERMARK, and
DEVFREQ_LOW_WATERMARK.

> +};
> +
>  /**
>   * struct devfreq_dev_status - Data given from devfreq user device to
>   *                          governors. Represents the performance
> @@ -68,6 +74,16 @@ struct devfreq_dev_status {
>   *                     status to devfreq, which is used by governors.
>   * @get_cur_freq:      The device should provide the current frequency
>   *                     at which it is operating.
> + * @set_high_wmark:    This is an optional callback to set high
> + *                     watermark for watermark event. The value is
> + *                     be scaled between 0 and 1000 where 1000 equals to
> + *                     100% load. Setting this value to 1000 disables
> + *                     the event
> + * @set_low_wmark:     This is an optional callback to set low
> + *                     watermark for watermark event. The value is
> + *                     be scaled between 0 and 1000 where 1000 equals to
> + *                     100% load. Setting this value to 0 disables the
> + *                     event.

>From the comment it is not clear whether the 0..1000 range corresponds
to the load between two frequencies in the frequencies table, or
covers the whole frequency table. I understand it is the former, but
it would be nice to explicitly state it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/