MA-USB Host driver provides USB connectivity over an available
network, allowing host device to access remote USB devices attached
to one or more MA USB devices (accessible via network).
This driver has been developed to enable the host to communicate
with DislayLink products supporting MA USB protocol (MA USB device,
in terms of MA USB Specification).
MA-USB protocol used by MA-USB Host driver has been implemented in
accordance with MA USB Specification Release 1.0b.
This driver depends on the functions provided by DisplayLink's
user-space driver.
Signed-off-by: Vladimir Stankovic <[email protected]>
---
drivers/staging/Kconfig | 3 +
drivers/staging/Makefile | 1 +
drivers/staging/mausb_host/Kconfig | 15 +
drivers/staging/mausb_host/Makefile | 30 +
.../mausb_host/include/common/ma_usb.h | 851 ++++++++
.../mausb_host/include/common/mausb_address.h | 38 +
.../include/common/mausb_driver_status.h | 20 +
.../mausb_host/include/common/mausb_event.h | 228 +++
drivers/staging/mausb_host/include/hcd/hub.h | 120 ++
drivers/staging/mausb_host/include/hcd/vhcd.h | 81 +
.../mausb_host/include/hpal/data_common.h | 67 +
.../staging/mausb_host/include/hpal/data_in.h | 18 +
.../mausb_host/include/hpal/data_out.h | 22 +
.../staging/mausb_host/include/hpal/hpal.h | 174 ++
.../mausb_host/include/hpal/isoch_in.h | 22 +
.../mausb_host/include/hpal/isoch_out.h | 21 +
.../mausb_host/include/hpal/mausb_events.h | 97 +
.../include/hpal/network_callbacks.h | 20 +
.../mausb_host/include/link/mausb_ip_link.h | 89 +
.../include/utils/mausb_data_iterator.h | 55 +
.../mausb_host/include/utils/mausb_logs.h | 35 +
.../mausb_host/include/utils/mausb_mmap.h | 22 +
.../include/utils/mausb_ring_buffer.h | 53 +
.../mausb_host/include/utils/mausb_version.h | 17 +
drivers/staging/mausb_host/src/hcd/hub.c | 1801 +++++++++++++++++
.../staging/mausb_host/src/hcd/module_init.c | 239 +++
drivers/staging/mausb_host/src/hcd/vhcd.c | 216 ++
.../staging/mausb_host/src/hpal/data_common.c | 139 ++
drivers/staging/mausb_host/src/hpal/data_in.c | 82 +
.../staging/mausb_host/src/hpal/data_out.c | 207 ++
drivers/staging/mausb_host/src/hpal/hpal.c | 1327 ++++++++++++
.../staging/mausb_host/src/hpal/isoch_in.c | 243 +++
.../staging/mausb_host/src/hpal/isoch_out.c | 362 ++++
.../mausb_host/src/hpal/mausb_events.c | 661 ++++++
.../mausb_host/src/hpal/network_callbacks.c | 162 ++
.../mausb_host/src/link/mausb_ip_link.c | 364 ++++
.../src/utils/mausb_data_iterator.c | 300 +++
.../staging/mausb_host/src/utils/mausb_mmap.c | 358 ++++
.../mausb_host/src/utils/mausb_ring_buffer.c | 140 ++
39 files changed, 8700 insertions(+)
create mode 100644 drivers/staging/mausb_host/Kconfig
create mode 100644 drivers/staging/mausb_host/Makefile
create mode 100644 drivers/staging/mausb_host/include/common/ma_usb.h
create mode 100644 drivers/staging/mausb_host/include/common/mausb_address.h
create mode 100644 drivers/staging/mausb_host/include/common/mausb_driver_status.h
create mode 100644 drivers/staging/mausb_host/include/common/mausb_event.h
create mode 100644 drivers/staging/mausb_host/include/hcd/hub.h
create mode 100644 drivers/staging/mausb_host/include/hcd/vhcd.h
create mode 100644 drivers/staging/mausb_host/include/hpal/data_common.h
create mode 100644 drivers/staging/mausb_host/include/hpal/data_in.h
create mode 100644 drivers/staging/mausb_host/include/hpal/data_out.h
create mode 100644 drivers/staging/mausb_host/include/hpal/hpal.h
create mode 100644 drivers/staging/mausb_host/include/hpal/isoch_in.h
create mode 100644 drivers/staging/mausb_host/include/hpal/isoch_out.h
create mode 100644 drivers/staging/mausb_host/include/hpal/mausb_events.h
create mode 100644 drivers/staging/mausb_host/include/hpal/network_callbacks.h
create mode 100644 drivers/staging/mausb_host/include/link/mausb_ip_link.h
create mode 100644 drivers/staging/mausb_host/include/utils/mausb_data_iterator.h
create mode 100644 drivers/staging/mausb_host/include/utils/mausb_logs.h
create mode 100644 drivers/staging/mausb_host/include/utils/mausb_mmap.h
create mode 100644 drivers/staging/mausb_host/include/utils/mausb_ring_buffer.h
create mode 100644 drivers/staging/mausb_host/include/utils/mausb_version.h
create mode 100644 drivers/staging/mausb_host/src/hcd/hub.c
create mode 100644 drivers/staging/mausb_host/src/hcd/module_init.c
create mode 100644 drivers/staging/mausb_host/src/hcd/vhcd.c
create mode 100644 drivers/staging/mausb_host/src/hpal/data_common.c
create mode 100644 drivers/staging/mausb_host/src/hpal/data_in.c
create mode 100644 drivers/staging/mausb_host/src/hpal/data_out.c
create mode 100644 drivers/staging/mausb_host/src/hpal/hpal.c
create mode 100644 drivers/staging/mausb_host/src/hpal/isoch_in.c
create mode 100644 drivers/staging/mausb_host/src/hpal/isoch_out.c
create mode 100644 drivers/staging/mausb_host/src/hpal/mausb_events.c
create mode 100644 drivers/staging/mausb_host/src/hpal/network_callbacks.c
create mode 100644 drivers/staging/mausb_host/src/link/mausb_ip_link.c
create mode 100644 drivers/staging/mausb_host/src/utils/mausb_data_iterator.c
create mode 100644 drivers/staging/mausb_host/src/utils/mausb_mmap.c
create mode 100644 drivers/staging/mausb_host/src/utils/mausb_ring_buffer.c
This code is not terrible. It would have helped a lot if you had
run it through checkpatch --strict.
This driver initializes most local variables to zero which disables
GCC's uninitialized error variable checking and generally makes the code
harder to understand. Try to remove this as much as you can.
On Mon, Jan 20, 2020 at 09:30:43AM +0000, Vladimir Stankovic wrote:
> +++ b/drivers/staging/mausb_host/Kconfig
> @@ -0,0 +1,15 @@
> +#
> +# Copyright (c) 2019 - 2020 DisplayLink (UK) Ltd.
> +#
> +# This file is subject to the terms and conditions of the GNU General Public
> +# License v2. See the file COPYING in the main directory of this archive for
> +# more details.
> +#
> +
> +config HOST_MAUSB
> + bool "MA-USB Host Driver"
> + depends on USB=y
Why can't HOST_MAUSB be built as a module?
> + default y
It should default to disabled.
> +static int mausb_insert_usb_device(struct mausb_dev *mdevs,
> + struct mausb_usb_device_ctx *usb_device)
> +{
> + struct rb_node **new_node = &(mdevs->usb_devices.rb_node),
> + *parent = NULL;
This is another this which the code has all over. Split these up into
two lines.
struct rb_node **new_node = &mdevs->usb_devices.rb_node;
struct rb_node *parent = NULL;
Search for all the lines that end in a comma and fix them.
> +static int mausb_hcd_start(struct usb_hcd *hcd)
> +{
> + mausb_pr_info("");
There is too much debug output. Here we can use ftrace to get this
information. A lot of the warning messages are not clear. One just
says "Fragmentation" without telling the user what to do. I guess
search for quotes and think about rephrasing or deleting stuff.
> +
> + hcd->power_budget = 0;
> + hcd->uses_new_polling = 1;
> + return 0;
> +}
[ snip ]
> +static int mausb_drop_endpoint(struct usb_hcd *hcd, struct usb_device *dev,
> + struct usb_host_endpoint *endpoint)
> +{
> + int8_t port_number = 0;
> + int status = 0,
> + retries = 0;
> + struct hub_ctx *hub = (struct hub_ctx *)hcd->hcd_priv;
> + struct mausb_device *ma_dev;
> + struct mausb_usb_device_ctx *usb_device_ctx;
> + struct mausb_endpoint_ctx *endpoint_ctx = endpoint->hcpriv;
> + unsigned long flags;
> +
> + port_number = get_root_hub_port_number(dev);
> +
> + if (port_number < 0 || port_number >= NUMBER_OF_PORTS) {
> + mausb_pr_info("port_number out of range, port_number=%x",
> + port_number);
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave(&mhcd->lock, flags);
> + ma_dev = hub->ma_devs[port_number].ma_dev;
> + spin_unlock_irqrestore(&mhcd->lock, flags);
> +
> + if (!ma_dev) {
> + mausb_pr_err("MAUSB device not found on port_number=%d",
> + port_number);
> + return -ENODEV;
> + }
> +
> + usb_device_ctx =
> + mausb_find_usb_device(&hub->ma_devs[port_number], dev);
> +
> + if (!endpoint_ctx) {
> + mausb_pr_err("Endpoint context doesn't exist");
> + return 0;
> + }
> + if (!usb_device_ctx) {
> + mausb_pr_err("Usb device context doesn't exist");
> + return 0;
> + }
> +
> + mausb_pr_info("Start dropping ep_handle=%#x, dev_handle=%#x",
> + endpoint_ctx->ep_handle, endpoint_ctx->dev_handle);
> +
> + if (atomic_read(&ma_dev->unresponsive_client)) {
> + mausb_pr_err("Client is not responsive anymore - drop endpoint immediately");
> + endpoint->hcpriv = NULL;
> + kfree(endpoint_ctx);
> + return status;
This is an example where disabling GCC's uninitialized variable checking
hurts the code. This should be "return 0;" or "return -ESOMETHING;".
> + }
> +
> + status = mausb_epinactivate_event_to_user(ma_dev,
> + usb_device_ctx->dev_handle,
> + endpoint_ctx->ep_handle);
> +
> + mausb_pr_info("epinactivate request ep_handle=%#x, dev_handle=%#x, status=%d",
> + endpoint_ctx->ep_handle, endpoint_ctx->dev_handle,
> + status);
> +
> + while (true) {
> + status = mausb_epdelete_event_to_user(ma_dev,
> + usb_device_ctx->dev_handle,
> + endpoint_ctx->ep_handle);
> +
> + mausb_pr_info("ep_handle_delete_request, ep_handle=%#x, dev_handle=%#x, status=%d",
> + endpoint_ctx->ep_handle, endpoint_ctx->dev_handle,
> + status);
> + /* sleep for 10 ms to give device some time */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Delete this an just put 10ms in the usleep_range() parameters. Ideally
code should document itself.
> + if (status == -EBUSY) {
> + if (++retries < MAUSB_BUSY_RETRIES_COUNT)
> + usleep_range(MAUSB_BUSY_TIMEOUT_MIN,
> + MAUSB_BUSY_TIMEOUT_MAX);
Delete the MAUSB_BUSY_TIMEOUT_MIN defines.
> + else
> + return status;
return -EBUSY. There are a bunch of places which "return status;"
that should be updated to "return 0;" etc.
> + } else {
> + break;
> + }
> + }
> +
> + mausb_pr_info("Endpoint dropped ep_handle=%#x, dev_handle=%#x",
> + endpoint_ctx->ep_handle, endpoint_ctx->dev_handle);
> +
> + endpoint->hcpriv = NULL;
> + kfree(endpoint_ctx);
> + return status;
> +}
> +
[ snip ]
> + if (unlikely(dev_speed == -EINVAL)) {
> + mausb_pr_err("bad dev_speed");
> + return -EINVAL;
> + }
Remove all the likely()/unlikely(). Those only belong in core kernel,
not in drivers. Search and delete.
if (dev_speed < 0)
return dev_speed;
[ snip ]
> + if (!usb_device_ctx ||
> + usb_device_ctx->dev_handle == DEV_HANDLE_NOT_ASSIGNED)
> + return 0;
Align conditions like this:
if (!usb_device_ctx ||
usb_device_ctx->dev_handle == DEV_HANDLE_NOT_ASSIGNED)
return 0;
> +#ifdef ISOCH_CALLBACKS
> +int mausb_sec_event_ring_setup(struct usb_hcd *hcd, unsigned int intr_num)
> +{
> + mausb_pr_debug("");
> + return 0;
> +}
If possible, let's delete all the dummy functions.
> + if (mausb_isoch_data_event(event))
> + if (event->data.direction == MAUSB_DATA_MSG_DIRECTION_IN)
> + status = mausb_receive_isoch_in_data(dev, event,
> + urb_ctx);
> + else
> + status = mausb_receive_isoch_out(dev, event, urb_ctx);
> + else
> + if (event->data.direction == MAUSB_DATA_MSG_DIRECTION_IN)
> + status = mausb_receive_in_data(dev, event, urb_ctx);
> + else
> + status = mausb_receive_out_data(dev, event, urb_ctx);
Multi-line indent blocks get curly braces for readability.
> +static int mausb_init_header_data_chunk(struct ma_usb_hdr_common *common_hdr,
> + struct list_head *chunks_list,
> + uint32_t *num_of_data_chunks)
> +{
> + int status = mausb_add_data_chunk(common_hdr,
> + MAUSB_TRANSFER_HDR_SIZE,
> + chunks_list);
> + /* Success */
> + if (!status)
> + ++(*num_of_data_chunks);
> +
> + return status;
> +}
When you need comments to explain the success path it means the code
is messy. The success path should be at indent level one tab and the
failure path should be indented two tabs. Always do "failure handling",
not "success handling" like this.
int status;
status = mausb_add_data_chunk(common_hdr, MAUSB_TRANSFER_HDR_SIZE,
chunks_list);
if (status)
return status;
++(*num_of_data_chunks);
return 0;
> +static int mausb_init_control_data_chunk(struct mausb_event *event,
> + struct list_head *chunks_list,
> + uint32_t *num_of_data_chunks)
> +{
> + int status = 0;
> +
> + if (!event->data.first_control_packet)
> + goto l_return;
Just do a direct return. This do-nothing goto is pointless. What does
the l in l_return even mean? It's confusing to the readers because is
this really supposed to be a success path?
if (!event->data.first_control_packet)
return 0;
A "return 0;" is clearly intentional and instantly clear.
> +
> + status = mausb_add_data_chunk(
> + ((struct urb *)event->data.urb)->setup_packet,
> + MAUSB_CONTROL_SETUP_SIZE, chunks_list);
> + /* Success */
> + if (!status)
> + ++(*num_of_data_chunks);
> +
> +l_return:
> + return status;
> +}
Anyway, run it through checkpatch and resend, then I look at it
properly.
regards,
dan carpenter
On Mon, Jan 20, 2020 at 09:30:43AM +0000, Vladimir Stankovic wrote:
> +int mausb_enqueue_event_from_user(uint8_t madev_addr, uint32_t all_events)
> +{
> + unsigned long flags;
> + uint16_t num_of_completed,
> + num_of_events;
> + struct mausb_device *dev;
> +
> + spin_lock_irqsave(&mss.lock, flags);
> + dev = mausb_get_dev_from_addr_unsafe(madev_addr);
> +
> + if (!dev) {
> + spin_unlock_irqrestore(&mss.lock, flags);
> + return 0;
> + }
> +
> + spin_lock_irqsave(&dev->num_of_user_events_lock, flags);
> + num_of_completed = (uint16_t)all_events +
> + (uint16_t)dev->num_of_user_events;
> + num_of_events = (all_events >> (8 * sizeof(num_of_events))) +
> + (dev->num_of_user_events >> (8 * sizeof(num_of_events)));
> + dev->num_of_user_events = num_of_completed;
> + dev->num_of_user_events |= (uint32_t)num_of_events <<
> + (8 * sizeof(num_of_events));
I might be missing something. Why can't we just declare two struct
members instead of doing these bit shifts to fit two values into
dev->num_of_user_events?
> + spin_unlock_irqrestore(&dev->num_of_user_events_lock, flags);
> + queue_work(dev->workq, &dev->work);
> + spin_unlock_irqrestore(&mss.lock, flags);
> +
> + return 0;
> +}
regards,
dan carpenter
On Mon, Jan 20, 2020 at 09:30:43AM +0000, Vladimir Stankovic wrote:
> MA-USB Host driver provides USB connectivity over an available
> network, allowing host device to access remote USB devices attached
> to one or more MA USB devices (accessible via network).
>
> This driver has been developed to enable the host to communicate
> with DislayLink products supporting MA USB protocol (MA USB device,
> in terms of MA USB Specification).
>
> MA-USB protocol used by MA-USB Host driver has been implemented in
> accordance with MA USB Specification Release 1.0b.
>
> This driver depends on the functions provided by DisplayLink's
> user-space driver.
>
> Signed-off-by: Vladimir Stankovic <[email protected]>
Why is this being submitted to staging and not to the "real" part of the
kernel? You need a TODO file that lists what is left to be done to the
driver to get it merged to the proper place in the kernel tree. Can you
please resubmit with that file added to the patch?
thanks,
greg k-h
Hi Greg,
Our intention was to follow Linux kernel development process and add our
driver to staging first.
Will resubmit patch with TODO added.
Regards,
Vladimir
On 22.1.20. 08:03, [email protected] wrote:
> On Mon, Jan 20, 2020 at 09:30:43AM +0000, Vladimir Stankovic wrote:
>> MA-USB Host driver provides USB connectivity over an available
>> network, allowing host device to access remote USB devices attached
>> to one or more MA USB devices (accessible via network).
>>
>> This driver has been developed to enable the host to communicate
>> with DislayLink products supporting MA USB protocol (MA USB device,
>> in terms of MA USB Specification).
>>
>> MA-USB protocol used by MA-USB Host driver has been implemented in
>> accordance with MA USB Specification Release 1.0b.
>>
>> This driver depends on the functions provided by DisplayLink's
>> user-space driver.
>>
>> Signed-off-by: Vladimir Stankovic <[email protected]>
>
> Why is this being submitted to staging and not to the "real" part of the
> kernel? You need a TODO file that lists what is left to be done to the
> driver to get it merged to the proper place in the kernel tree. Can you
> please resubmit with that file added to the patch?
>
> thanks,
>
> greg k-h
>
On Wed, Jan 22, 2020 at 07:40:59AM +0000, Vladimir Stankovic wrote:
> Hi Greg,
>
> Our intention was to follow Linux kernel development process and add our
> driver to staging first.
That's not the "normal" development process at all, where did you read
that?
staging is only for code that needs lots of work, and almost always
merging a driver through staging takes _more_ work from the submitter
than it does to submit it through the "normal" subsystem.
So if you want to do more work, hey, by all means, send it here :)
thanks,
greg k-h
Hi Greg,
It was section 2.5 of the kernel development process, "staging trees".
In particular, statement "where many sub-directories for drivers or
filesystems that are on their way to being added to the kernel tree
live" caught our attention.
Now, by reading it once again, I see that the rest of the section is in
line with your comment.
We'll address all comments received so far, and resubmit patch onto
appropriate repository. With that being said, is USB subsystem tree
(drivers/usb within usb.git repo) correct one? Please, advise.
Thanks.
Regards,
Vladimir.
On 22.1.20. 08:48, [email protected] wrote:
> On Wed, Jan 22, 2020 at 07:40:59AM +0000, Vladimir Stankovic wrote:
>> Hi Greg,
>>
>> Our intention was to follow Linux kernel development process and add our
>> driver to staging first.
>
> That's not the "normal" development process at all, where did you read
> that?
>
> staging is only for code that needs lots of work, and almost always
> merging a driver through staging takes _more_ work from the submitter
> than it does to submit it through the "normal" subsystem.
>
> So if you want to do more work, hey, by all means, send it here :)
>
> thanks,
>
> greg k-h
>
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Wed, Jan 22, 2020 at 08:16:09AM +0000, Vladimir Stankovic wrote:
> Hi Greg,
>
> It was section 2.5 of the kernel development process, "staging trees".
> In particular, statement "where many sub-directories for drivers or
> filesystems that are on their way to being added to the kernel tree
> live" caught our attention.
>
> Now, by reading it once again, I see that the rest of the section is in
> line with your comment.
>
> We'll address all comments received so far, and resubmit patch onto
> appropriate repository. With that being said, is USB subsystem tree
> (drivers/usb within usb.git repo) correct one? Please, advise.
If this is a USB host driver, then yes, drivers/usb/host/ would be the
correct location for this.
Note, at first glance, there is a bunch of work to do on this to get it
into "real" mergable shape. Be sure to at the very least get it
'checkpatch.pl clean' before submitting it, that will help out a lot, if
it is not already done.
thanks,
greg k-h
On 22.1.20. 09:49, [email protected] wrote:
> A: http://en.wikipedia.org/wiki/Top_post
> <http://en.wikipedia.org/wiki/Top_post>
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
> A: No.
> Q: Should I include quotations after my reply?
>
> http://daringfireball.net/2007/07/on_top
> <http://daringfireball.net/2007/07/on_top>
>
> On Wed, Jan 22, 2020 at 08:16:09AM +0000, Vladimir Stankovic wrote:
> > Hi Greg,
> >
> > It was section 2.5 of the kernel development process, "staging trees".
> > In particular, statement "where many sub-directories for drivers or
> > filesystems that are on their way to being added to the kernel tree
> > live" caught our attention.
> >
> > Now, by reading it once again, I see that the rest of the section is in
> > line with your comment.
> >
> > We'll address all comments received so far, and resubmit patch onto
> > appropriate repository. With that being said, is USB subsystem tree
> > (drivers/usb within usb.git repo) correct one? Please, advise.
>
> If this is a USB host driver, then yes, drivers/usb/host/ would be the
> correct location for this.
>
> Note, at first glance, there is a bunch of work to do on this to get it
> into "real" mergable shape. Be sure to at the very least get it
> 'checkpatch.pl clean' before submitting it, that will help out a lot, if
> it is not already done.
>
> thanks,
>
> greg k-h
Failed to set my Thunderbird properly, sorry.
Thanks for your feedback. We'll proceed as advised.
Regards,
Vladimir.