[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6073817.31tnzDBltd@diego>
Date: Wed, 21 Aug 2024 11:17:06 +0200
From: Heiko Stübner <heiko@...ech.de>
To: Dragan Simic <dsimic@...jaro.org>
Cc: linux-rockchip@...ts.infradead.org, linux-phy@...ts.infradead.org,
vkoul@...nel.org, kishon@...nel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject:
Re: [PATCH v2 3/3] phy: phy-rockchip-inno-usb2: Improve error handling while
probing
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@...jaro.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
Powered by blists - more mailing lists