On 2024-08-21 11:17, Heiko Stübner wrote:

Am Mittwoch, 21. August 2024, 11:09:03 CEST schrieb Dragan Simic:

quoted

On 2024-08-21 10:44, Heiko Stübner wrote:

quoted

Am Mittwoch, 21. August 2024, 09:37:55 CEST schrieb Dragan Simic:

quoted

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

quoted

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