[PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[pavankumaryalagada]

From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>

Use %pe to print error pointers directly and display their
error names.

Keep the other PTR_ERR() call because its return value is
needed for ret.

Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
---
v2:
- Use %pe in the firmware management case, as suggested by Jeremy Clifton,
  to match the other cases.

v1: https://lore.kernel.org/outreachy/as0nhVbagZa8Birm@user/T/#t
---
 drivers/staging/greybus/fw-core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/greybus/fw-core.c b/drivers/staging/greybus/fw-core.c
index 2016a74f137f..78321be80013 100644
--- a/drivers/staging/greybus/fw-core.c
+++ b/drivers/staging/greybus/fw-core.c
@@ -93,8 +93,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
 			if (IS_ERR(connection)) {
 				ret = PTR_ERR(connection);
 				dev_err(&bundle->dev,
-					"failed to create management connection (%d)\n",
-					ret);
+					"failed to create management connection (%pe)\n",
+					connection);
 				goto err_destroy_connections;
 			}
 
@@ -112,8 +112,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
 			connection = gb_connection_create(bundle, cport_id,
 							  gb_fw_download_request_handler);
 			if (IS_ERR(connection)) {
-				dev_err(&bundle->dev, "failed to create download connection (%ld)\n",
-					PTR_ERR(connection));
+				dev_err(&bundle->dev, "failed to create download connection (%pe)\n",
+					connection);
 			} else {
 				fw_core->download_connection = connection;
 			}
@@ -131,8 +131,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
 			connection = gb_connection_create(bundle, cport_id,
 							  NULL);
 			if (IS_ERR(connection)) {
-				dev_err(&bundle->dev, "failed to create SPI connection (%ld)\n",
-					PTR_ERR(connection));
+				dev_err(&bundle->dev, "failed to create SPI connection (%pe)\n",
+					connection);
 			} else {
 				fw_core->spi_connection = connection;
 			}
@@ -149,8 +149,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
 			connection = gb_connection_create(bundle, cport_id,
 							  NULL);
 			if (IS_ERR(connection)) {
-				dev_err(&bundle->dev, "failed to create Authentication connection (%ld)\n",
-					PTR_ERR(connection));
+				dev_err(&bundle->dev, "failed to create Authentication connection (%pe)\n",
+					connection);
 			} else {
 				fw_core->cap_connection = connection;
 			}
-- 
2.43.0

Re: [PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[Alex Elder]

On 10/12/26 9:24 PM, pavankumaryalagada@gmail.com wrote:
> From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> 
> Use %pe to print error pointers directly and display their
> error names.
> 
> Keep the other PTR_ERR() call because its return value is
> needed for ret.
> 
> Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>


Generally I think what you're doing is OK.  It looks
like %pe is better than just %d, because it prints
the error *name* for the pointer-coded error.

But I have two general questions about this effort:

- There are other places in the Greybus code (even in
   just the staging source files) where this exact same
   thing could be done.  Why limit this to "fw-core.c" only?

- Do you think that all Greybus error messages should be
   converted to use %pe (passing ERR_PTR() if necessary)
   instead of %d for showing the error number?

I think your suggested change is a net positive, but
doing it in some spots but not others means it is
adding a bit of inconsistency.

					-Alex

> ---
> v2:
> - Use %pe in the firmware management case, as suggested by Jeremy Clifton,
>    to match the other cases.
> 
> v1: https://lore.kernel.org/outreachy/as0nhVbagZa8Birm@user/T/#t
> ---
>   drivers/staging/greybus/fw-core.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/greybus/fw-core.c b/drivers/staging/greybus/fw-core.c
> index 2016a74f137f..78321be80013 100644
> --- a/drivers/staging/greybus/fw-core.c
> +++ b/drivers/staging/greybus/fw-core.c
> @@ -93,8 +93,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
>   			if (IS_ERR(connection)) {
>   				ret = PTR_ERR(connection);
>   				dev_err(&bundle->dev,
> -					"failed to create management connection (%d)\n",
> -					ret);
> +					"failed to create management connection (%pe)\n",
> +					connection);
>   				goto err_destroy_connections;
>   			}
>   
> @@ -112,8 +112,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
>   			connection = gb_connection_create(bundle, cport_id,
>   							  gb_fw_download_request_handler);
>   			if (IS_ERR(connection)) {
> -				dev_err(&bundle->dev, "failed to create download connection (%ld)\n",
> -					PTR_ERR(connection));
> +				dev_err(&bundle->dev, "failed to create download connection (%pe)\n",
> +					connection);
>   			} else {
>   				fw_core->download_connection = connection;
>   			}
> @@ -131,8 +131,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
>   			connection = gb_connection_create(bundle, cport_id,
>   							  NULL);
>   			if (IS_ERR(connection)) {
> -				dev_err(&bundle->dev, "failed to create SPI connection (%ld)\n",
> -					PTR_ERR(connection));
> +				dev_err(&bundle->dev, "failed to create SPI connection (%pe)\n",
> +					connection);
>   			} else {
>   				fw_core->spi_connection = connection;
>   			}
> @@ -149,8 +149,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
>   			connection = gb_connection_create(bundle, cport_id,
>   							  NULL);
>   			if (IS_ERR(connection)) {
> -				dev_err(&bundle->dev, "failed to create Authentication connection (%ld)\n",
> -					PTR_ERR(connection));
> +				dev_err(&bundle->dev, "failed to create Authentication connection (%pe)\n",
> +					connection);
>   			} else {
>   				fw_core->cap_connection = connection;
>   			}

Re: [PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[Yalagada Pavan Kumar]

On Tue, Oct 13, 2026 at 03:11:51PM -0500, Alex Elder wrote:
> On 10/12/26 9:24 PM, pavankumaryalagada@gmail.com wrote:
> > From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> > 
> > Use %pe to print error pointers directly and display their
> > error names.
> > 
> > Keep the other PTR_ERR() call because its return value is
> > needed for ret.
> > 
> > Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> 
> 
> Generally I think what you're doing is OK.  It looks
> like %pe is better than just %d, because it prints
> the error *name* for the pointer-coded error.
> 
> But I have two general questions about this effort:
> 
> - There are other places in the Greybus code (even in
>   just the staging source files) where this exact same
>   thing could be done.  Why limit this to "fw-core.c" only?

I initially limited the change to "fw-core.c" because I was
focusing on the error messages in that file.

> 
> - Do you think that all Greybus error messages should be
>   converted to use %pe (passing ERR_PTR() if necessary)
>   instead of %d for showing the error number?

Using %pe prints the error name instead of just the
error number, which makes the error messages easier
to understand.

For example, the current message is:

	failed to create download connection (-12)

with %pe, it becomes:

	failed to create download connection (-ENOMEM)

I think the second message is easier to understand because
it shows the error name instead of just error number.

I also found other PTR_ERR() calls in Greybus. some of them
are used to return error codes and should remain unchanged.

For example, in fw-core.c:

	if (IS_ERR(connection)) {
		ret = PTR_ERR(connection);
		dev_err(&bundle->dev,
			"failed to create management connection (%pe)\n",
			connection);
		goto err_destroy_connections;
	}

Here, PTR_ERR(connection) is needed to store the error code in ret
for the error-handling path, while passing connection to %pe makes
the error message more readable.

> 
> I think your suggested change is a net positive, but
> doing it in some spots but not others means it is
> adding a bit of inconsistency.

I agree that similar cases elsewhere in Greybus are worth
checking to keep error reporting consistent. I will look
at the other Greybus files for cases where using %pe would
be appropriate.

Would you prefer me to extend this change to the other
relevant Greybus files, or keep this patch focused on
fw-core.c and submit a separate patch for the remaining
changes?

Thanks for the suggestions.

Pavan
> 
> 					-Alex
> 
> > ---
> > v2:
> > - Use %pe in the firmware management case, as suggested by Jeremy Clifton,
> >    to match the other cases.
> > 
> > v1: https://lore.kernel.org/outreachy/as0nhVbagZa8Birm@user/T/#t
> > ---
> >   drivers/staging/greybus/fw-core.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/fw-core.c b/drivers/staging/greybus/fw-core.c
> > index 2016a74f137f..78321be80013 100644
> > --- a/drivers/staging/greybus/fw-core.c
> > +++ b/drivers/staging/greybus/fw-core.c
> > @@ -93,8 +93,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
> >   			if (IS_ERR(connection)) {
> >   				ret = PTR_ERR(connection);
> >   				dev_err(&bundle->dev,
> > -					"failed to create management connection (%d)\n",
> > -					ret);
> > +					"failed to create management connection (%pe)\n",
> > +					connection);
> >   				goto err_destroy_connections;
> >   			}
> > @@ -112,8 +112,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
> >   			connection = gb_connection_create(bundle, cport_id,
> >   							  gb_fw_download_request_handler);
> >   			if (IS_ERR(connection)) {
> > -				dev_err(&bundle->dev, "failed to create download connection (%ld)\n",
> > -					PTR_ERR(connection));
> > +				dev_err(&bundle->dev, "failed to create download connection (%pe)\n",
> > +					connection);
> >   			} else {
> >   				fw_core->download_connection = connection;
> >   			}
> > @@ -131,8 +131,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
> >   			connection = gb_connection_create(bundle, cport_id,
> >   							  NULL);
> >   			if (IS_ERR(connection)) {
> > -				dev_err(&bundle->dev, "failed to create SPI connection (%ld)\n",
> > -					PTR_ERR(connection));
> > +				dev_err(&bundle->dev, "failed to create SPI connection (%pe)\n",
> > +					connection);
> >   			} else {
> >   				fw_core->spi_connection = connection;
> >   			}
> > @@ -149,8 +149,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle,
> >   			connection = gb_connection_create(bundle, cport_id,
> >   							  NULL);
> >   			if (IS_ERR(connection)) {
> > -				dev_err(&bundle->dev, "failed to create Authentication connection (%ld)\n",
> > -					PTR_ERR(connection));
> > +				dev_err(&bundle->dev, "failed to create Authentication connection (%pe)\n",
> > +					connection);
> >   			} else {
> >   				fw_core->cap_connection = connection;
> >   			}
> 

Re: [PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[Alex Elder]

On 10/13/26 4:43 PM, Yalagada Pavan Kumar wrote:
>> I think your suggested change is a net positive, but
>> doing it in some spots but not others means it is
>> adding a bit of inconsistency.
> I agree that similar cases elsewhere in Greybus are worth
> checking to keep error reporting consistent. I will look
> at the other Greybus files for cases where using %pe would
> be appropriate.
> 
> Would you prefer me to extend this change to the other
> relevant Greybus files, or keep this patch focused on
> fw-core.c and submit a separate patch for the remaining
> changes?

I guess I mainly wanted to understand your intent.  I don't
really mind your proposed change, but I would really prefer
to have it be part of an overall plan to upgrade all error
messages this way.

Ideally error (and other) messages are done consistently,
and so changing them in just one file sort of raises a flag
for me.

Can you survey the messages elsewhere in the Greybus code
and get a sense of how much work it will be to do this
comprehensively?  Your current patch affects four spots in
one source file; how many others would there be?

And if updating everything is a task you're willing to do,
then I think I would prefer you extend this change.

					-Alex

Re: [PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[Yalagada Pavan Kumar]

On Wed, Oct 14, 2026 at 08:32:54AM -0500, Alex Elder wrote:
> On 10/13/26 4:43 PM, Yalagada Pavan Kumar wrote:
> > > I think your suggested change is a net positive, but
> > > doing it in some spots but not others means it is
> > > adding a bit of inconsistency.
> > I agree that similar cases elsewhere in Greybus are worth
> > checking to keep error reporting consistent. I will look
> > at the other Greybus files for cases where using %pe would
> > be appropriate.
> > 
> > Would you prefer me to extend this change to the other
> > relevant Greybus files, or keep this patch focused on
> > fw-core.c and submit a separate patch for the remaining
> > changes?
> 
> I guess I mainly wanted to understand your intent.  I don't
> really mind your proposed change, but I would really prefer
> to have it be part of an overall plan to upgrade all error
> messages this way.

I checked all Greybus source files to identify other places where
%pe could be used.

> 
> Ideally error (and other) messages are done consistently,
> and so changing them in just one file sort of raises a flag
> for me.

I agree that keeping error messages consistent across Greybus.

> 
> Can you survey the messages elsewhere in the Greybus code
> and get a sense of how much work it will be to do this
> comprehensively?  Your current patch affects four spots in
> one source file; how many others would there be?

I found 12 additional cases in arche-apb-ctrl.c and arche-platform.c
where using %pe would be appropriate. I also found one additional
case in uart.c:954 where the error message could be improved by
printing the error name.

I will keep PTR_ERR() calls that are needed to return error codes
unchanged.

> 
> And if updating everything is a task you're willing to do,
> then I think I would prefer you extend this change.

I am willing to make these additional changes. Since my original patch
is already in staging-next, should i send a v3 of the existing patch
or submit a new patch covering all the relevant cases?

Thanks for your guidance.

Pavan

> 
> 					-Alex

Re: [PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[Jeremy Dean Clifton]

On 10/13/26 3:43 PM, Yalagada Pavan Kumar wrote:
> On Tue, Oct 13, 2026 at 03:11:51PM -0500, Alex Elder wrote:
>> On 10/12/26 9:24 PM, pavankumaryalagada@gmail.com wrote:
>>> From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
>>>
>>> Use %pe to print error pointers directly and display their
>>> error names.
>>>
>>> Keep the other PTR_ERR() call because its return value is
>>> needed for ret.
>>>
>>> Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
>>
>>
>> Generally I think what you're doing is OK.  It looks
>> like %pe is better than just %d, because it prints
>> the error *name* for the pointer-coded error.
>>
>> But I have two general questions about this effort:
>>
>> - There are other places in the Greybus code (even in
>>    just the staging source files) where this exact same
>>    thing could be done.  Why limit this to "fw-core.c" only?
> 
> I initially limited the change to "fw-core.c" because I was
> focusing on the error messages in that file.
> 
>>
>> - Do you think that all Greybus error messages should be
>>    converted to use %pe (passing ERR_PTR() if necessary)
>>    instead of %d for showing the error number?
> 
> Using %pe prints the error name instead of just the
> error number, which makes the error messages easier
> to understand.
> 
> For example, the current message is:
> 
> 	failed to create download connection (-12)
> 
> with %pe, it becomes:
> 
> 	failed to create download connection (-ENOMEM)
> 
> I think the second message is easier to understand because
> it shows the error name instead of just error number.
> 
> I also found other PTR_ERR() calls in Greybus. some of them
> are used to return error codes and should remain unchanged.
> 
> For example, in fw-core.c:
> 
> 	if (IS_ERR(connection)) {
> 		ret = PTR_ERR(connection);
> 		dev_err(&bundle->dev,
> 			"failed to create management connection (%pe)\n",
> 			connection);
> 		goto err_destroy_connections;
> 	}
> 
> Here, PTR_ERR(connection) is needed to store the error code in ret
> for the error-handling path, while passing connection to %pe makes
> the error message more readable.
> 
>>
>> I think your suggested change is a net positive, but
>> doing it in some spots but not others means it is
>> adding a bit of inconsistency.
> 
> I agree that similar cases elsewhere in Greybus are worth
> checking to keep error reporting consistent. I will look
> at the other Greybus files for cases where using %pe would
> be appropriate.
> 
> Would you prefer me to extend this change to the other
> relevant Greybus files, or keep this patch focused on
> fw-core.c and submit a separate patch for the remaining
> changes?
Also, you could coordinate something with Julia to use Coccinelle for 
this. Using regular expressions, you could programmatically find all the 
places where %pe and connection can be in place of %d or %ld and the 
return from PTR_ERR().

Jeremy Clifton

Re: [PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[Julia Lawall]

On Wed, 14 Oct 2026, Yalagada Pavan Kumar wrote:

> On Wed, Oct 14, 2026 at 08:32:54AM -0500, Alex Elder wrote:
> > On 10/13/26 4:43 PM, Yalagada Pavan Kumar wrote:
> > > > I think your suggested change is a net positive, but
> > > > doing it in some spots but not others means it is
> > > > adding a bit of inconsistency.
> > > I agree that similar cases elsewhere in Greybus are worth
> > > checking to keep error reporting consistent. I will look
> > > at the other Greybus files for cases where using %pe would
> > > be appropriate.
> > >
> > > Would you prefer me to extend this change to the other
> > > relevant Greybus files, or keep this patch focused on
> > > fw-core.c and submit a separate patch for the remaining
> > > changes?
> >
> > I guess I mainly wanted to understand your intent.  I don't
> > really mind your proposed change, but I would really prefer
> > to have it be part of an overall plan to upgrade all error
> > messages this way.
>
> I checked all Greybus source files to identify other places where
> %pe could be used.
>
> >
> > Ideally error (and other) messages are done consistently,
> > and so changing them in just one file sort of raises a flag
> > for me.
>
> I agree that keeping error messages consistent across Greybus.
>
> >
> > Can you survey the messages elsewhere in the Greybus code
> > and get a sense of how much work it will be to do this
> > comprehensively?  Your current patch affects four spots in
> > one source file; how many others would there be?
>
> I found 12 additional cases in arche-apb-ctrl.c and arche-platform.c
> where using %pe would be appropriate. I also found one additional
> case in uart.c:954 where the error message could be improved by
> printing the error name.
>
> I will keep PTR_ERR() calls that are needed to return error codes
> unchanged.
>
> >
> > And if updating everything is a task you're willing to do,
> > then I think I would prefer you extend this change.
>
> I am willing to make these additional changes. Since my original patch
> is already in staging-next, should i send a v3 of the existing patch
> or submit a new patch covering all the relevant cases?

v3

julia

>
> Thanks for your guidance.
>
> Pavan
>
> >
> > 					-Alex
>
>

Re: [PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[Julia Lawall]

On Wed, 14 Oct 2026, Jeremy Dean Clifton wrote:

> On 10/13/26 3:43 PM, Yalagada Pavan Kumar wrote:
> > On Tue, Oct 13, 2026 at 03:11:51PM -0500, Alex Elder wrote:
> > > On 10/12/26 9:24 PM, pavankumaryalagada@gmail.com wrote:
> > > > From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> > > >
> > > > Use %pe to print error pointers directly and display their
> > > > error names.
> > > >
> > > > Keep the other PTR_ERR() call because its return value is
> > > > needed for ret.
> > > >
> > > > Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
> > >
> > >
> > > Generally I think what you're doing is OK.  It looks
> > > like %pe is better than just %d, because it prints
> > > the error *name* for the pointer-coded error.
> > >
> > > But I have two general questions about this effort:
> > >
> > > - There are other places in the Greybus code (even in
> > >    just the staging source files) where this exact same
> > >    thing could be done.  Why limit this to "fw-core.c" only?
> >
> > I initially limited the change to "fw-core.c" because I was
> > focusing on the error messages in that file.
> >
> > >
> > > - Do you think that all Greybus error messages should be
> > >    converted to use %pe (passing ERR_PTR() if necessary)
> > >    instead of %d for showing the error number?
> >
> > Using %pe prints the error name instead of just the
> > error number, which makes the error messages easier
> > to understand.
> >
> > For example, the current message is:
> >
> > 	failed to create download connection (-12)
> >
> > with %pe, it becomes:
> >
> > 	failed to create download connection (-ENOMEM)
> >
> > I think the second message is easier to understand because
> > it shows the error name instead of just error number.
> >
> > I also found other PTR_ERR() calls in Greybus. some of them
> > are used to return error codes and should remain unchanged.
> >
> > For example, in fw-core.c:
> >
> > 	if (IS_ERR(connection)) {
> > 		ret = PTR_ERR(connection);
> > 		dev_err(&bundle->dev,
> > 			"failed to create management connection (%pe)\n",
> > 			connection);
> > 		goto err_destroy_connections;
> > 	}
> >
> > Here, PTR_ERR(connection) is needed to store the error code in ret
> > for the error-handling path, while passing connection to %pe makes
> > the error message more readable.
> >
> > >
> > > I think your suggested change is a net positive, but
> > > doing it in some spots but not others means it is
> > > adding a bit of inconsistency.
> >
> > I agree that similar cases elsewhere in Greybus are worth
> > checking to keep error reporting consistent. I will look
> > at the other Greybus files for cases where using %pe would
> > be appropriate.
> >
> > Would you prefer me to extend this change to the other
> > relevant Greybus files, or keep this patch focused on
> > fw-core.c and submit a separate patch for the remaining
> > changes?
> Also, you could coordinate something with Julia to use Coccinelle for this.
> Using regular expressions, you could programmatically find all the places
> where %pe and connection can be in place of %d or %ld and the return from
> PTR_ERR().

Coccinelle could probably be used for finding opportunities.  Modifying
strings can be a bit awkward, but is not impossible.

julia

>
> Jeremy Clifton
>
>