Re: [PATCH] cros_ec_spi: Even though we're RT priority, don't bump cpu freq

[Qais Yousef]

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@xxxxxxxxxxxx>
> ---
> NOTE: This would cause a conflict if the patch
> https://lore.kernel.org/r/20200422112831.870192415@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx/

>  	};
>  	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
>