usb: core: endpoint: Add error messages for usb_create_ep_devs()

usb: core: endpoint: Add error messages for usb_create_ep_devs()

[Suwan Kim]

usb_create_ep_devs() returns error code if an error occurs. But
usb_new_device() and create_intf_ep_devs() which use usb_create_ep_devs()
to create endpoint devices ignore the error and cut the propagation
of the error when usb_create_ep_devs() fails. So, usb_create_ep_devs()
prints an error message for debugging or notification purposes if an
error occurs.

Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
---
 drivers/usb/core/endpoint.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/endpoint.c b/drivers/usb/core/endpoint.c
index 1c2c04079676..ad3902c023d9 100644
--- a/drivers/usb/core/endpoint.c
+++ b/drivers/usb/core/endpoint.c
@@ -182,6 +182,7 @@ int usb_create_ep_devs(struct device *parent,
 
 	ep_dev = kzalloc(sizeof(*ep_dev), GFP_KERNEL);
 	if (!ep_dev) {
+		dev_err(parent, "endpoint device alloc failed\n");
 		retval = -ENOMEM;
 		goto exit;
 	}
@@ -194,8 +195,10 @@ int usb_create_ep_devs(struct device *parent,
 	dev_set_name(&ep_dev->dev, "ep_%02x", endpoint->desc.bEndpointAddress);
 
 	retval = device_register(&ep_dev->dev);
-	if (retval)
+	if (retval) {
+		dev_err(parent, "endpoint device register failed\n");
 		goto error_register;
+	}
 
 	device_enable_async_suspend(&ep_dev->dev);
 	endpoint->ep_dev = ep_dev;

usb: core: endpoint: Add error messages for usb_create_ep_devs()

[Greg Kroah-Hartman]

On Mon, Jan 21, 2019 at 11:30:55PM +0900, Suwan Kim wrote:
> usb_create_ep_devs() returns error code if an error occurs. But
> usb_new_device() and create_intf_ep_devs() which use usb_create_ep_devs()
> to create endpoint devices ignore the error and cut the propagation
> of the error when usb_create_ep_devs() fails. So, usb_create_ep_devs()
> prints an error message for debugging or notification purposes if an
> error occurs.
> 
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
>  drivers/usb/core/endpoint.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/endpoint.c b/drivers/usb/core/endpoint.c
> index 1c2c04079676..ad3902c023d9 100644
> --- a/drivers/usb/core/endpoint.c
> +++ b/drivers/usb/core/endpoint.c
> @@ -182,6 +182,7 @@ int usb_create_ep_devs(struct device *parent,
>  
>  	ep_dev = kzalloc(sizeof(*ep_dev), GFP_KERNEL);
>  	if (!ep_dev) {
> +		dev_err(parent, "endpoint device alloc failed\n");

kzalloc() prints out an error if it fails, why print out it again here?

>  		retval = -ENOMEM;
>  		goto exit;
>  	}
> @@ -194,8 +195,10 @@ int usb_create_ep_devs(struct device *parent,
>  	dev_set_name(&ep_dev->dev, "ep_%02x", endpoint->desc.bEndpointAddress);
>  
>  	retval = device_register(&ep_dev->dev);
> -	if (retval)
> +	if (retval) {
> +		dev_err(parent, "endpoint device register failed\n");

Doesn't device_add() provide an error if this fails?  If not, what can
we do about it here?

And even if you do want this message, it's not the parent that failed to
register, so this might be a bit misleading of a message.

What problem are you having that you are trying to fix here?  Have you
seen the call to device_register() fail for an endpoint?

thanks,

greg k-h

usb: core: endpoint: Add error messages for usb_create_ep_devs()

[Suwan Kim]

On Mon, Jan 21, 2019 at 03:35:34PM +0100, Greg KH wrote:
> On Mon, Jan 21, 2019 at 11:30:55PM +0900, Suwan Kim wrote:
> > usb_create_ep_devs() returns error code if an error occurs. But
> > usb_new_device() and create_intf_ep_devs() which use usb_create_ep_devs()
> > to create endpoint devices ignore the error and cut the propagation
> > of the error when usb_create_ep_devs() fails. So, usb_create_ep_devs()
> > prints an error message for debugging or notification purposes if an
> > error occurs.
> > 
> > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> > ---
> >  drivers/usb/core/endpoint.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/core/endpoint.c b/drivers/usb/core/endpoint.c
> > index 1c2c04079676..ad3902c023d9 100644
> > --- a/drivers/usb/core/endpoint.c
> > +++ b/drivers/usb/core/endpoint.c
> > @@ -182,6 +182,7 @@ int usb_create_ep_devs(struct device *parent,
> >  
> >  	ep_dev = kzalloc(sizeof(*ep_dev), GFP_KERNEL);
> >  	if (!ep_dev) {
> > +		dev_err(parent, "endpoint device alloc failed\n");
> 
> kzalloc() prints out an error if it fails, why print out it again here?
> 
> >  		retval = -ENOMEM;
> >  		goto exit;
> >  	}
> > @@ -194,8 +195,10 @@ int usb_create_ep_devs(struct device *parent,
> >  	dev_set_name(&ep_dev->dev, "ep_%02x", endpoint->desc.bEndpointAddress);
> >  
> >  	retval = device_register(&ep_dev->dev);
> > -	if (retval)
> > +	if (retval) {
> > +		dev_err(parent, "endpoint device register failed\n");
> 
> Doesn't device_add() provide an error if this fails?  If not, what can
> we do about it here?
> 
> And even if you do want this message, it's not the parent that failed to
> register, so this might be a bit misleading of a message.
 
Oh... I was misunderstading. Thank you for pointing out.

> What problem are you having that you are trying to fix here?  Have you
> seen the call to device_register() fail for an endpoint?
> 
> thanks,
> 
> greg k-h

Thank you very much for reviewing my code. I did not see the error
actually happening. I just wanted to fix things about ignoring errors.
I will discard this patch if there are really few errors.

Regards
Suwan Kim