[U-Boot] [PATCH 1/8] usb: r8a66597: return -ENOTSUPP from unimplemented submit_int_msg

[U-Boot] [PATCH 1/8] usb: r8a66597: return -ENOTSUPP from unimplemented submit_int_msg

[Michal Suchanek]

---
 drivers/usb/host/r8a66597-hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 3c263e51c160..7b699d3f4788 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -826,7 +826,7 @@ int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 {
 	/* no implement */
 	R8A66597_DPRINT("%s\n", __func__);
-	return 0;
+	return -ENOTSUPP;
 }
 
 int usb_lowlevel_init(int index, enum usb_init_type init, void **controller)
-- 
2.21.0

[U-Boot] [PATCH 2/8] usb: sl811-hcd: return -ENOTSUPP from unimplemented submit_int_msg

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 drivers/usb/host/sl811-hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/sl811-hcd.c b/drivers/usb/host/sl811-hcd.c
index daba0dcd1aee..4fd2ad464312 100644
--- a/drivers/usb/host/sl811-hcd.c
+++ b/drivers/usb/host/sl811-hcd.c
@@ -388,7 +388,7 @@ int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 {
 	PDEBUG(0, "dev = %p pipe = %#lx buf = %p size = %d int = %d\n", dev, pipe,
 	       buffer, len, interval);
-	return -1;
+	return -ENOTSUPP;
 }
 
 /*
-- 
2.21.0

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchanek]

Causes unbound key repeat on error otherwise.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 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..948f9fd68490 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 4/8] usb: storage: submit_int_msg -> usb_submit_int_msg

[Michal Suchanek]

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

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

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 8c889bb1a648..e589e9addc6a 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -650,7 +650,7 @@ 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,
+	usb_submit_int_msg(us->pusb_dev, us->irqpipe,
 			(void *) &us->ip_data, us->irqmaxp, us->irqinterval);
 	timeout = 1000;
 	while (timeout--) {
-- 
2.21.0

[U-Boot] [PATCH 5/8] 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>
---
 common/usb.c                      |  2 +-
 drivers/usb/host/dwc2.c           |  8 +++++---
 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, 37 insertions(+), 27 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index b70f614d244f..3ae71c98aaf4 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -197,7 +197,7 @@ int usb_disable_asynch(int disable)
 int usb_submit_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..3a44f5b14509 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 */
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 7b699d3f4788..fe23a88733be 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 4fd2ad464312..21872285feeb 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 420a30e49fa1..dde3e601da45 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 6/8] usb: add usb_submit_int_msg_nonblock

[Michal Suchanek]

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

diff --git a/common/usb.c b/common/usb.c
index 3ae71c98aaf4..d8302d39a91a 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -200,6 +200,15 @@ int usb_submit_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_submit_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 dde3e601da45..d1e7112f4c4e 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_submit_int_msg(struct usb_device *dev, unsigned long pipe,
 			void *buffer, int transfer_len, int interval);
+int usb_submit_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 7/8] usb: kbd: use usb_submit_int_msg_nonblock for polling

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 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 948f9fd68490..23a56f7cb305 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_submit_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 8/8] dwc2: use the nonblock argument in submit_int_msg

[Michal Suchanek]

An USB 1.1 keybaord connected to dwc2 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 an the
probe times out. We don't want to wait for a keypress or timeout while
polling for keypresses so implement the nonblock variant that exits early.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 drivers/usb/host/dwc2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 3a44f5b14509..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;
 	}
 }
@@ -1294,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)
-- 
2.21.0

[U-Boot] [PATCH 1/8] usb: r8a66597: return -ENOTSUPP from unimplemented submit_int_msg

[Marek Vasut]

On 7/1/19 5:56 PM, Michal Suchanek wrote:

SoB line missing, commit message missing, please fix globally.

> ---
>  drivers/usb/host/r8a66597-hcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
> index 3c263e51c160..7b699d3f4788 100644
> --- a/drivers/usb/host/r8a66597-hcd.c
> +++ b/drivers/usb/host/r8a66597-hcd.c
> @@ -826,7 +826,7 @@ int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
>  {
>  	/* no implement */
>  	R8A66597_DPRINT("%s\n", __func__);
> -	return 0;
> +	return -ENOTSUPP;
>  }
>  
>  int usb_lowlevel_init(int index, enum usb_init_type init, void **controller)
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/1/19 5:56 PM, Michal Suchanek wrote:
> Causes unbound key repeat on error otherwise.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>  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..948f9fd68490 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],

Shouldn't you propagate return value from this function ? It can return
ENOTSUPP.

> +					 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)
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Marek Vasut]

On 7/1/19 5:56 PM, Michal Suchanek wrote:
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>  common/usb.c  | 9 +++++++++
>  include/usb.h | 2 ++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/common/usb.c b/common/usb.c
> index 3ae71c98aaf4..d8302d39a91a 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> +}

Is this wrapper really necessary ?

>  /*
>   * 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 dde3e601da45..d1e7112f4c4e 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_submit_int_msg(struct usb_device *dev, unsigned long pipe,
>  			void *buffer, int transfer_len, int interval);
> +int usb_submit_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,
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Tue, 2 Jul 2019 13:58:30 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/1/19 5:56 PM, Michal Suchanek wrote:
> > Causes unbound key repeat on error otherwise.
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >  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..948f9fd68490 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],  
> 
> Shouldn't you propagate return value from this function ? It can return
> ENOTSUPP.
> 

If it did then probing keyboard would fail and we would not get here.

Thanks

Michal

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/2/19 3:04 PM, Michal Suchánek wrote:
> On Tue, 2 Jul 2019 13:58:30 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/1/19 5:56 PM, Michal Suchanek wrote:
>>> Causes unbound key repeat on error otherwise.
>>>
>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>> ---
>>>  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..948f9fd68490 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],  
>>
>> Shouldn't you propagate return value from this function ? It can return
>> ENOTSUPP.
>>
> 
> If it did then probing keyboard would fail and we would not get here.

So there is no chance this function could return an error here, ever ?
E.g. what if it's implemented and someone yanks the keyboard cable out
just at the right time ?

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Michal Suchánek]

On Tue, 2 Jul 2019 13:59:49 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/1/19 5:56 PM, Michal Suchanek wrote:
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >  common/usb.c  | 9 +++++++++
> >  include/usb.h | 2 ++
> >  2 files changed, 11 insertions(+)
> > 
> > diff --git a/common/usb.c b/common/usb.c
> > index 3ae71c98aaf4..d8302d39a91a 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> > +}  
> 
> Is this wrapper really necessary ?
> 

Avoids changing other users of the code. Not that there are that many,
anyway.

Thanks

Michal

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Bin Meng]

Hi Michal,

On Tue, Jul 2, 2019 at 10:14 PM Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Tue, 2 Jul 2019 13:59:49 +0200
> Marek Vasut <marex@denx.de> wrote:
>
> > On 7/1/19 5:56 PM, Michal Suchanek wrote:
> > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > ---
> > >  common/usb.c  | 9 +++++++++
> > >  include/usb.h | 2 ++
> > >  2 files changed, 11 insertions(+)
> > >
> > > diff --git a/common/usb.c b/common/usb.c
> > > index 3ae71c98aaf4..d8302d39a91a 100644
> > > --- a/common/usb.c
> > > +++ b/common/usb.c
> > > @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> > > +}
> >
> > Is this wrapper really necessary ?
> >
>
> Avoids changing other users of the code. Not that there are that many,
> anyway.

Then I wonder why not change your codes to call submit_int_msg() instead?

Regards,
Bin

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Tue, 2 Jul 2019 15:11:07 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/2/19 3:04 PM, Michal Suchánek wrote:
> > On Tue, 2 Jul 2019 13:58:30 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 7/1/19 5:56 PM, Michal Suchanek wrote:  
> >>> Causes unbound key repeat on error otherwise.
> >>>
> >>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >>> ---
> >>>  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..948f9fd68490 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],    
> >>
> >> Shouldn't you propagate return value from this function ? It can return
> >> ENOTSUPP.
> >>  
> > 
> > If it did then probing keyboard would fail and we would not get here.  
> 
> So there is no chance this function could return an error here, ever ?
> E.g. what if it's implemented and someone yanks the keyboard cable out
> just at the right time ?

It returns errors all the time with dwc2. That's why we need to check
for the error condition. We should not get here if probing the keyboard
failed, though. So if the function is not supported we will not get
here. Anyway, if it's not supported or the keyboard is missing it by
definition cannot provide useful result so we should not process it.

Thanks

Michal

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Michal Suchánek]

On Tue, 2 Jul 2019 22:16:02 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:

> Hi Michal,
> 
> On Tue, Jul 2, 2019 at 10:14 PM Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Tue, 2 Jul 2019 13:59:49 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >  
> > > On 7/1/19 5:56 PM, Michal Suchanek wrote:  
> > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > ---
> > > >  common/usb.c  | 9 +++++++++
> > > >  include/usb.h | 2 ++
> > > >  2 files changed, 11 insertions(+)
> > > >
> > > > diff --git a/common/usb.c b/common/usb.c
> > > > index 3ae71c98aaf4..d8302d39a91a 100644
> > > > --- a/common/usb.c
> > > > +++ b/common/usb.c
> > > > @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> > > > +}  
> > >
> > > Is this wrapper really necessary ?
> > >  
> >
> > Avoids changing other users of the code. Not that there are that many,
> > anyway.  
> 
> Then I wonder why not change your codes to call submit_int_msg() instead?
> 
Why do we have usb_submit_int_msg in the first place?

Thanks

Michal

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Bin Meng]

Hi Michal,

On Tue, Jul 2, 2019 at 10:25 PM Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Tue, 2 Jul 2019 22:16:02 +0800
> Bin Meng <bmeng.cn@gmail.com> wrote:
>
> > Hi Michal,
> >
> > On Tue, Jul 2, 2019 at 10:14 PM Michal Suchánek <msuchanek@suse.de> wrote:
> > >
> > > On Tue, 2 Jul 2019 13:59:49 +0200
> > > Marek Vasut <marex@denx.de> wrote:
> > >
> > > > On 7/1/19 5:56 PM, Michal Suchanek wrote:
> > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > ---
> > > > >  common/usb.c  | 9 +++++++++
> > > > >  include/usb.h | 2 ++
> > > > >  2 files changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/common/usb.c b/common/usb.c
> > > > > index 3ae71c98aaf4..d8302d39a91a 100644
> > > > > --- a/common/usb.c
> > > > > +++ b/common/usb.c
> > > > > @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> > > > > +}
> > > >
> > > > Is this wrapper really necessary ?
> > > >
> > >
> > > Avoids changing other users of the code. Not that there are that many,
> > > anyway.
> >
> > Then I wonder why not change your codes to call submit_int_msg() instead?
> >
> Why do we have usb_submit_int_msg in the first place?

I am happy to remove that. A patch is welcome. :)

Regards,
Bin

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Michal Suchánek]

On Tue, 2 Jul 2019 22:29:42 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:

> Hi Michal,
> 
> On Tue, Jul 2, 2019 at 10:25 PM Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Tue, 2 Jul 2019 22:16:02 +0800
> > Bin Meng <bmeng.cn@gmail.com> wrote:
> >  
> > > Hi Michal,
> > >
> > > On Tue, Jul 2, 2019 at 10:14 PM Michal Suchánek <msuchanek@suse.de> wrote:  
> > > >
> > > > On Tue, 2 Jul 2019 13:59:49 +0200
> > > > Marek Vasut <marex@denx.de> wrote:
> > > >  
> > > > > On 7/1/19 5:56 PM, Michal Suchanek wrote:  
> > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > ---
> > > > > >  common/usb.c  | 9 +++++++++
> > > > > >  include/usb.h | 2 ++
> > > > > >  2 files changed, 11 insertions(+)
> > > > > >
> > > > > > diff --git a/common/usb.c b/common/usb.c
> > > > > > index 3ae71c98aaf4..d8302d39a91a 100644
> > > > > > --- a/common/usb.c
> > > > > > +++ b/common/usb.c
> > > > > > @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> > > > > > +}  
> > > > >
> > > > > Is this wrapper really necessary ?
> > > > >  
> > > >
> > > > Avoids changing other users of the code. Not that there are that many,
> > > > anyway.  
> > >
> > > Then I wonder why not change your codes to call submit_int_msg() instead?
> > >  
> > Why do we have usb_submit_int_msg in the first place?  
> 
> I am happy to remove that. A patch is welcome. :)
> 

So the answer is we have wrappers for USB messages, and while
submit_int_msg is trivial others are not. To keep reasonable interface
all messages should be wrapped.

Thanks

Michal

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Bin Meng]

Hi Michal,

On Tue, Jul 2, 2019 at 10:58 PM Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Tue, 2 Jul 2019 22:29:42 +0800
> Bin Meng <bmeng.cn@gmail.com> wrote:
>
> > Hi Michal,
> >
> > On Tue, Jul 2, 2019 at 10:25 PM Michal Suchánek <msuchanek@suse.de> wrote:
> > >
> > > On Tue, 2 Jul 2019 22:16:02 +0800
> > > Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > > Hi Michal,
> > > >
> > > > On Tue, Jul 2, 2019 at 10:14 PM Michal Suchánek <msuchanek@suse.de> wrote:
> > > > >
> > > > > On Tue, 2 Jul 2019 13:59:49 +0200
> > > > > Marek Vasut <marex@denx.de> wrote:
> > > > >
> > > > > > On 7/1/19 5:56 PM, Michal Suchanek wrote:
> > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > ---
> > > > > > >  common/usb.c  | 9 +++++++++
> > > > > > >  include/usb.h | 2 ++
> > > > > > >  2 files changed, 11 insertions(+)
> > > > > > >
> > > > > > > diff --git a/common/usb.c b/common/usb.c
> > > > > > > index 3ae71c98aaf4..d8302d39a91a 100644
> > > > > > > --- a/common/usb.c
> > > > > > > +++ b/common/usb.c
> > > > > > > @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> > > > > > > +}
> > > > > >
> > > > > > Is this wrapper really necessary ?
> > > > > >
> > > > >
> > > > > Avoids changing other users of the code. Not that there are that many,
> > > > > anyway.
> > > >
> > > > Then I wonder why not change your codes to call submit_int_msg() instead?
> > > >
> > > Why do we have usb_submit_int_msg in the first place?
> >
> > I am happy to remove that. A patch is welcome. :)
> >
>
> So the answer is we have wrappers for USB messages, and while
> submit_int_msg is trivial others are not. To keep reasonable interface
> all messages should be wrapped.

I don't see usb_submit_control_msg() or usb_submit_bulk_msg(), so I
think we should remove usb_submit_int_msg() and just leave
submit_int_msg() for drivers to call.

Regards,
Bin

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Michal Suchánek]

On Tue, 2 Jul 2019 23:11:01 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:

> Hi Michal,
> 
> On Tue, Jul 2, 2019 at 10:58 PM Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Tue, 2 Jul 2019 22:29:42 +0800
> > Bin Meng <bmeng.cn@gmail.com> wrote:
> >  
> > > Hi Michal,
> > >
> > > On Tue, Jul 2, 2019 at 10:25 PM Michal Suchánek <msuchanek@suse.de> wrote:  
> > > >
> > > > On Tue, 2 Jul 2019 22:16:02 +0800
> > > > Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >  
> > > > > Hi Michal,
> > > > >
> > > > > On Tue, Jul 2, 2019 at 10:14 PM Michal Suchánek <msuchanek@suse.de> wrote:  
> > > > > >
> > > > > > On Tue, 2 Jul 2019 13:59:49 +0200
> > > > > > Marek Vasut <marex@denx.de> wrote:
> > > > > >  
> > > > > > > On 7/1/19 5:56 PM, Michal Suchanek wrote:  
> > > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > > ---
> > > > > > > >  common/usb.c  | 9 +++++++++
> > > > > > > >  include/usb.h | 2 ++
> > > > > > > >  2 files changed, 11 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/common/usb.c b/common/usb.c
> > > > > > > > index 3ae71c98aaf4..d8302d39a91a 100644
> > > > > > > > --- a/common/usb.c
> > > > > > > > +++ b/common/usb.c
> > > > > > > > @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> > > > > > > > +}  
> > > > > > >
> > > > > > > Is this wrapper really necessary ?
> > > > > > >  
> > > > > >
> > > > > > Avoids changing other users of the code. Not that there are that many,
> > > > > > anyway.  
> > > > >
> > > > > Then I wonder why not change your codes to call submit_int_msg() instead?
> > > > >  
> > > > Why do we have usb_submit_int_msg in the first place?  
> > >
> > > I am happy to remove that. A patch is welcome. :)
> > >  
> >
> > So the answer is we have wrappers for USB messages, and while
> > submit_int_msg is trivial others are not. To keep reasonable interface
> > all messages should be wrapped.  
> 
> I don't see usb_submit_control_msg() or usb_submit_bulk_msg(), so I
> think we should remove usb_submit_int_msg() and just leave
> submit_int_msg() for drivers to call.

There is no _submit_ in those. See include/usb.h

Thanks

Michal

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Bin Meng]

On Tue, Jul 2, 2019 at 11:30 PM Michal Suchánek <msuchanek@suse.de> wrote:
>
> On Tue, 2 Jul 2019 23:11:01 +0800
> Bin Meng <bmeng.cn@gmail.com> wrote:
>
> > Hi Michal,
> >
> > On Tue, Jul 2, 2019 at 10:58 PM Michal Suchánek <msuchanek@suse.de> wrote:
> > >
> > > On Tue, 2 Jul 2019 22:29:42 +0800
> > > Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > > Hi Michal,
> > > >
> > > > On Tue, Jul 2, 2019 at 10:25 PM Michal Suchánek <msuchanek@suse.de> wrote:
> > > > >
> > > > > On Tue, 2 Jul 2019 22:16:02 +0800
> > > > > Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > > Hi Michal,
> > > > > >
> > > > > > On Tue, Jul 2, 2019 at 10:14 PM Michal Suchánek <msuchanek@suse.de> wrote:
> > > > > > >
> > > > > > > On Tue, 2 Jul 2019 13:59:49 +0200
> > > > > > > Marek Vasut <marex@denx.de> wrote:
> > > > > > >
> > > > > > > > On 7/1/19 5:56 PM, Michal Suchanek wrote:
> > > > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > > > ---
> > > > > > > > >  common/usb.c  | 9 +++++++++
> > > > > > > > >  include/usb.h | 2 ++
> > > > > > > > >  2 files changed, 11 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/common/usb.c b/common/usb.c
> > > > > > > > > index 3ae71c98aaf4..d8302d39a91a 100644
> > > > > > > > > --- a/common/usb.c
> > > > > > > > > +++ b/common/usb.c
> > > > > > > > > @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> > > > > > > > > +}
> > > > > > > >
> > > > > > > > Is this wrapper really necessary ?
> > > > > > > >
> > > > > > >
> > > > > > > Avoids changing other users of the code. Not that there are that many,
> > > > > > > anyway.
> > > > > >
> > > > > > Then I wonder why not change your codes to call submit_int_msg() instead?
> > > > > >
> > > > > Why do we have usb_submit_int_msg in the first place?
> > > >
> > > > I am happy to remove that. A patch is welcome. :)
> > > >
> > >
> > > So the answer is we have wrappers for USB messages, and while
> > > submit_int_msg is trivial others are not. To keep reasonable interface
> > > all messages should be wrapped.
> >
> > I don't see usb_submit_control_msg() or usb_submit_bulk_msg(), so I
> > think we should remove usb_submit_int_msg() and just leave
> > submit_int_msg() for drivers to call.
>
> There is no _submit_ in those. See include/usb.h
>

OK, so we should just rename usb_submit_int_msg() to usb_int_msg() and
everything is consistent, no?

Regards,
Bin

[U-Boot] [PATCH 6/8] usb: add usb_submit_int_msg_nonblock

[Michal Suchánek]

On Tue, 2 Jul 2019 23:39:42 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:

> On Tue, Jul 2, 2019 at 11:30 PM Michal Suchánek <msuchanek@suse.de> wrote:
> >
> > On Tue, 2 Jul 2019 23:11:01 +0800
> > Bin Meng <bmeng.cn@gmail.com> wrote:
> >  
> > > Hi Michal,
> > >
> > > On Tue, Jul 2, 2019 at 10:58 PM Michal Suchánek <msuchanek@suse.de> wrote:  
> > > >
> > > > On Tue, 2 Jul 2019 22:29:42 +0800
> > > > Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >  
> > > > > Hi Michal,
> > > > >
> > > > > On Tue, Jul 2, 2019 at 10:25 PM Michal Suchánek <msuchanek@suse.de> wrote:  
> > > > > >
> > > > > > On Tue, 2 Jul 2019 22:16:02 +0800
> > > > > > Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > >  
> > > > > > > Hi Michal,
> > > > > > >
> > > > > > > On Tue, Jul 2, 2019 at 10:14 PM Michal Suchánek <msuchanek@suse.de> wrote:  
> > > > > > > >
> > > > > > > > On Tue, 2 Jul 2019 13:59:49 +0200
> > > > > > > > Marek Vasut <marex@denx.de> wrote:
> > > > > > > >  
> > > > > > > > > On 7/1/19 5:56 PM, Michal Suchanek wrote:  
> > > > > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > > > > ---
> > > > > > > > > >  common/usb.c  | 9 +++++++++
> > > > > > > > > >  include/usb.h | 2 ++
> > > > > > > > > >  2 files changed, 11 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/common/usb.c b/common/usb.c
> > > > > > > > > > index 3ae71c98aaf4..d8302d39a91a 100644
> > > > > > > > > > --- a/common/usb.c
> > > > > > > > > > +++ b/common/usb.c
> > > > > > > > > > @@ -200,6 +200,15 @@ int usb_submit_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_submit_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);
> > > > > > > > > > +}  
> > > > > > > > >
> > > > > > > > > Is this wrapper really necessary ?
> > > > > > > > >  
> > > > > > > >
> > > > > > > > Avoids changing other users of the code. Not that there are that many,
> > > > > > > > anyway.  
> > > > > > >
> > > > > > > Then I wonder why not change your codes to call submit_int_msg() instead?
> > > > > > >  
> > > > > > Why do we have usb_submit_int_msg in the first place?  
> > > > >
> > > > > I am happy to remove that. A patch is welcome. :)
> > > > >  
> > > >
> > > > So the answer is we have wrappers for USB messages, and while
> > > > submit_int_msg is trivial others are not. To keep reasonable interface
> > > > all messages should be wrapped.  
> > >
> > > I don't see usb_submit_control_msg() or usb_submit_bulk_msg(), so I
> > > think we should remove usb_submit_int_msg() and just leave
> > > submit_int_msg() for drivers to call.  
> >
> > There is no _submit_ in those. See include/usb.h
> >  
> 
> OK, so we should just rename usb_submit_int_msg() to usb_int_msg() and
> everything is consistent, no?
> 

Yes, looks like the _submit in usb_submit_int_msg is superfluous.


Thanks

Michal

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/2/19 4:22 PM, Michal Suchánek wrote:
> On Tue, 2 Jul 2019 15:11:07 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/2/19 3:04 PM, Michal Suchánek wrote:
>>> On Tue, 2 Jul 2019 13:58:30 +0200
>>> Marek Vasut <marex@denx.de> wrote:
>>>   
>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:  
>>>>> Causes unbound key repeat on error otherwise.
>>>>>
>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>>>> ---
>>>>>  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..948f9fd68490 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],    
>>>>
>>>> Shouldn't you propagate return value from this function ? It can return
>>>> ENOTSUPP.
>>>>  
>>>
>>> If it did then probing keyboard would fail and we would not get here.  
>>
>> So there is no chance this function could return an error here, ever ?
>> E.g. what if it's implemented and someone yanks the keyboard cable out
>> just at the right time ?
> 
> It returns errors all the time with dwc2. That's why we need to check
> for the error condition. We should not get here if probing the keyboard
> failed, though. So if the function is not supported we will not get
> here. Anyway, if it's not supported or the keyboard is missing it by
> definition cannot provide useful result so we should not process it.

Except you start ignoring the error value from e.g. malfunctioning
keyboard here, instead of propagating it, correct ?

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Tue, 2 Jul 2019 18:58:54 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/2/19 4:22 PM, Michal Suchánek wrote:
> > On Tue, 2 Jul 2019 15:11:07 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 7/2/19 3:04 PM, Michal Suchánek wrote:  
> >>> On Tue, 2 Jul 2019 13:58:30 +0200
> >>> Marek Vasut <marex@denx.de> wrote:
> >>>     
> >>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:    
> >>>>> Causes unbound key repeat on error otherwise.
> >>>>>
> >>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >>>>> ---
> >>>>>  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..948f9fd68490 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],      
> >>>>
> >>>> Shouldn't you propagate return value from this function ? It can return
> >>>> ENOTSUPP.
> >>>>    
> >>>
> >>> If it did then probing keyboard would fail and we would not get here.    
> >>
> >> So there is no chance this function could return an error here, ever ?
> >> E.g. what if it's implemented and someone yanks the keyboard cable out
> >> just at the right time ?  
> > 
> > It returns errors all the time with dwc2. That's why we need to check
> > for the error condition. We should not get here if probing the keyboard
> > failed, though. So if the function is not supported we will not get
> > here. Anyway, if it's not supported or the keyboard is missing it by
> > definition cannot provide useful result so we should not process it.  
> 
> Except you start ignoring the error value from e.g. malfunctioning
> keyboard here, instead of propagating it, correct ?

It was never propagated to start with. The return value was not checked
at all. What I do here is check the return value and not process the
data on error whatever it contains (like the keypress returned last
time valid data was received).

Thanks

Michal

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Tue, 2 Jul 2019 19:50:05 +0200
Michal Suchánek <msuchanek@suse.de> wrote:

> On Tue, 2 Jul 2019 18:58:54 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
> > On 7/2/19 4:22 PM, Michal Suchánek wrote:  
> > > On Tue, 2 Jul 2019 15:11:07 +0200
> > > Marek Vasut <marex@denx.de> wrote:
> > >     
> > >> On 7/2/19 3:04 PM, Michal Suchánek wrote:    
> > >>> On Tue, 2 Jul 2019 13:58:30 +0200
> > >>> Marek Vasut <marex@denx.de> wrote:
> > >>>       
> > >>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:      
> > >>>>> Causes unbound key repeat on error otherwise.
> > >>>>>
> > >>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > >>>>> ---
> > >>>>>  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..948f9fd68490 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],        
> > >>>>
> > >>>> Shouldn't you propagate return value from this function ? It can return
> > >>>> ENOTSUPP.
> > >>>>      
> > >>>
> > >>> If it did then probing keyboard would fail and we would not get here.      
> > >>
> > >> So there is no chance this function could return an error here, ever ?
> > >> E.g. what if it's implemented and someone yanks the keyboard cable out
> > >> just at the right time ?    
> > > 
> > > It returns errors all the time with dwc2. That's why we need to check
> > > for the error condition. We should not get here if probing the keyboard
> > > failed, though. So if the function is not supported we will not get
> > > here. Anyway, if it's not supported or the keyboard is missing it by
> > > definition cannot provide useful result so we should not process it.    
> > 
> > Except you start ignoring the error value from e.g. malfunctioning
> > keyboard here, instead of propagating it, correct ?  
> 
> It was never propagated to start with. The return value was not checked
> at all. What I do here is check the return value and not process the
> data on error whatever it contains (like the keypress returned last
> time valid data was received).

Maybe the error here is using usb_kbd_irq_worker rather than
usb_kbd_irq which checks some status before calling usb_kbd_irq_worker.

Nonetheless, checking the return value from usb_submit_int_msg works
for me.

Thanks

Michal

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/2/19 7:50 PM, Michal Suchánek wrote:
> On Tue, 2 Jul 2019 18:58:54 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/2/19 4:22 PM, Michal Suchánek wrote:
>>> On Tue, 2 Jul 2019 15:11:07 +0200
>>> Marek Vasut <marex@denx.de> wrote:
>>>   
>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:  
>>>>> On Tue, 2 Jul 2019 13:58:30 +0200
>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>     
>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:    
>>>>>>> Causes unbound key repeat on error otherwise.
>>>>>>>
>>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>>>>>> ---
>>>>>>>  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..948f9fd68490 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],      
>>>>>>
>>>>>> Shouldn't you propagate return value from this function ? It can return
>>>>>> ENOTSUPP.
>>>>>>    
>>>>>
>>>>> If it did then probing keyboard would fail and we would not get here.    
>>>>
>>>> So there is no chance this function could return an error here, ever ?
>>>> E.g. what if it's implemented and someone yanks the keyboard cable out
>>>> just at the right time ?  
>>>
>>> It returns errors all the time with dwc2. That's why we need to check
>>> for the error condition. We should not get here if probing the keyboard
>>> failed, though. So if the function is not supported we will not get
>>> here. Anyway, if it's not supported or the keyboard is missing it by
>>> definition cannot provide useful result so we should not process it.  
>>
>> Except you start ignoring the error value from e.g. malfunctioning
>> keyboard here, instead of propagating it, correct ?
> 
> It was never propagated to start with. The return value was not checked
> at all. What I do here is check the return value and not process the
> data on error whatever it contains (like the keypress returned last
> time valid data was received).

I can see a patch which checks usb_kbd_poll_for_event() return value.
Can you add one ?

And then propagate the usb_submit_int_msg() return value too.

-- 
Best regards,
Marek Vasut

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-56 Email: marex at denx.de

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Tue, 2 Jul 2019 20:38:27 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/2/19 7:50 PM, Michal Suchánek wrote:
> > On Tue, 2 Jul 2019 18:58:54 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 7/2/19 4:22 PM, Michal Suchánek wrote:  
> >>> On Tue, 2 Jul 2019 15:11:07 +0200
> >>> Marek Vasut <marex@denx.de> wrote:
> >>>     
> >>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:    
> >>>>> On Tue, 2 Jul 2019 13:58:30 +0200
> >>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>       
> >>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:      
> >>>>>>> Causes unbound key repeat on error otherwise.
> >>>>>>>
> >>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >>>>>>> ---
> >>>>>>>  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..948f9fd68490 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],        
> >>>>>>
> >>>>>> Shouldn't you propagate return value from this function ? It can return
> >>>>>> ENOTSUPP.
> >>>>>>      
> >>>>>
> >>>>> If it did then probing keyboard would fail and we would not get here.      
> >>>>
> >>>> So there is no chance this function could return an error here, ever ?
> >>>> E.g. what if it's implemented and someone yanks the keyboard cable out
> >>>> just at the right time ?    
> >>>
> >>> It returns errors all the time with dwc2. That's why we need to check
> >>> for the error condition. We should not get here if probing the keyboard
> >>> failed, though. So if the function is not supported we will not get
> >>> here. Anyway, if it's not supported or the keyboard is missing it by
> >>> definition cannot provide useful result so we should not process it.    
> >>
> >> Except you start ignoring the error value from e.g. malfunctioning
> >> keyboard here, instead of propagating it, correct ?  
> > 
> > It was never propagated to start with. The return value was not checked
> > at all. What I do here is check the return value and not process the
> > data on error whatever it contains (like the keypress returned last
> > time valid data was received).  
> 
> I can see a patch which checks usb_kbd_poll_for_event() return value.
> Can you add one ?

What for? Apparently the keypress is processed in usb_kbd_irq_worker.
So checking the return value is needed to decide if the worker should
run, and is not particularly useful outside usb_kbd_poll_for_event. We
could signal a getc() failure but do we have any code handling getc()
failures?

Thanks

Michal

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/2/19 9:31 PM, Michal Suchánek wrote:
> On Tue, 2 Jul 2019 20:38:27 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/2/19 7:50 PM, Michal Suchánek wrote:
>>> On Tue, 2 Jul 2019 18:58:54 +0200
>>> Marek Vasut <marex@denx.de> wrote:
>>>   
>>>> On 7/2/19 4:22 PM, Michal Suchánek wrote:  
>>>>> On Tue, 2 Jul 2019 15:11:07 +0200
>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>     
>>>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:    
>>>>>>> On Tue, 2 Jul 2019 13:58:30 +0200
>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>       
>>>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:      
>>>>>>>>> Causes unbound key repeat on error otherwise.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>>>>>>>> ---
>>>>>>>>>  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..948f9fd68490 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],        
>>>>>>>>
>>>>>>>> Shouldn't you propagate return value from this function ? It can return
>>>>>>>> ENOTSUPP.
>>>>>>>>      
>>>>>>>
>>>>>>> If it did then probing keyboard would fail and we would not get here.      
>>>>>>
>>>>>> So there is no chance this function could return an error here, ever ?
>>>>>> E.g. what if it's implemented and someone yanks the keyboard cable out
>>>>>> just at the right time ?    
>>>>>
>>>>> It returns errors all the time with dwc2. That's why we need to check
>>>>> for the error condition. We should not get here if probing the keyboard
>>>>> failed, though. So if the function is not supported we will not get
>>>>> here. Anyway, if it's not supported or the keyboard is missing it by
>>>>> definition cannot provide useful result so we should not process it.    
>>>>
>>>> Except you start ignoring the error value from e.g. malfunctioning
>>>> keyboard here, instead of propagating it, correct ?  
>>>
>>> It was never propagated to start with. The return value was not checked
>>> at all. What I do here is check the return value and not process the
>>> data on error whatever it contains (like the keypress returned last
>>> time valid data was received).  
>>
>> I can see a patch which checks usb_kbd_poll_for_event() return value.
>> Can you add one ?
> 
> What for? Apparently the keypress is processed in usb_kbd_irq_worker.
> So checking the return value is needed to decide if the worker should
> run, and is not particularly useful outside usb_kbd_poll_for_event. We
> could signal a getc() failure but do we have any code handling getc()
> failures?

I presume getc() might signal EOF if the underlying hardware fails.
But in general, it's a good practice to not ignore errors.

-- 
Best regards,
Marek Vasut

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-56 Email: marex at denx.de

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Tue, 2 Jul 2019 23:20:28 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/2/19 9:31 PM, Michal Suchánek wrote:
> > On Tue, 2 Jul 2019 20:38:27 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 7/2/19 7:50 PM, Michal Suchánek wrote:  
> >>> On Tue, 2 Jul 2019 18:58:54 +0200
> >>> Marek Vasut <marex@denx.de> wrote:
> >>>     
> >>>> On 7/2/19 4:22 PM, Michal Suchánek wrote:    
> >>>>> On Tue, 2 Jul 2019 15:11:07 +0200
> >>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>       
> >>>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:      
> >>>>>>> On Tue, 2 Jul 2019 13:58:30 +0200
> >>>>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>>>         
> >>>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:        
> >>>>>>>>> Causes unbound key repeat on error otherwise.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >>>>>>>>> ---
> >>>>>>>>>  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..948f9fd68490 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],          
> >>>>>>>>
> >>>>>>>> Shouldn't you propagate return value from this function ? It can return
> >>>>>>>> ENOTSUPP.
> >>>>>>>>        
> >>>>>>>
> >>>>>>> If it did then probing keyboard would fail and we would not get here.        
> >>>>>>
> >>>>>> So there is no chance this function could return an error here, ever ?
> >>>>>> E.g. what if it's implemented and someone yanks the keyboard cable out
> >>>>>> just at the right time ?      
> >>>>>
> >>>>> It returns errors all the time with dwc2. That's why we need to check
> >>>>> for the error condition. We should not get here if probing the keyboard
> >>>>> failed, though. So if the function is not supported we will not get
> >>>>> here. Anyway, if it's not supported or the keyboard is missing it by
> >>>>> definition cannot provide useful result so we should not process it.      
> >>>>
> >>>> Except you start ignoring the error value from e.g. malfunctioning
> >>>> keyboard here, instead of propagating it, correct ?    
> >>>
> >>> It was never propagated to start with. The return value was not checked
> >>> at all. What I do here is check the return value and not process the
> >>> data on error whatever it contains (like the keypress returned last
> >>> time valid data was received).    
> >>
> >> I can see a patch which checks usb_kbd_poll_for_event() return value.
> >> Can you add one ?  
> > 
> > What for? Apparently the keypress is processed in usb_kbd_irq_worker.
> > So checking the return value is needed to decide if the worker should
> > run, and is not particularly useful outside usb_kbd_poll_for_event. We
> > could signal a getc() failure but do we have any code handling getc()
> > failures?  
> 
> I presume getc() might signal EOF if the underlying hardware fails.
> But in general, it's a good practice to not ignore errors.
> 

It is not such a great idea. You might have multiple input hardware (ie
serial and usb keyboard). What does it mean that usb keyboard failed in
this context?

So in my view the ultimate consumer of getc() has no use for the error
so there is no point in propagating it.

Thanks

Michal

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/3/19 11:46 AM, Michal Suchánek wrote:
> On Tue, 2 Jul 2019 23:20:28 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/2/19 9:31 PM, Michal Suchánek wrote:
>>> On Tue, 2 Jul 2019 20:38:27 +0200
>>> Marek Vasut <marex@denx.de> wrote:
>>>   
>>>> On 7/2/19 7:50 PM, Michal Suchánek wrote:  
>>>>> On Tue, 2 Jul 2019 18:58:54 +0200
>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>     
>>>>>> On 7/2/19 4:22 PM, Michal Suchánek wrote:    
>>>>>>> On Tue, 2 Jul 2019 15:11:07 +0200
>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>       
>>>>>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:      
>>>>>>>>> On Tue, 2 Jul 2019 13:58:30 +0200
>>>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>         
>>>>>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:        
>>>>>>>>>>> Causes unbound key repeat on error otherwise.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>>>>>>>>>> ---
>>>>>>>>>>>  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..948f9fd68490 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],          
>>>>>>>>>>
>>>>>>>>>> Shouldn't you propagate return value from this function ? It can return
>>>>>>>>>> ENOTSUPP.
>>>>>>>>>>        
>>>>>>>>>
>>>>>>>>> If it did then probing keyboard would fail and we would not get here.        
>>>>>>>>
>>>>>>>> So there is no chance this function could return an error here, ever ?
>>>>>>>> E.g. what if it's implemented and someone yanks the keyboard cable out
>>>>>>>> just at the right time ?      
>>>>>>>
>>>>>>> It returns errors all the time with dwc2. That's why we need to check
>>>>>>> for the error condition. We should not get here if probing the keyboard
>>>>>>> failed, though. So if the function is not supported we will not get
>>>>>>> here. Anyway, if it's not supported or the keyboard is missing it by
>>>>>>> definition cannot provide useful result so we should not process it.      
>>>>>>
>>>>>> Except you start ignoring the error value from e.g. malfunctioning
>>>>>> keyboard here, instead of propagating it, correct ?    
>>>>>
>>>>> It was never propagated to start with. The return value was not checked
>>>>> at all. What I do here is check the return value and not process the
>>>>> data on error whatever it contains (like the keypress returned last
>>>>> time valid data was received).    
>>>>
>>>> I can see a patch which checks usb_kbd_poll_for_event() return value.
>>>> Can you add one ?  
>>>
>>> What for? Apparently the keypress is processed in usb_kbd_irq_worker.
>>> So checking the return value is needed to decide if the worker should
>>> run, and is not particularly useful outside usb_kbd_poll_for_event. We
>>> could signal a getc() failure but do we have any code handling getc()
>>> failures?  
>>
>> I presume getc() might signal EOF if the underlying hardware fails.
>> But in general, it's a good practice to not ignore errors.
>>
> 
> It is not such a great idea. You might have multiple input hardware (ie
> serial and usb keyboard). What does it mean that usb keyboard failed in
> this context?

I'd say, the behavior is undefined ?

> So in my view the ultimate consumer of getc() has no use for the error
> so there is no point in propagating it.

Ignoring errors and not reporting them isn't nice either, so what other
option(s) do we have here ?

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Wed, 3 Jul 2019 13:26:50 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/3/19 11:46 AM, Michal Suchánek wrote:
> > On Tue, 2 Jul 2019 23:20:28 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 7/2/19 9:31 PM, Michal Suchánek wrote:  
> >>> On Tue, 2 Jul 2019 20:38:27 +0200
> >>> Marek Vasut <marex@denx.de> wrote:
> >>>     
> >>>> On 7/2/19 7:50 PM, Michal Suchánek wrote:    
> >>>>> On Tue, 2 Jul 2019 18:58:54 +0200
> >>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>       
> >>>>>> On 7/2/19 4:22 PM, Michal Suchánek wrote:      
> >>>>>>> On Tue, 2 Jul 2019 15:11:07 +0200
> >>>>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>>>         
> >>>>>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:        
> >>>>>>>>> On Tue, 2 Jul 2019 13:58:30 +0200
> >>>>>>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>           
> >>>>>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:          
> >>>>>>>>>>> Causes unbound key repeat on error otherwise.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  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..948f9fd68490 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],            
> >>>>>>>>>>
> >>>>>>>>>> Shouldn't you propagate return value from this function ? It can return
> >>>>>>>>>> ENOTSUPP.
> >>>>>>>>>>          
> >>>>>>>>>
> >>>>>>>>> If it did then probing keyboard would fail and we would not get here.          
> >>>>>>>>
> >>>>>>>> So there is no chance this function could return an error here, ever ?
> >>>>>>>> E.g. what if it's implemented and someone yanks the keyboard cable out
> >>>>>>>> just at the right time ?        
> >>>>>>>
> >>>>>>> It returns errors all the time with dwc2. That's why we need to check
> >>>>>>> for the error condition. We should not get here if probing the keyboard
> >>>>>>> failed, though. So if the function is not supported we will not get
> >>>>>>> here. Anyway, if it's not supported or the keyboard is missing it by
> >>>>>>> definition cannot provide useful result so we should not process it.        
> >>>>>>
> >>>>>> Except you start ignoring the error value from e.g. malfunctioning
> >>>>>> keyboard here, instead of propagating it, correct ?      
> >>>>>
> >>>>> It was never propagated to start with. The return value was not checked
> >>>>> at all. What I do here is check the return value and not process the
> >>>>> data on error whatever it contains (like the keypress returned last
> >>>>> time valid data was received).      
> >>>>
> >>>> I can see a patch which checks usb_kbd_poll_for_event() return value.
> >>>> Can you add one ?    
> >>>
> >>> What for? Apparently the keypress is processed in usb_kbd_irq_worker.
> >>> So checking the return value is needed to decide if the worker should
> >>> run, and is not particularly useful outside usb_kbd_poll_for_event. We
> >>> could signal a getc() failure but do we have any code handling getc()
> >>> failures?    
> >>
> >> I presume getc() might signal EOF if the underlying hardware fails.
> >> But in general, it's a good practice to not ignore errors.
> >>  
> > 
> > It is not such a great idea. You might have multiple input hardware (ie
> > serial and usb keyboard). What does it mean that usb keyboard failed in
> > this context?  
> 
> I'd say, the behavior is undefined ?

But we need to define it which the code does by ignoring the
device-specific error and relying on devices that are still working
(like a serial port) or for which error detection is not available
(like most serial ports).

> 
> > So in my view the ultimate consumer of getc() has no use for the error
> > so there is no point in propagating it.  
> 
> Ignoring errors and not reporting them isn't nice either, so what other
> option(s) do we have here ?

Ignoring the errors is exactly the desirable behavior when facing
broken hardware like dwc2. On non-broken hardware you will get fewer
errors to ignore. It is up to the device driver to report device
failure with a message when the error condition could be informative to
the user (such as previously working device going away completely).

Thanks

Michal

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/3/19 1:43 PM, Michal Suchánek wrote:
> On Wed, 3 Jul 2019 13:26:50 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/3/19 11:46 AM, Michal Suchánek wrote:
>>> On Tue, 2 Jul 2019 23:20:28 +0200
>>> Marek Vasut <marex@denx.de> wrote:
>>>   
>>>> On 7/2/19 9:31 PM, Michal Suchánek wrote:  
>>>>> On Tue, 2 Jul 2019 20:38:27 +0200
>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>     
>>>>>> On 7/2/19 7:50 PM, Michal Suchánek wrote:    
>>>>>>> On Tue, 2 Jul 2019 18:58:54 +0200
>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>       
>>>>>>>> On 7/2/19 4:22 PM, Michal Suchánek wrote:      
>>>>>>>>> On Tue, 2 Jul 2019 15:11:07 +0200
>>>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>         
>>>>>>>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:        
>>>>>>>>>>> On Tue, 2 Jul 2019 13:58:30 +0200
>>>>>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>           
>>>>>>>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:          
>>>>>>>>>>>>> Causes unbound key repeat on error otherwise.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>  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..948f9fd68490 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],            
>>>>>>>>>>>>
>>>>>>>>>>>> Shouldn't you propagate return value from this function ? It can return
>>>>>>>>>>>> ENOTSUPP.
>>>>>>>>>>>>          
>>>>>>>>>>>
>>>>>>>>>>> If it did then probing keyboard would fail and we would not get here.          
>>>>>>>>>>
>>>>>>>>>> So there is no chance this function could return an error here, ever ?
>>>>>>>>>> E.g. what if it's implemented and someone yanks the keyboard cable out
>>>>>>>>>> just at the right time ?        
>>>>>>>>>
>>>>>>>>> It returns errors all the time with dwc2. That's why we need to check
>>>>>>>>> for the error condition. We should not get here if probing the keyboard
>>>>>>>>> failed, though. So if the function is not supported we will not get
>>>>>>>>> here. Anyway, if it's not supported or the keyboard is missing it by
>>>>>>>>> definition cannot provide useful result so we should not process it.        
>>>>>>>>
>>>>>>>> Except you start ignoring the error value from e.g. malfunctioning
>>>>>>>> keyboard here, instead of propagating it, correct ?      
>>>>>>>
>>>>>>> It was never propagated to start with. The return value was not checked
>>>>>>> at all. What I do here is check the return value and not process the
>>>>>>> data on error whatever it contains (like the keypress returned last
>>>>>>> time valid data was received).      
>>>>>>
>>>>>> I can see a patch which checks usb_kbd_poll_for_event() return value.
>>>>>> Can you add one ?    
>>>>>
>>>>> What for? Apparently the keypress is processed in usb_kbd_irq_worker.
>>>>> So checking the return value is needed to decide if the worker should
>>>>> run, and is not particularly useful outside usb_kbd_poll_for_event. We
>>>>> could signal a getc() failure but do we have any code handling getc()
>>>>> failures?    
>>>>
>>>> I presume getc() might signal EOF if the underlying hardware fails.
>>>> But in general, it's a good practice to not ignore errors.
>>>>  
>>>
>>> It is not such a great idea. You might have multiple input hardware (ie
>>> serial and usb keyboard). What does it mean that usb keyboard failed in
>>> this context?  
>>
>> I'd say, the behavior is undefined ?
> 
> But we need to define it which the code does by ignoring the
> device-specific error and relying on devices that are still working
> (like a serial port) or for which error detection is not available
> (like most serial ports).

Maybe the error should still be propagated to the input layer , and not
ignored at the USB layer ?

>>> So in my view the ultimate consumer of getc() has no use for the error
>>> so there is no point in propagating it.  
>>
>> Ignoring errors and not reporting them isn't nice either, so what other
>> option(s) do we have here ?
> 
> Ignoring the errors is exactly the desirable behavior when facing
> broken hardware like dwc2. On non-broken hardware you will get fewer
> errors to ignore. It is up to the device driver to report device
> failure with a message when the error condition could be informative to
> the user (such as previously working device going away completely).

I thought this error is a keyboard failure though , and has nothing to
do with the USB controller ?

[...]

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Wed, 3 Jul 2019 13:48:00 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/3/19 1:43 PM, Michal Suchánek wrote:
> > On Wed, 3 Jul 2019 13:26:50 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 7/3/19 11:46 AM, Michal Suchánek wrote:  
> >>> On Tue, 2 Jul 2019 23:20:28 +0200
> >>> Marek Vasut <marex@denx.de> wrote:
> >>>     
> >>>> On 7/2/19 9:31 PM, Michal Suchánek wrote:    
> >>>>> On Tue, 2 Jul 2019 20:38:27 +0200
> >>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>       
> >>>>>> On 7/2/19 7:50 PM, Michal Suchánek wrote:      
> >>>>>>> On Tue, 2 Jul 2019 18:58:54 +0200
> >>>>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>>>         
> >>>>>>>> On 7/2/19 4:22 PM, Michal Suchánek wrote:        
> >>>>>>>>> On Tue, 2 Jul 2019 15:11:07 +0200
> >>>>>>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>           
> >>>>>>>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:          
> >>>>>>>>>>> On Tue, 2 Jul 2019 13:58:30 +0200
> >>>>>>>>>>> Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>>>             
> >>>>>>>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:            
> >>>>>>>>>>>>> Causes unbound key repeat on error otherwise.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>>  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..948f9fd68490 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],              
> >>>>>>>>>>>>
> >>>>>>>>>>>> Shouldn't you propagate return value from this function ? It can return
> >>>>>>>>>>>> ENOTSUPP.
> >>>>>>>>>>>>            
> >>>>>>>>>>>
> >>>>>>>>>>> If it did then probing keyboard would fail and we would not get here.            
> >>>>>>>>>>
> >>>>>>>>>> So there is no chance this function could return an error here, ever ?
> >>>>>>>>>> E.g. what if it's implemented and someone yanks the keyboard cable out
> >>>>>>>>>> just at the right time ?          
> >>>>>>>>>
> >>>>>>>>> It returns errors all the time with dwc2. That's why we need to check
> >>>>>>>>> for the error condition. We should not get here if probing the keyboard
> >>>>>>>>> failed, though. So if the function is not supported we will not get
> >>>>>>>>> here. Anyway, if it's not supported or the keyboard is missing it by
> >>>>>>>>> definition cannot provide useful result so we should not process it.          
> >>>>>>>>
> >>>>>>>> Except you start ignoring the error value from e.g. malfunctioning
> >>>>>>>> keyboard here, instead of propagating it, correct ?        
> >>>>>>>
> >>>>>>> It was never propagated to start with. The return value was not checked
> >>>>>>> at all. What I do here is check the return value and not process the
> >>>>>>> data on error whatever it contains (like the keypress returned last
> >>>>>>> time valid data was received).        
> >>>>>>
> >>>>>> I can see a patch which checks usb_kbd_poll_for_event() return value.
> >>>>>> Can you add one ?      
> >>>>>
> >>>>> What for? Apparently the keypress is processed in usb_kbd_irq_worker.
> >>>>> So checking the return value is needed to decide if the worker should
> >>>>> run, and is not particularly useful outside usb_kbd_poll_for_event. We
> >>>>> could signal a getc() failure but do we have any code handling getc()
> >>>>> failures?      
> >>>>
> >>>> I presume getc() might signal EOF if the underlying hardware fails.
> >>>> But in general, it's a good practice to not ignore errors.
> >>>>    
> >>>
> >>> It is not such a great idea. You might have multiple input hardware (ie
> >>> serial and usb keyboard). What does it mean that usb keyboard failed in
> >>> this context?    
> >>
> >> I'd say, the behavior is undefined ?  
> > 
> > But we need to define it which the code does by ignoring the
> > device-specific error and relying on devices that are still working
> > (like a serial port) or for which error detection is not available
> > (like most serial ports).  
> 
> Maybe the error should still be propagated to the input layer , and not
> ignored at the USB layer ?

Maybe. I would leave the discussion of handling errors in the input
layer for a separate patchset.

> 
> >>> So in my view the ultimate consumer of getc() has no use for the error
> >>> so there is no point in propagating it.    
> >>
> >> Ignoring errors and not reporting them isn't nice either, so what other
> >> option(s) do we have here ?  
> > 
> > Ignoring the errors is exactly the desirable behavior when facing
> > broken hardware like dwc2. On non-broken hardware you will get fewer
> > errors to ignore. It is up to the device driver to report device
> > failure with a message when the error condition could be informative to
> > the user (such as previously working device going away completely).  
> 
> I thought this error is a keyboard failure though , and has nothing to
> do with the USB controller ?

As far as I know the keyboard is working fine but the controller is
failing to deliver some messages.

Thanks

Michal

[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Marek Vasut]

On 7/3/19 6:41 PM, Michal Suchánek wrote:
> On Wed, 3 Jul 2019 13:48:00 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/3/19 1:43 PM, Michal Suchánek wrote:
>>> On Wed, 3 Jul 2019 13:26:50 +0200
>>> Marek Vasut <marex@denx.de> wrote:
>>>   
>>>> On 7/3/19 11:46 AM, Michal Suchánek wrote:  
>>>>> On Tue, 2 Jul 2019 23:20:28 +0200
>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>     
>>>>>> On 7/2/19 9:31 PM, Michal Suchánek wrote:    
>>>>>>> On Tue, 2 Jul 2019 20:38:27 +0200
>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>       
>>>>>>>> On 7/2/19 7:50 PM, Michal Suchánek wrote:      
>>>>>>>>> On Tue, 2 Jul 2019 18:58:54 +0200
>>>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>         
>>>>>>>>>> On 7/2/19 4:22 PM, Michal Suchánek wrote:        
>>>>>>>>>>> On Tue, 2 Jul 2019 15:11:07 +0200
>>>>>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>           
>>>>>>>>>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:          
>>>>>>>>>>>>> On Tue, 2 Jul 2019 13:58:30 +0200
>>>>>>>>>>>>> Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>>>             
>>>>>>>>>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:            
>>>>>>>>>>>>>>> Causes unbound key repeat on error otherwise.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>  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..948f9fd68490 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],              
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Shouldn't you propagate return value from this function ? It can return
>>>>>>>>>>>>>> ENOTSUPP.
>>>>>>>>>>>>>>            
>>>>>>>>>>>>>
>>>>>>>>>>>>> If it did then probing keyboard would fail and we would not get here.            
>>>>>>>>>>>>
>>>>>>>>>>>> So there is no chance this function could return an error here, ever ?
>>>>>>>>>>>> E.g. what if it's implemented and someone yanks the keyboard cable out
>>>>>>>>>>>> just at the right time ?          
>>>>>>>>>>>
>>>>>>>>>>> It returns errors all the time with dwc2. That's why we need to check
>>>>>>>>>>> for the error condition. We should not get here if probing the keyboard
>>>>>>>>>>> failed, though. So if the function is not supported we will not get
>>>>>>>>>>> here. Anyway, if it's not supported or the keyboard is missing it by
>>>>>>>>>>> definition cannot provide useful result so we should not process it.          
>>>>>>>>>>
>>>>>>>>>> Except you start ignoring the error value from e.g. malfunctioning
>>>>>>>>>> keyboard here, instead of propagating it, correct ?        
>>>>>>>>>
>>>>>>>>> It was never propagated to start with. The return value was not checked
>>>>>>>>> at all. What I do here is check the return value and not process the
>>>>>>>>> data on error whatever it contains (like the keypress returned last
>>>>>>>>> time valid data was received).        
>>>>>>>>
>>>>>>>> I can see a patch which checks usb_kbd_poll_for_event() return value.
>>>>>>>> Can you add one ?      
>>>>>>>
>>>>>>> What for? Apparently the keypress is processed in usb_kbd_irq_worker.
>>>>>>> So checking the return value is needed to decide if the worker should
>>>>>>> run, and is not particularly useful outside usb_kbd_poll_for_event. We
>>>>>>> could signal a getc() failure but do we have any code handling getc()
>>>>>>> failures?      
>>>>>>
>>>>>> I presume getc() might signal EOF if the underlying hardware fails.
>>>>>> But in general, it's a good practice to not ignore errors.
>>>>>>    
>>>>>
>>>>> It is not such a great idea. You might have multiple input hardware (ie
>>>>> serial and usb keyboard). What does it mean that usb keyboard failed in
>>>>> this context?    
>>>>
>>>> I'd say, the behavior is undefined ?  
>>>
>>> But we need to define it which the code does by ignoring the
>>> device-specific error and relying on devices that are still working
>>> (like a serial port) or for which error detection is not available
>>> (like most serial ports).  
>>
>> Maybe the error should still be propagated to the input layer , and not
>> ignored at the USB layer ?
> 
> Maybe. I would leave the discussion of handling errors in the input
> layer for a separate patchset.

It's literally 3-line change to make usb_kbd_poll_for_event() return the
error code.

>>>>> So in my view the ultimate consumer of getc() has no use for the error
>>>>> so there is no point in propagating it.    
>>>>
>>>> Ignoring errors and not reporting them isn't nice either, so what other
>>>> option(s) do we have here ?  
>>>
>>> Ignoring the errors is exactly the desirable behavior when facing
>>> broken hardware like dwc2. On non-broken hardware you will get fewer
>>> errors to ignore. It is up to the device driver to report device
>>> failure with a message when the error condition could be informative to
>>> the user (such as previously working device going away completely).  
>>
>> I thought this error is a keyboard failure though , and has nothing to
>> do with the USB controller ?
> 
> As far as I know the keyboard is working fine but the controller is
> failing to deliver some messages.

Uh, would you be willing to debug that ?

[...]