[PATCH v2 0/3] Improve error handling in Rockchip Inno USB 2.0 PHY driver

[PATCH v2 0/3] Improve error handling in Rockchip Inno USB 2.0 PHY driver

[Dragan Simic]

This is a small series that improves error handling in the probe path
of the Rockchip Innosilicon USB 2.0 PHY driver, by returning the actual
error code in one place and by using dev_err_probe() properly in multiple
places.  It also performs a bunch of small, rather trivial code cleanups,
to make the code neater and a bit easier to read.

Changes in v2:
  - Expanded into a small series, after a suggestion from Heiko [1] to
    use dev_err_probe(), because it all happens in the probe path

Link to v1: https://lore.kernel.org/linux-phy/5fa7796d71e2f46344e972bc98a54539f55b6109.1723551599.git.dsimic@manjaro.org/T/#u

[1] https://lore.kernel.org/linux-phy/4927264.xgNZFEDtJV@diego/

Dragan Simic (3):
  phy: phy-rockchip-inno-usb2: Perform trivial code cleanups
  phy: phy-rockchip-inno-usb2: Handle failed extcon allocation better
  phy: phy-rockchip-inno-usb2: Improve error handling while probing

 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 63 ++++++++-----------
 1 file changed, 25 insertions(+), 38 deletions(-)


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 1/3] phy: phy-rockchip-inno-usb2: Perform trivial code cleanups

[Dragan Simic]

Perform a few trivial code cleanups, e.g. to obey the reverse Christmas tree
rule, to avoid use of "{ ... }" code blocks where they aren't really needed,
or to avoid line wrapping by using the 100-column width better.

No intended functional changes are introduced by these code cleanups.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 27 +++++++------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 4f71373ae6e1..113bfc717ff0 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -418,9 +418,9 @@ rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
 
 static int rockchip_usb2phy_extcon_register(struct rockchip_usb2phy *rphy)
 {
-	int ret;
 	struct device_node *node = rphy->dev->of_node;
 	struct extcon_dev *edev;
+	int ret;
 
 	if (of_property_read_bool(node, "extcon")) {
 		edev = extcon_get_edev_by_phandle(rphy->dev, 0);
@@ -445,7 +445,6 @@ static int rockchip_usb2phy_extcon_register(struct rockchip_usb2phy *rphy)
 	}
 
 	rphy->edev = edev;
-
 	return 0;
 }
 
@@ -1327,21 +1326,19 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
 	struct rockchip_usb2phy *rphy;
 	const struct rockchip_usb2phy_cfg *phy_cfgs;
 	unsigned int reg;
-	int index, ret;
+	int index = 0, ret;
 
 	rphy = devm_kzalloc(dev, sizeof(*rphy), GFP_KERNEL);
 	if (!rphy)
 		return -ENOMEM;
 
 	if (!dev->parent || !dev->parent->of_node) {
 		rphy->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,usbgrf");
 		if (IS_ERR(rphy->grf)) {
 			dev_err(dev, "failed to locate usbgrf\n");
 			return PTR_ERR(rphy->grf);
 		}
-	}
-
-	else {
+	} else {
 		rphy->grf = syscon_node_to_regmap(dev->parent->of_node);
 		if (IS_ERR(rphy->grf))
 			return PTR_ERR(rphy->grf);
@@ -1358,16 +1355,14 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
 	}
 
 	if (of_property_read_u32_index(np, "reg", 0, &reg)) {
-		dev_err(dev, "the reg property is not assigned in %pOFn node\n",
-			np);
+		dev_err(dev, "the reg property is not assigned in %pOFn node\n", np);
 		return -EINVAL;
 	}
 
 	/* support address_cells=2 */
 	if (of_property_count_u32_elems(np, "reg") > 2 && reg == 0) {
 		if (of_property_read_u32_index(np, "reg", 1, &reg)) {
-			dev_err(dev, "the reg property is not assigned in %pOFn node\n",
-				np);
+			dev_err(dev, "the reg property is not assigned in %pOFn node\n", np);
 			return -EINVAL;
 		}
 	}
@@ -1386,8 +1381,7 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/* find out a proper config which can be matched with dt. */
-	index = 0;
+	/* find a proper config that can be matched with the DT */
 	do {
 		if (phy_cfgs[index].reg == reg) {
 			rphy->phy_cfg = &phy_cfgs[index];
@@ -1407,10 +1401,9 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
 		return PTR_ERR(rphy->phy_reset);
 
 	rphy->clk = devm_clk_get_optional_enabled(dev, "phyclk");
-	if (IS_ERR(rphy->clk)) {
+	if (IS_ERR(rphy->clk))
 		return dev_err_probe(&pdev->dev, PTR_ERR(rphy->clk),
 				     "failed to get phyclk\n");
-	}
 
 	ret = rockchip_usb2phy_clk480m_register(rphy);
 	if (ret) {
@@ -1446,13 +1439,11 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
 
 		/* initialize otg/host port separately */
 		if (of_node_name_eq(child_np, "host-port")) {
-			ret = rockchip_usb2phy_host_port_init(rphy, rport,
-							      child_np);
+			ret = rockchip_usb2phy_host_port_init(rphy, rport, child_np);
 			if (ret)
 				goto put_child;
 		} else {
-			ret = rockchip_usb2phy_otg_port_init(rphy, rport,
-							     child_np);
+			ret = rockchip_usb2phy_otg_port_init(rphy, rport, child_np);
 			if (ret)
 				goto put_child;
 		}

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/3] phy: phy-rockchip-inno-usb2: Perform trivial code cleanups

[Heiko Stübner]

Am Mittwoch, 21. August 2024, 09:37:53 CEST schrieb Dragan Simic:
> Perform a few trivial code cleanups, e.g. to obey the reverse Christmas tree
> rule, to avoid use of "{ ... }" code blocks where they aren't really needed,
> or to avoid line wrapping by using the 100-column width better.
> 
> No intended functional changes are introduced by these code cleanups.
> 
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>

> @@ -445,7 +445,6 @@ static int rockchip_usb2phy_extcon_register(struct rockchip_usb2phy *rphy)
>  	}
>  
>  	rphy->edev = edev;
> -
>  	return 0;
>  }

depending on how nitpicky we want to be, I'm not very fond of this change.
Assigning the extcon-dev and returning "0" are two different actions, and
I think most drivers do actually use a blank line between those.

But that really is just a style preference for me, so anyway

Reviewed-by: Heiko Stuebner <heiko@sntech.de>



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/3] phy: phy-rockchip-inno-usb2: Perform trivial code cleanups

[Dragan Simic]

Hello Heiko,

On 2024-08-21 10:39, Heiko Stübner wrote:
> Am Mittwoch, 21. August 2024, 09:37:53 CEST schrieb Dragan Simic:
>> Perform a few trivial code cleanups, e.g. to obey the reverse 
>> Christmas tree
>> rule, to avoid use of "{ ... }" code blocks where they aren't really 
>> needed,
>> or to avoid line wrapping by using the 100-column width better.
>> 
>> No intended functional changes are introduced by these code cleanups.
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> 
>> @@ -445,7 +445,6 @@ static int rockchip_usb2phy_extcon_register(struct 
>> rockchip_usb2phy *rphy)
>>  	}
>> 
>>  	rphy->edev = edev;
>> -
>>  	return 0;
>>  }
> 
> depending on how nitpicky we want to be, I'm not very fond of this 
> change.
> Assigning the extcon-dev and returning "0" are two different actions, 
> and
> I think most drivers do actually use a blank line between those.
> 
> But that really is just a style preference for me, so anyway
> 
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Thanks for your review; I had similar thoughts, believe it or not. :)  
If
there will be v3, I'll bring the empty separator line back.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 2/3] phy: phy-rockchip-inno-usb2: Handle failed extcon allocation better

[Dragan Simic]

Return the actual error code upon failure to allocate extcon device, instead
of hardcoding -ENOMEM.  Use dev_err_probe() to also log appropriate messages,
which is fine because the containing function is used in the probe path.

Helped-by: Heiko Stubner <heiko@sntech.de>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 113bfc717ff0..05af46dda11d 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -435,7 +435,8 @@ static int rockchip_usb2phy_extcon_register(struct rockchip_usb2phy *rphy)
 						rockchip_usb2phy_extcon_cable);
 
 		if (IS_ERR(edev))
-			return -ENOMEM;
+			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) {

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/3] phy: phy-rockchip-inno-usb2: Handle failed extcon allocation better

[Heiko Stübner]

Am Mittwoch, 21. August 2024, 09:37:54 CEST schrieb Dragan Simic:
> Return the actual error code upon failure to allocate extcon device, instead
> of hardcoding -ENOMEM.  Use dev_err_probe() to also log appropriate messages,
> which is fine because the containing function is used in the probe path.
> 
> Helped-by: Heiko Stubner <heiko@sntech.de>

How did I help with this? :-D

Reviewed-by: Heiko Stuebner <heiko@sntech.de>





-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/3] phy: phy-rockchip-inno-usb2: Handle failed extcon allocation better

[Dragan Simic]

On 2024-08-21 10:41, Heiko Stübner wrote:
> Am Mittwoch, 21. August 2024, 09:37:54 CEST schrieb Dragan Simic:
>> Return the actual error code upon failure to allocate extcon device, 
>> instead
>> of hardcoding -ENOMEM.  Use dev_err_probe() to also log appropriate 
>> messages,
>> which is fine because the containing function is used in the probe 
>> path.
>> 
>> Helped-by: Heiko Stubner <heiko@sntech.de>
> 
> How did I help with this? :-D
> 
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Thanks for your review.  Well, you suggested that dev_err_probe() is 
used. :)

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/3] phy: phy-rockchip-inno-usb2: Handle failed extcon allocation better

[Christophe JAILLET]

Le 21/08/2024 à 09:37, Dragan Simic a écrit :
> Return the actual error code upon failure to allocate extcon device, instead
> of hardcoding -ENOMEM.  Use dev_err_probe() to also log appropriate messages,
> which is fine because the containing function is used in the probe path.
> 
> Helped-by: Heiko Stubner <heiko@sntech.de>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>   drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 113bfc717ff0..05af46dda11d 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -435,7 +435,8 @@ static int rockchip_usb2phy_extcon_register(struct rockchip_usb2phy *rphy)
>   						rockchip_usb2phy_extcon_cable);
>   
>   		if (IS_ERR(edev))
> -			return -ENOMEM;
> +			return dev_err_probe(rphy->dev, PTR_ERR(edev),
> +					     "failed to allocate extcon device\n");

Returning PTR_ERR(edev) may make sense, but I don't think that adding a 
dev_err_probe() really helps.

devm_extcon_dev_allocate() can only return -EINVAL if it 2nd argument is 
NULL. It is trivial to see that it can't happen here, 
rockchip_usb2phy_extcon_cable is a global variable.

in all other cases, it returns -ENOMEM because of a failed memory 
allocation. In this case, usually it is not needed to log anything 
because it is already loud enough.

Just my 2c.

CJ


>   
>   		ret = devm_extcon_dev_register(rphy->dev, edev);
>   		if (ret) {
> 
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/3] phy: phy-rockchip-inno-usb2: Handle failed extcon allocation better

[Dragan Simic]

Hello Christophe,

On 2024-08-21 13:17, Christophe JAILLET wrote:
> Le 21/08/2024 à 09:37, Dragan Simic a écrit :
>> Return the actual error code upon failure to allocate extcon device, 
>> instead
>> of hardcoding -ENOMEM.  Use dev_err_probe() to also log appropriate 
>> messages,
>> which is fine because the containing function is used in the probe 
>> path.
>> 
>> Helped-by: Heiko Stubner <heiko@sntech.de>
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>   drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
>> b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>> index 113bfc717ff0..05af46dda11d 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>> @@ -435,7 +435,8 @@ static int rockchip_usb2phy_extcon_register(struct 
>> rockchip_usb2phy *rphy)
>>   						rockchip_usb2phy_extcon_cable);
>>     		if (IS_ERR(edev))
>> -			return -ENOMEM;
>> +			return dev_err_probe(rphy->dev, PTR_ERR(edev),
>> +					     "failed to allocate extcon device\n");
> 
> Returning PTR_ERR(edev) may make sense, but I don't think that adding
> a dev_err_probe() really helps.
> 
> devm_extcon_dev_allocate() can only return -EINVAL if it 2nd argument
> is NULL. It is trivial to see that it can't happen here,
> rockchip_usb2phy_extcon_cable is a global variable.
> 
> in all other cases, it returns -ENOMEM because of a failed memory
> allocation. In this case, usually it is not needed to log anything
> because it is already loud enough.

True, using dev_err_probe() in this case is somewhat redundant,
but it falls under the "still fine to use" category, [1] because
the error code passed to dev_err_probe() is received from another
function that was invoked.

On the other hand, another patch in this series tries to be strict
in another direction, by not using dev_err_probe() where the error
code passed to it is basically hardcoded. [2]

I hope this makes sense.

[1] 
https://lore.kernel.org/linux-phy/cover.1724225528.git.dsimic@manjaro.org/T/#mc3af7d24e31ed885732e6f26ca0d107b157d478b
[2] 
https://lore.kernel.org/linux-phy/cover.1724225528.git.dsimic@manjaro.org/T/#ma26b614412787814dab7923987b8c814f7e86beb

>>     		ret = devm_extcon_dev_register(rphy->dev, edev);
>>   		if (ret) {
>> 
>> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[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>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 33 ++++++++-----------
 1 file changed, 14 insertions(+), 19 deletions(-)

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;
 		}
 	}

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Heiko Stübner]

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?



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[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.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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




-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Dragan Simic]

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy