Messages in this thread | | | Date | Fri, 12 Jun 2020 13:52:50 +0100 | From | Qais Yousef <> | Subject | Re: [PATCH] cros_ec_spi: Even though we're RT priority, don't bump cpu freq |
| |
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 >
| |