[U-Boot] [PATCH 1/8] usb: r8a66597: return -ENOTSUPP from unimplemented submit_int_msg
--- 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)
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; }
/*
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)
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)
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
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 ?
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
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 ?
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
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
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.
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
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.
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
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 ?
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
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 ?
[...]
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
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 ?
[...]
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--) {
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
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,
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,
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
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
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
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
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
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
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
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
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
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) || \
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)
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)
participants (4)
-
Bin Meng -
Marek Vasut -
Michal Suchanek -
Michal Suchánek