From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5ABC3C43603 for ; Mon, 9 Dec 2019 19:27:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1D654205ED for ; Mon, 9 Dec 2019 19:27:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="PoJ1fJmM" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727189AbfLIT1u (ORCPT ); Mon, 9 Dec 2019 14:27:50 -0500 Received: from mail-pl1-f193.google.com ([209.85.214.193]:35025 "EHLO mail-pl1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727178AbfLIT1q (ORCPT ); Mon, 9 Dec 2019 14:27:46 -0500 Received: by mail-pl1-f193.google.com with SMTP id s10so6213621plp.2 for ; Mon, 09 Dec 2019 11:27:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=kB1Dm5QdRvo+4ntSSP2QV88L91oQIBSmXvZFFUy0ooA=; b=PoJ1fJmMamJi9zmUxFThCbE3g18OtYx+ZbHroN6REARVBPp7YdFot6D8opjlokllNL 6N6lRGKDBdpGC06SZAXrdviGmwlAZb74nsaj5RQl/6+7s93sxu4zheqUX/1AiQzE4I3M QUBedDSudYhvTsrHXcPIki6A/TIDu0zrvXaTk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=kB1Dm5QdRvo+4ntSSP2QV88L91oQIBSmXvZFFUy0ooA=; b=I4/3IO7Kv8u3Sd6qewD04OHMQLbjjI6Y2Pp9TPHNYCbD9wMPHZ49FP6AADnb+u1LID sibHdr8udjtB0t9JkB5SUq1U4UtpDW8lF6fD4WCyP5BHF3bb7QxfS1hrYy8e0wHDUCt7 wvhtewypaE1EohP1PGp0YJQ41Z3XPpATXCcIbGkdloIU73tPZMrCeF8szFBK44kBOcG3 Hp+bPfWTZCpF/Fp0Jstgy31rI4xXv4XitZF4sZ4q/eLg4L02ISs0UVy+jkDebefC1MQu iSMELwPTMQSv0NhadBV9U0eOTIfnpGqgeosu6Ew5z45TZ09N6FJ34WohnumMsmvRIpqi NVpw== X-Gm-Message-State: APjAAAWXCxMvVOj0b7WoYYMMdmUuPtfa/m0HsmATVmJTgKtXyTVnvq6R EEYZPOQaazVyUkuDNBFHJgSOdA== X-Google-Smtp-Source: APXvYqzLvXqT8hfm/ru3JfLfbM93M1FuOEkMfJe5bZLb6DkkwtltgsbvJNl7gPppZoHhimF9CGXe7w== X-Received: by 2002:a17:902:9007:: with SMTP id a7mr5337266plp.46.1575919664605; Mon, 09 Dec 2019 11:27:44 -0800 (PST) Received: from localhost ([2620:15c:202:1:4fff:7a6b:a335:8fde]) by smtp.gmail.com with ESMTPSA id m13sm315926pga.70.2019.12.09.11.27.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 09 Dec 2019 11:27:44 -0800 (PST) Date: Mon, 9 Dec 2019 11:27:42 -0800 From: Matthias Kaehlcke To: Kamil Konieczny Cc: Bartlomiej Zolnierkiewicz , Chanwoo Choi , Krzysztof Kozlowski , Kyungmin Park , linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, Marek Szyprowski , MyungJoo Ham Subject: Re: [PATCH 2/4] PM / devfreq: add possibility for delayed work Message-ID: <20191209192742.GP228856@google.com> References: <20191209144425.13321-1-k.konieczny@samsung.com> <20191209144425.13321-3-k.konieczny@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191209144425.13321-3-k.konieczny@samsung.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Mon, Dec 09, 2019 at 03:44:23PM +0100, Kamil Konieczny wrote: > Current devfreq workqueue uses deferred timer. Introduce sysfs > file delayed_timer and use it for change from deferred to > delayed work. The default is to use old deferred one, which > saves power, but can miss increased demand for higher bus > frequency if timer was assigned to idle cpu. > > Signed-off-by: Kamil Konieczny > --- > Documentation/ABI/testing/sysfs-class-devfreq | 10 ++++ > drivers/devfreq/devfreq.c | 46 ++++++++++++++++++- > include/linux/devfreq.h | 2 + > 3 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq > index 9758eb85ade3..07bfd0df6a4a 100644 > --- a/Documentation/ABI/testing/sysfs-class-devfreq > +++ b/Documentation/ABI/testing/sysfs-class-devfreq > @@ -30,6 +30,16 @@ Description: > target_freq when get_cur_freq() is not implemented by > devfreq driver. > > +What: /sys/class/devfreq/.../delayed_timer > +Date: December 2019 > +Contact: Kamil Konieczny > +Description: > + This ABI shows or clears timer type used by devfreq > + workqueue. When 0, it uses default deferred timer. > + When set to 1 devfreq will use delayed timer. Example > + useage: > + echo 1 > /sys/class/devfreq/.../delayed_timer > + > What: /sys/class/devfreq/.../target_freq > Date: September 2012 > Contact: Rajagopal Venkat > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 955949c6fc1f..c277d1770fef 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -445,7 +445,11 @@ void devfreq_monitor_start(struct devfreq *devfreq) > if (devfreq->governor->interrupt_driven) > return; > > - INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); > + if (devfreq->delayed_timer) > + INIT_DELAYED_WORK(&devfreq->work, devfreq_monitor); > + else > + INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); > + > if (devfreq->profile->polling_ms) > queue_delayed_work(devfreq_wq, &devfreq->work, > msecs_to_jiffies(devfreq->profile->polling_ms)); > @@ -698,6 +702,7 @@ struct devfreq *devfreq_add_device(struct device *dev, > devfreq->last_status.current_frequency = profile->initial_freq; > devfreq->data = data; > devfreq->nb.notifier_call = devfreq_notifier_call; > + devfreq->delayed_timer = false; devfreq is zero initialized (allocated with kzalloc()), hence this is unnecessary. > > if (!devfreq->profile->max_state && !devfreq->profile->freq_table) { > mutex_unlock(&devfreq->lock); > @@ -1288,6 +1293,44 @@ static ssize_t available_governors_show(struct device *d, > } > static DEVICE_ATTR_RO(available_governors); > > +static ssize_t delayed_timer_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + int i; > + > + i = to_devfreq(dev)->delayed_timer ? 1 : 0; > + return sprintf(buf, "%d\n", i); get rid of 'i' and just use df->delayed_timer in sprintf(). > +} > + > +static ssize_t delayed_timer_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct devfreq *df = to_devfreq(dev); > + bool old_timer; Not a great name, the variable doesn't hold a timer. I'd suggest something like 'prev_val'. > + int value, ret; > + > + if (!df->governor) > + return -EINVAL; > + > + ret = kstrtoint(buf, 10, &value); > + if (ret || (value != 1 && value != 0)) > + return -EINVAL; use kstrtobool() instead of partially re-implementing it. > + > + mutex_lock(&df->lock); > + old_timer = df->delayed_timer; > + df->delayed_timer = value == 0 ? false : true; What's wrong with: df->delayed_timer = value; ? > + mutex_unlock(&df->lock); Does the locking as is actually provide any value? The use case seems to be concurrent setting of the sysfs attribute. The lock is released after the assignment, hence the value of 'df->delayed_timer' could be overwritten before the condition below is evaluated. If you want to protect against this case you need something like this: // don't release the lock before evaluating the condition > + if (old_timer != df->delayed_timer) { mutex_unlock(&df->lock); > + devfreq_monitor_stop(df); > + devfreq_monitor_start(df); > + } else { mutex_unlock(&df->lock); } I don't pretend it's pretty ;-)