Re: [PATCH v2 3/3] phy: phy-rockchip-inno-usb2: Improve error handling while probing

[Heiko Stübner]

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@xxxxxxxxxxx>
> > 
> >> @@ -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