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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8bb17c887eb9122d90e0887068b32da2@manjaro.org>
Date: Wed, 21 Aug 2024 11:34:41 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Heiko Stübner <heiko@...ech.de>
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

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

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