Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 12 Jun 2020 13:52:50 +0100
From:   Qais Yousef <qais.yousef@arm.com>
To:     Douglas Anderson <dianders@chromium.org>
Cc:     Benson Leung <bleung@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        hsinyi@chromium.org, joelaf@google.com, peterz@infradead.org,
        drinkcat@chromium.org, gwendal@chromium.org, qperret@google.com,
        ctheegal@codeaurora.org, Guenter Roeck <groeck@chromium.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] cros_ec_spi: Even though we're RT priority, don't bump
 cpu freq
Message-ID: <20200612125250.7bwjfnxhurdf5bwj@e107158-lin.cambridge.arm.com>
References: <20200610151818.1.I666ecd9c6f3c6405bd75831a21001b8109b6438c@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200610151818.1.I666ecd9c6f3c6405bd75831a21001b8109b6438c@changeid>
User-Agent: NeoMutt/20171215
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 06/10/20 15:18, Douglas Anderson wrote:
> The cros_ec_spi driver is realtime priority so that it doesn't get
> preempted by other taks while it's talking to the EC but overall it
> really doesn't need lots of compute power.  Unfortunately, by default,
> the kernel assumes that all realtime tasks should cause the cpufreq to
> jump to max and burn through power to get things done as quickly as
> possible.  That's just not the correct behavior for cros_ec_spi.
> 
> Switch to manually overriding the default.
> 
> This won't help us if our work moves over to the SPI pump thread but
> that's not the common code path.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> NOTE: This would cause a conflict if the patch
> https://lore.kernel.org/r/20200422112831.870192415@infradead.org lands
> first
> 
>  drivers/platform/chrome/cros_ec_spi.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index debea5c4c829..76d59d5e7efd 100644
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -709,8 +709,11 @@ static void cros_ec_spi_high_pri_release(void *worker)
>  static int cros_ec_spi_devm_high_pri_alloc(struct device *dev,
>  					   struct cros_ec_spi *ec_spi)
>  {
> -	struct sched_param sched_priority = {
> -		.sched_priority = MAX_RT_PRIO / 2,
> +	struct sched_attr sched_attr = {
> +		.sched_policy	= SCHED_FIFO,
> +		.sched_priority	= MAX_RT_PRIO / 2,
> +		.sched_flags	= SCHED_FLAG_UTIL_CLAMP_MIN,
> +		.sched_util_min	= 0,

Hmm I thought Peter already removed all users that change RT priority directly.

https://lore.kernel.org/lkml/20200422112719.826676174@infradead.org/

>  	};
>  	int err;
>  
> @@ -728,8 +731,7 @@ static int cros_ec_spi_devm_high_pri_alloc(struct device *dev,
>  	if (err)
>  		return err;
>  
> -	err = sched_setscheduler_nocheck(ec_spi->high_pri_worker->task,
> -					 SCHED_FIFO, &sched_priority);
> +	err = sched_setattr_nocheck(ec_spi->high_pri_worker->task, &sched_attr);
>  	if (err)
>  		dev_err(dev, "Can't set cros_ec high pri priority: %d\n", err);
>  	return err;

If I read the code correctly, if you fail here cros_ec_spi_probe() will fail
too and the whole thing will not be loaded. If you compile without uclamp then
sched_setattr_nocheck() will always fail with -EOPNOTSUPP.

Why can't you manage the priority and boost value of such tasks via your init
scripts instead?

I have to admit I need to think about whether it makes sense to have a generic
API that allows drivers to opt-out of the default boosting behavior for their
RT tasks.

Thanks

--
Qais Yousef

> -- 
> 2.27.0.290.gba653c62da-goog
>