Message ID | 20231203091208.38360-1-liuhaoran14@163.com |
---|---|
State | Changes Requested |
Delegated to: | Andi Shyti |
Headers | show |
Series | [i2c] viperboard: Add error handling in vprbrd_i2c_probe | expand |
Hi Haoran, the patch makes sense, but... On Sun, Dec 03, 2023 at 01:12:08AM -0800, Haoran Liu wrote: > This patch introduces improved error handling for the i2c_add_adapter. > The necessity for this enhancement was identified through static analysis, > revealing that the function previously did not properly manage potential > failures of i2c_add_adapter. Prior to this patch, a failure in > i2c_add_adapter could result in partial initialization of the I2C adapter, > leading to unstable operation. Please use the imperative form. Instead of starting with 'This patch introduces...' you can write something like 'Check the return value of i2c_add_adapter...'. "Check" is in imperative form. Using the imperative form is crucial for a clear understanding of what the patch accomplishes. > Although the error addressed by this patch may not occur in the current > environment, I still suggest implementing these error handling routines > if the function is not highly time-sensitive. As the environment evolves > or the code gets reused in different contexts, there's a possibility that > these errors might occur. In case you find this addition unnecessary, I > completely understand and respect your perspective. My intention was to > enhance the robustness of the code, but I acknowledge that practical > considerations and current functionality might not warrant this change > at this point. The second paragraph does not describe anything functional related to the patch; it's a message to the reviewers. Please add it after the '---' section. > Signed-off-by: Haoran Liu <liuhaoran14@163.com> > --- > drivers/i2c/busses/i2c-viperboard.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-viperboard.c b/drivers/i2c/busses/i2c-viperboard.c > index 9e153b5b0e8e..13a07290ecf6 100644 > --- a/drivers/i2c/busses/i2c-viperboard.c > +++ b/drivers/i2c/busses/i2c-viperboard.c > @@ -400,7 +400,11 @@ static int vprbrd_i2c_probe(struct platform_device *pdev) > vb_i2c->i2c.dev.parent = &pdev->dev; > > /* attach to i2c layer */ > - i2c_add_adapter(&vb_i2c->i2c); > + ret = i2c_add_adapter(&vb_i2c->i2c); > + if (ret) { > + dev_err(&pdev->dev, "i2c_add_adapter failed: %d\n", ret); Please use: return dev_err_probe(&pdev->dev, ret, "..."); Additionally, I suggest rewording the error message to something that might be more meaningful for non-kernel developers, such as: "Failed to register the i2c adapter" Thanks, Andi > + return ret; > + } > > platform_set_drvdata(pdev, vb_i2c); > > -- > 2.17.1 >
diff --git a/drivers/i2c/busses/i2c-viperboard.c b/drivers/i2c/busses/i2c-viperboard.c index 9e153b5b0e8e..13a07290ecf6 100644 --- a/drivers/i2c/busses/i2c-viperboard.c +++ b/drivers/i2c/busses/i2c-viperboard.c @@ -400,7 +400,11 @@ static int vprbrd_i2c_probe(struct platform_device *pdev) vb_i2c->i2c.dev.parent = &pdev->dev; /* attach to i2c layer */ - i2c_add_adapter(&vb_i2c->i2c); + ret = i2c_add_adapter(&vb_i2c->i2c); + if (ret) { + dev_err(&pdev->dev, "i2c_add_adapter failed: %d\n", ret); + return ret; + } platform_set_drvdata(pdev, vb_i2c);
This patch introduces improved error handling for the i2c_add_adapter. The necessity for this enhancement was identified through static analysis, revealing that the function previously did not properly manage potential failures of i2c_add_adapter. Prior to this patch, a failure in i2c_add_adapter could result in partial initialization of the I2C adapter, leading to unstable operation. Although the error addressed by this patch may not occur in the current environment, I still suggest implementing these error handling routines if the function is not highly time-sensitive. As the environment evolves or the code gets reused in different contexts, there's a possibility that these errors might occur. In case you find this addition unnecessary, I completely understand and respect your perspective. My intention was to enhance the robustness of the code, but I acknowledge that practical considerations and current functionality might not warrant this change at this point. Signed-off-by: Haoran Liu <liuhaoran14@163.com> --- drivers/i2c/busses/i2c-viperboard.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)