[PATCH 1/1] powerpc/ps3: Fix error return code in ps3_register_devices()

[PATCH 1/1] powerpc/ps3: Fix error return code in ps3_register_devices()

[Zhen Lei]

When call ps3_start_probe_thread() failed, further initialization should
be stopped and the returned error code should be propagated.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/powerpc/platforms/ps3/device-init.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c
index e87360a0fb40..9b6d8ca8fc01 100644
--- a/arch/powerpc/platforms/ps3/device-init.c
+++ b/arch/powerpc/platforms/ps3/device-init.c
@@ -955,6 +955,8 @@ static int __init ps3_register_devices(void)
 	/* ps3_repository_dump_bus_info(); */
 
 	result = ps3_start_probe_thread(PS3_BUS_TYPE_STORAGE);
+	if (result < 0)
+		return result;
 
 	ps3_register_vuart_devices();
 
-- 
2.25.1

Re: [PATCH 1/1] powerpc/ps3: Fix error return code in ps3_register_devices()

[Michael Ellerman]

Zhen Lei <thunder.leizhen@huawei.com> writes:
> When call ps3_start_probe_thread() failed, further initialization should
> be stopped and the returned error code should be propagated.

It's not clear to me that's a good change.

> diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c
> index e87360a0fb40..9b6d8ca8fc01 100644
> --- a/arch/powerpc/platforms/ps3/device-init.c
> +++ b/arch/powerpc/platforms/ps3/device-init.c
> @@ -955,6 +955,8 @@ static int __init ps3_register_devices(void)
>  	/* ps3_repository_dump_bus_info(); */
>  
>  	result = ps3_start_probe_thread(PS3_BUS_TYPE_STORAGE);
> +	if (result < 0)
> +		return result;

If you bail out here you skip:

>  	ps3_register_vuart_devices();


Which I suspect means there will be no console output?

Presumably the system won't boot if the probe thread fails, but it might
at least print an oops, whereas if we return we might get nothing at
all. Though I'm just guessing, I don't know this code that well.

Anyway please leave this code alone unless you're willing to test your
changes, or at least provide a more thorough justification for them.

cheers

Re: [PATCH 1/1] powerpc/ps3: Fix error return code in ps3_register_devices()

[Geoff Levand]

Hi,

On 5/20/21 5:20 AM, Michael Ellerman wrote:
> Zhen Lei <thunder.leizhen@huawei.com> writes:
>> When call ps3_start_probe_thread() failed, further initialization should
>> be stopped and the returned error code should be propagated.
...
>> --- a/arch/powerpc/platforms/ps3/device-init.c
>>  
>>  	result = ps3_start_probe_thread(PS3_BUS_TYPE_STORAGE);
>> +	if (result < 0)
>> +		return result;
> 
> If you bail out here you skip:
> 
>>  	ps3_register_vuart_devices();
> 
> Which I suspect means there will be no console output?
> 
> Presumably the system won't boot if the probe thread fails, but it might
> at least print an oops, whereas if we return we might get nothing at
> all. Though I'm just guessing, I don't know this code that well.

That probe is for the storage devices (PS3_BUS_TYPE_STORAGE).

There are cases where the system is usable even if the storage
devices are not available, for example, when using an NFS root
filesystem.

ps3_start_probe_thread was made to be quite verbose on error
to make up for it's return value not being checked.

> Anyway please leave this code alone unless you're willing to test your
> changes, or at least provide a more thorough justification for them.

Agreed, this change should not be merged.

-Geoff

Re: [PATCH 1/1] powerpc/ps3: Fix error return code in ps3_register_devices()

[Leizhen (ThunderTown)]

On 2021/5/24 4:15, Geoff Levand wrote:
> Hi,
> 
> On 5/20/21 5:20 AM, Michael Ellerman wrote:
>> Zhen Lei <thunder.leizhen@huawei.com> writes:
>>> When call ps3_start_probe_thread() failed, further initialization should
>>> be stopped and the returned error code should be propagated.
> ...
>>> --- a/arch/powerpc/platforms/ps3/device-init.c
>>>  
>>>  	result = ps3_start_probe_thread(PS3_BUS_TYPE_STORAGE);
>>> +	if (result < 0)
>>> +		return result;
>>
>> If you bail out here you skip:
>>
>>>  	ps3_register_vuart_devices();
>>
>> Which I suspect means there will be no console output?
>>
>> Presumably the system won't boot if the probe thread fails, but it might
>> at least print an oops, whereas if we return we might get nothing at
>> all. Though I'm just guessing, I don't know this code that well.
> 
> That probe is for the storage devices (PS3_BUS_TYPE_STORAGE).
> 
> There are cases where the system is usable even if the storage
> devices are not available, for example, when using an NFS root
> filesystem.
> 
> ps3_start_probe_thread was made to be quite verbose on error
> to make up for it's return value not being checked.

So should we delete the local variable 'result' and ignore the return value?
- result = ps3_start_probe_thread(PS3_BUS_TYPE_STORAGE);
- (void)ps3_start_probe_thread(PS3_BUS_TYPE_STORAGE);

> 
>> Anyway please leave this code alone unless you're willing to test your
>> changes, or at least provide a more thorough justification for them.
> 
> Agreed, this change should not be merged.
> 
> -Geoff
> 
> .
>