| Message ID | 866445027a4f41165c872f494b04c218b6e67b09.1724225528.git.dsimic@manjaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve error handling in Rockchip Inno USB 2.0 PHY driver | expand |
Am Mittwoch, 21. August 2024, 09:37:55 CEST schrieb Dragan Simic: > Improve error handling in the probe path by using function dev_err_probe() > where appropriate, and by no longer using it rather pointlessly in one place > that actually produces a single, hardcoded error code. > > Signed-off-by: Dragan Simic <dsimic@manjaro.org> > @@ -1375,8 +1372,10 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev) > rphy->irq = platform_get_irq_optional(pdev, 0); > platform_set_drvdata(pdev, rphy); > > - if (!phy_cfgs) > - return dev_err_probe(dev, -EINVAL, "phy configs are not assigned!\n"); > + if (!phy_cfgs) { > + dev_err(dev, "phy configs are not assigned\n"); > + return -EINVAL; > + } > > ret = rockchip_usb2phy_extcon_register(rphy); > if (ret) I really don't understand the rationale here. Using dev_err_probe here is just fine and with that change you just introduce more lines of code for exactly the same functionality?
On 2024-08-21 10:44, Heiko Stübner wrote: > Am Mittwoch, 21. August 2024, 09:37:55 CEST schrieb Dragan Simic: >> Improve error handling in the probe path by using function >> dev_err_probe() >> where appropriate, and by no longer using it rather pointlessly in one >> place >> that actually produces a single, hardcoded error code. >> >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> > >> @@ -1375,8 +1372,10 @@ static int rockchip_usb2phy_probe(struct >> platform_device *pdev) >> rphy->irq = platform_get_irq_optional(pdev, 0); >> platform_set_drvdata(pdev, rphy); >> >> - if (!phy_cfgs) >> - return dev_err_probe(dev, -EINVAL, "phy configs are not >> assigned!\n"); >> + if (!phy_cfgs) { >> + dev_err(dev, "phy configs are not assigned\n"); >> + return -EINVAL; >> + } >> >> ret = rockchip_usb2phy_extcon_register(rphy); >> if (ret) > > I really don't understand the rationale here. Using dev_err_probe here > is just fine and with that change you just introduce more lines of code > for exactly the same functionality? As we know, dev_err_probe() decides how to log the received error message based on the error code it receives, but in this case the error code is hardcoded as -EINVAL. Thus, in this case it isn't about keeping the LoC count a bit lower, but about using dev_err() where the resulting outcome of error logging is aleady known, and where logging the error code actually isn't helpful, because it's hardcoded and the logged error message already tells everything about the error condition. In other words, it's about being as precise as possible when deciding between dev_err() and dev_err_probe(), in both directions. I hope it makes sense.
Am Mittwoch, 21. August 2024, 11:09:03 CEST schrieb Dragan Simic: > On 2024-08-21 10:44, Heiko Stübner wrote: > > Am Mittwoch, 21. August 2024, 09:37:55 CEST schrieb Dragan Simic: > >> Improve error handling in the probe path by using function > >> dev_err_probe() > >> where appropriate, and by no longer using it rather pointlessly in one > >> place > >> that actually produces a single, hardcoded error code. > >> > >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> > > > >> @@ -1375,8 +1372,10 @@ static int rockchip_usb2phy_probe(struct > >> platform_device *pdev) > >> rphy->irq = platform_get_irq_optional(pdev, 0); > >> platform_set_drvdata(pdev, rphy); > >> > >> - if (!phy_cfgs) > >> - return dev_err_probe(dev, -EINVAL, "phy configs are not > >> assigned!\n"); > >> + if (!phy_cfgs) { > >> + dev_err(dev, "phy configs are not assigned\n"); > >> + return -EINVAL; > >> + } > >> > >> ret = rockchip_usb2phy_extcon_register(rphy); > >> if (ret) > > > > I really don't understand the rationale here. Using dev_err_probe here > > is just fine and with that change you just introduce more lines of code > > for exactly the same functionality? > > As we know, dev_err_probe() decides how to log the received error > message > based on the error code it receives, but in this case the error code is > hardcoded as -EINVAL. Thus, in this case it isn't about keeping the LoC > count a bit lower, but about using dev_err() where the resulting outcome > of error logging is aleady known, and where logging the error code > actually > isn't helpful, because it's hardcoded and the logged error message > already > tells everything about the error condition. > > In other words, it's about being as precise as possible when deciding > between > dev_err() and dev_err_probe(), in both directions. I hope it makes > sense. I'd disagree a bit, using one format only creates a way nicer pattern in the driver, by not mixing different styles. dev_err_probe documentation seems to agree [0], by stating: "Using this helper in your probe function is totally fine even if @err is known to never be -EPROBE_DEFER. The benefit compared to a normal dev_err() is the standardized format of the error code, it being emitted symbolically (i.e. you get "EAGAIN" instead of "-35") and the fact that the error code is returned which allows more compact error paths." [0] https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/core.c#L5009
On 2024-08-21 11:17, Heiko Stübner wrote: > Am Mittwoch, 21. August 2024, 11:09:03 CEST schrieb Dragan Simic: >> On 2024-08-21 10:44, Heiko Stübner wrote: >> > Am Mittwoch, 21. August 2024, 09:37:55 CEST schrieb Dragan Simic: >> >> Improve error handling in the probe path by using function >> >> dev_err_probe() >> >> where appropriate, and by no longer using it rather pointlessly in one >> >> place >> >> that actually produces a single, hardcoded error code. >> >> >> >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >> > >> >> @@ -1375,8 +1372,10 @@ static int rockchip_usb2phy_probe(struct >> >> platform_device *pdev) >> >> rphy->irq = platform_get_irq_optional(pdev, 0); >> >> platform_set_drvdata(pdev, rphy); >> >> >> >> - if (!phy_cfgs) >> >> - return dev_err_probe(dev, -EINVAL, "phy configs are not >> >> assigned!\n"); >> >> + if (!phy_cfgs) { >> >> + dev_err(dev, "phy configs are not assigned\n"); >> >> + return -EINVAL; >> >> + } >> >> >> >> ret = rockchip_usb2phy_extcon_register(rphy); >> >> if (ret) >> > >> > I really don't understand the rationale here. Using dev_err_probe here >> > is just fine and with that change you just introduce more lines of code >> > for exactly the same functionality? >> >> As we know, dev_err_probe() decides how to log the received error >> message >> based on the error code it receives, but in this case the error code >> is >> hardcoded as -EINVAL. Thus, in this case it isn't about keeping the >> LoC >> count a bit lower, but about using dev_err() where the resulting >> outcome >> of error logging is aleady known, and where logging the error code >> actually >> isn't helpful, because it's hardcoded and the logged error message >> already >> tells everything about the error condition. >> >> In other words, it's about being as precise as possible when deciding >> between >> dev_err() and dev_err_probe(), in both directions. I hope it makes >> sense. > > I'd disagree a bit, using one format only creates a way nicer pattern > in the > driver, by not mixing different styles. > > dev_err_probe documentation seems to agree [0], by stating: > > "Using this helper in your probe function is totally fine even if @err > is > known to never be -EPROBE_DEFER. > The benefit compared to a normal dev_err() is the standardized format > of the error code, it being emitted symbolically (i.e. you get > "EAGAIN" > instead of "-35") and the fact that the error code is returned which > allows > more compact error paths." Yes, I saw that already in the documentation. Though, it might be debatable does hardcoding the passed error code to some value qualifies as knowing that it can't be -EPROBE_DEFER. The way I read that part of the documentation is that using dev_err_probe() is fine without going into the implementation of the previously invoked function that may fail, and researching can it actually return -EPROBE_DEFER or not. Also, the invoked function may change at some point in future and start returning -EPROBE_DEFER, but a hardcoded error code that's produced locally can't become changed that way. In addition to that, we already have at least a couple of instances [1][2] in the same function in which dev_err() is used when the error code is hardcoded, so there's actually already another pattern to follow. I know that replacing dev_err_probe() with dev_err() may look strange in a patch that mostly performs the opposite replacement, but the patch just tries to be strict and precise, and to follow other examples of how dev_err() is already used in the same function when the error code is produced locally instead of being received from another invoked function. > [0] > https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/core.c#L5009 [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/rockchip/phy-rockchip-inno-usb2.c?h=v6.11-rc4#n1361 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/rockchip/phy-rockchip-inno-usb2.c?h=v6.11-rc4#n1369
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c index 05af46dda11d..30b6134365ec 100644 --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c @@ -424,25 +424,22 @@ static int rockchip_usb2phy_extcon_register(struct rockchip_usb2phy *rphy) if (of_property_read_bool(node, "extcon")) { edev = extcon_get_edev_by_phandle(rphy->dev, 0); - if (IS_ERR(edev)) { - if (PTR_ERR(edev) != -EPROBE_DEFER) - dev_err(rphy->dev, "Invalid or missing extcon\n"); - return PTR_ERR(edev); - } + if (IS_ERR(edev)) + return dev_err_probe(rphy->dev, PTR_ERR(edev), + "invalid or missing extcon\n"); } else { /* Initialize extcon device */ edev = devm_extcon_dev_allocate(rphy->dev, rockchip_usb2phy_extcon_cable); if (IS_ERR(edev)) return dev_err_probe(rphy->dev, PTR_ERR(edev), "failed to allocate extcon device\n"); ret = devm_extcon_dev_register(rphy->dev, edev); - if (ret) { - dev_err(rphy->dev, "failed to register extcon device\n"); - return ret; - } + if (ret) + return dev_err_probe(rphy->dev, ret, + "failed to register extcon device\n"); } rphy->edev = edev; @@ -1375,8 +1372,10 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev) rphy->irq = platform_get_irq_optional(pdev, 0); platform_set_drvdata(pdev, rphy); - if (!phy_cfgs) - return dev_err_probe(dev, -EINVAL, "phy configs are not assigned!\n"); + if (!phy_cfgs) { + dev_err(dev, "phy configs are not assigned\n"); + return -EINVAL; + } ret = rockchip_usb2phy_extcon_register(rphy); if (ret) @@ -1407,10 +1406,8 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev) "failed to get phyclk\n"); ret = rockchip_usb2phy_clk480m_register(rphy); - if (ret) { - dev_err(dev, "failed to register 480m output clock\n"); - return ret; - } + if (ret) + return dev_err_probe(dev, ret, "failed to register 480m output clock\n"); if (rphy->phy_cfg->phy_tuning) { ret = rphy->phy_cfg->phy_tuning(rphy); @@ -1430,8 +1427,7 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev) phy = devm_phy_create(dev, child_np, &rockchip_usb2phy_ops); if (IS_ERR(phy)) { - dev_err_probe(dev, PTR_ERR(phy), "failed to create phy\n"); - ret = PTR_ERR(phy); + ret = dev_err_probe(dev, PTR_ERR(phy), "failed to create phy\n"); goto put_child; } @@ -1466,8 +1462,7 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev) "rockchip_usb2phy", rphy); if (ret) { - dev_err(rphy->dev, - "failed to request usb2phy irq handle\n"); + dev_err_probe(rphy->dev, ret, "failed to request usb2phy irq handle\n"); goto put_child; } }
Improve error handling in the probe path by using function dev_err_probe() where appropriate, and by no longer using it rather pointlessly in one place that actually produces a single, hardcoded error code. Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 33 ++++++++----------- 1 file changed, 14 insertions(+), 19 deletions(-)