diff mbox

ACPI / CPPC: set an error code on probe error path

Message ID 20161130192254.GE28180@mwanda (mailing list archive)
State Accepted, archived
Delegated to: Rafael Wysocki
Headers show

Commit Message

Dan Carpenter Nov. 30, 2016, 7:22 p.m. UTC 
We should return -EINVAL if get_cpu_device() fails.

Fixes: 158c998ea44b ("ACPI / CPPC: add sysfs support to compute delivered performance")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Rafael J. Wysocki Nov. 30, 2016, 9:53 p.m. UTC | #1 
On Wed, Nov 30, 2016 at 8:22 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> We should return -EINVAL if get_cpu_device() fails.
>
> Fixes: 158c998ea44b ("ACPI / CPPC: add sysfs support to compute delivered performance")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index d0d0504..e0ea8f5 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -784,8 +784,10 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>
>         /* Add per logical CPU nodes for reading its feedback counters. */
>         cpu_dev = get_cpu_device(pr->id);
> -       if (!cpu_dev)
> +       if (!cpu_dev) {
> +               ret = -EINVAL;

ret is initialized here AFAICS.

Do you that its value is not the right one?

>                 goto out_free;
> +       }
>
>         ret = kobject_init_and_add(&cpc_ptr->kobj, &cppc_ktype, &cpu_dev->kobj,
>                         "acpi_cppc");
> --

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Nov. 30, 2016, 9:55 p.m. UTC | #2 
On Wed, Nov 30, 2016 at 10:53 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Nov 30, 2016 at 8:22 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> We should return -EINVAL if get_cpu_device() fails.
>>
>> Fixes: 158c998ea44b ("ACPI / CPPC: add sysfs support to compute delivered performance")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index d0d0504..e0ea8f5 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -784,8 +784,10 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>
>>         /* Add per logical CPU nodes for reading its feedback counters. */
>>         cpu_dev = get_cpu_device(pr->id);
>> -       if (!cpu_dev)
>> +       if (!cpu_dev) {
>> +               ret = -EINVAL;
>
> ret is initialized here AFAICS.
>
> Do you that its value is not the right one?

This was supposed to be "Do you mean that ...", sorry.

>
>>                 goto out_free;
>> +       }
>>
>>         ret = kobject_init_and_add(&cpc_ptr->kobj, &cppc_ktype, &cpu_dev->kobj,
>>                         "acpi_cppc");
>> --

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter Nov. 30, 2016, 10:03 p.m. UTC | #3 
On Wed, Nov 30, 2016 at 10:53:55PM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 30, 2016 at 8:22 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > We should return -EINVAL if get_cpu_device() fails.
> >
> > Fixes: 158c998ea44b ("ACPI / CPPC: add sysfs support to compute delivered performance")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > index d0d0504..e0ea8f5 100644
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -784,8 +784,10 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
> >
> >         /* Add per logical CPU nodes for reading its feedback counters. */
> >         cpu_dev = get_cpu_device(pr->id);
> > -       if (!cpu_dev)
> > +       if (!cpu_dev) {
> > +               ret = -EINVAL;
> 
> ret is initialized here AFAICS.
> 
> Do you that its value is not the right one?

I'm looking at linux-next.  It's set to zero but we presumably want to
return an error code.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Nov. 30, 2016, 10:14 p.m. UTC | #4 
On Wed, Nov 30, 2016 at 11:03 PM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Wed, Nov 30, 2016 at 10:53:55PM +0100, Rafael J. Wysocki wrote:
>> On Wed, Nov 30, 2016 at 8:22 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > We should return -EINVAL if get_cpu_device() fails.
>> >
>> > Fixes: 158c998ea44b ("ACPI / CPPC: add sysfs support to compute delivered performance")
>> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> >
>> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> > index d0d0504..e0ea8f5 100644
>> > --- a/drivers/acpi/cppc_acpi.c
>> > +++ b/drivers/acpi/cppc_acpi.c
>> > @@ -784,8 +784,10 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>> >
>> >         /* Add per logical CPU nodes for reading its feedback counters. */
>> >         cpu_dev = get_cpu_device(pr->id);
>> > -       if (!cpu_dev)
>> > +       if (!cpu_dev) {
>> > +               ret = -EINVAL;
>>
>> ret is initialized here AFAICS.
>>
>> Do you that its value is not the right one?
>
> I'm looking at linux-next.  It's set to zero but we presumably want to
> return an error code.

OK

It was slightly unclear what the bug was.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Dec. 1, 2016, 2:14 p.m. UTC | #5 
On Wednesday, November 30, 2016 11:14:23 PM Rafael J. Wysocki wrote:
> On Wed, Nov 30, 2016 at 11:03 PM, Dan Carpenter
> <dan.carpenter@oracle.com> wrote:
> > On Wed, Nov 30, 2016 at 10:53:55PM +0100, Rafael J. Wysocki wrote:
> >> On Wed, Nov 30, 2016 at 8:22 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> > We should return -EINVAL if get_cpu_device() fails.
> >> >
> >> > Fixes: 158c998ea44b ("ACPI / CPPC: add sysfs support to compute delivered performance")
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> >
> >> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> >> > index d0d0504..e0ea8f5 100644
> >> > --- a/drivers/acpi/cppc_acpi.c
> >> > +++ b/drivers/acpi/cppc_acpi.c
> >> > @@ -784,8 +784,10 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
> >> >
> >> >         /* Add per logical CPU nodes for reading its feedback counters. */
> >> >         cpu_dev = get_cpu_device(pr->id);
> >> > -       if (!cpu_dev)
> >> > +       if (!cpu_dev) {
> >> > +               ret = -EINVAL;
> >>
> >> ret is initialized here AFAICS.
> >>
> >> Do you that its value is not the right one?
> >
> > I'm looking at linux-next.  It's set to zero but we presumably want to
> > return an error code.
> 
> OK
> 
> It was slightly unclear what the bug was.

Applied now.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index d0d0504..e0ea8f5 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -784,8 +784,10 @@  int acpi_cppc_processor_probe(struct acpi_processor *pr)
 
 	/* Add per logical CPU nodes for reading its feedback counters. */
 	cpu_dev = get_cpu_device(pr->id);
-	if (!cpu_dev)
+	if (!cpu_dev) {
+		ret = -EINVAL;
 		goto out_free;
+	}
 
 	ret = kobject_init_and_add(&cpc_ptr->kobj, &cppc_ktype, &cpu_dev->kobj,
 			"acpi_cppc");