Linux Power Management development
 help / color / mirror / Atom feed
From: Viresh Kumar <viresh.kumar@linaro.org>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org, rjw@rjwysocki.net,
	linux-kernel@vger.kernel.org,
	Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>,
	linux-pm@vger.kernel.org
Subject: Re: [PATCH] cpufreq: powernv: Check  negative value returned by cpufreq_table_find_index_dl()
Date: Wed, 21 Feb 2018 11:24:50 +0530	[thread overview]
Message-ID: <20180221055450.GO28462@vireshk-i7> (raw)
In-Reply-To: <874lmasxxx.fsf@concordia.ellerman.id.au>

On 21-02-18, 16:39, Michael Ellerman wrote:
> Viresh Kumar <viresh.kumar@linaro.org> writes:

> > AFAICT, you will get -1 here only if the freq table had no valid
> > frequencies (or the freq table is empty). Why would that happen ?
> 
> Bugs?

The cupfreq driver shouldn't have registered itself in that case (i.e.
if the cpufreq table is empty).

> Or if you ask for a target_freq that is higher than anything in the
> table.

You will still get a valid index in that case.

There is only once case where we return -1, when the cpufreq table
doesn't have any valid frequencies.

> Or the API changes, and we forget to update this call site.

I am not sure we can do much about that right now.

> If you're saying that cpufreq_table_find_index_dl() can NEVER fail,

Yes, if we have at least one valid frequency in the table, otherwise
the cpufreq driver shouldn't have registered itself.

> then
> write it so that it can never fail and change it to return unsigned int.

But what should we do when there is no frequency in the cpufreq table?
Just in case where a driver is buggy and tries to call this routine
for an invalid table.

> Having it potentially return -1, which is then used to index an array
> and not handling that is just asking for bugs to happen.

I understand what you are trying to say here, but I don't know what
can be done to prevent this here.

What we can do is change the return type to void and pass a int
pointer to the routine, but that wouldn't change anything at all. That
pointers variable can still have -1 in it.

-- 
viresh

  reply	other threads:[~2018-02-21  5:54 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-12 10:21 [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl() Shilpasri G Bhat
2018-02-12 10:29 ` Viresh Kumar
2018-02-12 10:33   ` Shilpasri G Bhat
2018-02-12 10:40     ` Viresh Kumar
2018-02-26  9:48     ` Rafael J. Wysocki
2018-02-21  5:39   ` Michael Ellerman
2018-02-21  5:54     ` Viresh Kumar [this message]
2018-02-21  9:27       ` Rafael J. Wysocki
2018-02-21 10:02         ` Viresh Kumar
2018-02-21 10:17           ` Rafael J. Wysocki
2018-02-21 10:19             ` Viresh Kumar
2018-02-21 13:13         ` Michael Ellerman
2018-02-21 13:25           ` Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180221055450.GO28462@vireshk-i7 \
    --to=viresh.kumar@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    --cc=rjw@rjwysocki.net \
    --cc=shilpa.bhat@linux.vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox