linux-kernel - Re: [PATCH] cpufreq: acpi: Add comment under ACPI_ADR_SPACE_SYSTEM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1402816.XYNGOIhCWR@vostro.rjw.lan>
Date:	Sat, 12 Oct 2013 22:41:08 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	linaro-kernel@...ts.linaro.org, patches@...aro.org,
	cpufreq@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpufreq: acpi: Add comment under ACPI_ADR_SPACE_SYSTEM_IO case

On Saturday, October 12, 2013 08:58:10 PM Viresh Kumar wrote:
> On 12/10/2013, Rafael J. Wysocki <rjw@...k.pl> wrote:
> > Well, please merge it with the existing comment and use the usual format
> > for comments that are longer than two lines.
> 
> I thought these are separate comments and so keeping them separate
> might be better,

No, comments that go like

	/* Comment 1. */
	/* Comment 2. */

do not even *look* separate.

In general you can do something like

	/*
	 * Comment 1.
	 *
	 * Comment 2.
	 */

in such cases, but again in this particular case the comments aren't even
logically separate in my opinion. ->

> so that this one doesn't get deleted in case somebody
> is removing the other one.. and vice versa..
> 
> Anyway its fixed in attached commit now :)
> 
> commit ecb9ef81b50eb5e8559f7d132ef46803c8272091
> Author: Viresh Kumar <viresh.kumar@...aro.org>
> Date:   Sat Oct 12 07:00:01 2013 +0530
> 
>     cpufreq: acpi: Add comment under ACPI_ADR_SPACE_SYSTEM_IO case
> 
>     policy->cur is now set by cpufreq core when cpufreq_driver->get()
> is defined and
>     so drivers aren't required to set it. When space_id is
> ACPI_ADR_SPACE_SYSTEM_IO
>     for acpi cpufreq driver it doesn't set ->get to a valid function
> pointer and so
>     policy->cur is required to be set by driver.
> 
>     This is already followed in acpi-cpufreq driver. This patch adds a comment
>     describing why we need to set policy->cur from driver.
> 
>     Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>     Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index a8dac7b..d2df543 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -837,7 +837,12 @@ static int acpi_cpufreq_cpu_init(struct
> cpufreq_policy *policy)
> 
>         switch (perf->control_register.space_id) {
>         case ACPI_ADR_SPACE_SYSTEM_IO:
> -               /* Current speed is unknown and not detectable by IO port */
> +               /*
> +                * Current speed is unknown and not detectable by IO port.
> +                * policy->cur wouldn't be set by core as cpufreq_driver->get()
> +                * is NULL for this space_id and so we need to set policy->cur
> +                * here.
> +                */

-> What about this:

	/*
	 * The core will not set policy->cur, because cpufreq_driver->get is NULL,
	 * so we need to set it here.  However, we have to guess it, because the
	 * current speed is unknown and not detectable via IO ports.
	 */

>                 policy->cur = acpi_cpufreq_guess_freq(data, policy->cpu);
>                 break;
>         case ACPI_ADR_SPACE_FIXED_HARDWARE:

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/