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

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

Reported-by: Hulk Robot <redacted>
Signed-off-by: Zhen Lei <redacted>
---
 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

Zhen Lei [off-list ref] 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.

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

Hi,

On 5/20/21 5:20 AM, Michael Ellerman wrote:

Zhen Lei [off-list ref] writes:

quoted

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

...

quoted

--- 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:

quoted

 	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

On 2021/5/24 4:15, Geoff Levand wrote:

Hi,

On 5/20/21 5:20 AM, Michael Ellerman wrote:

quoted

Zhen Lei [off-list ref] writes:

quoted

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

...

quoted

quoted

--- 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:

quoted

 	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);

quoted

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

.