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

[Alexandre Courbot]

On Fri, Dec 5, 2014 at 10:41 PM, Arto Merilainen <amerilainen@xxxxxxxxxx> wrote:
> From: Shridhar Rasal <srasal@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Arto Merilainen <amerilainen@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/