[U-Boot] [PATCH v3 1/7] usb_kdb: only process events succesfully received

[U-Boot] [PATCH v3 1/7] usb_kdb: only process events succesfully received

[Michal Suchanek]

Causes unbound key repeat on error otherwise.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: fix indentation
---
 common/usb_kbd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index cc99c6be0720..fc9419e0238a 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -339,10 +339,9 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
 	struct usb_kbd_pdata *data = dev->privptr;
 
 	/* Submit a interrupt transfer request */
-	usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
-			   data->intinterval);
-
-	usb_kbd_irq_worker(dev);
+	if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
+				  data->intpktsize, data->intinterval))
+		usb_kbd_irq_worker(dev);
 #elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) || \
       defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
 #if defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
-- 
2.21.0

[U-Boot] [PATCH v3 2/7] usb: usb_submit_int_msg -> usb_int_msg

[Michal Suchanek]

This aligns naming with usb_bulk_msg and usb_control_msg.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: new patch
---
 common/usb.c     | 2 +-
 common/usb_kbd.c | 6 +++---
 include/usb.h    | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index b70f614d244f..704937dec8a8 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -194,7 +194,7 @@ int usb_disable_asynch(int disable)
 /*
  * submits an Interrupt Message
  */
-int usb_submit_int_msg(struct usb_device *dev, unsigned long pipe,
+int usb_int_msg(struct usb_device *dev, unsigned long pipe,
 			void *buffer, int transfer_len, int interval)
 {
 	return submit_int_msg(dev, pipe, buffer, transfer_len, interval);
diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index fc9419e0238a..4c23023d4e59 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -339,7 +339,7 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
 	struct usb_kbd_pdata *data = dev->privptr;
 
 	/* Submit a interrupt transfer request */
-	if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
+	if (!usb_int_msg(dev, data->intpipe, &data->new[0],
 				  data->intpktsize, data->intinterval))
 		usb_kbd_irq_worker(dev);
 #elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) || \
@@ -503,8 +503,8 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
 	if (usb_get_report(dev, iface->desc.bInterfaceNumber,
 			   1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) {
 #else
-	if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
-			       data->intinterval) < 0) {
+	if (usb_int_msg(dev, data->intpipe, data->new, data->intpktsize,
+			data->intinterval) < 0) {
 #endif
 		printf("Failed to get keyboard state from device %04x:%04x\n",
 		       dev->descriptor.idVendor, dev->descriptor.idProduct);
diff --git a/include/usb.h b/include/usb.h
index 420a30e49fa1..8cd73863b876 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -261,7 +261,7 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
 			void *data, unsigned short size, int timeout);
 int usb_bulk_msg(struct usb_device *dev, unsigned int pipe,
 			void *data, int len, int *actual_length, int timeout);
-int usb_submit_int_msg(struct usb_device *dev, unsigned long pipe,
+int usb_int_msg(struct usb_device *dev, unsigned long pipe,
 			void *buffer, int transfer_len, int interval);
 int usb_disable_asynch(int disable);
 int usb_maxpacket(struct usb_device *dev, unsigned long pipe);
-- 
2.21.0

[U-Boot] [PATCH v3 3/7] usb: storage: submit_int_msg -> usb_int_msg

[Michal Suchanek]

Use the wrapper because the unwrapped function prototype will be changed
in the following patch.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: usb_submit_int_msg -> usb_int_msg
v3: fix indentation
---
 common/usb_storage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 8c889bb1a648..995a96baa57d 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -650,8 +650,8 @@ static int usb_stor_CBI_get_status(struct scsi_cmd *srb, struct us_data *us)
 	int timeout;
 
 	us->ip_wanted = 1;
-	submit_int_msg(us->pusb_dev, us->irqpipe,
-			(void *) &us->ip_data, us->irqmaxp, us->irqinterval);
+	usb_int_msg(us->pusb_dev, us->irqpipe,
+		    (void *) &us->ip_data, us->irqmaxp, us->irqinterval);
 	timeout = 1000;
 	while (timeout--) {
 		if (us->ip_wanted == 0)
-- 
2.21.0

[U-Boot] [PATCH v3 4/7] usb: Add nonblock argument to submit_int_msg

[Michal Suchanek]

This will be used to implement non-blocking keyboard polling in case of
errors.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: ad missing hunk from last patch
---
 common/usb.c                      |  2 +-
 drivers/usb/host/dwc2.c           | 12 +++++++-----
 drivers/usb/host/ehci-hcd.c       | 13 ++++++++-----
 drivers/usb/host/ohci-hcd.c       |  4 ++--
 drivers/usb/host/r8a66597-hcd.c   |  2 +-
 drivers/usb/host/sl811-hcd.c      |  2 +-
 drivers/usb/host/usb-uclass.c     |  4 ++--
 drivers/usb/host/xhci.c           | 13 ++++++++-----
 drivers/usb/musb-new/musb_uboot.c | 12 +++++++-----
 include/usb.h                     |  4 ++--
 10 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 704937dec8a8..f57c0e8cdf57 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -197,7 +197,7 @@ int usb_disable_asynch(int disable)
 int usb_int_msg(struct usb_device *dev, unsigned long pipe,
 			void *buffer, int transfer_len, int interval)
 {
-	return submit_int_msg(dev, pipe, buffer, transfer_len, interval);
+	return submit_int_msg(dev, pipe, buffer, transfer_len, interval, false);
 }
 
 /*
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index a62a2f8a951d..b4121a49b805 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -1108,7 +1108,8 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
 }
 
 int _submit_int_msg(struct dwc2_priv *priv, struct usb_device *dev,
-		    unsigned long pipe, void *buffer, int len, int interval)
+		    unsigned long pipe, void *buffer, int len, int interval,
+		    bool nonblock)
 {
 	unsigned long timeout;
 	int ret;
@@ -1236,9 +1237,10 @@ int submit_bulk_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 }
 
 int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
-		   int len, int interval)
+		   int len, int interval, bool nonblock)
 {
-	return _submit_int_msg(&local, dev, pipe, buffer, len, interval);
+	return _submit_int_msg(&local, dev, pipe, buffer, len, interval,
+			       nonblock);
 }
 
 /* U-Boot USB control interface */
@@ -1292,13 +1294,13 @@ static int dwc2_submit_bulk_msg(struct udevice *dev, struct usb_device *udev,
 
 static int dwc2_submit_int_msg(struct udevice *dev, struct usb_device *udev,
 			       unsigned long pipe, void *buffer, int length,
-			       int interval)
+			       int interval, bool nonblock)
 {
 	struct dwc2_priv *priv = dev_get_priv(dev);
 
 	debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev);
 
-	return _submit_int_msg(priv, udev, pipe, buffer, length, interval);
+	return _submit_int_msg(priv, udev, pipe, buffer, length, interval, nonblock);
 }
 
 static int dwc2_usb_ofdata_to_platdata(struct udevice *dev)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 4b28db70a566..61a61abb2112 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1482,7 +1482,8 @@ out:
 }
 
 static int _ehci_submit_int_msg(struct usb_device *dev, unsigned long pipe,
-				void *buffer, int length, int interval)
+				void *buffer, int length, int interval,
+				bool nonblock)
 {
 	void *backbuffer;
 	struct int_queue *queue;
@@ -1532,9 +1533,10 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 }
 
 int submit_int_msg(struct usb_device *dev, unsigned long pipe,
-		   void *buffer, int length, int interval)
+		   void *buffer, int length, int interval, bool nonblock)
 {
-	return _ehci_submit_int_msg(dev, pipe, buffer, length, interval);
+	return _ehci_submit_int_msg(dev, pipe, buffer, length, interval,
+				    nonblock);
 }
 
 struct int_queue *create_int_queue(struct usb_device *dev,
@@ -1576,10 +1578,11 @@ static int ehci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev,
 
 static int ehci_submit_int_msg(struct udevice *dev, struct usb_device *udev,
 			       unsigned long pipe, void *buffer, int length,
-			       int interval)
+			       int interval, bool nonblock)
 {
 	debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev);
-	return _ehci_submit_int_msg(udev, pipe, buffer, length, interval);
+	return _ehci_submit_int_msg(udev, pipe, buffer, length, interval,
+				    nonblock);
 }
 
 static struct int_queue *ehci_create_int_queue(struct udevice *dev,
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 2b0df88f49ec..f1cc6547fbcb 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -1700,7 +1700,7 @@ int submit_bulk_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 }
 
 int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
-		int transfer_len, int interval)
+		int transfer_len, int interval, bool nonblock)
 {
 	info("submit_int_msg");
 	return submit_common_msg(&gohci, dev, pipe, buffer, transfer_len, NULL,
@@ -2149,7 +2149,7 @@ static int ohci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev,
 
 static int ohci_submit_int_msg(struct udevice *dev, struct usb_device *udev,
 			       unsigned long pipe, void *buffer, int length,
-			       int interval)
+			       int interval, bool nonblock)
 {
 	ohci_t *ohci = dev_get_priv(usb_get_bus(dev));
 
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 3c263e51c160..83056feeae22 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -822,7 +822,7 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe,
 }
 
 int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
-			int transfer_len, int interval)
+			int transfer_len, int interval, bool nonblock)
 {
 	/* no implement */
 	R8A66597_DPRINT("%s\n", __func__);
diff --git a/drivers/usb/host/sl811-hcd.c b/drivers/usb/host/sl811-hcd.c
index daba0dcd1aee..e08da6130bd5 100644
--- a/drivers/usb/host/sl811-hcd.c
+++ b/drivers/usb/host/sl811-hcd.c
@@ -384,7 +384,7 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 }
 
 int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
-		   int len, int interval)
+		   int len, int interval, bool nonblock)
 {
 	PDEBUG(0, "dev = %p pipe = %#lx buf = %p size = %d int = %d\n", dev, pipe,
 	       buffer, len, interval);
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index 6e118b5a8ffa..35934fab0e45 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -31,7 +31,7 @@ int usb_disable_asynch(int disable)
 }
 
 int submit_int_msg(struct usb_device *udev, unsigned long pipe, void *buffer,
-		   int length, int interval)
+		   int length, int interval, bool nonblock)
 {
 	struct udevice *bus = udev->controller_dev;
 	struct dm_usb_ops *ops = usb_get_ops(bus);
@@ -39,7 +39,7 @@ int submit_int_msg(struct usb_device *udev, unsigned long pipe, void *buffer,
 	if (!ops->interrupt)
 		return -ENOSYS;
 
-	return ops->interrupt(bus, udev, pipe, buffer, length, interval);
+	return ops->interrupt(bus, udev, pipe, buffer, length, interval, nonblock);
 }
 
 int submit_control_msg(struct usb_device *udev, unsigned long pipe,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 44c5f2d264c1..b3e4dcd66fa1 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1109,7 +1109,8 @@ unknown:
  * @return 0
  */
 static int _xhci_submit_int_msg(struct usb_device *udev, unsigned long pipe,
-				void *buffer, int length, int interval)
+				void *buffer, int length, int interval,
+				bool nonblock)
 {
 	if (usb_pipetype(pipe) != PIPE_INTERRUPT) {
 		printf("non-interrupt pipe (type=%lu)", usb_pipetype(pipe));
@@ -1277,9 +1278,10 @@ int submit_bulk_msg(struct usb_device *udev, unsigned long pipe, void *buffer,
 }
 
 int submit_int_msg(struct usb_device *udev, unsigned long pipe, void *buffer,
-		   int length, int interval)
+		   int length, int interval, bool nonblock)
 {
-	return _xhci_submit_int_msg(udev, pipe, buffer, length, interval);
+	return _xhci_submit_int_msg(udev, pipe, buffer, length, interval,
+				    nonblock);
 }
 
 /**
@@ -1386,10 +1388,11 @@ static int xhci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev,
 
 static int xhci_submit_int_msg(struct udevice *dev, struct usb_device *udev,
 			       unsigned long pipe, void *buffer, int length,
-			       int interval)
+			       int interval, bool nonblock)
 {
 	debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev);
-	return _xhci_submit_int_msg(udev, pipe, buffer, length, interval);
+	return _xhci_submit_int_msg(udev, pipe, buffer, length, interval,
+				    nonblock);
 }
 
 static int xhci_alloc_device(struct udevice *dev, struct usb_device *udev)
diff --git a/drivers/usb/musb-new/musb_uboot.c b/drivers/usb/musb-new/musb_uboot.c
index 9c8cc6e58443..9eb593402ea0 100644
--- a/drivers/usb/musb-new/musb_uboot.c
+++ b/drivers/usb/musb-new/musb_uboot.c
@@ -110,7 +110,7 @@ static int _musb_submit_bulk_msg(struct musb_host_data *host,
 
 static int _musb_submit_int_msg(struct musb_host_data *host,
 	struct usb_device *dev, unsigned long pipe,
-	void *buffer, int len, int interval)
+	void *buffer, int len, int interval, bool nonblock)
 {
 	construct_urb(&host->urb, &host->hep, dev, USB_ENDPOINT_XFER_INT, pipe,
 		      buffer, len, NULL, interval);
@@ -268,9 +268,10 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe,
 }
 
 int submit_int_msg(struct usb_device *dev, unsigned long pipe,
-		   void *buffer, int length, int interval)
+		   void *buffer, int length, int interval, bool nonblock)
 {
-	return _musb_submit_int_msg(&musb_host, dev, pipe, buffer, length, interval);
+	return _musb_submit_int_msg(&musb_host, dev, pipe, buffer, length,
+				    interval, nonblock);
 }
 
 struct int_queue *create_int_queue(struct usb_device *dev,
@@ -320,10 +321,11 @@ static int musb_submit_bulk_msg(struct udevice *dev, struct usb_device *udev,
 
 static int musb_submit_int_msg(struct udevice *dev, struct usb_device *udev,
 			       unsigned long pipe, void *buffer, int length,
-			       int interval)
+			       int interval, bool nonblock)
 {
 	struct musb_host_data *host = dev_get_priv(dev);
-	return _musb_submit_int_msg(host, udev, pipe, buffer, length, interval);
+	return _musb_submit_int_msg(host, udev, pipe, buffer, length, interval,
+				    nonblock);
 }
 
 static struct int_queue *musb_create_int_queue(struct udevice *dev,
diff --git a/include/usb.h b/include/usb.h
index 8cd73863b876..6ca2eb30d08a 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -184,7 +184,7 @@ int submit_bulk_msg(struct usb_device *dev, unsigned long pipe,
 int submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 			int transfer_len, struct devrequest *setup);
 int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
-			int transfer_len, int interval);
+			int transfer_len, int interval, bool nonblock);
 
 #if defined CONFIG_USB_EHCI_HCD || defined CONFIG_USB_MUSB_HOST \
 	|| CONFIG_IS_ENABLED(DM_USB)
@@ -708,7 +708,7 @@ struct dm_usb_ops {
 	 */
 	int (*interrupt)(struct udevice *bus, struct usb_device *udev,
 			 unsigned long pipe, void *buffer, int length,
-			 int interval);
+			 int interval, bool nonblock);
 
 	/**
 	 * create_int_queue() - Create and queue interrupt packets
-- 
2.21.0

[U-Boot] [PATCH v3 5/7] usb: add usb_int_msg_nonblock

[Michal Suchanek]

Variant of the int_msg wrapper that does not introduce excessive retry
delay on error.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: usb_submit_int_msg -> usb_int_msg
---
 common/usb.c  | 9 +++++++++
 include/usb.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/common/usb.c b/common/usb.c
index f57c0e8cdf57..1bd60b24a555 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -200,6 +200,15 @@ int usb_int_msg(struct usb_device *dev, unsigned long pipe,
 	return submit_int_msg(dev, pipe, buffer, transfer_len, interval, false);
 }
 
+/*
+ * submits an Interrupt Message without retry
+ */
+int usb_int_msg_nonblock(struct usb_device *dev, unsigned long pipe,
+			void *buffer, int transfer_len, int interval)
+{
+	return submit_int_msg(dev, pipe, buffer, transfer_len, interval, true);
+}
+
 /*
  * submits a control message and waits for comletion (at least timeout * 1ms)
  * If timeout is 0, we don't wait for completion (used as example to set and
diff --git a/include/usb.h b/include/usb.h
index 6ca2eb30d08a..b0a079766652 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -263,6 +263,8 @@ int usb_bulk_msg(struct usb_device *dev, unsigned int pipe,
 			void *data, int len, int *actual_length, int timeout);
 int usb_int_msg(struct usb_device *dev, unsigned long pipe,
 			void *buffer, int transfer_len, int interval);
+int usb_int_msg_nonblock(struct usb_device *dev, unsigned long pipe,
+			void *buffer, int transfer_len, int interval);
 int usb_disable_asynch(int disable);
 int usb_maxpacket(struct usb_device *dev, unsigned long pipe);
 int usb_get_configuration_no(struct usb_device *dev, int cfgno,
-- 
2.21.0

[U-Boot] [PATCH v3 6/7] usb: kbd: use usb_int_msg_nonblock for polling

[Michal Suchanek]

With the following patch it avoids excessive delays with USB 1.1
keyboard connected to high-speed USB hub conncted to dwc2.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: usb_submit_int_msg -> usb_int_msg
---
 common/usb_kbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 4c23023d4e59..57a3cba496fe 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -339,7 +339,7 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
 	struct usb_kbd_pdata *data = dev->privptr;
 
 	/* Submit a interrupt transfer request */
-	if (!usb_int_msg(dev, data->intpipe, &data->new[0],
+	if (!usb_int_msg_nonblock(dev, data->intpipe, &data->new[0],
 				  data->intpktsize, data->intinterval))
 		usb_kbd_irq_worker(dev);
 #elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) || \
-- 
2.21.0

[U-Boot] [PATCH v3 7/7] dwc2: use the nonblock argument in submit_int_msg

[Michal Suchanek]

An USB 1.1 keyboard connected to dwc2 through a high-speed hub does not
report status until it changes. With this patch you can enable keyboard
by pressing a key while USB devices are probed. Without a keypress no
state is reported and the probe times out. We don't want to wait for a
keypress or timeout while polling for keypresses so implement an int_msg
nonblock variant that exits early on error.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2: move superfluous hunk to earlier patch
---
 drivers/usb/host/dwc2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index b4121a49b805..78829d56199c 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -1123,7 +1123,7 @@ int _submit_int_msg(struct dwc2_priv *priv, struct usb_device *dev,
 			return -ETIMEDOUT;
 		}
 		ret = _submit_bulk_msg(priv, dev, pipe, buffer, len);
-		if (ret != -EAGAIN)
+		if ((ret != -EAGAIN) || nonblock)
 			return ret;
 	}
 }
-- 
2.21.0

[U-Boot] [PATCH v3 1/7] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/5/19 12:44 PM, Michal Suchanek wrote:
> Causes unbound key repeat on error otherwise.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> v2: fix indentation

What changed in V3 ?

> ---
>  common/usb_kbd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index cc99c6be0720..fc9419e0238a 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -339,10 +339,9 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>  	struct usb_kbd_pdata *data = dev->privptr;
>  
>  	/* Submit a interrupt transfer request */
> -	usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
> -			   data->intinterval);
> -
> -	usb_kbd_irq_worker(dev);
> +	if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
> +				  data->intpktsize, data->intinterval))

This still doesn't propagate errors.

[U-Boot] [PATCH v3 5/7] usb: add usb_int_msg_nonblock

[Marek Vasut]

On 7/5/19 12:44 PM, Michal Suchanek wrote:
> Variant of the int_msg wrapper that does not introduce excessive retry
> delay on error.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> v2: usb_submit_int_msg -> usb_int_msg
> ---
>  common/usb.c  | 9 +++++++++
>  include/usb.h | 2 ++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/common/usb.c b/common/usb.c
> index f57c0e8cdf57..1bd60b24a555 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -200,6 +200,15 @@ int usb_int_msg(struct usb_device *dev, unsigned long pipe,
>  	return submit_int_msg(dev, pipe, buffer, transfer_len, interval, false);
>  }
>  
> +/*
> + * submits an Interrupt Message without retry
> + */
> +int usb_int_msg_nonblock(struct usb_device *dev, unsigned long pipe,
> +			void *buffer, int transfer_len, int interval)
> +{
> +	return submit_int_msg(dev, pipe, buffer, transfer_len, interval, true);
> +}

Wouldn't it be shorter to just call submit_int_msg() directly, with the
extra parameter ?

[U-Boot] [PATCH v3 2/7] usb: usb_submit_int_msg -> usb_int_msg

[Bin Meng]

On Fri, Jul 5, 2019 at 6:44 PM Michal Suchanek <msuchanek@suse.de> wrote:
>
> This aligns naming with usb_bulk_msg and usb_control_msg.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> v2: new patch
> ---
>  common/usb.c     | 2 +-
>  common/usb_kbd.c | 6 +++---
>  include/usb.h    | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/common/usb.c b/common/usb.c
> index b70f614d244f..704937dec8a8 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -194,7 +194,7 @@ int usb_disable_asynch(int disable)
>  /*
>   * submits an Interrupt Message
>   */
> -int usb_submit_int_msg(struct usb_device *dev, unsigned long pipe,
> +int usb_int_msg(struct usb_device *dev, unsigned long pipe,
>                         void *buffer, int transfer_len, int interval)

nits: indentation

>  {
>         return submit_int_msg(dev, pipe, buffer, transfer_len, interval);
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index fc9419e0238a..4c23023d4e59 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -339,7 +339,7 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>         struct usb_kbd_pdata *data = dev->privptr;
>
>         /* Submit a interrupt transfer request */
> -       if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
> +       if (!usb_int_msg(dev, data->intpipe, &data->new[0],
>                                   data->intpktsize, data->intinterval))

ditto

>                 usb_kbd_irq_worker(dev);
>  #elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) || \
> @@ -503,8 +503,8 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
>         if (usb_get_report(dev, iface->desc.bInterfaceNumber,
>                            1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) {
>  #else
> -       if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
> -                              data->intinterval) < 0) {
> +       if (usb_int_msg(dev, data->intpipe, data->new, data->intpktsize,
> +                       data->intinterval) < 0) {
>  #endif
>                 printf("Failed to get keyboard state from device %04x:%04x\n",
>                        dev->descriptor.idVendor, dev->descriptor.idProduct);
> diff --git a/include/usb.h b/include/usb.h
> index 420a30e49fa1..8cd73863b876 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -261,7 +261,7 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
>                         void *data, unsigned short size, int timeout);
>  int usb_bulk_msg(struct usb_device *dev, unsigned int pipe,
>                         void *data, int len, int *actual_length, int timeout);
> -int usb_submit_int_msg(struct usb_device *dev, unsigned long pipe,
> +int usb_int_msg(struct usb_device *dev, unsigned long pipe,
>                         void *buffer, int transfer_len, int interval);

ditto

>  int usb_disable_asynch(int disable);
>  int usb_maxpacket(struct usb_device *dev, unsigned long pipe);
> --

Regards,
Bin

[U-Boot] [PATCH v3 3/7] usb: storage: submit_int_msg -> usb_int_msg

[Bin Meng]

On Fri, Jul 5, 2019 at 6:44 PM Michal Suchanek <msuchanek@suse.de> wrote:
>
> Use the wrapper because the unwrapped function prototype will be changed
> in the following patch.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> v2: usb_submit_int_msg -> usb_int_msg
> v3: fix indentation
> ---
>  common/usb_storage.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH v3 1/7] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Fri, 5 Jul 2019 14:12:36 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/5/19 12:44 PM, Michal Suchanek wrote:
> > Causes unbound key repeat on error otherwise.
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > v2: fix indentation  
> 
> What changed in V3 ?
> 
> > ---
> >  common/usb_kbd.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> > index cc99c6be0720..fc9419e0238a 100644
> > --- a/common/usb_kbd.c
> > +++ b/common/usb_kbd.c
> > @@ -339,10 +339,9 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
> >  	struct usb_kbd_pdata *data = dev->privptr;
> >  
> >  	/* Submit a interrupt transfer request */
> > -	usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
> > -			   data->intinterval);
> > -
> > -	usb_kbd_irq_worker(dev);
> > +	if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
> > +				  data->intpktsize, data->intinterval))  
> 
> This still doesn't propagate errors.

Yes, it does not. I don't have a grand design that would make use of
these propagated errors so I don't know what errors to propagate.

You could, say, want to propagate errors from kbd_irq_worker as well.

Until there is a way to make use of the errors you really don't know.

Might be good idea to document that getting EAGAIN from
usb_int_msg_nonblock is expected.

Thanks

Michal

[U-Boot] [PATCH v3 2/7] usb: usb_submit_int_msg -> usb_int_msg

[Michal Suchánek]

On Fri, 5 Jul 2019 21:51:43 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:

> On Fri, Jul 5, 2019 at 6:44 PM Michal Suchanek <msuchanek@suse.de> wrote:
> >
> > This aligns naming with usb_bulk_msg and usb_control_msg.
> >
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > v2: new patch
> > ---
> >  common/usb.c     | 2 +-
> >  common/usb_kbd.c | 6 +++---
> >  include/usb.h    | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/common/usb.c b/common/usb.c
> > index b70f614d244f..704937dec8a8 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -194,7 +194,7 @@ int usb_disable_asynch(int disable)
> >  /*
> >   * submits an Interrupt Message
> >   */
> > -int usb_submit_int_msg(struct usb_device *dev, unsigned long pipe,
> > +int usb_int_msg(struct usb_device *dev, unsigned long pipe,
> >                         void *buffer, int transfer_len, int interval)  
> 
> nits: indentation
> 
> >  {
> >         return submit_int_msg(dev, pipe, buffer, transfer_len, interval);
> > diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> > index fc9419e0238a..4c23023d4e59 100644
> > --- a/common/usb_kbd.c
> > +++ b/common/usb_kbd.c
> > @@ -339,7 +339,7 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
> >         struct usb_kbd_pdata *data = dev->privptr;
> >
> >         /* Submit a interrupt transfer request */
> > -       if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
> > +       if (!usb_int_msg(dev, data->intpipe, &data->new[0],
> >                                   data->intpktsize, data->intinterval))  
> 
> ditto

This is the correct indentation
> 
> >                 usb_kbd_irq_worker(dev);
> >  #elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) || \
> > @@ -503,8 +503,8 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
> >         if (usb_get_report(dev, iface->desc.bInterfaceNumber,
> >                            1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) {
> >  #else
> > -       if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,
> > -                              data->intinterval) < 0) {
> > +       if (usb_int_msg(dev, data->intpipe, data->new, data->intpktsize,
> > +                       data->intinterval) < 0) {
> >  #endif
> >                 printf("Failed to get keyboard state from device %04x:%04x\n",
> >                        dev->descriptor.idVendor, dev->descriptor.idProduct);
> > diff --git a/include/usb.h b/include/usb.h
> > index 420a30e49fa1..8cd73863b876 100644
> > --- a/include/usb.h
> > +++ b/include/usb.h
> > @@ -261,7 +261,7 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
> >                         void *data, unsigned short size, int timeout);
> >  int usb_bulk_msg(struct usb_device *dev, unsigned int pipe,
> >                         void *data, int len, int *actual_length, int timeout);
> > -int usb_submit_int_msg(struct usb_device *dev, unsigned long pipe,
> > +int usb_int_msg(struct usb_device *dev, unsigned long pipe,
> >                         void *buffer, int transfer_len, int interval);  
> 
> ditto

Here the indentation matches the surrounding declarations.

Thanks

Michal

[U-Boot] [PATCH v3 5/7] usb: add usb_int_msg_nonblock

[Michal Suchánek]

On Fri, 5 Jul 2019 14:13:50 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/5/19 12:44 PM, Michal Suchanek wrote:
> > Variant of the int_msg wrapper that does not introduce excessive retry
> > delay on error.
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > v2: usb_submit_int_msg -> usb_int_msg
> > ---
> >  common/usb.c  | 9 +++++++++
> >  include/usb.h | 2 ++
> >  2 files changed, 11 insertions(+)
> > 
> > diff --git a/common/usb.c b/common/usb.c
> > index f57c0e8cdf57..1bd60b24a555 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -200,6 +200,15 @@ int usb_int_msg(struct usb_device *dev, unsigned long pipe,
> >  	return submit_int_msg(dev, pipe, buffer, transfer_len, interval, false);
> >  }
> >  
> > +/*
> > + * submits an Interrupt Message without retry
> > + */
> > +int usb_int_msg_nonblock(struct usb_device *dev, unsigned long pipe,
> > +			void *buffer, int transfer_len, int interval)
> > +{
> > +	return submit_int_msg(dev, pipe, buffer, transfer_len, interval, true);
> > +}  
> 
> Wouldn't it be shorter to just call submit_int_msg() directly, with the
> extra parameter ?

It is easier to understand the change this way. The idea is that only
some very specific callers need to know about the difference.

Thanks

Michal

[U-Boot] [PATCH v3 5/7] usb: add usb_int_msg_nonblock

[Marek Vasut]

On 7/10/19 5:52 PM, Michal Suchánek wrote:
> On Fri, 5 Jul 2019 14:13:50 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/5/19 12:44 PM, Michal Suchanek wrote:
>>> Variant of the int_msg wrapper that does not introduce excessive retry
>>> delay on error.
>>>
>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>> ---
>>> v2: usb_submit_int_msg -> usb_int_msg
>>> ---
>>>  common/usb.c  | 9 +++++++++
>>>  include/usb.h | 2 ++
>>>  2 files changed, 11 insertions(+)
>>>
>>> diff --git a/common/usb.c b/common/usb.c
>>> index f57c0e8cdf57..1bd60b24a555 100644
>>> --- a/common/usb.c
>>> +++ b/common/usb.c
>>> @@ -200,6 +200,15 @@ int usb_int_msg(struct usb_device *dev, unsigned long pipe,
>>>  	return submit_int_msg(dev, pipe, buffer, transfer_len, interval, false);
>>>  }
>>>  
>>> +/*
>>> + * submits an Interrupt Message without retry
>>> + */
>>> +int usb_int_msg_nonblock(struct usb_device *dev, unsigned long pipe,
>>> +			void *buffer, int transfer_len, int interval)
>>> +{
>>> +	return submit_int_msg(dev, pipe, buffer, transfer_len, interval, true);
>>> +}  
>>
>> Wouldn't it be shorter to just call submit_int_msg() directly, with the
>> extra parameter ?
> 
> It is easier to understand the change this way. The idea is that only
> some very specific callers need to know about the difference.

It's just another layer of meaningless obfuscation. Explain what the
parameter does in a comment and be done with it.

[U-Boot] [PATCH v3 1/7] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/10/19 5:47 PM, Michal Suchánek wrote:
> On Fri, 5 Jul 2019 14:12:36 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/5/19 12:44 PM, Michal Suchanek wrote:
>>> Causes unbound key repeat on error otherwise.
>>>
>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>> ---
>>> v2: fix indentation  
>>
>> What changed in V3 ?
>>
>>> ---
>>>  common/usb_kbd.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>>> index cc99c6be0720..fc9419e0238a 100644
>>> --- a/common/usb_kbd.c
>>> +++ b/common/usb_kbd.c
>>> @@ -339,10 +339,9 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>>>  	struct usb_kbd_pdata *data = dev->privptr;
>>>  
>>>  	/* Submit a interrupt transfer request */
>>> -	usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
>>> -			   data->intinterval);
>>> -
>>> -	usb_kbd_irq_worker(dev);
>>> +	if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
>>> +				  data->intpktsize, data->intinterval))  
>>
>> This still doesn't propagate errors.
> 
> Yes, it does not. I don't have a grand design that would make use of
> these propagated errors so I don't know what errors to propagate.

Each and every called of usb_kbd_poll_for_event() returns some return
value, so these functions should at least be aware of the new errors
which might come from usb_kbd_poll_for_event().

> You could, say, want to propagate errors from kbd_irq_worker as well.
> 
> Until there is a way to make use of the errors you really don't know.
> 
> Might be good idea to document that getting EAGAIN from
> usb_int_msg_nonblock is expected.


[...]

[U-Boot] [PATCH v3 1/7] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Fri, 12 Jul 2019 06:05:45 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/10/19 5:47 PM, Michal Suchánek wrote:
> > On Fri, 5 Jul 2019 14:12:36 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 7/5/19 12:44 PM, Michal Suchanek wrote:  
> >>> Causes unbound key repeat on error otherwise.
> >>>
> >>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >>> ---
> >>> v2: fix indentation    
> >>
> >> What changed in V3 ?
> >>  
> >>> ---
> >>>  common/usb_kbd.c | 7 +++----
> >>>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> >>> index cc99c6be0720..fc9419e0238a 100644
> >>> --- a/common/usb_kbd.c
> >>> +++ b/common/usb_kbd.c
> >>> @@ -339,10 +339,9 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
> >>>  	struct usb_kbd_pdata *data = dev->privptr;
> >>>  
> >>>  	/* Submit a interrupt transfer request */
> >>> -	usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
> >>> -			   data->intinterval);
> >>> -
> >>> -	usb_kbd_irq_worker(dev);
> >>> +	if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
> >>> +				  data->intpktsize, data->intinterval))    
> >>
> >> This still doesn't propagate errors.  
> > 
> > Yes, it does not. I don't have a grand design that would make use of
> > these propagated errors so I don't know what errors to propagate.  
> 
> Each and every called of usb_kbd_poll_for_event() returns some return
> value, 
A void
> so these functions should at least be aware of the new errors
> which might come from usb_kbd_poll_for_event().

No errors come from the function. The u-boot input flow is designed for
usb_kbd_poll_for_event handling errors internally.

The function is only used by the usb_kbd specific testc and getc. The
input users are supposed to use testc to see if there is input
available and getc to get input (and it should not block if testc
indicated that there is input). The result of testc is treated as
boolean so there is no room for error handling. The result of getc is
treated as character input and I found no user that implements any kind
of error handling. Also at the getc time the testc error is no longer
available.

If you want to redesign it to handle errors in the caller then please
do share the design. Without that it is not clear which errors make
sense to propagate.

Thanks

Michal

[U-Boot] [PATCH v3 1/7] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/12/19 4:24 PM, Michal Suchánek wrote:
> On Fri, 12 Jul 2019 06:05:45 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/10/19 5:47 PM, Michal Suchánek wrote:
>>> On Fri, 5 Jul 2019 14:12:36 +0200
>>> Marek Vasut <marex@denx.de> wrote:
>>>   
>>>> On 7/5/19 12:44 PM, Michal Suchanek wrote:  
>>>>> Causes unbound key repeat on error otherwise.
>>>>>
>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>>>> ---
>>>>> v2: fix indentation    
>>>>
>>>> What changed in V3 ?
>>>>  
>>>>> ---
>>>>>  common/usb_kbd.c | 7 +++----
>>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>>>>> index cc99c6be0720..fc9419e0238a 100644
>>>>> --- a/common/usb_kbd.c
>>>>> +++ b/common/usb_kbd.c
>>>>> @@ -339,10 +339,9 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
>>>>>  	struct usb_kbd_pdata *data = dev->privptr;
>>>>>  
>>>>>  	/* Submit a interrupt transfer request */
>>>>> -	usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
>>>>> -			   data->intinterval);
>>>>> -
>>>>> -	usb_kbd_irq_worker(dev);
>>>>> +	if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
>>>>> +				  data->intpktsize, data->intinterval))    
>>>>
>>>> This still doesn't propagate errors.  
>>>
>>> Yes, it does not. I don't have a grand design that would make use of
>>> these propagated errors so I don't know what errors to propagate.  
>>
>> Each and every called of usb_kbd_poll_for_event() returns some return
>> value, 
> A void
>> so these functions should at least be aware of the new errors
>> which might come from usb_kbd_poll_for_event().
> 
> No errors come from the function. The u-boot input flow is designed for
> usb_kbd_poll_for_event handling errors internally.
> 
> The function is only used by the usb_kbd specific testc and getc. The
> input users are supposed to use testc to see if there is input
> available and getc to get input (and it should not block if testc
> indicated that there is input). The result of testc is treated as
> boolean so there is no room for error handling. The result of getc is
> treated as character input and I found no user that implements any kind
> of error handling. Also at the getc time the testc error is no longer
> available.
> 
> If you want to redesign it to handle errors in the caller then please
> do share the design. Without that it is not clear which errors make
> sense to propagate.

Presumably, usb_kbd_tstc() should return 0 if usb_kbd_poll_for_event()
returns an error code or somesuch ?

[U-Boot] [PATCH v3 1/7] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Mon, 15 Jul 2019 06:28:01 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/12/19 4:24 PM, Michal Suchánek wrote:
> > On Fri, 12 Jul 2019 06:05:45 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 7/10/19 5:47 PM, Michal Suchánek wrote:  
> >>> On Fri, 5 Jul 2019 14:12:36 +0200
> >>> Marek Vasut <marex@denx.de> wrote:
> >>>     
> >>>> On 7/5/19 12:44 PM, Michal Suchanek wrote:    
> >>>>> Causes unbound key repeat on error otherwise.
> >>>>>
> >>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >>>>> ---
> >>>>> v2: fix indentation      
> >>>>
> >>>> What changed in V3 ?
> >>>>    
> >>>>> ---
> >>>>>  common/usb_kbd.c | 7 +++----
> >>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> >>>>> index cc99c6be0720..fc9419e0238a 100644
> >>>>> --- a/common/usb_kbd.c
> >>>>> +++ b/common/usb_kbd.c
> >>>>> @@ -339,10 +339,9 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
> >>>>>  	struct usb_kbd_pdata *data = dev->privptr;
> >>>>>  
> >>>>>  	/* Submit a interrupt transfer request */
> >>>>> -	usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
> >>>>> -			   data->intinterval);
> >>>>> -
> >>>>> -	usb_kbd_irq_worker(dev);
> >>>>> +	if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],
> >>>>> +				  data->intpktsize, data->intinterval))      
> >>>>
> >>>> This still doesn't propagate errors.    
> >>>
> >>> Yes, it does not. I don't have a grand design that would make use of
> >>> these propagated errors so I don't know what errors to propagate.    
> >>
> >> Each and every called of usb_kbd_poll_for_event() returns some return
> >> value,   
> > A void  
> >> so these functions should at least be aware of the new errors
> >> which might come from usb_kbd_poll_for_event().  
> > 
> > No errors come from the function. The u-boot input flow is designed for
> > usb_kbd_poll_for_event handling errors internally.
> > 
> > The function is only used by the usb_kbd specific testc and getc. The
> > input users are supposed to use testc to see if there is input
> > available and getc to get input (and it should not block if testc
> > indicated that there is input). The result of testc is treated as
> > boolean so there is no room for error handling. The result of getc is
> > treated as character input and I found no user that implements any kind
> > of error handling. Also at the getc time the testc error is no longer
> > available.
> > 
> > If you want to redesign it to handle errors in the caller then please
> > do share the design. Without that it is not clear which errors make
> > sense to propagate.  
> 
> Presumably, usb_kbd_tstc() should return 0 if usb_kbd_poll_for_event()
> returns an error code or somesuch ?

The return value only reflects if there are characters in the buffer.
Technically there can be pre-existing characters in the buffer even if
error was returned, and there can be no characters even if keyboard
state was successfully received. Handling the error here with the
existing interface does not make sense.

Thanks

Michal