[v1] firmware: arm_scmi: Use dev_err_probe() simplify the code

[PATCH] firmware: arm_scmi: Use dev_err_probe() simplify the code

Posted by long.yunjian@zte.com.cn 7 months, 1 week ago

From: Yumeng Fang <fang.yumeng@zte.com.cn>

In the probe path, dev_err() can be replaced with dev_err_probe()
which will check if error code is -EPROBE_DEFER and prints the
error name. It also sets the defer probe reason which can be
checked later through debugfs.

Signed-off-by: Yumeng Fang <fang.yumeng@zte.com.cn>
---
 drivers/firmware/arm_scmi/transports/mailbox.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
index bd041c99b92b..816e79537935 100644
--- a/drivers/firmware/arm_scmi/transports/mailbox.c
+++ b/drivers/firmware/arm_scmi/transports/mailbox.c
@@ -13,6 +13,7 @@
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/dev_printk.h>

 #include "../common.h"

@@ -215,10 +216,8 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	smbox->chan = mbox_request_channel(cl, tx ? 0 : p2a_chan);
 	if (IS_ERR(smbox->chan)) {
 		ret = PTR_ERR(smbox->chan);
-		if (ret != -EPROBE_DEFER)
-			dev_err(cdev,
-				"failed to request SCMI %s mailbox\n", desc);
-		return ret;
+		return dev_err_probe(cdev, ret,
+				     "failed to request SCMI %s mailbox\n", desc);
 	}

 	/* Additional unidirectional channel for TX if needed */
@@ -226,9 +225,8 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 		smbox->chan_receiver = mbox_request_channel(cl, a2p_rx_chan);
 		if (IS_ERR(smbox->chan_receiver)) {
 			ret = PTR_ERR(smbox->chan_receiver);
-			if (ret != -EPROBE_DEFER)
-				dev_err(cdev, "failed to request SCMI Tx Receiver mailbox\n");
-			return ret;
+			return dev_err_probe(cdev, ret,
+					     "failed to request SCMI Tx Receiver mailbox\n");
 		}
 	}

@@ -236,9 +234,8 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 		smbox->chan_platform_receiver = mbox_request_channel(cl, p2a_rx_chan);
 		if (IS_ERR(smbox->chan_platform_receiver)) {
 			ret = PTR_ERR(smbox->chan_platform_receiver);
-			if (ret != -EPROBE_DEFER)
-				dev_err(cdev, "failed to request SCMI P2A Receiver mailbox\n");
-			return ret;
+			return dev_err_probe(cdev, ret,
+					     "failed to request SCMI P2A Receiver mailbox\n");
 		}
 	}

-- 
2.25.1

Re: [PATCH] firmware: arm_scmi: Use dev_err_probe() simplify the code

Posted by Christophe JAILLET 7 months, 1 week ago

Le 15/05/2025 à 14:38, long.yunjian@zte.com.cn a écrit :
> From: Yumeng Fang <fang.yumeng@zte.com.cn>
> 
> In the probe path, dev_err() can be replaced with dev_err_probe()
> which will check if error code is -EPROBE_DEFER and prints the
> error name. It also sets the defer probe reason which can be
> checked later through debugfs.
> 
> Signed-off-by: Yumeng Fang <fang.yumeng@zte.com.cn>
> ---
>   drivers/firmware/arm_scmi/transports/mailbox.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
> index bd041c99b92b..816e79537935 100644
> --- a/drivers/firmware/arm_scmi/transports/mailbox.c
> +++ b/drivers/firmware/arm_scmi/transports/mailbox.c
> @@ -13,6 +13,7 @@
>   #include <linux/of_address.h>
>   #include <linux/platform_device.h>
>   #include <linux/slab.h>
> +#include <linux/dev_printk.h>
> 

includes are (mostly) alphabetically ordered right-now.
So, It would be better to keep this logic, IMHO.

CJ

Re: [PATCH] firmware: arm_scmi: Use dev_err_probe() simplify the code

Posted by long.yunjian@zte.com.cn 7 months ago

> Dan Carpenter:
> It's probably better to get rid of the "ret = PTR_ERR(smbox->chan);"
> assignment as well.  Then it's a one liner:
>
>       if (IS_ERR(smbox->chan))
>         return dev_err_probe(cdev, PTR_ERR(smbox->chan),
>                      "failed to request SCMI %s mailbox\n", desc);
>
> Christophe Jaillet:
>>  #include <linux/of_address.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>> +#include <linux/dev_printk.h>
>
> includes are (mostly) alphabetically ordered right-now.
> So, It would be better to keep this logic, IMHO.


Dear Dan Carpenter and Christophe Jaillet,
Thank you both for your invaluable suggestions! 
I have thoroughly revised the code based on your feedback.
I will send the PATCH V2 and look forward to your further review.


Best regards,
Fang Yumeng

Original


From: christophe.jaillet <christophe.jaillet@wanadoo.fr>
To: Long Yunjian10171699;sudeep.holla <sudeep.holla@arm.com>;
Cc: cristian.marussi <cristian.marussi@arm.com>;peng.fan <peng.fan@nxp.com>;justin.chen <justin.chen@broadcom.com>;florian.fainelli <florian.fainelli@broadcom.com>;arm-scmi <arm-scmi@vger.kernel.org>;linux-arm-kernel <linux-arm-kernel@lists.infradead.org>;linux-kernel <linux-kernel@vger.kernel.org>;Fang Yumeng00336438;Mou Yi10205508;Ouyang Maochun10090504;Xu Lifeng10013465;
Date: 2025/05/16 23:26
Subject: Re: [PATCH] firmware: arm_scmi: Use dev_err_probe() simplify the code

Le 15/05/2025 à 14:38, long.yunjian@zte.com.cn a écrit :
> From: Yumeng Fang <fang.yumeng@zte.com.cn> 
>  
> In the probe path, dev_err() can be replaced with dev_err_probe()
> which will check if error code is -EPROBE_DEFER and prints the
> error name. It also sets the defer probe reason which can be
> checked later through debugfs.
>  
> Signed-off-by: Yumeng Fang <fang.yumeng@zte.com.cn> 
> ---
>   drivers/firmware/arm_scmi/transports/mailbox.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
>  
> diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
> index bd041c99b92b..816e79537935 100644
> --- a/drivers/firmware/arm_scmi/transports/mailbox.c
> +++ b/drivers/firmware/arm_scmi/transports/mailbox.c
> @@ -13,6 +13,7 @@
>   #include <linux/of_address.h> 
>   #include <linux/platform_device.h> 
>   #include <linux/slab.h> 
> +#include <linux/dev_printk.h> 
>  
 
includes are (mostly) alphabetically ordered right-now.
So, It would be better to keep this logic, IMHO.
 
CJ

Re: [PATCH] firmware: arm_scmi: Use dev_err_probe() simplify the code

Posted by Cristian Marussi 7 months, 1 week ago

On Thu, May 15, 2025 at 08:38:55PM +0800, long.yunjian@zte.com.cn wrote:
> From: Yumeng Fang <fang.yumeng@zte.com.cn>
> 

Hi,

> In the probe path, dev_err() can be replaced with dev_err_probe()
> which will check if error code is -EPROBE_DEFER and prints the
> error name. It also sets the defer probe reason which can be
> checked later through debugfs.

All true...but...if you look at the main scmi_probe() function all of these
failures are trapped at that level currently on the return path...

see the call chain from

scmi_probe()
	....
	ret = scmi_channels_setup(info); 
	...

...so your probe errors will be overridden there with a more generic message
left in debugfs at the top level.

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: Use dev_err_probe() simplify the code

Posted by Christophe JAILLET 7 months, 1 week ago

Le 15/05/2025 à 15:59, Cristian Marussi a écrit :
> On Thu, May 15, 2025 at 08:38:55PM +0800, long.yunjian@zte.com.cn wrote:
>> From: Yumeng Fang <fang.yumeng@zte.com.cn>
>>
> 
> Hi,
> 
>> In the probe path, dev_err() can be replaced with dev_err_probe()
>> which will check if error code is -EPROBE_DEFER and prints the
>> error name. It also sets the defer probe reason which can be
>> checked later through debugfs.
> 
> All true...but...if you look at the main scmi_probe() function all of these
> failures are trapped at that level currently on the return path...
> 
> see the call chain from
> 
> scmi_probe()
> 	....
> 	ret = scmi_channels_setup(info);
> 	...
> 
> ...so your probe errors will be overridden there with a more generic message
> left in debugfs at the top level.

This is only true only when -EPROBE_DEFER is returned.

In other cases, I think that we would get 2 messages. The specific one 
from scmi_channels_setup() and a generic one from scmi_probe().

in such a case, the one in scmi_channels_setup() will be better, because 
it will log the error code in a human readable format, which is not the 
case now.

So, I think that the patch:
   - simplify the code
   - improve the error messages in some cases

If -EPROBE_DEFER is returned, I think that the additional call would 
just but a harmless no-op.

CJ

> 
> Thanks,
> Cristian
> 
>

Re: [PATCH] firmware: arm_scmi: Use dev_err_probe() simplify the code

Posted by Dan Carpenter 7 months, 1 week ago

On Thu, May 15, 2025 at 02:59:24PM +0100, Cristian Marussi wrote:
> On Thu, May 15, 2025 at 08:38:55PM +0800, long.yunjian@zte.com.cn wrote:
> > From: Yumeng Fang <fang.yumeng@zte.com.cn>
> > 
> 
> Hi,
> 
> > In the probe path, dev_err() can be replaced with dev_err_probe()
> > which will check if error code is -EPROBE_DEFER and prints the
> > error name. It also sets the defer probe reason which can be
> > checked later through debugfs.
> 
> All true...but...if you look at the main scmi_probe() function all of these
> failures are trapped at that level currently on the return path...
> 
> see the call chain from
> 
> scmi_probe()
> 	....
> 	ret = scmi_channels_setup(info); 
> 	...
> 
> ...so your probe errors will be overridden there with a more generic message
> left in debugfs at the top level.

Good point.  But that feels like a mistake in dev_err_probe().
Ideally, it would print the first error message.  I bet someone
will eventually fix this.

regards,
dan carpenter

Re: [PATCH] firmware: arm_scmi: Use dev_err_probe() simplify the code

Posted by Dan Carpenter 7 months, 1 week ago

On Thu, May 15, 2025 at 08:38:55PM +0800, long.yunjian@zte.com.cn wrote:
> From: Yumeng Fang <fang.yumeng@zte.com.cn>
> 
> In the probe path, dev_err() can be replaced with dev_err_probe()
> which will check if error code is -EPROBE_DEFER and prints the
> error name. It also sets the defer probe reason which can be
> checked later through debugfs.
> 
> Signed-off-by: Yumeng Fang <fang.yumeng@zte.com.cn>

When you're resending someone else's patch you need to add your own
Signed-off-by: line to the end of the list.

Please, could you resend a v2 patch with your signature?

> ---
>  drivers/firmware/arm_scmi/transports/mailbox.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
> index bd041c99b92b..816e79537935 100644
> --- a/drivers/firmware/arm_scmi/transports/mailbox.c
> +++ b/drivers/firmware/arm_scmi/transports/mailbox.c
> @@ -13,6 +13,7 @@
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/dev_printk.h>
> 
>  #include "../common.h"
> 
> @@ -215,10 +216,8 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>  	smbox->chan = mbox_request_channel(cl, tx ? 0 : p2a_chan);
>  	if (IS_ERR(smbox->chan)) {
>  		ret = PTR_ERR(smbox->chan);
> -		if (ret != -EPROBE_DEFER)
> -			dev_err(cdev,
> -				"failed to request SCMI %s mailbox\n", desc);
> -		return ret;
> +		return dev_err_probe(cdev, ret,

It's probably better to get rid of the "ret = PTR_ERR(smbox->chan);"
assignment as well.  Then it's a one liner:

  	if (IS_ERR(smbox->chan))
		return dev_err_probe(cdev, PTR_ERR(smbox->chan),
				     "failed to request SCMI %s mailbox\n", desc);

Same for the others as well.

regards,
dan carpenter