media: atomisp: Add error check in create_host_regular_capture_pipeline()
Subsystems (4)
PATCH v2 1/1 media: atomisp: Add error check in create_host_regular_capture_pipeline()
When calling ia_css_pipeline_create_and_add_stage() function, there's no condition to check whether ia_css_pipeline_create_and_add_stage() throw an error or not. So add conditional check for the return value of ia_css_pipeline_create_and_add_stage() function.
This is reported by Coverity Scan with CID 1408952 as UNUSED_VALUE.
---
v1 -> v2: Add Fixes tag (suggested by Dan Carpenter).
v1:
https://lore.kernel.org/all/[email protected]/
I am not sure if omitting the error check is intentional or not, but from what I see from another ia_css_pipeline_create_and_add_stage() usage in the same function, we check the return value after the call and return the error code if it's non-zero.
I also don't have the device to test this change.
drivers/staging/media/atomisp/pci/sh_css.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/staging/media/atomisp/pci/sh_css.c +4 -0
@@ -7041,6 +7041,10 @@ create_host_regular_capture_pipeline(struct ia_css_pipe *pipe) err =ia_css_pipeline_create_and_add_stage (me, &stage_desc, ¤t_stage); +if (err) { +IA_CSS_LEAVE_ERR_PRIVATE (err); +return err; + } local_in_frame = current_stage->args.out_frame[0 ]; } err = add_capture_pp_stage(pipe, me, local_in_frame,
base-commit: 028ef9c96e96197026887c0f092424679298aae8
Signature · 1 line
2.53.0
Quoted 6 lines · Robertus Diawan Chris
When calling
ia_css_pipeline_create_and_add_stage()function, there's no condition to check whetheria_css_pipeline_create_and_add_stage()throw an error or not. So add conditional check for the return value ofia_css_pipeline_create_and_add_stage()function.This is reported by Coverity Scan with CID 1408952 as
UNUSED_VALUE.
Unneeded detail in the commit message. Use the comment block for that (as there changelog right now and other stuff you explained).
Quoted 5 lines
Fixes a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")Signed-off-by Robertus Diawan Chris <[email protected]>---
v1 -> v2: Add Fixes tag (suggested by Dan Carpenter).
Quoted 7 lines
v1:
https://lore.kernel.org/all/[email protected]/I am not sure if omitting the error check is intentional or not, but from what I see from another
ia_css_pipeline_create_and_add_stage()usage in the same function, we check the return value after the call and return the error code if it's non-zero.
Okay, while this observation sounds like a +1 to the suggested change, you still need to dive into the code and try to get if ia_css_pipeline_create_and_add_stage() failure is fatal or not for create_host_regular_capture_pipeline().
Quoted 1 line
I also don't have the device to test this change.
It's a pity as this driver is quite complicated thing...
Signature · 2 lines
With Best Regards, Andy Shevchenko
Hello Andy,
Quoted 10 lines · Andy Shevchenko
Quoted 6 lines · Robertus Diawan Chris
When calling
ia_css_pipeline_create_and_add_stage()function, there's no condition to check whetheria_css_pipeline_create_and_add_stage()throw an error or not. So add conditional check for the return value ofia_css_pipeline_create_and_add_stage()function.This is reported by Coverity Scan with CID 1408952 as
UNUSED_VALUE.Unneeded detail in the commit message. Use the comment block for that (as there changelog right now and other stuff you explained).
Alright, I will keep that in mind for future patches.
Quoted 13 lines
Quoted 5 lines
Fixes a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")Signed-off-by Robertus Diawan Chris <[email protected]>---
v1 -> v2: Add Fixes tag (suggested by Dan Carpenter).Quoted 7 lines
v1:
https://lore.kernel.org/all/[email protected]/I am not sure if omitting the error check is intentional or not, but from what I see from another
ia_css_pipeline_create_and_add_stage()usage in the same function, we check the return value after the call and return the error code if it's non-zero.
Quoted 4 lines
Okay, while this observation sounds like a +1 to the suggested change, you still need to dive into the code and try to get if
ia_css_pipeline_create_and_add_stage()failure is fatal or not forcreate_host_regular_capture_pipeline().
After diving further into the code, I am not confident enough to continue this patch. I lack the context and not sure if this _seemingly_ small change can have a big side effect for the overall driver.
With that in mind, I will not continue with this patch. I will try to take a look at another driver.
Quoted 3 lines
Quoted 1 line
I also don't have the device to test this change.
It's a pity as this driver is quite complicated thing...
Yeah... Let's just say it is _a sight to behold_ for newcomer like myself :)
Thank you for taking the time to check this patch.
Best regards,
Robertus Diawan Chris