Re: [PATCH 4/4] usb: dwc3: gadget: check if dep->frame_number is still valid
Hi,
Thinh Nguyen writes:
>> Felipe Balbi writes:
>>> In the case of interval of 1ms, it will start on the next interval.
frame_number + max(4, interval) will start at least 4 uframes in the
future.
In any case, what about immediately retry the START_TRANSFER command
with a new frame_number + (interval*retry) should it fail with
bus-expiry? You can set the number of retries to maybe 5 times. This
should remove the need to do time stamping.
>>> That seems like a good idea. Something like below? (on top of $subject)
>>>
>>> modified drivers/usb/dwc3/core.h
>>> @@ -37,6 +37,7 @@
>>> #define DWC3_EP0_SETUP_SIZE512
>>> #define DWC3_ENDPOINTS_NUM 32
>>> #define DWC3_XHCI_RESOURCES_NUM2
>>> +#define DWC3_ISOC_MAX_RETRIES 5
>>>
>>> #define DWC3_SCRATCHBUF_SIZE 4096/* each buffer is assumed to be
>>> 4KiB */
>>> #define DWC3_EVENT_BUFFERS_SIZE4096
>>> modified drivers/usb/dwc3/gadget.c
>>> @@ -1271,20 +1271,29 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep
>>> *dep)
>>> u64 current_timestamp;
>>> u64 diff_timestamp;
>>> u32 elapsed_frames;
>>> + int retries;
>>> + int delta = 1
>>> + int ret;
>>>
>>> if (list_empty(&dep->pending_list)) {
>>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>>> return -EAGAIN;
>>> }
>>>
>>> - current_timestamp = ktime_get_ns();
>>> - diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> - elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>> + for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>>> + current_timestamp = ktime_get_ns();
>>> + diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> + elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>>
>>> - dep->frame_number += elapsed_frames;
>>> - dep->frame_number = DWC3_ALIGN_FRAME(dep);
>>> + dep->frame_number += elapsed_frames + (delta * i);
>> the other possibility is that we can call DWC3_ALIGN_FRAME() n times
>> since that will put the transfer on the following interval. That would
>> look like so:
>>
>> modified drivers/usb/dwc3/core.h
>> @@ -37,6 +37,7 @@
>> #define DWC3_EP0_SETUP_SIZE 512
>> #define DWC3_ENDPOINTS_NUM 32
>> #define DWC3_XHCI_RESOURCES_NUM 2
>> +#define DWC3_ISOC_MAX_RETRIES 5
>>
>> #define DWC3_SCRATCHBUF_SIZE4096/* each buffer is assumed to be
>> 4KiB */
>> #define DWC3_EVENT_BUFFERS_SIZE 4096
>> modified drivers/usb/dwc3/gadget.c
>> @@ -27,7 +27,7 @@
>> #include "gadget.h"
>> #include "io.h"
>>
>> -#define DWC3_ALIGN_FRAME(d) (((d)->frame_number + (d)->interval) \
>> +#define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval *
>> (n))) \
>> & ~((d)->interval - 1))
>>
>> /**
>> @@ -1271,20 +1271,28 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep
>> *dep)
>> u64 current_timestamp;
>> u64 diff_timestamp;
>> u32 elapsed_frames;
>> +int retries;
>> +int ret;
>>
>> if (list_empty(&dep->pending_list)) {
>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>> return -EAGAIN;
>> }
>>
>> -current_timestamp = ktime_get_ns();
>> -diff_timestamp = current_timestamp - dep->frame_timestamp;
>> -elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>> +for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>> +current_timestamp = ktime_get_ns();
>> +diff_timestamp = current_timestamp - dep->frame_timestamp;
>> +elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>
>> -dep->frame_number += elapsed_frames;
>> -dep->frame_number = DWC3_ALIGN_FRAME(dep);
>> +dep->frame_number += elapsed_frames;
>> +dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
>>
>> -return __dwc3_gadget_kick_transfer(dep);
>> +ret = __dwc3_gadget_kick_transfer(dep);
>> +if (ret != -EAGAIN)
>> +break;
>> +}
>> +
>> +return ret;
>> }
>>
>> static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request
>> *req)
>>
>>
> I like the second method. But do we need to keep track of the
> frame_timestamp? I don't think it accurately reflects the timestamp of
no we don't. I've since removed it from my local tree.
> XferNotReady frame number.
> As for the number of retries, we should adjust it according to the
> service interval. For example, for larger service interval such as 16,
> then we don't need to try more than once. To calculate the max number of
> retries, we can do this check (where interval is from 1 to 16):
>
> if (interval >= (16 - (MAX_NUM_RETRIES >> 1))
> num_retries = 1 << (16 - interval);
>
> Please check my math..
>
> If it fails for over 5 times in a row, then we should probably wait for
> the next XferNotReady to get the frame_number again. Also 5 is an
> arbitrary number I came up without any testing, we
Re: [RESEND PATCH v2] usb: dwc2: disable power_down on rockchip for regression
Hal Emmerich writes: > From 04fbf78e4e569bf872f1ffcb0a6f9b89569dc913 Mon Sep 17 00:00:00 2001 > From: Hal Emmerich > Date: Thu, 19 Jul 2018 21:48:08 -0500 > Subject: [PATCH] usb: dwc2: disable power_down on rockchip devices > > The bug would let the usb controller enter partial power down, > which was formally known as hibernate, upon boot if nothing was plugged > in to the port. Partial power down couldn't be exited properly, so any > usb devices plugged in after boot would not be usable. > > Before the name change, params.hibernation was false by default, so > _dwc2_hcd_suspend() would skip entering hibernation. With the > rename, _dwc2_hcd_suspend() was changed to use params.power_down > to decide whether or not to enter partial power down. > > Since params.power_down is non-zero by default, it needs to be set > to 0 for rockchip devices to restore functionality. > > This bug was reported in the linux-usb thread: > REGRESSION: usb: dwc2: USB device not seen after boot > > The commit that caused this regression is: > 6d23ee9caa6790aea047f9aca7f3c03cb8d96eb6 What are these weird space characters you're using in your commit log? I've fixed it locally this time, but please fix your editor. -- balbi signature.asc Description: PGP signature
Re: [PATCH 4/4] usb: dwc3: gadget: check if dep->frame_number is still valid
On 11/14/2018 12:42 AM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen writes:
>>> Felipe Balbi writes:
In the case of interval of 1ms, it will start on the next interval.
> frame_number + max(4, interval) will start at least 4 uframes in the
> future.
>
> In any case, what about immediately retry the START_TRANSFER command
> with a new frame_number + (interval*retry) should it fail with
> bus-expiry? You can set the number of retries to maybe 5 times. This
> should remove the need to do time stamping.
That seems like a good idea. Something like below? (on top of $subject)
modified drivers/usb/dwc3/core.h
@@ -37,6 +37,7 @@
#define DWC3_EP0_SETUP_SIZE 512
#define DWC3_ENDPOINTS_NUM32
#define DWC3_XHCI_RESOURCES_NUM 2
+#define DWC3_ISOC_MAX_RETRIES 5
#define DWC3_SCRATCHBUF_SIZE 4096/* each buffer is assumed to be
4KiB */
#define DWC3_EVENT_BUFFERS_SIZE 4096
modified drivers/usb/dwc3/gadget.c
@@ -1271,20 +1271,29 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep
*dep)
u64 current_timestamp;
u64 diff_timestamp;
u32 elapsed_frames;
+ int retries;
+ int delta = 1
+ int ret;
if (list_empty(&dep->pending_list)) {
dep->flags |= DWC3_EP_PENDING_REQUEST;
return -EAGAIN;
}
- current_timestamp = ktime_get_ns();
- diff_timestamp = current_timestamp - dep->frame_timestamp;
- elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
+ for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
+ current_timestamp = ktime_get_ns();
+ diff_timestamp = current_timestamp - dep->frame_timestamp;
+ elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
- dep->frame_number += elapsed_frames;
- dep->frame_number = DWC3_ALIGN_FRAME(dep);
+ dep->frame_number += elapsed_frames + (delta * i);
>>> the other possibility is that we can call DWC3_ALIGN_FRAME() n times
>>> since that will put the transfer on the following interval. That would
>>> look like so:
>>>
>>> modified drivers/usb/dwc3/core.h
>>> @@ -37,6 +37,7 @@
>>> #define DWC3_EP0_SETUP_SIZE512
>>> #define DWC3_ENDPOINTS_NUM 32
>>> #define DWC3_XHCI_RESOURCES_NUM2
>>> +#define DWC3_ISOC_MAX_RETRIES 5
>>>
>>> #define DWC3_SCRATCHBUF_SIZE 4096/* each buffer is assumed to be
>>> 4KiB */
>>> #define DWC3_EVENT_BUFFERS_SIZE4096
>>> modified drivers/usb/dwc3/gadget.c
>>> @@ -27,7 +27,7 @@
>>> #include "gadget.h"
>>> #include "io.h"
>>>
>>> -#define DWC3_ALIGN_FRAME(d)(((d)->frame_number + (d)->interval) \
>>> +#define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval *
>>> (n))) \
>>> & ~((d)->interval - 1))
>>>
>>> /**
>>> @@ -1271,20 +1271,28 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep
>>> *dep)
>>> u64 current_timestamp;
>>> u64 diff_timestamp;
>>> u32 elapsed_frames;
>>> + int retries;
>>> + int ret;
>>>
>>> if (list_empty(&dep->pending_list)) {
>>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>>> return -EAGAIN;
>>> }
>>>
>>> - current_timestamp = ktime_get_ns();
>>> - diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> - elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>> + for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>>> + current_timestamp = ktime_get_ns();
>>> + diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> + elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>>
>>> - dep->frame_number += elapsed_frames;
>>> - dep->frame_number = DWC3_ALIGN_FRAME(dep);
>>> + dep->frame_number += elapsed_frames;
>>> + dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
>>>
>>> - return __dwc3_gadget_kick_transfer(dep);
>>> + ret = __dwc3_gadget_kick_transfer(dep);
>>> + if (ret != -EAGAIN)
>>> + break;
>>> + }
>>> +
>>> + return ret;
>>> }
>>>
>>> static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request
>>> *req)
>>>
>>>
>> I like the second method. But do we need to keep track of the
>> frame_timestamp? I don't think it accurately reflects the timestamp of
> no we don't. I've since removed it from my local tree.
>
>> XferNotReady frame number.
>> As for the number of retries, we should adjust it according to the
>> service interval. For example, for larger service interval such as 16,
>> then we don't need to try more than once. To calculate the max number of
>> retries, we can do this check (where interval is from 1 to 16):
>>
>> if (interval >= (16 - (MAX_NUM_RETRIES >> 1))
>> num_retries = 1 << (16 - interval);
>>
>> Please check my math..
>>
>> If it fails for over 5 times in a row, then we should pr
Re: [PATCH 4/4] usb: dwc3: gadget: check if dep->frame_number is still valid
Hi,
Thinh Nguyen writes:
>>> If it fails for over 5 times in a row, then we should probably wait for
>>> the next XferNotReady to get the frame_number again. Also 5 is an
>>> arbitrary number I came up without any testing, we can probably decide
>>> on a better number. What do you think?
>> I have this now:
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 131028501752..3390fa46ea30 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -37,6 +37,7 @@
>> #define DWC3_EP0_SETUP_SIZE512
>> #define DWC3_ENDPOINTS_NUM 32
>> #define DWC3_XHCI_RESOURCES_NUM2
>> +#define DWC3_ISOC_MAX_RETRIES 5
>>
>> #define DWC3_SCRATCHBUF_SIZE 4096/* each buffer is assumed to be 4KiB
>> */
>> #define DWC3_EVENT_BUFFERS_SIZE4096
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index d8c7ad0c22e8..1590516735cb 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -27,7 +27,7 @@
>> #include "gadget.h"
>> #include "io.h"
>>
>> -#define DWC3_ALIGN_FRAME(d)(((d)->frame_number + (d)->interval) \
>> +#define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval * (n)))
>> \
>> & ~((d)->interval - 1))
>>
>> /**
>> @@ -1268,13 +1268,24 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>
>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>> {
>> + int retries;
>> + int ret;
>> + int i;
>> +
>> if (list_empty(&dep->pending_list)) {
>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>> return -EAGAIN;
>> }
>>
>> - dep->frame_number = DWC3_ALIGN_FRAME(dep);
>> - return __dwc3_gadget_kick_transfer(dep);
>> + for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>> + dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
>
> Shouldn't this be like this?
> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
good catch, I'll fix.
>> +
>> + ret = __dwc3_gadget_kick_transfer(dep);
>> + if (ret != -EAGAIN)
>> + break;
>> + }
>> +
>> + return ret;
>> }
>>
>> static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request
>> *req)
>>
>> This means we will increment in full intervals, up to 5 intervals in the
>> future. If it still fails, then there's not much we can do.
>>
> I agree. Also, depending on the application requirement, we may want to
> giveback/cleanup request(s) every time we fail so that the data won't be
> pushed back/delayed for too long.
> We could experiment and decide on the maximum delay time (base on the
> service interval length and number of retries) to be considered
> acceptable to start giveback stale requests.
That's something we can consider in the future. I'd say that the gadget
driver should, then, tell us how much slack to give.
--
balbi
signature.asc
Description: PGP signature
[PATCH] Revert "usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers"
From: Shen Jing
This reverts commit b4194da3f9087dd38d91b40f9bec42d59ce589a8
since it causes list corruption followed by kernel panic:
Workqueue: adb ffs_aio_cancel_worker
RIP: 0010:__list_add_valid+0x4d/0x70
Call Trace:
insert_work+0x47/0xb0
__queue_work+0xf6/0x400
queue_work_on+0x65/0x70
dwc3_gadget_giveback+0x44/0x50 [dwc3]
dwc3_gadget_ep_dequeue+0x83/0x2d0 [dwc3]
? finish_wait+0x80/0x80
usb_ep_dequeue+0x1e/0x90
process_one_work+0x18c/0x3b0
worker_thread+0x3c/0x390
? process_one_work+0x3b0/0x3b0
kthread+0x11e/0x140
? kthread_create_worker_on_cpu+0x70/0x70
ret_from_fork+0x3a/0x50
This issue is seen with warm reboot stability testing.
Signed-off-by: Shen Jing
Signed-off-by: Saranya Gopal
Signed-off-by: Felipe Balbi
---
drivers/usb/gadget/function/f_fs.c | 26 --
1 file changed, 8 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 645b0c0a763f..9ec666299430 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -219,7 +219,6 @@ struct ffs_io_data {
struct mm_struct *mm;
struct work_struct work;
- struct work_struct cancellation_work;
struct usb_ep *ep;
struct usb_request *req;
@@ -1154,31 +1153,22 @@ ffs_epfile_open(struct inode *inode, struct file *file)
return 0;
}
-static void ffs_aio_cancel_worker(struct work_struct *work)
-{
- struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
- cancellation_work);
-
- ENTER();
-
- usb_ep_dequeue(io_data->ep, io_data->req);
-}
-
static int ffs_aio_cancel(struct kiocb *kiocb)
{
struct ffs_io_data *io_data = kiocb->private;
- struct ffs_data *ffs = io_data->ffs;
+ struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
int value;
ENTER();
- if (likely(io_data && io_data->ep && io_data->req)) {
- INIT_WORK(&io_data->cancellation_work, ffs_aio_cancel_worker);
- queue_work(ffs->io_completion_wq, &io_data->cancellation_work);
- value = -EINPROGRESS;
- } else {
+ spin_lock_irq(&epfile->ffs->eps_lock);
+
+ if (likely(io_data && io_data->ep && io_data->req))
+ value = usb_ep_dequeue(io_data->ep, io_data->req);
+ else
value = -EINVAL;
- }
+
+ spin_unlock_irq(&epfile->ffs->eps_lock);
return value;
}
--
2.19.1
[PATCH v2 2/4] usb: dwc3: trace: log ep commands in hex
They are much more useful in hexadecimal than in decimal. Moreover,
generic commands are already logged in hex.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/trace.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index f22714cce070..50fb6f2d92dd 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -199,7 +199,7 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
__entry->param2 = params->param2;
__entry->cmd_status = cmd_status;
),
- TP_printk("%s: cmd '%s' [%d] params %08x %08x %08x --> status: %s",
+ TP_printk("%s: cmd '%s' [%x] params %08x %08x %08x --> status: %s",
__get_str(name), dwc3_gadget_ep_cmd_string(__entry->cmd),
__entry->cmd, __entry->param0,
__entry->param1, __entry->param2,
--
2.19.1
[PATCH v2 1/4] usb: dwc3: gadget: return errors from __dwc3_gadget_start_isoc()
Sometimes, errors happen when kicking transfers from
__dwc3_gadget_start_isoc(). In those cases, we need to pass along the
error so gadget driver can make informed decisions.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 64481da433ae..18c63137348d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1266,17 +1266,17 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
return DWC3_DSTS_SOFFN(reg);
}
-static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
+static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
{
if (list_empty(&dep->pending_list)) {
dev_info(dep->dwc->dev, "%s: ran out of requests\n",
dep->name);
dep->flags |= DWC3_EP_PENDING_REQUEST;
- return;
+ return -EAGAIN;
}
dep->frame_number = DWC3_ALIGN_FRAME(dep);
- __dwc3_gadget_kick_transfer(dep);
+ return __dwc3_gadget_kick_transfer(dep);
}
static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request
*req)
@@ -1317,8 +1317,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep,
struct dwc3_request *req)
if ((dep->flags & DWC3_EP_PENDING_REQUEST)) {
if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) {
- __dwc3_gadget_start_isoc(dep);
- return 0;
+ return __dwc3_gadget_start_isoc(dep);
}
}
}
@@ -2381,7 +2380,7 @@ static void
dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,
const struct dwc3_event_depevt *event)
{
dwc3_gadget_endpoint_frame_from_event(dep, event);
- __dwc3_gadget_start_isoc(dep);
+ (void) __dwc3_gadget_start_isoc(dep);
}
static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
--
2.19.1
[PATCH v2 3/4] usb: dwc3: gadget: remove unnecessary dev_info()
Running out of requests on isochronous endpoints is part of normal
operation. We don't really need to know about it every time it
happens.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 18c63137348d..d8c7ad0c22e8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1269,8 +1269,6 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
{
if (list_empty(&dep->pending_list)) {
- dev_info(dep->dwc->dev, "%s: ran out of requests\n",
- dep->name);
dep->flags |= DWC3_EP_PENDING_REQUEST;
return -EAGAIN;
}
--
2.19.1
[PATCH v2 4/4] usb: dwc3: gadget: check if dep->frame_number is still valid
Gadget driver may take an unbounded amount of time to queue requests
after XferNotReady. This is important for isochronous endpoints which
need to be started for a specific (micro-)frame.
If we fail to start a transfer for isochronous endpoint, let's try
queueing to a future interval and see if that helps. We will stop trying
if we fail a start transfer for 5 intervals in the future.
Signed-off-by: Felipe Balbi
---
changes since v1:
- switch to a retry approach instead of capturing timestamps
drivers/usb/dwc3/core.h | 1 +
drivers/usb/dwc3/gadget.c | 17 ++---
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index ea59f9b4908a..85599ad6648f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -38,6 +38,7 @@
#define DWC3_EP0_SETUP_SIZE512
#define DWC3_ENDPOINTS_NUM 32
#define DWC3_XHCI_RESOURCES_NUM2
+#define DWC3_ISOC_MAX_RETRIES 5
#define DWC3_SCRATCHBUF_SIZE 4096/* each buffer is assumed to be 4KiB */
#define DWC3_EVENT_BUFFERS_SIZE4096
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d8c7ad0c22e8..1590516735cb 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -27,7 +27,7 @@
#include "gadget.h"
#include "io.h"
-#define DWC3_ALIGN_FRAME(d)(((d)->frame_number + (d)->interval) \
+#define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval * (n))) \
& ~((d)->interval - 1))
/**
@@ -1268,13 +1268,24 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
{
+ int retries;
+ int ret;
+ int i;
+
if (list_empty(&dep->pending_list)) {
dep->flags |= DWC3_EP_PENDING_REQUEST;
return -EAGAIN;
}
- dep->frame_number = DWC3_ALIGN_FRAME(dep);
- return __dwc3_gadget_kick_transfer(dep);
+ for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
+ dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
+
+ ret = __dwc3_gadget_kick_transfer(dep);
+ if (ret != -EAGAIN)
+ break;
+ }
+
+ return ret;
}
static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request
*req)
--
2.19.1
Re: [PATCH] xhci: handle stop ep on invalid CStream
Hi On 13.11.2018 08:48, Henry Lin wrote: Below Note in xHCI spec 6.4.2.1 describes a Transfer Event is generated for Stop Endpoint Command on invalid CStream: CStream is not valid until a Streams endpoint transitions to the Start Stream state for the first time. A Transfer Event generated by a Stop Endpoint Command shall report '0' in the TRB Pointer and TRB Length fields if the command is executed and CStream is invalid. Refer to section 4.12.1. But, current implementation will output below error messages once driver receives the Transfer Event for invalid CStream: [ 133.184633] xhci_hcd :01:00.2: ERROR Transfer event for unknown stream ring slot 1 ep 2 [ 133.184635] xhci_hcd :01:00.2: @00046044a420 1b00 01038001 This issue can be observed while uas driver is handling SCSI command that connected UAS device doesn't support. UAS device rejects the unsupported command and causes Data-In stream for SCSI command data stopped when CStream is invalid. This patch handles the transfer event to remove false error messages. A patch by Sandeep Singh that takes care of the same COMP_STOPPED_LENGTH_INVALID case with zero TRB pointer just got applied, so this should now be taken care of. https://marc.info/?l=linux-usb&m=153753620603125&w=2 It's in Greg's usb-linus branch: d9193ef xhci: Add check for invalid byte size error when UAS devices are connected. -Mathias
[PATCH 01/14] usb: dwc3: core: Clean up ULPI device
From: Andy Shevchenko If dwc3_core_init_mode() fails with deferred probe, next probe fails on sysfs with sysfs: cannot create duplicate filename '/devices/pci:00/:00:11.0/dwc3.0.auto/dwc3.0.auto.ulpi' To avoid this failure, clean up ULPI device. Cc: Signed-off-by: Andy Shevchenko Signed-off-by: Felipe Balbi --- drivers/usb/dwc3/core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 75cff57b2629..8f6d9c6f016e 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1505,6 +1505,7 @@ static int dwc3_probe(struct platform_device *pdev) err5: dwc3_event_buffers_cleanup(dwc); + dwc3_ulpi_exit(dwc); err4: dwc3_free_scratch_buffers(dwc); -- 2.19.1
[PATCH 03/14] usb: dwc3: gadget: fix ISOC TRB type on unaligned transfers
When chaining ISOC TRBs together, only the first ISOC TRB should be of
type ISOC_FIRST, all others should be of type ISOC. This patch fixes
that.
Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
Cc: # v4.11+
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 06e22afdf3d1..9faad896b3a1 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1081,7 +1081,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
/* Now prepare one extra TRB to align transfer size */
trb = &dep->trb_pool[dep->trb_enqueue];
__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
- maxp - rem, false, 0,
+ maxp - rem, false, 1,
req->request.stream_id,
req->request.short_not_ok,
req->request.no_interrupt);
@@ -1125,7 +1125,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep
*dep,
/* Now prepare one extra TRB to align transfer size */
trb = &dep->trb_pool[dep->trb_enqueue];
__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,
- false, 0, req->request.stream_id,
+ false, 1, req->request.stream_id,
req->request.short_not_ok,
req->request.no_interrupt);
} else if (req->request.zero && req->request.length &&
@@ -1141,7 +1141,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep
*dep,
/* Now prepare one extra TRB to handle ZLP */
trb = &dep->trb_pool[dep->trb_enqueue];
__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0,
- false, 0, req->request.stream_id,
+ false, 1, req->request.stream_id,
req->request.short_not_ok,
req->request.no_interrupt);
} else {
--
2.19.1
[PATCH 07/14] usb: dwc3: gadget: extract dwc3_gadget_ep_skip_trbs()
Extract the logic for skipping over TRBs to its own function. This
makes the code slightly more readable and makes it easier to move this
call to its final resting place as a following patch.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 61 +++
1 file changed, 24 insertions(+), 37 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 28bf7ade07db..fae37972a78b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1346,6 +1346,29 @@ static int dwc3_gadget_ep_queue(struct usb_ep *ep,
struct usb_request *request,
return ret;
}
+static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request
*req)
+{
+ int i;
+
+ /*
+* If request was already started, this means we had to
+* stop the transfer. With that we also need to ignore
+* all TRBs used by the request, however TRBs can only
+* be modified after completion of END_TRANSFER
+* command. So what we do here is that we wait for
+* END_TRANSFER completion and only after that, we jump
+* over TRBs by clearing HWO and incrementing dequeue
+* pointer.
+*/
+ for (i = 0; i < req->num_trbs; i++) {
+ struct dwc3_trb *trb;
+
+ trb = req->trb + i;
+ trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
+ dwc3_ep_inc_deq(dep);
+ }
+}
+
static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
struct usb_request *request)
{
@@ -1373,38 +1396,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
break;
}
if (r == req) {
- int i;
-
/* wait until it is processed */
dwc3_stop_active_transfer(dep, true);
-
- /*
-* If request was already started, this means we had to
-* stop the transfer. With that we also need to ignore
-* all TRBs used by the request, however TRBs can only
-* be modified after completion of END_TRANSFER
-* command. So what we do here is that we wait for
-* END_TRANSFER completion and only after that, we jump
-* over TRBs by clearing HWO and incrementing dequeue
-* pointer.
-*
-* Note that we have 2 possible types of transfers here:
-*
-* i) Linear buffer request
-* ii) SG-list based request
-*
-* SG-list based requests will have r->num_pending_sgs
-* set to a valid number (> 0). Linear requests,
-* normally use a single TRB.
-*
-* For each of these two cases, if r->unaligned flag is
-* set, one extra TRB has been used to align transfer
-* size to wMaxPacketSize.
-*
-* All of these cases need to be taken into
-* consideration so we don't mess up our TRB ring
-* pointers.
-*/
wait_event_lock_irq(dep->wait_end_transfer,
!(dep->flags &
DWC3_EP_END_TRANSFER_PENDING),
dwc->lock);
@@ -1412,13 +1405,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
if (!r->trb)
goto out0;
- for (i = 0; i < r->num_trbs; i++) {
- struct dwc3_trb *trb;
-
- trb = r->trb + i;
- trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
- dwc3_ep_inc_deq(dep);
- }
+ dwc3_gadget_ep_skip_trbs(dep, r);
goto out1;
}
dev_err(dwc->dev, "request %pK was not queued to %s\n",
--
2.19.1
[PATCH 06/14] usb: dwc3: gadget: use num_trbs when skipping TRBs on ->dequeue()
Now that we track how many TRBs a request uses, it's easier to skip
over them in case of a call to usb_ep_dequeue(). Let's do so and
simplify the code a bit.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 28
1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d945019a4139..28bf7ade07db 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1373,6 +1373,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
break;
}
if (r == req) {
+ int i;
+
/* wait until it is processed */
dwc3_stop_active_transfer(dep, true);
@@ -1410,32 +1412,12 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
if (!r->trb)
goto out0;
- if (r->num_pending_sgs) {
+ for (i = 0; i < r->num_trbs; i++) {
struct dwc3_trb *trb;
- int i = 0;
-
- for (i = 0; i < r->num_pending_sgs; i++) {
- trb = r->trb + i;
- trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
- dwc3_ep_inc_deq(dep);
- }
-
- if (r->needs_extra_trb) {
- trb = r->trb + r->num_pending_sgs + 1;
- trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
- dwc3_ep_inc_deq(dep);
- }
- } else {
- struct dwc3_trb *trb = r->trb;
+ trb = r->trb + i;
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
dwc3_ep_inc_deq(dep);
-
- if (r->needs_extra_trb) {
- trb = r->trb + 1;
- trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
- dwc3_ep_inc_deq(dep);
- }
}
goto out1;
}
@@ -1446,8 +1428,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
}
out1:
- /* giveback the request */
-
dwc3_gadget_giveback(dep, req, -ECONNRESET);
out0:
--
2.19.1
[PATCH 02/14] usb: dwc3: gadget: Properly check last unaligned/zero chain TRB
From: Thinh Nguyen
Current check for the last extra TRB for zero and unaligned transfers
does not account for isoc OUT. The last TRB of the Buffer Descriptor for
isoc OUT transfers will be retired with HWO=0. As a result, we won't
return early. The req->remaining will be updated to include the BUFSIZ
count of the extra TRB, and the actual number of transferred bytes
calculation will be wrong.
To fix this, check whether it's a short or zero packet and the last TRB
chain bit to return early.
Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
Cc:
Signed-off-by: Thinh Nguyen
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 679c12e14522..06e22afdf3d1 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2259,7 +2259,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct
dwc3_ep *dep,
* with one TRB pending in the ring. We need to manually clear HWO bit
* from that TRB.
*/
- if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) {
+ if ((req->zero || req->unaligned) && !(trb->ctrl & DWC3_TRB_CTRL_CHN)) {
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
return 1;
}
--
2.19.1
[PATCH 04/14] usb: dwc3: gadget: combine unaligned and zero flags
Both flags are used for the same purpose in dwc3: appending an extra
TRB at the end to deal with controller requirements. By combining both
flags into one, we make it clear that the situation is the same and
that they should be treated equally.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/core.h | 7 +++
drivers/usb/dwc3/gadget.c | 17 -
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index da1b6f61be05..b2ec1a287322 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -858,11 +858,11 @@ struct dwc3_hwparams {
* @epnum: endpoint number to which this request refers
* @trb: pointer to struct dwc3_trb
* @trb_dma: DMA address of @trb
- * @unaligned: true for OUT endpoints with length not divisible by maxp
+ * @needs_extra_trb: true when request needs one extra TRB (either due to ZLP
+ * or unaligned OUT)
* @direction: IN or OUT direction flag
* @mapped: true when request has been dma-mapped
* @started: request is started
- * @zero: wants a ZLP
*/
struct dwc3_request {
struct usb_request request;
@@ -878,11 +878,10 @@ struct dwc3_request {
struct dwc3_trb *trb;
dma_addr_t trb_dma;
- unsignedunaligned:1;
+ unsignedneeds_extra_trb:1;
unsigneddirection:1;
unsignedmapped:1;
unsignedstarted:1;
- unsignedzero:1;
};
/*
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9faad896b3a1..132bb6b9ae44 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1073,7 +1073,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
struct dwc3 *dwc = dep->dwc;
struct dwc3_trb *trb;
- req->unaligned = true;
+ req->needs_extra_trb = true;
/* prepare normal TRB */
dwc3_prepare_one_trb(dep, req, true, i);
@@ -1117,7 +1117,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep
*dep,
struct dwc3 *dwc = dep->dwc;
struct dwc3_trb *trb;
- req->unaligned = true;
+ req->needs_extra_trb = true;
/* prepare normal TRB */
dwc3_prepare_one_trb(dep, req, true, 0);
@@ -1133,7 +1133,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep
*dep,
struct dwc3 *dwc = dep->dwc;
struct dwc3_trb *trb;
- req->zero = true;
+ req->needs_extra_trb = true;
/* prepare normal TRB */
dwc3_prepare_one_trb(dep, req, true, 0);
@@ -1415,7 +1415,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
dwc3_ep_inc_deq(dep);
}
- if (r->unaligned || r->zero) {
+ if (r->needs_extra_trb) {
trb = r->trb + r->num_pending_sgs + 1;
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
dwc3_ep_inc_deq(dep);
@@ -1426,7 +1426,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
dwc3_ep_inc_deq(dep);
- if (r->unaligned || r->zero) {
+ if (r->needs_extra_trb) {
trb = r->trb + 1;
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
dwc3_ep_inc_deq(dep);
@@ -2259,7 +2259,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct
dwc3_ep *dep,
* with one TRB pending in the ring. We need to manually clear HWO bit
* from that TRB.
*/
- if ((req->zero || req->unaligned) && !(trb->ctrl & DWC3_TRB_CTRL_CHN)) {
+ if (req->needs_extra_trb && !(trb->ctrl & DWC3_TRB_CTRL_CHN)) {
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
return 1;
}
@@ -2336,11 +2336,10 @@ static int
dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
ret = dwc3_gadget_ep_reclaim_trb_linear(dep, req, event,
status);
- if (req->unaligned || req->zero) {
+ if (req->needs_extra_trb) {
ret = dwc3_gadget_ep_reclaim_trb_linear(dep, req, event,
status);
- req->unaligned = false;
- req->zero = false;
+ req->needs_extra_trb = false;
}
req->request.actual = req->request.length - req->remaining;
--
2.19.1
[PATCH 12/14] usb: dwc3: trace: log ep commands in hex
They are much more useful in hexadecimal than in decimal. Moreover,
generic commands are already logged in hex.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/trace.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index f22714cce070..50fb6f2d92dd 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -199,7 +199,7 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
__entry->param2 = params->param2;
__entry->cmd_status = cmd_status;
),
- TP_printk("%s: cmd '%s' [%d] params %08x %08x %08x --> status: %s",
+ TP_printk("%s: cmd '%s' [%x] params %08x %08x %08x --> status: %s",
__get_str(name), dwc3_gadget_ep_cmd_string(__entry->cmd),
__entry->cmd, __entry->param0,
__entry->param1, __entry->param2,
--
2.19.1
[PATCH 08/14] usb: dwc3: gadget: introduce cancelled_list
This list will host cancelled requests who still have TRBs being
processed.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/core.h | 2 ++
drivers/usb/dwc3/gadget.c | 1 +
drivers/usb/dwc3/gadget.h | 15 +++
3 files changed, 18 insertions(+)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b490d2c49c94..69ecb3fcc1ed 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -647,6 +647,7 @@ struct dwc3_event_buffer {
/**
* struct dwc3_ep - device side endpoint representation
* @endpoint: usb endpoint
+ * @cancelled_list: list of cancelled requests for this endpoint
* @pending_list: list of pending requests for this endpoint
* @started_list: list of started requests on this endpoint
* @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete
@@ -670,6 +671,7 @@ struct dwc3_event_buffer {
*/
struct dwc3_ep {
struct usb_ep endpoint;
+ struct list_headcancelled_list;
struct list_headpending_list;
struct list_headstarted_list;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fae37972a78b..3d50a3d69232 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2153,6 +2153,7 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8
epnum)
INIT_LIST_HEAD(&dep->pending_list);
INIT_LIST_HEAD(&dep->started_list);
+ INIT_LIST_HEAD(&dep->cancelled_list);
return 0;
}
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 2aacd1afd9ff..023a473648eb 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -79,6 +79,21 @@ static inline void dwc3_gadget_move_started_request(struct
dwc3_request *req)
list_move_tail(&req->list, &dep->started_list);
}
+/**
+ * dwc3_gadget_move_cancelled_request - move @req to the cancelled_list
+ * @req: the request to be moved
+ *
+ * Caller should take care of locking. This function will move @req from its
+ * current list to the endpoint's cancelled_list.
+ */
+static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request *req)
+{
+ struct dwc3_ep *dep = req->dep;
+
+ req->started = false;
+ list_move_tail(&req->list, &dep->cancelled_list);
+}
+
void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
int status);
--
2.19.1
[PATCH 05/14] usb: dwc3: gadget: track number of TRBs per request
This will help us remove the wait_event() from our ->dequeue().
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/core.h | 3 +++
drivers/usb/dwc3/gadget.c | 6 ++
2 files changed, 9 insertions(+)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b2ec1a287322..b490d2c49c94 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -858,6 +858,7 @@ struct dwc3_hwparams {
* @epnum: endpoint number to which this request refers
* @trb: pointer to struct dwc3_trb
* @trb_dma: DMA address of @trb
+ * @num_trbs: number of TRBs used by this request
* @needs_extra_trb: true when request needs one extra TRB (either due to ZLP
* or unaligned OUT)
* @direction: IN or OUT direction flag
@@ -878,6 +879,8 @@ struct dwc3_request {
struct dwc3_trb *trb;
dma_addr_t trb_dma;
+ unsignednum_trbs;
+
unsignedneeds_extra_trb:1;
unsigneddirection:1;
unsignedmapped:1;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 132bb6b9ae44..d945019a4139 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1046,6 +1046,8 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
req->trb_dma = dwc3_trb_dma_offset(dep, trb);
}
+ req->num_trbs++;
+
__dwc3_prepare_one_trb(dep, trb, dma, length, chain, node,
stream_id, short_not_ok, no_interrupt);
}
@@ -1080,6 +1082,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
/* Now prepare one extra TRB to align transfer size */
trb = &dep->trb_pool[dep->trb_enqueue];
+ req->num_trbs++;
__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
maxp - rem, false, 1,
req->request.stream_id,
@@ -1124,6 +1127,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep
*dep,
/* Now prepare one extra TRB to align transfer size */
trb = &dep->trb_pool[dep->trb_enqueue];
+ req->num_trbs++;
__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,
false, 1, req->request.stream_id,
req->request.short_not_ok,
@@ -1140,6 +1144,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep
*dep,
/* Now prepare one extra TRB to handle ZLP */
trb = &dep->trb_pool[dep->trb_enqueue];
+ req->num_trbs++;
__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0,
false, 1, req->request.stream_id,
req->request.short_not_ok,
@@ -2240,6 +2245,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct
dwc3_ep *dep,
dwc3_ep_inc_deq(dep);
trace_dwc3_complete_trb(dep, trb);
+ req->num_trbs--;
/*
* If we're in the middle of series of chained TRBs and we
--
2.19.1
[PATCH 11/14] usb: dwc3: gadget: return errors from __dwc3_gadget_start_isoc()
Sometimes, errors happen when kicking transfers from
__dwc3_gadget_start_isoc(). In those cases, we need to pass along the
error so gadget driver can make informed decisions.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 64481da433ae..18c63137348d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1266,17 +1266,17 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
return DWC3_DSTS_SOFFN(reg);
}
-static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
+static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
{
if (list_empty(&dep->pending_list)) {
dev_info(dep->dwc->dev, "%s: ran out of requests\n",
dep->name);
dep->flags |= DWC3_EP_PENDING_REQUEST;
- return;
+ return -EAGAIN;
}
dep->frame_number = DWC3_ALIGN_FRAME(dep);
- __dwc3_gadget_kick_transfer(dep);
+ return __dwc3_gadget_kick_transfer(dep);
}
static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request
*req)
@@ -1317,8 +1317,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep,
struct dwc3_request *req)
if ((dep->flags & DWC3_EP_PENDING_REQUEST)) {
if (!(dep->flags & DWC3_EP_TRANSFER_STARTED)) {
- __dwc3_gadget_start_isoc(dep);
- return 0;
+ return __dwc3_gadget_start_isoc(dep);
}
}
}
@@ -2381,7 +2380,7 @@ static void
dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,
const struct dwc3_event_depevt *event)
{
dwc3_gadget_endpoint_frame_from_event(dep, event);
- __dwc3_gadget_start_isoc(dep);
+ (void) __dwc3_gadget_start_isoc(dep);
}
static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
--
2.19.1
[PATCH 13/14] usb: dwc3: gadget: remove unnecessary dev_info()
Running out of requests on isochronous endpoints is part of normal
operation. We don't really need to know about it every time it
happens.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 18c63137348d..d8c7ad0c22e8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1269,8 +1269,6 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
{
if (list_empty(&dep->pending_list)) {
- dev_info(dep->dwc->dev, "%s: ran out of requests\n",
- dep->name);
dep->flags |= DWC3_EP_PENDING_REQUEST;
return -EAGAIN;
}
--
2.19.1
[PATCH 14/14] usb: dwc3: gadget: check if dep->frame_number is still valid
Gadget driver may take an unbounded amount of time to queue requests
after XferNotReady. This is important for isochronous endpoints which
need to be started for a specific (micro-)frame.
If we fail to start a transfer for isochronous endpoint, let's try
queueing to a future interval and see if that helps. We will stop trying
if we fail a start transfer for 5 intervals in the future.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/core.h | 1 +
drivers/usb/dwc3/gadget.c | 17 ++---
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index ea59f9b4908a..85599ad6648f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -38,6 +38,7 @@
#define DWC3_EP0_SETUP_SIZE512
#define DWC3_ENDPOINTS_NUM 32
#define DWC3_XHCI_RESOURCES_NUM2
+#define DWC3_ISOC_MAX_RETRIES 5
#define DWC3_SCRATCHBUF_SIZE 4096/* each buffer is assumed to be 4KiB */
#define DWC3_EVENT_BUFFERS_SIZE4096
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d8c7ad0c22e8..1590516735cb 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -27,7 +27,7 @@
#include "gadget.h"
#include "io.h"
-#define DWC3_ALIGN_FRAME(d)(((d)->frame_number + (d)->interval) \
+#define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval * (n))) \
& ~((d)->interval - 1))
/**
@@ -1268,13 +1268,24 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
{
+ int retries;
+ int ret;
+ int i;
+
if (list_empty(&dep->pending_list)) {
dep->flags |= DWC3_EP_PENDING_REQUEST;
return -EAGAIN;
}
- dep->frame_number = DWC3_ALIGN_FRAME(dep);
- return __dwc3_gadget_kick_transfer(dep);
+ for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
+ dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
+
+ ret = __dwc3_gadget_kick_transfer(dep);
+ if (ret != -EAGAIN)
+ break;
+ }
+
+ return ret;
}
static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request
*req)
--
2.19.1
[PATCH 09/14] usb: dwc3: gadget: move requests to cancelled_list
Whenever we have a request in flight, we can move it to the cancelled
list and later simply iterate over that list and skip over any TRBs we
find.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3d50a3d69232..64c130c2f7b4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1369,6 +1369,17 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep
*dep, struct dwc3_request *r
}
}
+static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
+{
+ struct dwc3_request *req;
+ struct dwc3_request *tmp;
+
+ list_for_each_entry_safe(req, tmp, &dep->cancelled_list, list) {
+ dwc3_gadget_ep_skip_trbs(dep, req);
+ dwc3_gadget_giveback(dep, req, -ECONNRESET);
+ }
+}
+
static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
struct usb_request *request)
{
@@ -1405,8 +1416,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
if (!r->trb)
goto out0;
- dwc3_gadget_ep_skip_trbs(dep, r);
- goto out1;
+ dwc3_gadget_move_cancelled_request(req);
+ dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+ goto out0;
}
dev_err(dwc->dev, "request %pK was not queued to %s\n",
request, ep->name);
@@ -1414,7 +1426,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
goto out0;
}
-out1:
dwc3_gadget_giveback(dep, req, -ECONNRESET);
out0:
--
2.19.1
[PATCH 10/14] usb: dwc3: gadget: remove wait_end_transfer
Now that we have a list of cancelled requests, we can skip over TRBs
when END_TRANSFER command completes.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/core.h | 3 ---
drivers/usb/dwc3/gadget.c | 40 +--
2 files changed, 1 insertion(+), 42 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 69ecb3fcc1ed..ea59f9b4908a 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -650,7 +650,6 @@ struct dwc3_event_buffer {
* @cancelled_list: list of cancelled requests for this endpoint
* @pending_list: list of pending requests for this endpoint
* @started_list: list of started requests on this endpoint
- * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete
* @lock: spinlock for endpoint request queue traversal
* @regs: pointer to first endpoint register
* @trb_pool: array of transaction buffers
@@ -675,8 +674,6 @@ struct dwc3_ep {
struct list_headpending_list;
struct list_headstarted_list;
- wait_queue_head_t wait_end_transfer;
-
spinlock_t lock;
void __iomem*regs;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 64c130c2f7b4..64481da433ae 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -647,8 +647,6 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
unsigned int action)
reg |= DWC3_DALEPENA_EP(dep->number);
dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
- init_waitqueue_head(&dep->wait_end_transfer);
-
if (usb_endpoint_xfer_control(desc))
goto out;
@@ -1409,15 +1407,11 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
if (r == req) {
/* wait until it is processed */
dwc3_stop_active_transfer(dep, true);
- wait_event_lock_irq(dep->wait_end_transfer,
- !(dep->flags &
DWC3_EP_END_TRANSFER_PENDING),
- dwc->lock);
if (!r->trb)
goto out0;
dwc3_gadget_move_cancelled_request(req);
- dwc3_gadget_ep_cleanup_cancelled_requests(dep);
goto out0;
}
dev_err(dwc->dev, "request %pK was not queued to %s\n",
@@ -1922,8 +1916,6 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
{
struct dwc3 *dwc = gadget_to_dwc(g);
unsigned long flags;
- int epnum;
- u32 tmo_eps = 0;
spin_lock_irqsave(&dwc->lock, flags);
@@ -1932,36 +1924,6 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
__dwc3_gadget_stop(dwc);
- for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
- struct dwc3_ep *dep = dwc->eps[epnum];
- int ret;
-
- if (!dep)
- continue;
-
- if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
- continue;
-
- ret =
wait_event_interruptible_lock_irq_timeout(dep->wait_end_transfer,
- !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
- dwc->lock, msecs_to_jiffies(5));
-
- if (ret <= 0) {
- /* Timed out or interrupted! There's nothing much
-* we can do so we just log here and print which
-* endpoints timed out at the end.
-*/
- tmo_eps |= 1 << epnum;
- dep->flags &= DWC3_EP_END_TRANSFER_PENDING;
- }
- }
-
- if (tmo_eps) {
- dev_err(dwc->dev,
- "end transfer timed out on endpoints 0x%x [bitmap]\n",
- tmo_eps);
- }
-
out:
dwc->gadget_driver = NULL;
spin_unlock_irqrestore(&dwc->lock, flags);
@@ -2457,7 +2419,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
if (cmd == DWC3_DEPCMD_ENDTRANSFER) {
dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
- wake_up(&dep->wait_end_transfer);
+ dwc3_gadget_ep_cleanup_cancelled_requests(dep);
}
break;
case DWC3_DEPEVT_STREAMEVT:
--
2.19.1
[PATCHv4] usb: gadget: f_fs: Allow scatter-gather buffers
Some protocols implemented in userspace with FunctionFS might require large
buffers, e.g. 64kB or more. Currently the said memory is allocated with
kmalloc, which might fail should system memory be highly fragmented.
On the other hand, some UDC hardware allows scatter-gather operation and
this patch takes advantage of this capability: if the requested buffer
is larger than PAGE_SIZE and the UDC allows scatter-gather operation, then
the buffer is allocated with vmalloc and a scatterlist describing it is
created and passed to usb request.
Signed-off-by: Andrzej Pietrasiewicz
---
v4:
- corrected the offset in invocation of sg_alloc_table_from_pages()
v3:
- simplified calculating number of pages the vmalloc'ed buffer occupies
- simplified types in ffs_build_sg_list()
- whitespace corrections
v2:
- added include directives for kvmalloc_array and vmalloc
drivers/usb/gadget/function/f_fs.c | 93 +++---
1 file changed, 86 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 3ada83d..1be865c 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -18,9 +18,12 @@
#include
#include
#include
+#include
#include
+#include
#include
#include
+#include
#include
#include
@@ -219,6 +222,8 @@ struct ffs_io_data {
struct usb_ep *ep;
struct usb_request *req;
+ struct sg_table sgt;
+ bool use_sg;
struct ffs_data *ffs;
};
@@ -750,6 +755,65 @@ static ssize_t ffs_copy_to_iter(void *data, int data_len,
struct iov_iter *iter)
return ret;
}
+/*
+ * allocate a virtually contiguous buffer and create a scatterlist describing
it
+ * @sg_table - pointer to a place to be filled with sg_table contents
+ * @size - required buffer size
+ */
+static void *ffs_build_sg_list(struct sg_table *sgt, size_t sz)
+{
+ struct page **pages;
+ void *vaddr, *ptr;
+ unsigned int n_pages;
+ int i;
+
+ vaddr = vmalloc(sz);
+ if (!vaddr)
+ return NULL;
+
+ n_pages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
+ pages = kvmalloc_array(n_pages, sizeof(struct page *), GFP_KERNEL);
+ if (!pages) {
+ vfree(vaddr);
+
+ return NULL;
+ }
+ for (i = 0, ptr = vaddr; i < n_pages; ++i, ptr += PAGE_SIZE)
+ pages[i] = vmalloc_to_page(ptr);
+
+ if (sg_alloc_table_from_pages(sgt, pages, n_pages, 0, sz, GFP_KERNEL)) {
+ kvfree(pages);
+ vfree(vaddr);
+
+ return NULL;
+ }
+ kvfree(pages);
+
+ return vaddr;
+}
+
+static inline void *ffs_alloc_buffer(struct ffs_io_data *io_data,
+ size_t data_len)
+{
+ if (io_data->use_sg)
+ return ffs_build_sg_list(&io_data->sgt, data_len);
+
+ return kmalloc(data_len, GFP_KERNEL);
+}
+
+static inline void ffs_free_buffer(struct ffs_io_data *io_data)
+{
+ if (!io_data->buf)
+ return;
+
+ if (io_data->use_sg) {
+ sg_free_table(&io_data->sgt);
+ vfree(io_data->buf);
+ } else {
+ kfree(io_data->buf);
+ }
+}
+
static void ffs_user_copy_worker(struct work_struct *work)
{
struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
@@ -777,7 +841,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
if (io_data->read)
kfree(io_data->to_free);
- kfree(io_data->buf);
+ ffs_free_buffer(io_data);
kfree(io_data);
}
@@ -933,6 +997,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct
ffs_io_data *io_data)
* earlier
*/
gadget = epfile->ffs->gadget;
+ io_data->use_sg = gadget->sg_supported && data_len > PAGE_SIZE;
spin_lock_irq(&epfile->ffs->eps_lock);
/* In the meantime, endpoint got disabled or changed. */
@@ -949,7 +1014,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct
ffs_io_data *io_data)
data_len = usb_ep_align_maybe(gadget, ep->ep, data_len);
spin_unlock_irq(&epfile->ffs->eps_lock);
- data = kmalloc(data_len, GFP_KERNEL);
+ data = ffs_alloc_buffer(io_data, data_len);
if (unlikely(!data)) {
ret = -ENOMEM;
goto error_mutex;
@@ -989,8 +1054,16 @@ static ssize_t ffs_epfile_io(struct file *file, struct
ffs_io_data *io_data)
bool interrupted = false;
req = ep->req;
- req->buf = data;
- req->length = data_len;
+ if (io_data->use_sg) {
+ req->buf = NULL;
+ req->sg = io_data->sgt.sgl;
+ req->num_sgs = io_data->sgt.nents;
+ } else {
+
[PATCH 1/2] Revert "usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers"
From: Shen Jing
This reverts commit b4194da3f9087dd38d91b40f9bec42d59ce589a8
since it causes list corruption followed by kernel panic:
Workqueue: adb ffs_aio_cancel_worker
RIP: 0010:__list_add_valid+0x4d/0x70
Call Trace:
insert_work+0x47/0xb0
__queue_work+0xf6/0x400
queue_work_on+0x65/0x70
dwc3_gadget_giveback+0x44/0x50 [dwc3]
dwc3_gadget_ep_dequeue+0x83/0x2d0 [dwc3]
? finish_wait+0x80/0x80
usb_ep_dequeue+0x1e/0x90
process_one_work+0x18c/0x3b0
worker_thread+0x3c/0x390
? process_one_work+0x3b0/0x3b0
kthread+0x11e/0x140
? kthread_create_worker_on_cpu+0x70/0x70
ret_from_fork+0x3a/0x50
This issue is seen with warm reboot stability testing.
Signed-off-by: Shen Jing
Signed-off-by: Saranya Gopal
Signed-off-by: Felipe Balbi
---
drivers/usb/gadget/function/f_fs.c | 26 --
1 file changed, 8 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 3ada83d81bda..31e8bf3578c8 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -215,7 +215,6 @@ struct ffs_io_data {
struct mm_struct *mm;
struct work_struct work;
- struct work_struct cancellation_work;
struct usb_ep *ep;
struct usb_request *req;
@@ -1073,31 +1072,22 @@ ffs_epfile_open(struct inode *inode, struct file *file)
return 0;
}
-static void ffs_aio_cancel_worker(struct work_struct *work)
-{
- struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
- cancellation_work);
-
- ENTER();
-
- usb_ep_dequeue(io_data->ep, io_data->req);
-}
-
static int ffs_aio_cancel(struct kiocb *kiocb)
{
struct ffs_io_data *io_data = kiocb->private;
- struct ffs_data *ffs = io_data->ffs;
+ struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
int value;
ENTER();
- if (likely(io_data && io_data->ep && io_data->req)) {
- INIT_WORK(&io_data->cancellation_work, ffs_aio_cancel_worker);
- queue_work(ffs->io_completion_wq, &io_data->cancellation_work);
- value = -EINPROGRESS;
- } else {
+ spin_lock_irq(&epfile->ffs->eps_lock);
+
+ if (likely(io_data && io_data->ep && io_data->req))
+ value = usb_ep_dequeue(io_data->ep, io_data->req);
+ else
value = -EINVAL;
- }
+
+ spin_unlock_irq(&epfile->ffs->eps_lock);
return value;
}
--
2.19.1
[PATCH] usb: dwc3: gadget: fix ISOC TRB type on unaligned transfers
When chaining ISOC TRBs together, only the first ISOC TRB should be of
type ISOC_FIRST, all others should be of type ISOC. This patch fixes
that.
Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
Cc: # v4.11+
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 06e22afdf3d1..9faad896b3a1 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1081,7 +1081,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
/* Now prepare one extra TRB to align transfer size */
trb = &dep->trb_pool[dep->trb_enqueue];
__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
- maxp - rem, false, 0,
+ maxp - rem, false, 1,
req->request.stream_id,
req->request.short_not_ok,
req->request.no_interrupt);
@@ -1125,7 +1125,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep
*dep,
/* Now prepare one extra TRB to align transfer size */
trb = &dep->trb_pool[dep->trb_enqueue];
__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,
- false, 0, req->request.stream_id,
+ false, 1, req->request.stream_id,
req->request.short_not_ok,
req->request.no_interrupt);
} else if (req->request.zero && req->request.length &&
@@ -1141,7 +1141,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep
*dep,
/* Now prepare one extra TRB to handle ZLP */
trb = &dep->trb_pool[dep->trb_enqueue];
__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0,
- false, 0, req->request.stream_id,
+ false, 1, req->request.stream_id,
req->request.short_not_ok,
req->request.no_interrupt);
} else {
--
2.19.1
Re: [PATCH v6 2/3] usb: dwc3: Add disabling of start_transfer failure quirk
Thinh Nguyen writes: > DWC_usb31 peripheral v1.70a-ea06 and prior needs a SW workaround for > isoc START TRANSFER command failure. However, some affected versions may > have RTL patches to fix this without a SW workaround. Add this quirk to > disable the SW workaround when it is not needed. > > Synopsys STAR 9001202023: Wrong microframe number for isochronous IN > endpoints. > > Signed-off-by: Thinh Nguyen unfortunately, does't apply to testing/next. Could you rebase?? checking file Documentation/devicetree/bindings/usb/dwc3.txt Hunk #1 FAILED at 37. 1 out of 1 hunk FAILED (patch 1/3 is applied, though) -- balbi signature.asc Description: PGP signature
[PATCH] usb: dwc3: drd: fix semicolon.cocci warnings
From: kbuild test robot
drivers/usb/dwc3/drd.c:491:2-3: Unneeded semicolon
Remove unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
Fixes: 24e2238d8c10 ("usb: dwc3: drd: Register a USB role switch")
CC: Heikki Krogerus
Signed-off-by: kbuild test robot
---
tree: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
testing/next
head: bad6c1502dac79c80ad5a7149fa308849c0191dd
commit: 24e2238d8c102f242ece57f45fbeb4014929aad4 [17/22] usb: dwc3: drd:
Register a USB role switch
drd.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -488,7 +488,7 @@ static int dwc3_usb_role_switch_set(stru
default:
mode = DWC3_GCTL_PRTCAP_OTG;
break;
- };
+ }
dwc3_set_mode(dev_get_drvdata(dev), mode);
return 0;
[balbi-usb:testing/next 17/22] drivers/usb/dwc3/drd.c:491:2-3: Unneeded semicolon
tree: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next head: bad6c1502dac79c80ad5a7149fa308849c0191dd commit: 24e2238d8c102f242ece57f45fbeb4014929aad4 [17/22] usb: dwc3: drd: Register a USB role switch coccinelle warnings: (new ones prefixed by >>) >> drivers/usb/dwc3/drd.c:491:2-3: Unneeded semicolon Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
[PATCH v7 0/2] usb: dwc3: Workaround isoc start_transfer failure
DWC_usb31 peripheral v1.70a-ea06 and prior needs a SW workaround for isoc START TRANSFER command failure. This patch series implements that workaround Change in v7: - Rebased on Felipe's branch Change in v6: - Defined more version types in "usb: dwc3: Track DWC_usb31 VERSIONTYPE" - Minor cleanup/fix in "usb: dwc3: Add workaround for isoc start transfer failure" Change in v5: - Splitted and resent from an old patch series - Cleanup and fixed review issues Change in v4: - None Change in v3: - None Change in v2: - None Thinh Nguyen (2): usb: dwc3: Add disabling of start_transfer failure quirk usb: dwc3: Add workaround for isoc start transfer failure Documentation/devicetree/bindings/usb/dwc3.txt | 3 + drivers/usb/dwc3/core.c| 2 + drivers/usb/dwc3/core.h| 13 +++ drivers/usb/dwc3/gadget.c | 131 + 4 files changed, 149 insertions(+) -- 2.11.0
[PATCH v7 1/2] usb: dwc3: Add disabling of start_transfer failure quirk
DWC_usb31 peripheral v1.70a-ea06 and prior needs a SW workaround for isoc START TRANSFER command failure. However, some affected versions may have RTL patches to fix this without a SW workaround. Add this quirk to disable the SW workaround when it is not needed. Synopsys STAR 9001202023: Wrong microframe number for isochronous IN endpoints. Signed-off-by: Thinh Nguyen Reviewed-by: Rob Herring --- Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt index 7e33e53e7c29..8e5265e9f658 100644 --- a/Documentation/devicetree/bindings/usb/dwc3.txt +++ b/Documentation/devicetree/bindings/usb/dwc3.txt @@ -39,6 +39,9 @@ Optional properties: - resets: a single pair of phandle and reset specifier - snps,usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM - snps,usb3_lpm_capable: determines if platform is USB3 LPM capable + - snps,dis-start-transfer-quirk: when set, disable isoc START TRANSFER command + failure SW work-around for DWC_usb31 version 1.70a-ea06 + and prior. - snps,disable_scramble_quirk: true when SW should disable data scrambling. Only really useful for FPGA builds. - snps,has-lpm-erratum: true when DWC3 was configured with LPM Erratum enabled -- 2.11.0
[PATCH v7 2/2] usb: dwc3: Add workaround for isoc start transfer failure
In DWC_usb31 version 1.70a-ea06 and prior, for highspeed and fullspeed
isochronous IN, BIT[15:14] of the 16-bit microframe number reported by
the XferNotReady event are invalid. The driver uses this number to
schedule the isochronous transfer and passes it to the START TRANSFER
command. Because this number is invalid, the command may fail. If
BIT[15:14] matches the internal 16-bit microframe, the START TRANSFER
command will pass and the transfer will start at the scheduled time, if
it is off by 1, the command will still pass, but the transfer will start
2 seconds in the future. For all other conditions, the START TRANSFER
command will fail with bus-expiry.
In order to workaround this issue, we can test for the correct
combination of BIT[15:14] by sending START TRANSFER commands with
different values of BIT[15:14]: 'b00, 'b01, 'b10, and 'b11. Each
combination is 2^14 uframe apart (or 2 seconds). 4 seconds into the
future will result in a bus-expiry status. As the result, within the 4
possible combinations for BIT[15:14], there will be 2 successful and 2
failure START COMMAND status. One of the 2 successful command status
will result in a 2-second delay start. The smaller BIT[15:14] value is
the correct combination.
Since there are only 4 outcomes and the results are ordered, we can
simply test 2 START TRANSFER commands with BIT[15:14] combinations 'b00
and 'b01 to deduce the smaller successful combination.
Let test0 = test status for combination 'b00 and test1 = test status for
'b01 of BIT[15:14]. The correct combination is as follow:
if test0 fails and test1 passes, BIT[15:14] is 'b01
if test0 fails and test1 fails, BIT[15:14] is 'b10
if test0 passes and test1 fails, BIT[15:14] is 'b11
if test0 passes and test1 passes, BIT[15:14] is 'b00
Synopsys STAR 9001202023: Wrong microframe number for isochronous IN
endpoints.
Signed-off-by: Thinh Nguyen
---
drivers/usb/dwc3/core.c | 2 +
drivers/usb/dwc3/core.h | 13 +
drivers/usb/dwc3/gadget.c | 131 ++
3 files changed, 146 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b8414814b869..77e7d9ae770f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1248,6 +1248,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
"snps,is-utmi-l1-suspend");
device_property_read_u8(dev, "snps,hird-threshold",
&hird_threshold);
+ dwc->dis_start_transfer_quirk = device_property_read_bool(dev,
+ "snps,dis-start-transfer-quirk");
dwc->usb3_lpm_capable = device_property_read_bool(dev,
"snps,usb3_lpm_capable");
dwc->usb2_lpm_disable = device_property_read_bool(dev,
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index ed0359d1216d..30299da211e5 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -667,6 +667,10 @@ struct dwc3_event_buffer {
* @name: a human readable name e.g. ep1out-bulk
* @direction: true for TX, false for RX
* @stream_capable: true when streams are enabled
+ * @combo_num: the test combination BIT[15:14] of the frame number to test
+ * isochronous START TRANSFER command failure workaround
+ * @start_cmd_status: the status of testing START TRANSFER command with
+ * combo_num = 'b00
*/
struct dwc3_ep {
struct usb_ep endpoint;
@@ -716,6 +720,10 @@ struct dwc3_ep {
unsigneddirection:1;
unsignedstream_capable:1;
+
+ /* For isochronous START TRANSFER workaround only */
+ u8 combo_num;
+ int start_cmd_status;
};
enum dwc3_phy {
@@ -983,6 +991,8 @@ struct dwc3_scratchpad_array {
* @pullups_connected: true when Run/Stop bit is set
* @setup_packet_pending: true when there's a Setup Packet in FIFO. Workaround
* @three_stage_setup: set if we perform a three phase setup
+ * @dis_start_transfer_quirk: set if start_transfer failure SW workaround is
+ * not needed for DWC_usb31 version 1.70a-ea06 and below
* @usb3_lpm_capable: set if hadrware supports Link Power Management
* @usb2_lpm_disable: set to disable usb2 lpm
* @disable_scramble_quirk: set if we enable the disable scramble quirk
@@ -1119,6 +1129,8 @@ struct dwc3 {
#define DWC3_REVISION_IS_DWC31 0x8000
#define DWC3_USB31_REVISION_110A (0x3131302a | DWC3_REVISION_IS_DWC31)
#define DWC3_USB31_REVISION_120A (0x3132302a | DWC3_REVISION_IS_DWC31)
+#define DWC3_USB31_REVISION_160A (0x3136302a | DWC3_REVISION_IS_DWC31)
+#define DWC3_USB31_REVISION_170A (0x3137302a | DWC3_REVISION_IS_DWC31)
u32 version_type;
@@ -1172,6 +1184,7 @@ struct dwc3 {
unsignedpullups_connected:1;
unsignedsetup_packet_pending:1;
unsigned
Re: [PATCH v6 2/3] usb: dwc3: Add disabling of start_transfer failure quirk
Hi, On 11/14/2018 3:41 AM, Felipe Balbi wrote: > Thinh Nguyen writes: > >> DWC_usb31 peripheral v1.70a-ea06 and prior needs a SW workaround for >> isoc START TRANSFER command failure. However, some affected versions may >> have RTL patches to fix this without a SW workaround. Add this quirk to >> disable the SW workaround when it is not needed. >> >> Synopsys STAR 9001202023: Wrong microframe number for isochronous IN >> endpoints. >> >> Signed-off-by: Thinh Nguyen > unfortunately, does't apply to testing/next. Could you rebase?? > Done. Thanks, Thinh
Re: [PATCH 1/2] usb: gadget: Add start_frame to usb_request
Hi Felipe,
On 11/7/2018 11:03 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen writes:
>> Similar to URB's start_frame, add a field start_frame to the usb_request
>> to report the scheduled (micro)frame number of an isochronous transfer.
>> This option is useful for debugging purposes.
>>
>> Signed-off-by: Thinh Nguyen
>> ---
>> include/linux/usb/gadget.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index e5cd84a0f84a..ed9dbbce55ee 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -50,6 +50,7 @@ struct usb_ep;
>> * @short_not_ok: When reading data, makes short packets be
>> * treated as errors (queue stops advancing till cleanup).
>> * @dma_mapped: Indicates if request has been mapped to DMA (internal)
>> + * @start_frame: the reported (micro)frame of the scheduled isoc transfer
>> * @complete: Function called when request completes, so this request and
>> * its buffer may be re-used. The function will always be called with
>> * interrupts disabled, and it must not sleep.
>> @@ -107,6 +108,8 @@ struct usb_request {
>> unsignedshort_not_ok:1;
>> unsigneddma_mapped:1;
>>
>> +int start_frame;/* ISO ONLY */
> this name is a bit misleading. I can see functions trying to use it to
> request that a particular request start on a specific frame. Would
> "started_frame" be a better name, perhaps?
What do you think of changing this field name to "frame_number"? It's
generic enough so that this field can potentially be updated in the
future to be used as a starting frame in the future, for synchronization.
Thinh
Re: [PATCH 1/2] usb: gadget: Add start_frame to usb_request
Hi,
Thinh Nguyen writes:
> Hi Felipe,
>
> On 11/7/2018 11:03 PM, Felipe Balbi wrote:
>> Hi,
>>
>> Thinh Nguyen writes:
>>> Similar to URB's start_frame, add a field start_frame to the usb_request
>>> to report the scheduled (micro)frame number of an isochronous transfer.
>>> This option is useful for debugging purposes.
>>>
>>> Signed-off-by: Thinh Nguyen
>>> ---
>>> include/linux/usb/gadget.h | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>> index e5cd84a0f84a..ed9dbbce55ee 100644
>>> --- a/include/linux/usb/gadget.h
>>> +++ b/include/linux/usb/gadget.h
>>> @@ -50,6 +50,7 @@ struct usb_ep;
>>> * @short_not_ok: When reading data, makes short packets be
>>> * treated as errors (queue stops advancing till cleanup).
>>> * @dma_mapped: Indicates if request has been mapped to DMA (internal)
>>> + * @start_frame: the reported (micro)frame of the scheduled isoc transfer
>>> * @complete: Function called when request completes, so this request and
>>> * its buffer may be re-used. The function will always be called with
>>> * interrupts disabled, and it must not sleep.
>>> @@ -107,6 +108,8 @@ struct usb_request {
>>> unsignedshort_not_ok:1;
>>> unsigneddma_mapped:1;
>>>
>>> + int start_frame;/* ISO ONLY */
>> this name is a bit misleading. I can see functions trying to use it to
>> request that a particular request start on a specific frame. Would
>> "started_frame" be a better name, perhaps?
>
> What do you think of changing this field name to "frame_number"? It's
> generic enough so that this field can potentially be updated in the
> future to be used as a starting frame in the future, for synchronization.
I don't mind, but remember changing semantics will mean auditing every
user of this :-)
--
balbi
[GIT PULL] usb fixes for v4.20-rc2
Hi Greg, here's the first set of fixes for v4.20-rc cycle. Nothing really major, actually. Let me now if you want anything to be changed Cheers The following changes since commit 651022382c7f8da46cb4872a545ee1da6d097d2a: Linux 4.20-rc1 (2018-11-04 15:37:52 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git fixes-for-v4.20-rc2 for you to fetch changes up to 2fc6d4be35fb1e262f209758e25bfe2b7a113a7f: usb: dwc3: gadget: fix ISOC TRB type on unaligned transfers (2018-11-14 12:43:52 +0200) For now only 5 small fixes. Most importantly, we have a fix for the TRB type used on unaligned transfers on dwc3. Also a fix for a NULL pointer dereference in dwc3_pci_remove(). Note that a recent commit on ffs was reverted because it causes a regression elsewere. Andy Shevchenko (1): usb: dwc3: core: Clean up ULPI device Dan Carpenter (1): usb: dwc2: pci: Fix an error code in probe Felipe Balbi (1): usb: dwc3: gadget: fix ISOC TRB type on unaligned transfers Kuppuswamy Sathyanarayanan (1): usb: dwc3: Fix NULL pointer exception in dwc3_pci_remove() Shen Jing (1): Revert "usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers" Thinh Nguyen (1): usb: dwc3: gadget: Properly check last unaligned/zero chain TRB drivers/usb/dwc2/pci.c | 1 + drivers/usb/dwc3/core.c| 1 + drivers/usb/dwc3/dwc3-pci.c| 4 +++- drivers/usb/dwc3/gadget.c | 8 drivers/usb/gadget/function/f_fs.c | 26 -- 5 files changed, 17 insertions(+), 23 deletions(-) -- balbi signature.asc Description: PGP signature
Re: [GIT PULL] usb fixes for v4.20-rc2
On Wed, Nov 14, 2018 at 01:33:10PM +0200, Felipe Balbi wrote: > > Hi Greg, > > here's the first set of fixes for v4.20-rc cycle. Nothing really major, > actually. > > Let me now if you want anything to be changed Now pulled, thanks. greg k-h
[balbi-usb:testing/next 17/22] drivers/usb/dwc3/drd.c:604: undefined reference to `usb_role_switch_unregister'
tree: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
testing/next
head: bad6c1502dac79c80ad5a7149fa308849c0191dd
commit: 24e2238d8c102f242ece57f45fbeb4014929aad4 [17/22] usb: dwc3: drd:
Register a USB role switch
config: x86_64-randconfig-s4-11151335 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout 24e2238d8c102f242ece57f45fbeb4014929aad4
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/usb/dwc3/drd.o: In function `dwc3_drd_exit':
>> drivers/usb/dwc3/drd.c:604: undefined reference to
>> `usb_role_switch_unregister'
drivers/usb/dwc3/drd.o: In function `dwc3_drd_init':
>> drivers/usb/dwc3/drd.c:563: undefined reference to `usb_role_switch_register'
>> drivers/usb/dwc3/drd.c:563: undefined reference to `usb_role_switch_register'
vim +604 drivers/usb/dwc3/drd.c
514
515 int dwc3_drd_init(struct dwc3 *dwc)
516 {
517 int ret, irq;
518
519 dwc->edev = dwc3_get_extcon(dwc);
520 if (IS_ERR(dwc->edev))
521 return PTR_ERR(dwc->edev);
522
523 if (dwc->edev) {
524 dwc->edev_nb.notifier_call = dwc3_drd_notifier;
525 ret = extcon_register_notifier(dwc->edev,
EXTCON_USB_HOST,
526 &dwc->edev_nb);
527 if (ret < 0) {
528 dev_err(dwc->dev, "couldn't register cable
notifier\n");
529 return ret;
530 }
531
532 dwc3_drd_update(dwc);
533 } else {
534 dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_OTG);
535 dwc->current_dr_role = DWC3_GCTL_PRTCAP_OTG;
536
537 /* use OTG block to get ID event */
538 irq = dwc3_otg_get_irq(dwc);
539 if (irq < 0)
540 return irq;
541
542 dwc->otg_irq = irq;
543
544 /* disable all OTG IRQs */
545 dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS);
546 /* clear all events */
547 dwc3_otg_clear_events(dwc);
548
549 ret = request_threaded_irq(dwc->otg_irq, dwc3_otg_irq,
550 dwc3_otg_thread_irq,
551 IRQF_SHARED, "dwc3-otg",
dwc);
552 if (ret) {
553 dev_err(dwc->dev, "failed to request irq #%d
--> %d\n",
554 dwc->otg_irq, ret);
555 ret = -ENODEV;
556 return ret;
557 }
558
559 dwc3_otg_init(dwc);
560 dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
561 }
562
> 563 dwc->role_sw = usb_role_switch_register(dwc->dev,
> &dwc3_role_switch);
564 if (ret) {
565 dwc3_drd_exit(dwc);
566 return PTR_ERR(dwc->role_sw);
567 }
568 return 0;
569 }
570
571 void dwc3_drd_exit(struct dwc3 *dwc)
572 {
573 unsigned long flags;
574
575 if (dwc->edev)
576 extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
577 &dwc->edev_nb);
578
579 cancel_work_sync(&dwc->drd_work);
580
581 /* debug user might have changed role, clean based on current
role */
582 switch (dwc->current_dr_role) {
583 case DWC3_GCTL_PRTCAP_HOST:
584 dwc3_host_exit(dwc);
585 break;
586 case DWC3_GCTL_PRTCAP_DEVICE:
587 dwc3_gadget_exit(dwc);
588 dwc3_event_buffers_cleanup(dwc);
589 break;
590 case DWC3_GCTL_PRTCAP_OTG:
591 dwc3_otg_exit(dwc);
592 spin_lock_irqsave(&dwc->lock, flags);
593 dwc->desired_otg_role = DWC3_OTG_ROLE_IDLE;
594 spin_unlock_irqrestore(&dwc->lock, flags);
595 dwc3_otg_update(dwc, 1);
596 break;
597 default:
598 break;
599 }
600
601 if (!dwc->edev)
602 free_irq(dwc->otg_irq, dwc);
603
> 604 usb_role_switch_unregister(dwc->role_sw);
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [balbi-usb:testing/next 17/22] drivers/usb/dwc3/drd.c:604: undefined reference to `usb_role_switch_unregister'
Hi Heikki, kbuild test robot writes: > tree: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git > testing/next > head: bad6c1502dac79c80ad5a7149fa308849c0191dd > commit: 24e2238d8c102f242ece57f45fbeb4014929aad4 [17/22] usb: dwc3: drd: > Register a USB role switch > config: x86_64-randconfig-s4-11151335 (attached as .config) > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 > reproduce: > git checkout 24e2238d8c102f242ece57f45fbeb4014929aad4 > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > >drivers/usb/dwc3/drd.o: In function `dwc3_drd_exit': >>> drivers/usb/dwc3/drd.c:604: undefined reference to >>> `usb_role_switch_unregister' >drivers/usb/dwc3/drd.o: In function `dwc3_drd_init': >>> drivers/usb/dwc3/drd.c:563: undefined reference to >>> `usb_role_switch_register' >>> drivers/usb/dwc3/drd.c:563: undefined reference to >>> `usb_role_switch_register' care to fix this? -- balbi
Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi wrote: > I was thinking about again to change from NULL to EPROBE_DEFER. > > extcon_get_extcon_dev() function was almost called in the probe function. > But, this function might be called on other position instead of probe. *Might be* sounds like a theoretical thing, care to share what is in you mind? Current users and more important the new coming one are *all* doing the same. > ENODEV is more correct error instead of EPROBE_DEFER. So, you are proposing to continue duplicating conversion from ENODEV to EPROBE_DEFER in *each* caller? > Sorry. I'll withdraw my opinion related acked-by tag until we are clarifying > it. I honestly don't know what to clarify here. When we would have a real case we can change API correspondingly. For now, the score is 5:0 with use cases in practice. > On 2018년 11월 12일 09:24, Chanwoo Choi wrote: > > On 2018년 11월 11일 03:10, Andy Shevchenko wrote: > >> All current users of extcon_get_extcon_dev() API considers > >> an extcon device a mandatory to appear. Thus, they all convert > >> NULL pointer to -EPROBE_DEFER error code. > >> > >> There is one more caller anticipated with the same requirements. > >> > >> To decrease a code duplication and a burden to the callers, > >> return -EPROBE_DEFER directly from extcon_get_extcon_dev(). -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
On Wed, Nov 14, 2018 at 2:34 AM Mark Brown wrote: > > On Mon, Nov 12, 2018 at 06:11:26PM +0200, Peter Ujfalusi wrote: > > > if we revert the commit then the original issue will re-surfaces. afaik > > it was not only audio which hit the 'last driver to be probed from the > > deferred list would never probe, unless we provoke the kernel to load > > additional module, or remove/reload the module' issue. > > Right, aren't we just going to be swapping one bug for another? Have anyone in possession of Davinchi tested most recent kernel with this revert? > > Do I understand correctly that in your case you have two modules > > (dwc3-pci and extcon-intel-mrfld) in a deferred probe loop, iow both of > > the drivers returns -EPROBE_DEFER and they just spin? > > > If both is deferring, how this supposed to work? > > I'm struggling to follow the original explanation too :( Sorry, guys, I confused a nit myself. The bug is there, but exxplanation is not fully corrent, indeed. I'll come back with more details later. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
On 2018년 11월 14일 17:35, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi wrote: > >> I was thinking about again to change from NULL to EPROBE_DEFER. >> >> extcon_get_extcon_dev() function was almost called in the probe function. >> But, this function might be called on other position instead of probe. > > *Might be* sounds like a theoretical thing, care to share what is in you mind? > Current users and more important the new coming one are *all* doing the same. > >> ENODEV is more correct error instead of EPROBE_DEFER. > > So, you are proposing to continue duplicating conversion from ENODEV > to EPROBE_DEFER in *each* caller? The extcon core don't know the caller situation is in either probe() or other position in the caller driver. The caller driver should decide the kind of error value by using the return value of extcon_get_extcon_dev(). extcon_get_extcon_dev() function cannot be modified for only one case. If some device driver call extcon_get_extcon_dev() out of probe() fuction, EPROBE_DEFER is not always correct. > >> Sorry. I'll withdraw my opinion related acked-by tag until we are clarifying >> it. > > I honestly don't know what to clarify here. > > When we would have a real case we can change API correspondingly. > For now, the score is 5:0 with use cases in practice. > >> On 2018년 11월 12일 09:24, Chanwoo Choi wrote: >>> On 2018년 11월 11일 03:10, Andy Shevchenko wrote: All current users of extcon_get_extcon_dev() API considers an extcon device a mandatory to appear. Thus, they all convert NULL pointer to -EPROBE_DEFER error code. There is one more caller anticipated with the same requirements. To decrease a code duplication and a burden to the callers, return -EPROBE_DEFER directly from extcon_get_extcon_dev(). > -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: > On 2018년 11월 14일 17:35, Andy Shevchenko wrote: > > On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi wrote: > > > >> I was thinking about again to change from NULL to EPROBE_DEFER. > >> > >> extcon_get_extcon_dev() function was almost called in the probe function. > >> But, this function might be called on other position instead of probe. > > > > *Might be* sounds like a theoretical thing, care to share what is in you > > mind? > > Current users and more important the new coming one are *all* doing the > > same. > > > >> ENODEV is more correct error instead of EPROBE_DEFER. > > > > So, you are proposing to continue duplicating conversion from ENODEV > > to EPROBE_DEFER in *each* caller? > > The extcon core don't know the caller situation is in either probe() or other > position > in the caller driver. The caller driver should decide the kind of error value > by using the return value of extcon_get_extcon_dev(). > > extcon_get_extcon_dev() function cannot be modified for only one case. > If some device driver call extcon_get_extcon_dev() out of probe() fuction, > EPROBE_DEFER is not always correct. I agree with this, but look at the current state of affairs. All users do the same. If we need to have another case we may consider this later. API inside the kernel are not carved in the stone. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
On 2018년 11월 14일 18:36, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: >> On 2018년 11월 14일 17:35, Andy Shevchenko wrote: >>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi wrote: >>> I was thinking about again to change from NULL to EPROBE_DEFER. extcon_get_extcon_dev() function was almost called in the probe function. But, this function might be called on other position instead of probe. >>> >>> *Might be* sounds like a theoretical thing, care to share what is in you >>> mind? >>> Current users and more important the new coming one are *all* doing the >>> same. >>> ENODEV is more correct error instead of EPROBE_DEFER. >>> >>> So, you are proposing to continue duplicating conversion from ENODEV >>> to EPROBE_DEFER in *each* caller? >> >> The extcon core don't know the caller situation is in either probe() or >> other position >> in the caller driver. The caller driver should decide the kind of error value >> by using the return value of extcon_get_extcon_dev(). >> >> extcon_get_extcon_dev() function cannot be modified for only one case. >> If some device driver call extcon_get_extcon_dev() out of probe() fuction, >> EPROBE_DEFER is not always correct. > > I agree with this, but look at the current state of affairs. All users do the > same. > If we need to have another case we may consider this later. Because we know the potential wrong case of this change, I can't agree this change. If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER, it is clear and then there are no problem on both current and future. > > API inside the kernel are not carved in the stone. > > -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH v1 1/5] drivercore: Revert "deferral race condition fix"
Andy, On 2018-11-14 10:45, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 2:34 AM Mark Brown wrote: >> >> On Mon, Nov 12, 2018 at 06:11:26PM +0200, Peter Ujfalusi wrote: >> >>> if we revert the commit then the original issue will re-surfaces. afaik >>> it was not only audio which hit the 'last driver to be probed from the >>> deferred list would never probe, unless we provoke the kernel to load >>> additional module, or remove/reload the module' issue. >> >> Right, aren't we just going to be swapping one bug for another? > > Have anyone in possession of Davinchi tested most recent kernel with > this revert? I don't think it is related to daVinci, in fact we have seen the very same issue with OMAP4. Fwiw this was my initial patch http://lkml.iu.edu/hypermail/linux/kernel/1404.0/01603.html Grant based his change on this. Note the part in the commit message: "The issue has been observed on embedded systems with CONFIG_PREEMPT enabled, audio support built as modules and using nfsroot for root filesystem." as I recall I was only able to reproduce the stall with nfsroot and buildroot fs. The timings were different compared to MMC rootfs. Anyways the patch fixes a real race condition which is generic: We have driverA and driverB as modules. driverB needs driverA to be registered to a framework (no direct, hard dependency). 1. driverA starts to probe 2. driverB starts to probe 3. driverB can not continue as driverA is not registered itself to the framework -> driverB will defer 4. driverA registers to the framework 5. driverA probes successfully 6. driver core will prepare the deferred probe list (note: driverB is still in it's probe, not in the deferred list) 7. driverB returns from it's probe with -EPROBE_DEFER and here we are, driverB is alone in the deferred list and the list is not going to be tried as driverA and driverB were the last ones to probe. So, driverB is in the deferred list and stays there until other driver probes successfully as the deferred driver list only tried when a driver loads successfully (to see if that satisfy any of the deferred driver). We have evidence that this has happened, it is a generic issue given that now days we have more frameworks drivers are relying on. >>> Do I understand correctly that in your case you have two modules >>> (dwc3-pci and extcon-intel-mrfld) in a deferred probe loop, iow both of >>> the drivers returns -EPROBE_DEFER and they just spin? >> >>> If both is deferring, how this supposed to work? >> >> I'm struggling to follow the original explanation too :( > > Sorry, guys, I confused a nit myself. The bug is there, but > exxplanation is not fully corrent, indeed. I'll come back with more > details later. > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
On Wed, Nov 14, 2018 at 11:48 AM Chanwoo Choi wrote: > > On 2018년 11월 14일 18:36, Andy Shevchenko wrote: > > On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: > >> On 2018년 11월 14일 17:35, Andy Shevchenko wrote: > >>> On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi > >>> wrote: > >>> > I was thinking about again to change from NULL to EPROBE_DEFER. > > extcon_get_extcon_dev() function was almost called in the probe function. > But, this function might be called on other position instead of probe. > >>> > >>> *Might be* sounds like a theoretical thing, care to share what is in you > >>> mind? > >>> Current users and more important the new coming one are *all* doing the > >>> same. > >>> > ENODEV is more correct error instead of EPROBE_DEFER. > >>> > >>> So, you are proposing to continue duplicating conversion from ENODEV > >>> to EPROBE_DEFER in *each* caller? > >> > >> The extcon core don't know the caller situation is in either probe() or > >> other position > >> in the caller driver. The caller driver should decide the kind of error > >> value > >> by using the return value of extcon_get_extcon_dev(). > >> > >> extcon_get_extcon_dev() function cannot be modified for only one case. > >> If some device driver call extcon_get_extcon_dev() out of probe() fuction, > >> EPROBE_DEFER is not always correct. > > > > I agree with this, but look at the current state of affairs. All users do > > the same. > > If we need to have another case we may consider this later. > > Because we know the potential wrong case of this change, I can't agree this > change. > If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER, > it is clear and then there are no problem on both current and future. Changing NULL to -ENODEV is a trading bad to worse. I would not go that way, so, it's your call. > > API inside the kernel are not carved in the stone. Only can repeat myself (see above). While I see *theoretical* rationale on your side, mine has *practical* proofs. So, I'm giving up on this and will duplicate same what it's done in 4 current callers. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
On 2018년 11월 14일 19:20, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 11:48 AM Chanwoo Choi wrote: >> >> On 2018년 11월 14일 18:36, Andy Shevchenko wrote: >>> On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: On 2018년 11월 14일 17:35, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi > wrote: > >> I was thinking about again to change from NULL to EPROBE_DEFER. >> >> extcon_get_extcon_dev() function was almost called in the probe function. >> But, this function might be called on other position instead of probe. > > *Might be* sounds like a theoretical thing, care to share what is in you > mind? > Current users and more important the new coming one are *all* doing the > same. > >> ENODEV is more correct error instead of EPROBE_DEFER. > > So, you are proposing to continue duplicating conversion from ENODEV > to EPROBE_DEFER in *each* caller? The extcon core don't know the caller situation is in either probe() or other position in the caller driver. The caller driver should decide the kind of error value by using the return value of extcon_get_extcon_dev(). extcon_get_extcon_dev() function cannot be modified for only one case. If some device driver call extcon_get_extcon_dev() out of probe() fuction, EPROBE_DEFER is not always correct. >>> >>> I agree with this, but look at the current state of affairs. All users do >>> the same. >>> If we need to have another case we may consider this later. >> >> Because we know the potential wrong case of this change, I can't agree this >> change. >> If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER, >> it is clear and then there are no problem on both current and future. > > Changing NULL to -ENODEV is a trading bad to worse. > I would not go that way, so, it's your call. If you think that this change is not necessary, just keep the current code without the modification. Please drop this patch on next version. > >>> API inside the kernel are not carved in the stone. > > Only can repeat myself (see above). While I see *theoretical* > rationale on your side, mine has *practical* proofs. > So, I'm giving up on this and will duplicate same what it's done in 4 > current callers. > I think that it is more important for removing the potential wrong case instead of removing the duplicate code. The many device drivers already decided the proper error value by using the return value of function of kernel framework. -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
On Wed, Nov 14, 2018 at 1:05 PM Chanwoo Choi wrote: > On 2018년 11월 14일 19:20, Andy Shevchenko wrote: > > On Wed, Nov 14, 2018 at 11:48 AM Chanwoo Choi wrote: > >> On 2018년 11월 14일 18:36, Andy Shevchenko wrote: > >>> On Wed, Nov 14, 2018 at 06:13:37PM +0900, Chanwoo Choi wrote: > On 2018년 11월 14일 17:35, Andy Shevchenko wrote: > > On Wed, Nov 14, 2018 at 1:53 AM Chanwoo Choi > > wrote: > > > >> I was thinking about again to change from NULL to EPROBE_DEFER. > >> > >> extcon_get_extcon_dev() function was almost called in the probe > >> function. > >> But, this function might be called on other position instead of probe. > > > > *Might be* sounds like a theoretical thing, care to share what is in > > you mind? > > Current users and more important the new coming one are *all* doing the > > same. > > > >> ENODEV is more correct error instead of EPROBE_DEFER. > > > > So, you are proposing to continue duplicating conversion from ENODEV > > to EPROBE_DEFER in *each* caller? > > The extcon core don't know the caller situation is in either probe() or > other position > in the caller driver. The caller driver should decide the kind of error > value > by using the return value of extcon_get_extcon_dev(). > > extcon_get_extcon_dev() function cannot be modified for only one case. > If some device driver call extcon_get_extcon_dev() out of probe() > fuction, > EPROBE_DEFER is not always correct. > >>> > >>> I agree with this, but look at the current state of affairs. All users do > >>> the same. > >>> If we need to have another case we may consider this later. > >> > >> Because we know the potential wrong case of this change, I can't agree > >> this change. > >> If extcon_get_extcon_dev() returns ENODEV instead of EPROBE_DEFER, > >> it is clear and then there are no problem on both current and future. > > > > Changing NULL to -ENODEV is a trading bad to worse. > > I would not go that way, so, it's your call. > > If you think that this change is not necessary, just keep the current code > without the modification. Not only this, the useless churn for no benefit to anyone, except some *theoretical* cases no one saw. > Please drop this patch on next version. I will. > >>> API inside the kernel are not carved in the stone. > > > > Only can repeat myself (see above). While I see *theoretical* > > rationale on your side, mine has *practical* proofs. > > So, I'm giving up on this and will duplicate same what it's done in 4 > > current callers. > > > > I think that it is more important for removing the potential wrong case > instead of removing the duplicate code. The many device drivers already > decided the proper error value by using the return value of function of > kernel framework. The API has been introduced back in 2012. commit 74c5d09bd562edc220d6e076b8f1e118819c178f Author: Donggeun Kim Date: Fri Apr 20 14:16:24 2012 +0900 So, you are insisting that 6.5 years of use in a way people are using it is wrong? I don't know what might change your mind, but for me it's a clear win-win to switch to deferred probe error code based on the *practical* evidence. But as I said, I gave up now. P.S. I still disagree with your arguments in relation to de facto use of an API. -- With Best Regards, Andy Shevchenko
[PATCH 4/4] usbnet: smsc95xx: check for csum being in last four bytes
The manual states that the checksum cannot lie in the last DWORD of the
transmission, so add a basic check for this and fall back to software
checksumming the packet.
This only seems to trigger for ACK packets with no options or data to
return to the other end, and the use of the tx-alignment option makes
it more likely to happen.
Signed-off-by: Ben Dooks
---
Fixes for v2:
- Fix spelling of check at Sergei's suggestion
- Move skb->len check into smsc95xx_can_tx_checksum()
- Change name of smsc95xx_can_checksum to explicitly say it is tx-csum
---
drivers/net/usb/smsc95xx.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 8f7c473f3260..cc78ef78cc93 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1997,6 +1997,23 @@ static u32 smsc95xx_calc_csum_preamble(struct sk_buff
*skb)
return (high_16 << 16) | low_16;
}
+/* The TX CSUM won't work if the checksum lies in the last 4 bytes of the
+ * transmission. This is fairly unlikely, only seems to trigger with some
+ * short TCP ACK packets sent.
+ *
+ * Note, this calculation should probably check for the alignment of the
+ * data as well, but a straight check for csum being in the last four bytes
+ * of the packet should be ok for now.
+*/
+static bool smsc95xx_can_tx_checksum(struct sk_buff *skb)
+{
+ unsigned int len = skb->len - skb_checksum_start_offset(skb);
+
+ if (skb->len <= 45)
+ return false;
+ return skb->csum_offset < (len - (4 + 1));
+}
+
static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
struct sk_buff *skb, gfp_t flags)
{
@@ -2021,7 +2038,7 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet
*dev,
tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
if (csum) {
- if (skb->len <= 45) {
+ if (!smsc95xx_can_tx_checksum(skb)) {
/* workaround - hardware tx checksum does not work
* properly with extremely small packets */
long csstart = skb_checksum_start_offset(skb);
--
2.19.1
SMSC95xx driver updates (round 1)
This is a series of a few driver cleanups and some fixups of the code for the SMSC95XX driver. There have been a few reviews, and the issues have been fixed so this should be ready for merging. I will work on the tx-alignment and the other bits of usbnet changes and produce at least two more patch series for this later.
[PATCH 2/4] usbnet: smsc95xx: simplify tx_fixup code
The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
put an 8-byte command header onto the packet. It would be easier
to do one skb_push() and then copy the data in once the push is
done.
We also make the code smaller by using proper unaligned puts for
the header. This merges in the CPU to LE32 conversion as well and
makes the whole sequence easier to understand hopefully.
Signed-off-by: Ben Dooks
---
Since v1:
- Add alignment changes using put_unaligned_le32()
---
drivers/net/usb/smsc95xx.c | 28 +---
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 401ec9feb495..50e97a159500 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -2006,6 +2006,7 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet
*dev,
bool csum = skb->ip_summed == CHECKSUM_PARTIAL;
int overhead = csum ? SMSC95XX_TX_OVERHEAD_CSUM : SMSC95XX_TX_OVERHEAD;
u32 tx_cmd_a, tx_cmd_b;
+ void *ptr;
/* We do not advertise SG, so skbs should be already linearized */
BUG_ON(skb_shinfo(skb)->nr_frags);
@@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet
*dev,
return NULL;
}
+ tx_cmd_b = (u32)skb->len;
+ tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
+
if (csum) {
if (skb->len <= 45) {
/* workaround - hardware tx checksum does not work
@@ -2032,24 +2036,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet
*dev,
csum = false;
} else {
u32 csum_preamble = smsc95xx_calc_csum_preamble(skb);
- skb_push(skb, 4);
- cpu_to_le32s(&csum_preamble);
- memcpy(skb->data, &csum_preamble, 4);
+ ptr = skb_push(skb, 4);
+ put_unaligned_le32(csum_preamble, ptr);
+
+ tx_cmd_a += 4;
+ tx_cmd_b += 4;
+ tx_cmd_b |= TX_CMD_B_CSUM_ENABLE;
}
}
- skb_push(skb, 4);
- tx_cmd_b = (u32)(skb->len - 4);
- if (csum)
- tx_cmd_b |= TX_CMD_B_CSUM_ENABLE;
- cpu_to_le32s(&tx_cmd_b);
- memcpy(skb->data, &tx_cmd_b, 4);
-
- skb_push(skb, 4);
- tx_cmd_a = (u32)(skb->len - 8) | TX_CMD_A_FIRST_SEG_ |
- TX_CMD_A_LAST_SEG_;
- cpu_to_le32s(&tx_cmd_a);
- memcpy(skb->data, &tx_cmd_a, 4);
+ ptr = skb_push(skb, 8);
+ put_unaligned_le32(tx_cmd_a, ptr);
+ put_unaligned_le32(tx_cmd_b, ptr+4);
return skb;
}
--
2.19.1
[PATCH 1/4] usbnet: smsc95xx: fix rx packet alignment
The smsc95xx driver already takes into account the NET_IP_ALIGN parameter when setting up the receive packet data, which means we do not need to worry about aligning the packets in the usbnet driver. Adding the EVENT_NO_IP_ALIGN means that the IPv4 header is now passed to the ip_rcv() routine with the start on an aligned address. Tested on Raspberry Pi B3. Signed-off-by: Ben Dooks --- drivers/net/usb/smsc95xx.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c index 06b4d290784d..401ec9feb495 100644 --- a/drivers/net/usb/smsc95xx.c +++ b/drivers/net/usb/smsc95xx.c @@ -1292,6 +1292,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf) dev->net->features |= NETIF_F_RXCSUM; dev->net->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM; + set_bit(EVENT_NO_IP_ALIGN, &dev->flags); smsc95xx_init_mac_address(dev); -- 2.19.1
[PATCH 3/4] usbnet: smsc95xx: fix memcpy for accessing rx-data
Change the RX code to use get_unaligned_le32() instead of the combo of memcpy and cpu_to_le32s(&var). Signed-off-by: Ben Dooks --- drivers/net/usb/smsc95xx.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c index 50e97a159500..8f7c473f3260 100644 --- a/drivers/net/usb/smsc95xx.c +++ b/drivers/net/usb/smsc95xx.c @@ -618,9 +618,7 @@ static void smsc95xx_status(struct usbnet *dev, struct urb *urb) return; } - memcpy(&intdata, urb->transfer_buffer, 4); - le32_to_cpus(&intdata); - + intdata = get_unaligned_le32(urb->transfer_buffer); netif_dbg(dev, link, dev->net, "intdata: 0x%08X\n", intdata); if (intdata & INT_ENP_PHY_INT_) @@ -1922,8 +1920,7 @@ static int smsc95xx_rx_fixup(struct usbnet *dev, struct sk_buff *skb) unsigned char *packet; u16 size; - memcpy(&header, skb->data, sizeof(header)); - le32_to_cpus(&header); + header = get_unaligned_le32(skb->data); skb_pull(skb, 4 + NET_IP_ALIGN); packet = skb->data; -- 2.19.1
Re: SMSC95xx driver updates (round 1)
On Mi, 2018-11-14 at 11:50 +, Ben Dooks wrote: > This is a series of a few driver cleanups and some fixups of the code > for the SMSC95XX driver. There have been a few reviews, and the issues > have been fixed so this should be ready for merging. > > I will work on the tx-alignment and the other bits of usbnet changes > and produce at least two more patch series for this later. That looks good to me. Regards Oliver
Re: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable endpoints
Hi,
Anurag Kumar Vulisha writes:
> When bulk streams are enabled for an endpoint, there can
> be a condition where the gadget controller waits for the
> host to issue prime transaction and the host controller
> waits for the gadget to issue ERDY. This condition could
> create a deadlock. To avoid such potential deadlocks, a
> timer is started after queuing any request for the stream
> capable endpoint in usb_ep_queue(). The gadget driver is
> expected to stop the timer if a valid stream event is found.
> If no stream event is found, the timer expires after the
> STREAM_TIMEOUT_MS value and a callback function registered
> by gadget driver to endpoint ops->stream_timeout API would be
> called, so that the gadget driver can handle this situation.
> This kind of behaviour is observed in dwc3 controller and
> expected to be generic issue with other controllers supporting
> bulk streams also.
>
> Signed-off-by: Anurag Kumar Vulisha
> ---
> Changes in v6:
> 1. This patch is newly added in this series to add timer into udc/core.c
> ---
> drivers/usb/gadget/udc/core.c | 71
> ++-
> include/linux/usb/gadget.h| 12
> 2 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index af88b48..41cc23b 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -52,6 +52,24 @@ static int udc_bind_to_driver(struct usb_udc *udc,
> /* -
> */
>
> /**
> + * usb_ep_stream_timeout - callback function for endpoint stream timeout
> timer
> + * @arg: pointer to struct timer_list
> + *
> + * This function gets called only when bulk streams are enabled in the
> endpoint
> + * and only after ep->stream_timeout_timer has expired. The timer gets
> expired
> + * only when the gadget controller failed to find a valid stream event for
> this
> + * endpoint. On timer expiry, this function calls the endpoint-specific
> timeout
> + * handler registered to endpoint ops->stream_timeout API.
> + */
> +static void usb_ep_stream_timeout(struct timer_list *arg)
> +{
> + struct usb_ep *ep = from_timer(ep, arg, stream_timeout_timer);
> +
> + if (ep->stream_capable && ep->ops->stream_timeout)
why is the timer only for stream endpoints? What if we want to run this
on non-stream capable eps?
> + ep->ops->stream_timeout(ep);
you don't ned an extra operation here, ep_dequeue should be enough.
> @@ -102,6 +132,10 @@ int usb_ep_enable(struct usb_ep *ep)
> if (ret)
> goto out;
>
> + if (ep->stream_capable)
> + timer_setup(&ep->stream_timeout_timer,
> + usb_ep_stream_timeout, 0);
the timer should be per-request, not per-endpoint. Otherwise in case of
multiple requests being queued, you can't give them different timeouts
> @@ -269,6 +321,13 @@ int usb_ep_queue(struct usb_ep *ep,
>
> ret = ep->ops->queue(ep, req, gfp_flags);
>
> + if (ep->stream_capable) {
> + ep->stream_timeout_timer.expires = jiffies +
> + msecs_to_jiffies(STREAM_TIMEOUT_MS);
timeout value should be passed by the gadget driver. Add a new
usb_ep_queue_timeout() that takes the new argument. Rename the current
usb_ep_queue() to static int __usb_ep_queue() with an extra argument for
timeout and introduce usb_ep_queue() without the argument, calling
__usb_ep_queue() passing timeout as 0.
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index e5cd84a..2ebaef0 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -144,6 +144,7 @@ struct usb_ep_ops {
>
> int (*fifo_status) (struct usb_ep *ep);
> void (*fifo_flush) (struct usb_ep *ep);
> + void (*stream_timeout) (struct usb_ep *ep);
why?
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
On Wed, Nov 14, 2018 at 1:17 PM Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 1:05 PM Chanwoo Choi wrote: > > > Changing NULL to -ENODEV is a trading bad to worse. > P.S. I still disagree with your arguments in relation to de facto use of an > API. I spoke to colleagues of mine and they are agree that semantically -EPROBE_DEFER is a wrong error code from API that matches against some list. On the other hand, they agree with me that changing NULL to -ENODEV is a trading bad to worse. So, I withdraw mine complaints against API. -- With Best Regards, Andy Shevchenko
RE: scsi_set_medium_removal timeout issue
On Wed, 14 Nov 2018, Zengtao (B) wrote: > I just enabled the scsi log in the middle of the umount operation, otherwise > I can't > reproduce the issue when the scsi log is enabled. > > >from the beginning. In any case, it wasn't what I wanted. I asked you to > >post the dmesg log, not the SCSI log. > > Please refer to the new attachment for dmesg log. Heh, yes, I see now. Martin, shouldn't sd_release() call sd_sync_cache() in the same way that sd_shutdown() does, before it calls scsi_set_medium_removal()? Alan Stern
Re: [PATCH 4/4] usbnet: smsc95xx: check for csum being in last four bytes
On 11/14/2018 02:50 PM, Ben Dooks wrote: > The manual states that the checksum cannot lie in the last DWORD of the > transmission, so add a basic check for this and fall back to software > checksumming the packet. > > This only seems to trigger for ACK packets with no options or data to > return to the other end, and the use of the tx-alignment option makes > it more likely to happen. > > Signed-off-by: Ben Dooks > --- > Fixes for v2: > - Fix spelling of check at Sergei's suggestion > - Move skb->len check into smsc95xx_can_tx_checksum() > - Change name of smsc95xx_can_checksum to explicitly say it is tx-csum > --- > drivers/net/usb/smsc95xx.c | 19 ++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c > index 8f7c473f3260..cc78ef78cc93 100644 > --- a/drivers/net/usb/smsc95xx.c > +++ b/drivers/net/usb/smsc95xx.c > @@ -1997,6 +1997,23 @@ static u32 smsc95xx_calc_csum_preamble(struct sk_buff > *skb) > return (high_16 << 16) | low_16; > } > > +/* The TX CSUM won't work if the checksum lies in the last 4 bytes of the > + * transmission. This is fairly unlikely, only seems to trigger with some > + * short TCP ACK packets sent. > + * > + * Note, this calculation should probably check for the alignment of the > + * data as well, but a straight check for csum being in the last four bytes > + * of the packet should be ok for now. > +*/ Missed a space before */. [...] MBR, Sergei
Re: [PATCH v2 RESEND] usb: dwc3: core: Fix ULPI PHYs and prevent phy_get/ulpi_init during suspend/resume
Hello,
After I apply this patch on 4.14 (or receive it with 4.14.70) I see a
regression with
the Qualcomm QUSB2 phy driver. I'm testing on a Dragonboard 820c.
In boot log I get:
[4.525502] phy phy-7412000.phy.6: QUSB2PHY pll lock failed: status reg = 0
[4.529232] phy phy-7412000.phy.6: phy init failed --> -16
[4.536170] dwc3 760.dwc3: failed to initialize core
[4.541743] dwc3: probe of 760.dwc3 failed with error -16
[4.549979] phy phy-7411000.phy.5: QUSB2PHY pll lock failed: status reg = 0
[4.552843] phy phy-7411000.phy.5: phy init failed --> -16
[4.559606] dwc3 6a0.dwc3: failed to initialize core
[4.565181] dwc3: probe of 6a0.dwc3 failed with error -16
I can provide a full log if needed.
Best regards,
Todor
On 12.02.2018 15:30, Roger Quadros wrote:
> In order for ULPI PHYs to work, dwc3_phy_setup() and dwc3_ulpi_init()
> must be doene before dwc3_core_get_phy().
>
> commit 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before
> initializing phys")
> broke this.
>
> The other issue is that dwc3_core_get_phy() and dwc3_ulpi_init() should
> be called only once during the life cycle of the driver. However,
> as dwc3_core_init() is called during system suspend/resume it will
> result in multiple calls to dwc3_core_get_phy() and dwc3_ulpi_init()
> which is wrong.
>
> Fix this by moving dwc3_ulpi_init() out of dwc3_phy_setup()
> into dwc3_core_ulpi_init(). Use a flag 'ulpi_ready' to ensure that
> dwc3_core_ulpi_init() is called only once from dwc3_core_init().
>
> Use another flag 'phys_ready' to call dwc3_core_get_phy() only once from
> dwc3_core_init().
>
> Fixes: 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before
> initializing phys")
> Fixes: f54edb539c11 ("usb: dwc3: core: initialize ULPI before trying to get
> the PHY")
> Cc: linux-stable # >= v4.13
> Signed-off-by: Roger Quadros
> ---
> Rebased on Felipe's testing/fixes branch.
>
> drivers/usb/dwc3/core.c | 47 ---
> drivers/usb/dwc3/core.h | 5 +
> 2 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 59511f2..f1d838a 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -486,6 +486,22 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
> parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
> }
>
> +static int dwc3_core_ulpi_init(struct dwc3 *dwc)
> +{
> + int intf;
> + int ret = 0;
> +
> + intf = DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3);
> +
> + if (intf == DWC3_GHWPARAMS3_HSPHY_IFC_ULPI ||
> + (intf == DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI &&
> + dwc->hsphy_interface &&
> + !strncmp(dwc->hsphy_interface, "ulpi", 4)))
> + ret = dwc3_ulpi_init(dwc);
> +
> + return ret;
> +}
> +
> /**
> * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
> * @dwc: Pointer to our controller context structure
> @@ -497,7 +513,6 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
> static int dwc3_phy_setup(struct dwc3 *dwc)
> {
> u32 reg;
> - int ret;
>
> reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>
> @@ -568,9 +583,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
> }
> /* FALLTHROUGH */
> case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
> - ret = dwc3_ulpi_init(dwc);
> - if (ret)
> - return ret;
> /* FALLTHROUGH */
> default:
> break;
> @@ -727,6 +739,7 @@ static void dwc3_core_setup_global_control(struct dwc3
> *dwc)
> }
>
> static int dwc3_core_get_phy(struct dwc3 *dwc);
> +static int dwc3_core_ulpi_init(struct dwc3 *dwc);
>
> /**
> * dwc3_core_init - Low-level initialization of DWC3 Core
> @@ -758,17 +771,27 @@ static int dwc3_core_init(struct dwc3 *dwc)
> dwc->maximum_speed = USB_SPEED_HIGH;
> }
>
> - ret = dwc3_core_get_phy(dwc);
> + ret = dwc3_phy_setup(dwc);
> if (ret)
> goto err0;
>
> - ret = dwc3_core_soft_reset(dwc);
> - if (ret)
> - goto err0;
> + if (!dwc->ulpi_ready) {
> + ret = dwc3_core_ulpi_init(dwc);
> + if (ret)
> + goto err0;
> + dwc->ulpi_ready = true;
> + }
>
> - ret = dwc3_phy_setup(dwc);
> + if (!dwc->phys_ready) {
> + ret = dwc3_core_get_phy(dwc);
> + if (ret)
> + goto err0a;
> + dwc->phys_ready = true;
> + }
> +
> + ret = dwc3_core_soft_reset(dwc);
> if (ret)
> - goto err0;
> + goto err0a;
>
> dwc3_core_setup_global_control(dwc);
> dwc3_core_num_eps(dwc);
> @@ -841,6 +864,9 @@ static int dwc3_core_init(struct dwc3 *dwc)
> phy_exit(dwc->usb2_generic_phy);
> phy_exit(dwc->usb3_generic_phy);
>
> +err0a:
> + dwc3_ul
[PATCH] USB: host: ehci: allow tine of highspeed nak-count
From: Ben Dooks
At least some systems benefit with less scheduling if the NAK count
value is set higher than the default 4. For instance a Tegra3 with
an SMSC9512 showed less interrupt load when this was changed to 14.
To allow the changing of this value, add a sysfs node to each of
the controllers to allow run-time changing.
Signed-off-by: Ben Dooks
---
drivers/usb/host/ehci-hcd.c | 1 +
drivers/usb/host/ehci-q.c | 4 +--
drivers/usb/host/ehci-sysfs.c | 52 +--
drivers/usb/host/ehci.h | 1 +
4 files changed, 54 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 8608ac513fb7..799262951f41 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -526,6 +526,7 @@ static int ehci_init(struct usb_hcd *hcd)
hw->hw_qtd_next = EHCI_LIST_END(ehci);
ehci->async->qh_state = QH_STATE_LINKED;
hw->hw_alt_next = QTD_NEXT(ehci, ehci->async->dummy->qtd_dma);
+ ehci->nak_tune_hs = EHCI_TUNE_RL_HS;
/* clear interrupt enables, set irq latency */
if (log2_irq_thresh < 0 || log2_irq_thresh > 6)
diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index 327630405695..ccb754893b5a 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -898,12 +898,12 @@ qh_make (
case USB_SPEED_HIGH:/* no TT involved */
info1 |= QH_HIGH_SPEED;
if (type == PIPE_CONTROL) {
- info1 |= (EHCI_TUNE_RL_HS << 28);
+ info1 |= ehci->nak_tune_hs << 28;
info1 |= 64 << 16; /* usb2 fixed maxpacket */
info1 |= QH_TOGGLE_CTL; /* toggle from qtd */
info2 |= (EHCI_TUNE_MULT_HS << 30);
} else if (type == PIPE_BULK) {
- info1 |= (EHCI_TUNE_RL_HS << 28);
+ info1 |= ehci->nak_tune_hs << 28;
/* The USB spec says that high speed bulk endpoints
* always use 512 byte maxpacket. But some device
* vendors decided to ignore that, and MSFT is happy
diff --git a/drivers/usb/host/ehci-sysfs.c b/drivers/usb/host/ehci-sysfs.c
index 8f75cb7b197c..d710d35282a6 100644
--- a/drivers/usb/host/ehci-sysfs.c
+++ b/drivers/usb/host/ehci-sysfs.c
@@ -145,19 +145,66 @@ static ssize_t uframe_periodic_max_store(struct device
*dev,
}
static DEVICE_ATTR_RW(uframe_periodic_max);
+static ssize_t nak_tune_hs_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ehci_hcd *ehci;
+
+ ehci = hcd_to_ehci(dev_get_drvdata(dev));
+ return scnprintf(buf, PAGE_SIZE, "%d\n", ehci->nak_tune_hs);
+}
+
+static ssize_t nak_tune_hs_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ehci_hcd *ehci;
+ unsignedval;
+ unsigned long flags;
+
+ ehci = hcd_to_ehci(dev_get_drvdata(dev));
+
+ if (kstrtouint(buf, 0, &val) < 0)
+ return -EINVAL;
+
+ if (val >= 15) {
+ ehci_info(ehci, "invalid value for nak_tune_hs (%d)\n", val);
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave (&ehci->lock, flags);
+ ehci->nak_tune_hs = val;
+ spin_unlock_irqrestore (&ehci->lock, flags);
+ return count;
+}
+
+static DEVICE_ATTR_RW(nak_tune_hs);
static inline int create_sysfs_files(struct ehci_hcd *ehci)
{
struct device *controller = ehci_to_hcd(ehci)->self.controller;
int i = 0;
+ i = device_create_file(controller, &dev_attr_nak_tune_hs);
+ if (i)
+ goto out;
+
+ i = device_create_file(controller, &dev_attr_uframe_periodic_max);
+ if (i)
+ goto out_nak;
+
/* with integrated TT there is no companion! */
if (!ehci_is_TDI(ehci))
i = device_create_file(controller, &dev_attr_companion);
if (i)
- goto out;
+ goto out_all;
- i = device_create_file(controller, &dev_attr_uframe_periodic_max);
+ return 0;
+out_all:
+ device_remove_file(controller, &dev_attr_uframe_periodic_max);
+out_nak:
+ device_remove_file(controller, &dev_attr_nak_tune_hs);
out:
return i;
}
@@ -170,5 +217,6 @@ static inline void remove_sysfs_files(struct ehci_hcd *ehci)
if (!ehci_is_TDI(ehci))
device_remove_file(controller, &dev_attr_companion);
+ device_remove_file(controller, &dev_attr_nak_tune_hs);
device_remove_file(controller, &dev_attr_uframe_periodic_max);
}
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index c8e9a48e1d51..1fb6f1ad8128 100644
---
Re: WARNING in usb_submit_urb (4)
On Tue, Nov 13, 2018 at 9:37 PM, Alan Stern wrote: > On Mon, 12 Nov 2018, syzbot wrote: > >> syzbot has found a reproducer for the following crash on: >> >> HEAD commit:e12e00e388de Merge tag 'kbuild-fixes-v4.20' of git://git.k.. >> git tree: upstream >> console output: https://syzkaller.appspot.com/x/log.txt?x=100e4ef540 >> kernel config: https://syzkaller.appspot.com/x/.config?x=8f215f21f041a0d7 >> dashboard link: https://syzkaller.appspot.com/bug?extid=7634edaea4d0b341c625 >> compiler: gcc (GCC) 8.0.1 20180413 (experimental) >> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11ce6fbd40 >> >> IMPORTANT: if you fix the bug, please add the following tag to the commit: >> Reported-by: syzbot+7634edaea4d0b341c...@syzkaller.appspotmail.com > > I tried reproducing this bug on my own system, following the > instructions at > > https://github.com/google/syzkaller/blob/master/docs/executing_syzkaller_programs.md > > The reproducer failed to run properly. It produced the following > output: > > > $ ./syz-execprog -cover=0 -threaded=1 -repeat=1 -procs=4 /tmp/repro.syz > 2018/11/13 15:29:32 parsed 1 programs > 2018/11/13 15:29:32 executed programs: 0 > 2018/11/13 15:29:32 result: failed=false hanged=false err=executor 3: failed: > tun: ioctl(TUNSETIFF) failed (errno 1) > loop failed (errno 0) > > > tun: ioctl(TUNSETIFF) failed (errno 1) > loop failed (errno 0) > > > The system is Fedora 28 running the 4.18.16-200.fc28.x86_64 kernel. > What should I do to investigate further? Hi Alan, Looking at "errno 1", it seems that syz-execprog doesn't have enough privileges to execute this ioctl, so you might need to run it as root. However the absence of a C reproducer points to the fact that this is some kind of a race condition. Those are quite sensitive to timing, and any difference in the used setup might affect their reproducibility. I would recommend building the exact kernel revision with the provided config. For me it took around 3 minutes to syz-execprog before I saw the WARNING. Thanks!
Re: [PATCH] HID: add driver for U2F Zero built-in LED and RNG
On 12/11/2018 03:17, Jiri Kosina wrote:
> On Thu, 1 Nov 2018, Andrej Shadura wrote:
>
>> Hi everyone,
>>
>> I’ve got a comment from Nick Kossifidis that I probably shouldn’t set
>> RNG’s quality to 1024. Adding linux-crypto@ to the loop.
>
> So, what was this about? Is there any resolution to it? :)
So far, not really. I talked to Keith Packard regarding a similar
setting in his ChaosKey driver, and I understand his opinion is that it
is appropriate there since he’s convinced about the quality of the
hardware he designed. I’m not sure what exactly I should set it to here.
>>
>> On 23/10/2018 16:46, Andrej Shadura wrote:
>>> U2F Zero supports custom commands for blinking the LED and getting data
>>> from the internal hardware RNG. Expose the blinking function as a LED
>>> device, and the internal hardware RNG as an HWRNG so that it can be used
>>> to feed the enthropy pool.
>>>
>>> Signed-off-by: Andrej Shadura
>>> ---
>>> drivers/hid/Kconfig | 15 ++
>>> drivers/hid/Makefile | 1 +
>>> drivers/hid/hid-ids.h | 1 +
>>> drivers/hid/hid-u2fzero.c | 371 ++
>>> 4 files changed, 388 insertions(+)
>>> create mode 100644 drivers/hid/hid-u2fzero.c
>>>
>>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>>> index 61e1953ff921..3de6bf202f20 100644
>>> --- a/drivers/hid/Kconfig
>>> +++ b/drivers/hid/Kconfig
>>> @@ -975,6 +975,21 @@ config HID_UDRAW_PS3
>>> Say Y here if you want to use the THQ uDraw gaming tablet for
>>> the PS3.
>>>
>>> +config HID_U2FZERO
>>> + tristate "U2F Zero LED and RNG support"
>>> + depends on HID
>>> + depends on LEDS_CLASS
>>> + help
>>> + Support for the LED of the U2F Zero device.
>>> +
>>> + U2F Zero supports custom commands for blinking the LED
>>> + and getting data from the internal hardware RNG.
>>> + The internal hardware can be used to feed the enthropy pool.
>>> +
>>> + U2F Zero only supports blinking its LED, so this driver doesn't
>>> + allow setting the brightness to anything but 1, which will
>>> + trigger a single blink and immediately reset to back 0.
>>> +
>>> config HID_WACOM
>>> tristate "Wacom Intuos/Graphire tablet support (USB)"
>>> depends on USB_HID
>>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>>> index bd7ac53b75c5..617b1a928b6a 100644
>>> --- a/drivers/hid/Makefile
>>> +++ b/drivers/hid/Makefile
>>> @@ -107,6 +107,7 @@ obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
>>> obj-$(CONFIG_HID_TIVO) += hid-tivo.o
>>> obj-$(CONFIG_HID_TOPSEED) += hid-topseed.o
>>> obj-$(CONFIG_HID_TWINHAN) += hid-twinhan.o
>>> +obj-$(CONFIG_HID_U2FZERO) += hid-u2fzero.o
>>> obj-$(CONFIG_HID_UCLOGIC) += hid-uclogic.o
>>> obj-$(CONFIG_HID_UDRAW_PS3)+= hid-udraw-ps3.o
>>> obj-$(CONFIG_HID_LED) += hid-led.o
>>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>> index bc49909aba8e..070a2fcd8a18 100644
>>> --- a/drivers/hid/hid-ids.h
>>> +++ b/drivers/hid/hid-ids.h
>>> @@ -309,6 +309,7 @@
>>> #define USB_DEVICE_ID_CYGNAL_RADIO_SI470X 0x818a
>>> #define USB_DEVICE_ID_FOCALTECH_FT_MULTITOUCH 0x81b9
>>> #define USB_DEVICE_ID_CYGNAL_CP21120xea90
>>> +#define USB_DEVICE_ID_U2F_ZERO 0x8acf
>>>
>>> #define USB_DEVICE_ID_CYGNAL_RADIO_SI4713 0x8244
>>>
>>> diff --git a/drivers/hid/hid-u2fzero.c b/drivers/hid/hid-u2fzero.c
>>> new file mode 100644
>>> index ..bd4077c93c97
>>> --- /dev/null
>>> +++ b/drivers/hid/hid-u2fzero.c
>>> @@ -0,0 +1,371 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * U2F Zero LED and RNG driver
>>> + *
>>> + * Copyright 2018 Andrej Shadura
>>> + * Loosely based on drivers/hid/hid-led.c
>>> + * and drivers/usb/misc/chaoskey.c
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation, version 2.
>>> + */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include "usbhid/usbhid.h"
>>> +#include "hid-ids.h"
>>> +
>>> +#define DRIVER_SHORT "u2fzero"
>>> +
>>> +#define HID_REPORT_SIZE64
>>> +
>>> +/* We only use broadcast (CID-less) messages */
>>> +#define CID_BROADCAST 0x
>>> +
>>> +struct u2f_hid_msg {
>>> + u32 cid;
>>> + union {
>>> + struct {
>>> + u8 cmd;
>>> + u8 bcnth;
>>> + u8 bcntl;
>>> + u8 data[HID_REPORT_SIZE - 7];
>>> + } init;
>>> + struct {
>>> + u8 seq;
>>> + u8 data[HID_REPORT_SIZE - 5];
>>> + } cont;
>>> + };
>>> +} __packed;
>>> +
>>> +struct u2f_hid_report {
>>> + u8 report_type;
>>> + struct u2f_hid_msg msg;
>>> +} __packed;
>>> +
>>> +#define U2F_HID_MSG_LEN(f) (size_t)(((f).init.bc
Re: [PATCH] USB: host: ehci: allow tine of highspeed nak-count
On Wed, 14 Nov 2018, Ben Dooks wrote:
> From: Ben Dooks
>
> At least some systems benefit with less scheduling if the NAK count
> value is set higher than the default 4. For instance a Tegra3 with
> an SMSC9512 showed less interrupt load when this was changed to 14.
Interesting. Why do you think this is? In theory, increasing the NAK
count RL value should cause a higher memory bus load and perhaps a
slight rise in the interrupt load (transfers will complete a little
more quickly because the controller tries harder to poll the endpoints
and see if they are ready).
> To allow the changing of this value, add a sysfs node to each of
> the controllers to allow run-time changing.
>
> Signed-off-by: Ben Dooks
The patch looks okay to me.
Acked-by: Alan Stern
Alan Stern
> ---
> drivers/usb/host/ehci-hcd.c | 1 +
> drivers/usb/host/ehci-q.c | 4 +--
> drivers/usb/host/ehci-sysfs.c | 52 +--
> drivers/usb/host/ehci.h | 1 +
> 4 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 8608ac513fb7..799262951f41 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -526,6 +526,7 @@ static int ehci_init(struct usb_hcd *hcd)
> hw->hw_qtd_next = EHCI_LIST_END(ehci);
> ehci->async->qh_state = QH_STATE_LINKED;
> hw->hw_alt_next = QTD_NEXT(ehci, ehci->async->dummy->qtd_dma);
> + ehci->nak_tune_hs = EHCI_TUNE_RL_HS;
>
> /* clear interrupt enables, set irq latency */
> if (log2_irq_thresh < 0 || log2_irq_thresh > 6)
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 327630405695..ccb754893b5a 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -898,12 +898,12 @@ qh_make (
> case USB_SPEED_HIGH:/* no TT involved */
> info1 |= QH_HIGH_SPEED;
> if (type == PIPE_CONTROL) {
> - info1 |= (EHCI_TUNE_RL_HS << 28);
> + info1 |= ehci->nak_tune_hs << 28;
> info1 |= 64 << 16; /* usb2 fixed maxpacket */
> info1 |= QH_TOGGLE_CTL; /* toggle from qtd */
> info2 |= (EHCI_TUNE_MULT_HS << 30);
> } else if (type == PIPE_BULK) {
> - info1 |= (EHCI_TUNE_RL_HS << 28);
> + info1 |= ehci->nak_tune_hs << 28;
> /* The USB spec says that high speed bulk endpoints
>* always use 512 byte maxpacket. But some device
>* vendors decided to ignore that, and MSFT is happy
> diff --git a/drivers/usb/host/ehci-sysfs.c b/drivers/usb/host/ehci-sysfs.c
> index 8f75cb7b197c..d710d35282a6 100644
> --- a/drivers/usb/host/ehci-sysfs.c
> +++ b/drivers/usb/host/ehci-sysfs.c
> @@ -145,19 +145,66 @@ static ssize_t uframe_periodic_max_store(struct device
> *dev,
> }
> static DEVICE_ATTR_RW(uframe_periodic_max);
>
> +static ssize_t nak_tune_hs_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ehci_hcd *ehci;
> +
> + ehci = hcd_to_ehci(dev_get_drvdata(dev));
> + return scnprintf(buf, PAGE_SIZE, "%d\n", ehci->nak_tune_hs);
> +}
> +
> +static ssize_t nak_tune_hs_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ehci_hcd *ehci;
> + unsignedval;
> + unsigned long flags;
> +
> + ehci = hcd_to_ehci(dev_get_drvdata(dev));
> +
> + if (kstrtouint(buf, 0, &val) < 0)
> + return -EINVAL;
> +
> + if (val >= 15) {
> + ehci_info(ehci, "invalid value for nak_tune_hs (%d)\n", val);
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave (&ehci->lock, flags);
> + ehci->nak_tune_hs = val;
> + spin_unlock_irqrestore (&ehci->lock, flags);
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(nak_tune_hs);
>
> static inline int create_sysfs_files(struct ehci_hcd *ehci)
> {
> struct device *controller = ehci_to_hcd(ehci)->self.controller;
> int i = 0;
>
> + i = device_create_file(controller, &dev_attr_nak_tune_hs);
> + if (i)
> + goto out;
> +
> + i = device_create_file(controller, &dev_attr_uframe_periodic_max);
> + if (i)
> + goto out_nak;
> +
> /* with integrated TT there is no companion! */
> if (!ehci_is_TDI(ehci))
> i = device_create_file(controller, &dev_attr_companion);
> if (i)
> - goto out;
> + goto out_all;
>
> - i = device_create_file(controller, &dev_attr_uframe_periodic_max);
> + return 0;
> +out_all:
> + device_remove_file(controller, &dev_attr_uframe_periodic_max);
>
Re: [PATCH] HID: add driver for U2F Zero built-in LED and RNG
On 14/11/2018 10:32, Andrej Shadura wrote: > On 12/11/2018 03:17, Jiri Kosina wrote: >> On Thu, 1 Nov 2018, Andrej Shadura wrote: >> >>> Hi everyone, >>> >>> I’ve got a comment from Nick Kossifidis that I probably shouldn’t set >>> RNG’s quality to 1024. Adding linux-crypto@ to the loop. >> >> So, what was this about? Is there any resolution to it? :) > > So far, not really. I talked to Keith Packard regarding a similar > setting in his ChaosKey driver, and I understand his opinion is that it > is appropriate there since he’s convinced about the quality of the > hardware he designed. I’m not sure what exactly I should set it to here. Just talked to Theodore Ts'o about this, it seems that it doesn’t really matter that much what to set it to, since this subsystem apparently will be reworked soon, and setting it to a fair value of 0 will apparently make it not feed the entropy pool at all, and with a non-zero value only one device with the highest value will be used. I’m tempted to resubmit the patch with 0 as the default (as Nick suggested) so that pro users can toggle it later from the userspace, but it doesn’t have the opportunity to potentially poison the entropy pool if it’s compromised. Conor (cc'ed), out of curiosity, could you please post some info on how the hardware RNG is implemented in U2F Zero? >>> On 23/10/2018 16:46, Andrej Shadura wrote: U2F Zero supports custom commands for blinking the LED and getting data from the internal hardware RNG. Expose the blinking function as a LED device, and the internal hardware RNG as an HWRNG so that it can be used to feed the enthropy pool. Signed-off-by: Andrej Shadura --- drivers/hid/Kconfig | 15 ++ drivers/hid/Makefile | 1 + drivers/hid/hid-ids.h | 1 + drivers/hid/hid-u2fzero.c | 371 ++ 4 files changed, 388 insertions(+) create mode 100644 drivers/hid/hid-u2fzero. -- Cheers, Andrej
Re: [PATCH v2 net-next 06/21] net: usb: aqc111: Introduce link management
> > Thats again because of this product has tightly integrated MAC+Phy. > > MAC FW controls system interface and reports/alters link state > > as a joint state on copper and SIF (even in dpa direct phy mode). > > > > We can't extract phy api into a standalone fully functional phylib > > therefore. > > Also as far as I know this particular phy is not available in the wild. > > So the point is that MAC firmware is managing SERDES and system interface > link. Linux can manage that SERDES link between the MAC and the PHY. There are two ways this can go: 1) You use phylib. When the PHY reports link, the adjust_link callback in the MAC is called. The phydev structure contains information about how you should configure the SERDES, SGMII, 2500Base-X, 5000Base-X. It works, but it is not so nice. 2) phylink gives you a much nicer API to do the same. Again, the PHY reports the link is up. phylink will then tell the MAC how to configure its end of the SERDES. The problem with phylink is that it expects a DT building. You don't have that, since this is a USB device. But you also don't need a lot of the features of phylink like SFPs, the i2c bus for the SFPs, GPIOs etc. So it should not be to hard to make this work without device tree. By using core linux code, we avoid bugs in firmware which nobody can fix. The Linux core code should be well tested and supported, but phylink is rather new, so might still have some corner cases. I also cannot imaging parts of the PHY driver will not be re-usable for other Aquantia PHYs. I have a board with an AQCS109 under my desk waiting for me to do something with it. I really would like a better PHY driver for it than the kernel currently has. Hopefully there is some code reuse possibilities here. Andrew
Re: [PATCH v2 net-next 19/21] net: usb: aqc111: Add support for wake on LAN by MAGIC packet
> We've considered that, but then thought about the following case: > > After such a sleep state where partner's capabilities were considered, > user may move with the unit and replug it into different link partner with > other, incompatible speed mask. That will anyway lead to wol link failure. WOL for a nomadic device? Is that even a real use case? Anyway, looking at the link partner capabilities is just really a corner case, that the LP only supports 1G. Do such devices exist? So skipping this is fine. But if you use phylib, you pretty much get it for free if you want it. Andrew
[PATCH AUTOSEL 4.18 52/59] net: smsc95xx: Fix MTU range
From: Stefan Wahren
[ Upstream commit 85b18b0237ce9986a81a1b9534b5e2ee116f5504 ]
The commit f77f0aee4da4 ("net: use core MTU range checking in USB NIC
drivers") introduce a common MTU handling for usbnet. But it's missing
the necessary changes for smsc95xx. So set the MTU range accordingly.
This patch has been tested on a Raspberry Pi 3.
Fixes: f77f0aee4da4 ("net: use core MTU range checking in USB NIC drivers")
Signed-off-by: Stefan Wahren
Signed-off-by: David S. Miller
Signed-off-by: Sasha Levin
---
drivers/net/usb/smsc95xx.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 2d17f3b9bb16..f2d01cb6f958 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1321,6 +1321,8 @@ static int smsc95xx_bind(struct usbnet *dev, struct
usb_interface *intf)
dev->net->ethtool_ops = &smsc95xx_ethtool_ops;
dev->net->flags |= IFF_MULTICAST;
dev->net->hard_header_len += SMSC95XX_TX_OVERHEAD_CSUM;
+ dev->net->min_mtu = ETH_MIN_MTU;
+ dev->net->max_mtu = ETH_DATA_LEN;
dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
pdata->dev = dev;
--
2.17.1
[PATCH AUTOSEL 4.9 04/13] usbnet: smsc95xx: disable carrier check while suspending
From: Frieder Schrempf
[ Upstream commit 7b900ead6cc66b2ee873cb042dfba169aa68b56c ]
We need to make sure, that the carrier check polling is disabled
while suspending. Otherwise we can end up with usbnet_read_cmd()
being issued when only usbnet_read_cmd_nopm() is allowed. If this
happens, read operations lock up.
Fixes: d69d169493 ("usbnet: smsc95xx: fix link detection for disabled
autonegotiation")
Signed-off-by: Frieder Schrempf
Reviewed-by: Raghuram Chary J
Signed-off-by: David S. Miller
Signed-off-by: Sasha Levin
---
drivers/net/usb/smsc95xx.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index a167116c..e29f4c0767eb 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1590,6 +1590,8 @@ static int smsc95xx_suspend(struct usb_interface *intf,
pm_message_t message)
return ret;
}
+ cancel_delayed_work_sync(&pdata->carrier_check);
+
if (pdata->suspend_flags) {
netdev_warn(dev->net, "error during last resume\n");
pdata->suspend_flags = 0;
@@ -1832,6 +1834,11 @@ static int smsc95xx_suspend(struct usb_interface *intf,
pm_message_t message)
*/
if (ret && PMSG_IS_AUTO(message))
usbnet_resume(intf);
+
+ if (ret)
+ schedule_delayed_work(&pdata->carrier_check,
+ CARRIER_CHECK_DELAY);
+
return ret;
}
--
2.17.1
[PATCH AUTOSEL 4.14 26/27] net: smsc95xx: Fix MTU range
From: Stefan Wahren
[ Upstream commit 85b18b0237ce9986a81a1b9534b5e2ee116f5504 ]
The commit f77f0aee4da4 ("net: use core MTU range checking in USB NIC
drivers") introduce a common MTU handling for usbnet. But it's missing
the necessary changes for smsc95xx. So set the MTU range accordingly.
This patch has been tested on a Raspberry Pi 3.
Fixes: f77f0aee4da4 ("net: use core MTU range checking in USB NIC drivers")
Signed-off-by: Stefan Wahren
Signed-off-by: David S. Miller
Signed-off-by: Sasha Levin
---
drivers/net/usb/smsc95xx.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 9b8afe4da73b..2f65975a121f 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1321,6 +1321,8 @@ static int smsc95xx_bind(struct usbnet *dev, struct
usb_interface *intf)
dev->net->ethtool_ops = &smsc95xx_ethtool_ops;
dev->net->flags |= IFF_MULTICAST;
dev->net->hard_header_len += SMSC95XX_TX_OVERHEAD_CSUM;
+ dev->net->min_mtu = ETH_MIN_MTU;
+ dev->net->max_mtu = ETH_DATA_LEN;
dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
pdata->dev = dev;
--
2.17.1
[PATCH AUTOSEL 4.14 05/27] usbnet: smsc95xx: disable carrier check while suspending
From: Frieder Schrempf
[ Upstream commit 7b900ead6cc66b2ee873cb042dfba169aa68b56c ]
We need to make sure, that the carrier check polling is disabled
while suspending. Otherwise we can end up with usbnet_read_cmd()
being issued when only usbnet_read_cmd_nopm() is allowed. If this
happens, read operations lock up.
Fixes: d69d169493 ("usbnet: smsc95xx: fix link detection for disabled
autonegotiation")
Signed-off-by: Frieder Schrempf
Reviewed-by: Raghuram Chary J
Signed-off-by: David S. Miller
Signed-off-by: Sasha Levin
---
drivers/net/usb/smsc95xx.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 99e684e39d35..9b8afe4da73b 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1598,6 +1598,8 @@ static int smsc95xx_suspend(struct usb_interface *intf,
pm_message_t message)
return ret;
}
+ cancel_delayed_work_sync(&pdata->carrier_check);
+
if (pdata->suspend_flags) {
netdev_warn(dev->net, "error during last resume\n");
pdata->suspend_flags = 0;
@@ -1840,6 +1842,11 @@ static int smsc95xx_suspend(struct usb_interface *intf,
pm_message_t message)
*/
if (ret && PMSG_IS_AUTO(message))
usbnet_resume(intf);
+
+ if (ret)
+ schedule_delayed_work(&pdata->carrier_check,
+ CARRIER_CHECK_DELAY);
+
return ret;
}
--
2.17.1
[PATCH AUTOSEL 4.18 11/59] usbnet: smsc95xx: disable carrier check while suspending
From: Frieder Schrempf
[ Upstream commit 7b900ead6cc66b2ee873cb042dfba169aa68b56c ]
We need to make sure, that the carrier check polling is disabled
while suspending. Otherwise we can end up with usbnet_read_cmd()
being issued when only usbnet_read_cmd_nopm() is allowed. If this
happens, read operations lock up.
Fixes: d69d169493 ("usbnet: smsc95xx: fix link detection for disabled
autonegotiation")
Signed-off-by: Frieder Schrempf
Reviewed-by: Raghuram Chary J
Signed-off-by: David S. Miller
Signed-off-by: Sasha Levin
---
drivers/net/usb/smsc95xx.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 262e7a3c23cb..2d17f3b9bb16 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1598,6 +1598,8 @@ static int smsc95xx_suspend(struct usb_interface *intf,
pm_message_t message)
return ret;
}
+ cancel_delayed_work_sync(&pdata->carrier_check);
+
if (pdata->suspend_flags) {
netdev_warn(dev->net, "error during last resume\n");
pdata->suspend_flags = 0;
@@ -1840,6 +1842,11 @@ static int smsc95xx_suspend(struct usb_interface *intf,
pm_message_t message)
*/
if (ret && PMSG_IS_AUTO(message))
usbnet_resume(intf);
+
+ if (ret)
+ schedule_delayed_work(&pdata->carrier_check,
+ CARRIER_CHECK_DELAY);
+
return ret;
}
--
2.17.1
Re: [PATCH v2 net-next 06/21] net: usb: aqc111: Introduce link management
On 11/14/18 1:16 PM, Andrew Lunn wrote:
>>> Thats again because of this product has tightly integrated MAC+Phy.
>>> MAC FW controls system interface and reports/alters link state
>>> as a joint state on copper and SIF (even in dpa direct phy mode).
>>>
>>> We can't extract phy api into a standalone fully functional phylib
>>> therefore.
>>> Also as far as I know this particular phy is not available in the wild.
>>
>> So the point is that MAC firmware is managing SERDES and system interface
>> link.
>
> Linux can manage that SERDES link between the MAC and the PHY. There
> are two ways this can go:
>
> 1) You use phylib. When the PHY reports link, the adjust_link callback
> in the MAC is called. The phydev structure contains information about
> how you should configure the SERDES, SGMII, 2500Base-X, 5000Base-X. It
> works, but it is not so nice.
>
> 2) phylink gives you a much nicer API to do the same. Again, the PHY
> reports the link is up. phylink will then tell the MAC how to
> configure its end of the SERDES. The problem with phylink is that it
> expects a DT building. You don't have that, since this is a USB
> device. But you also don't need a lot of the features of phylink like
> SFPs, the i2c bus for the SFPs, GPIOs etc. So it should not be to hard
> to make this work without device tree.
>
> By using core linux code, we avoid bugs in firmware which nobody can
> fix. The Linux core code should be well tested and supported, but
> phylink is rather new, so might still have some corner cases.
>
> I also cannot imaging parts of the PHY driver will not be re-usable
> for other Aquantia PHYs. I have a board with an AQCS109 under my desk
> waiting for me to do something with it. I really would like a better
> PHY driver for it than the kernel currently has. Hopefully there is
> some code reuse possibilities here.
Even if the firmware is helping, there is still value in using PHYLINK
to report things to Linux as Andrew is saying in that you get a lot of
things for free: auto-negotiation results, link_ksetttings_{get,set} etc.
--
Florian
Re: [PATCH v1 2/5] extcon: Return -EPROBE_DEFER when extcon device is not found
On 2018년 11월 14일 23:04, Andy Shevchenko wrote: > On Wed, Nov 14, 2018 at 1:17 PM Andy Shevchenko > wrote: >> On Wed, Nov 14, 2018 at 1:05 PM Chanwoo Choi wrote: > Changing NULL to -ENODEV is a trading bad to worse. > >> P.S. I still disagree with your arguments in relation to de facto use of an >> API. > > I spoke to colleagues of mine and they are agree that semantically > -EPROBE_DEFER is a wrong error code from API that matches against some > list. > On the other hand, they agree with me that changing NULL to -ENODEV is > a trading bad to worse. > > So, I withdraw mine complaints against API. > OK. Thanks. -- Best Regards, Chanwoo Choi Samsung Electronics
[PATCH V1 1/1] USB: serial: f81534: fix reading old/new IC config
The F81532/534 had a internal configuration space to save & control
IC state with address F81534_CUSTOM_ADDRESS_START (0x2f00). Layout
as following:
+00h: to indicate the section is valid
+01h~04h: UART Mode & port availability
+05h~08h: Output pin control on IC power on
+09h~12h: Output pin control on working <-- New added
Old driver will use +05~08h as default on working, but newer IC will
configed with shutdown mode(7) in 05h~08h and working mode with RS232(1)
in 09h~12h. It'll make mainstream driver not working.
This patch will make mainstream driver compatible older and newer IC.
If using a old IC, the +05h~08h will be 00h~06h, we'll direct apply it.
If using a new IC, the +05h~08h will be 07h or larger, we'll read +09h~12h
to apply newer configuration.
Signed-off-by: Ji-Ze Hong (Peter Hong)
---
drivers/usb/serial/f81534.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
index 380933db34dd..2b39bda035c7 100644
--- a/drivers/usb/serial/f81534.c
+++ b/drivers/usb/serial/f81534.c
@@ -45,14 +45,17 @@
#define F81534_CONFIG1_REG (0x09 + F81534_UART_BASE_ADDRESS)
#define F81534_DEF_CONF_ADDRESS_START 0x3000
-#define F81534_DEF_CONF_SIZE 8
+#define F81534_DEF_CONF_SIZE 12
#define F81534_CUSTOM_ADDRESS_START0x2f00
#define F81534_CUSTOM_DATA_SIZE0x10
#define F81534_CUSTOM_NO_CUSTOM_DATA 0xff
#define F81534_CUSTOM_VALID_TOKEN 0xf0
#define F81534_CONF_OFFSET 1
-#define F81534_CONF_GPIO_OFFSET4
+#define F81534_CONF_INIT_GPIO_OFFSET 4
+#define F81534_CONF_WORK_GPIO_OFFSET 8
+#define F81534_CONF_GPIO_SHUTDOWN 7
+#define F81534_CONF_GPIO_RS232 1
#define F81534_MAX_DATA_BLOCK 64
#define F81534_MAX_BUS_RETRY 20
@@ -1337,8 +1340,19 @@ static int f81534_set_port_output_pin(struct
usb_serial_port *port)
serial_priv = usb_get_serial_data(serial);
port_priv = usb_get_serial_port_data(port);
- idx = F81534_CONF_GPIO_OFFSET + port_priv->phy_num;
+ idx = F81534_CONF_INIT_GPIO_OFFSET + port_priv->phy_num;
value = serial_priv->conf_data[idx];
+ if (value >= F81534_CONF_GPIO_SHUTDOWN) {
+ /*
+* Newer IC configure will make transceiver in shutdown mode on
+* initial power on. We need enable it before using UARTs.
+*/
+ idx = F81534_CONF_WORK_GPIO_OFFSET + port_priv->phy_num;
+ value = serial_priv->conf_data[idx];
+ if (value >= F81534_CONF_GPIO_SHUTDOWN)
+ value = F81534_CONF_GPIO_RS232;
+ }
+
pins = &f81534_port_out_pins[port_priv->phy_num];
for (i = 0; i < ARRAY_SIZE(pins->pin); ++i) {
--
2.7.4
