| Message ID | 20190121143055.11781-1-suwan.kim027@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: core: endpoint: Add error messages for usb_create_ep_devs() | expand |
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
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
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_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(-)