Re: [V2 PATCH 01/10] added media agnostic (MA) USB HCD driver
On Wed, Nov 12, 2014 at 05:03:18PM -0500, Alan Stern wrote: On Wed, 12 Nov 2014, Sean O. Stalley wrote: Our plan to support multiple MA devices is to have them all connected to the same virtual host controller, so only 1 would be needed. Would you prefer we have 1 host controller instance per MA device? We are definitely open to suggestions on how this should be architected. I haven't read the MA USB spec, so I don't know how it's intended to work. Still, what happens if you create a virtual host controller with, say, 16 ports, and then someone wants to connect a 17th MA device? To summarize the spec: MA USB groups a host connected devices into MA service sets (MSS). The architectural limit is 254 MA devices per MSS. If the host needs to connect more devices than that, It can start a new MSS and connect to 254 more MA devices. Is supporting up to 254 devices on one machine sufficient? Would it make sense (and does the usb stack support) having 254 root ports on one host controller? If so, we could make our host controller instance have 254 ports. I'm guessing the hub driver may have a problem with this (especially for superspeed). If that doesn't make sense (or isn't supported), we can have 1 host controller instance per MA device. Would that be preferred? Also, I noticed that your patch adds a new bus type for these MA host controllers. It really seems like overkill to have a whole new bus type if there's only going to be one device on it. The bus was added when we were quickly trying to replace the platform device code. It's probably not the right thing to do. I'm still not sure why we can't make our hcd a platform device, especially since dummy_hcd the usbip's hcd are both platform devices. If we get rid of these locks, endpoints can't run simultaneously. MA USB IN endpoints have to copy data, which could take a while. I don't know what you mean by run simultaneously. Certainly multiple network packets can be transmitted and received concurrently even if you use a single spinlock, since your locking won't affect the networking subsystem. I meant we couldn't have 2 threads in our driver. With one lock, One thread would always have to wait for the other, even though they could be working on 2 different endpoints doing completely independent tasks. Couldn't this cause a bottleneck? Probably not enough to matter. After all, the other host controller drivers rely on a single spinlock. And if it did matter, you could drop the spinlock while copying the data. Good point. We can cut our driver down to using 1 lock. If we find that only having 1 spinlock does cause a bottleneck, we can deal with it then. Thanks, Sean ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [V2 PATCH 02/10] added media agnostic (MA) USB HCD roothubs
Thank you for your review. My responses are inline.
Greg has requested that we clean up the driver internally before
we resubmit another patchset to the mailing list. I will make
sure the changes you requested make it in, but it may be a while
before you see a patchset with the fixes included.
Thanks,
Sean O. Stalley
On Wed, Nov 12, 2014 at 09:35:42AM +0100, Oliver Neukum wrote:
On Mon, 2014-11-10 at 18:09 -0800, Stephanie Wallick wrote:
diff --git a/drivers/staging/mausb/drivers/mausb_hub.c
b/drivers/staging/mausb/drivers/mausb_hub.c
new file mode 100644
index 000..63c0fe4
--- /dev/null
+++ b/drivers/staging/mausb/drivers/mausb_hub.c
+/**
+ * Returns true if the given is the superspeed HCD. Note: The primary HCD
is
+ * High Speed and the shared HCD is SuperSpeed.
+ */
Why in that order?
We should probably switch this make the superspeed hub primary.
That way we match the xhci driver.
+bool mausb_is_ss_hcd(struct usb_hcd *hcd)
+{
+ if (usb_hcd_is_primary_hcd(hcd))
+ return false;
+ else
+ return true;
+}
+
+/**
+ * Called by usb core when polling for a port status change.
+ *
+ * @hcd: USB HCD being polled.
+ * @buf: Holds port status changes (if any).
+ *
+ * Returns zero if there is no status change, otherwise returns number of
+ * bytes in buf. When there is a status change on a port, the bit indexed
+ * at the port number + 1 (e.g. bit 2 for port 1) is set in the buffer.
+ */
+int mausb_hub_status_data(struct usb_hcd *hcd, char *buf)
+{
+ int i;
+ u16 port_change = 0;
+ u32 status = 0;
+ int ret = 1;
+ struct mausb_hcd *mhcd = usb_hcd_to_mausb_hcd(hcd);
+ struct mausb_root_hub*roothub = usb_hcd_to_roothub(hcd);
+
+ /*
+* Buf should never be more that 2 bytes. USB 3.0 hubs cannot have
+* more than 15 downstream ports.
+*/
+ buf[0] = 0;
+ if (MAUSB_ROOTHUB_NUM_PORTS 7) {
+ buf[1] = 0;
+ ret++;
+ }
Endianness bug.
Could you elaborate?
It was my understanding that this buffer was host-endian.
Is this an unacceptable way to clear the buffer?
+
+ for (i = 0; i MAUSB_ROOTHUB_NUM_PORTS; i++) {
+ port_change = roothub-port_status[i].wPortChange;
+ if (port_change)
+ status |= (1 (i + 1));
+ }
+
+ mausb_dbg(mhcd, %s: hub status is 0x%x\n, __func__, status);
+
+ /* hcd might be suspended, resume if there is a status change */
+ if (mhcd-disabled == 0) {
+ if ((hcd-state == HC_STATE_SUSPENDED) status)
+ usb_hcd_resume_root_hub(hcd);
+ }
+
+ memcpy(buf, (char *)status, ret);
+
+ return status ? ret : 0;
+}
+
+/**
+ * Sets the bitfields in the hub descriptor of the 2.0 root hub. Always
+ * returns zero.
+ */
+int mausb_set_hub_descriptor(struct usb_hub_descriptor *hub_des)
+{
+ /* set the values to the default */
+ hub_des-bDescLength = sizeof(struct usb_hub_descriptor);
+ hub_des-bDescriptorType = USB_DT_HUB;
+ hub_des-bNbrPorts= MAUSB_ROOTHUB_NUM_PORTS;
+ hub_des-wHubCharacteristics = MAUSB_ROOTHUB_CHAR;
+ hub_des-bPwrOn2PwrGood = MAUSB_ROOTHUB_PWR_ON_2_PWR_GOOD;
+ hub_des-bHubContrCurrent = MAUSB_ROOTHUB_CONTR_CURRENT;
Is that descriptor in bus or host endianness?
All of the fields are little-endian. We should be using cpu_to_le16()
when setting wHubCharacteristics.
+
+ return 0;
+}
+
+/**
+ * Sets the bitfields in the hub descriptor of the 3.0 root hub. Always
+ * returns zero.
Then why return anything?
Good point. I will change this (and mausb_set_hub_descriptor()) to be void.
+ */
+int mausb_set_ss_hub_descriptor(struct usb_hub_descriptor *hub_des)
+{
+ /* set the values to the default */
+ hub_des-bDescLength = sizeof(struct usb_hub_descriptor);
+ hub_des-bDescriptorType = USB_DT_SS_HUB;
+ hub_des-bNbrPorts= MAUSB_ROOTHUB_NUM_PORTS;
+ hub_des-wHubCharacteristics = MAUSB_ROOTHUB_CHAR;
+ hub_des-bPwrOn2PwrGood = MAUSB_ROOTHUB_PWR_ON_2_PWR_GOOD;
+ hub_des-bHubContrCurrent = MAUSB_ROOTHUB_CONTR_CURRENT;
+
+ /* USB3-specific parameters */
+ hub_des-u.ss.bHubHdrDecLat = MAUSB_ROOTHUB_HDR_DEC_LAT;
+ hub_des-u.ss.wHubDelay = MAUSB_ROOTHUB_DELAY;
+ hub_des-u.ss.DeviceRemovable = MAUSB_ALL_DEV_REMOVABLE;
+
+ return 0;
+}
+/**
+ * Contains all the structures required to emulate a root hub. One instance
+ * exists per root hub.
+ */
+struct __attribute__((__packed__)) mausb_root_hub {
Why __packed__ ?
Doesn't need to be. I will remove.
+
+ /* hub parameters */
+ struct usb_hub_descriptor descriptor;
+ struct usb_hub_status status;
+
+ /* port
Re: [PATCH 05/10] added media specific (MS) TCP drivers
Thank You for reviewing our code.
I believe most of the problems you pointed out in mausb_ioctl.c
were addressed in [V2 PATCH 5/10]. I am working on adding the proper
error checking to the TCP drivers.
Greg has requested that we clean up our code internally before
submitting another patchset to the mailing list. I will make sure
we fix the problems you pointed out, but it may be a while before
you see another patchset.
Thanks,
Sean
On Tue, Nov 04, 2014 at 09:48:33AM +0100, Tobias Klauser wrote:
On 2014-11-03 at 21:42:52 +0100, Stephanie Wallick
stephanie.s.wall...@intel.com wrote:
This is where we handle media specific packets and transport. The MS driver
interfaces with a media agnostic (MA) driver via a series of transfer pairs.
Transfer pairs consist of a set of functions to pass MA USB packets back
and forth between MA and MS drivers. There is one transfer pair per device
endpoint and one transfer pair for control/management traffic. When the MA
driver needs to send an MA USB packet, it hands the packet off to the MS
layer where the packet is converted into an MS form and sent via TCP over
the underlying ethernet or wireless medium. When the MS driver receives a
packet, it converts it into an MA USB packet and hands it off the the MA
driver for handling.
In addition, the MS driver provides an interface to inititate connection
events.
Because there are no physical MA USB ports in an MA USB host, the host must
be
notified via software when a device is connected.
Lastly, the MS driver contains a number of ioctl functions that are used by
a
utility to adjust medium-related driver parameters and connect or
disconnect the
MA USB host and device drivers.
Signed-off-by: Sean O. Stalley sean.stal...@intel.com
Signed-off-by: Stephanie Wallick stephanie.s.wall...@intel.com
---
drivers/staging/mausb/drivers/mausb_ioctl.c | 373 +++
drivers/staging/mausb/drivers/mausb_ioctl.h | 99 +
drivers/staging/mausb/drivers/mausb_msapi.c | 110 ++
drivers/staging/mausb/drivers/mausb_msapi.h | 232
drivers/staging/mausb/drivers/mausb_tcp-device.c | 147
drivers/staging/mausb/drivers/mausb_tcp-host.c | 144
drivers/staging/mausb/drivers/mausb_tcp.c| 446
+++
drivers/staging/mausb/drivers/mausb_tcp.h| 129 +++
8 files changed, 1680 insertions(+)
create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.c
create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.h
create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.c
create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.h
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-device.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-host.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.c
create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.h
diff --git a/drivers/staging/mausb/drivers/mausb_ioctl.c
b/drivers/staging/mausb/drivers/mausb_ioctl.c
new file mode 100644
index 000..0c6c6bd
--- /dev/null
+++ b/drivers/staging/mausb/drivers/mausb_ioctl.c
[...]
+/**
+ * This function is used to send a message to the user, in other words, the
+ * calling process. It basically copies the message one byte at a time.
+ *
+ * @msg: The message to be sent to the user.
+ * @buffer:The buffer in which to put the message. This buffer was
given to
+ * us to fill.
+ */
+void to_user(char *msg, long unsigned int buffer)
+{
+ int length = (int)strlen(msg);
+ int bytes = 0;
+
+ while (length *msg) {
+ put_user(*(msg++), (char *)buffer++);
+ length--;
+ bytes++;
+ }
Any reason not to use copy_to_user here? That way, access_ok would only
need to be executed once for the whole range.
In any case, the return value of put_user/copy_to_user will need to be
checked.
+
+ put_user('\0', (char *)buffer + bytes);
+}
[...]
+/**
+ * This function is used to read from the device file. From the
perspective of
+ * the device, the user is reading information from us. This is one of the
+ * entry points to this module.
+ *
+ * @file: The device file. We don't use it directly, but it's passed in.
+ * @buffer:The buffer to put the message into.
+ * @length:The max length to be read.
+ * @offset:File offset, which we don't use but it is passed in
nontheless.
+ */
+static ssize_t mausb_read(struct file *file, char __user *buffer,
+ size_t length, loff_t *offset)
+{
+ int bytes_read = 0;
+
+ if (*message_point == 0)
+ return 0;
+ while (length *message_point) {
+ put_user(*(message_point++), buffer++);
+ length--;
+ bytes_read++;
+ }
See comment for to_user above. Why
Re: [V2 PATCH 01/10] added media agnostic (MA) USB HCD driver
Thanks for reviewing. My responses are inline.
Greg has asked that we clean up this code internally before we
send out another patchset to the mailing list. I will address
the issues you pointed out, but it may be a while before you see
another patchset.
Thanks Again,
Sean
On Tue, Nov 11, 2014 at 10:54:30AM -0500, Alan Stern wrote:
On Mon, 10 Nov 2014, Stephanie Wallick wrote:
+static struct mausb_hcd mhcd;
Only one statically-allocated structure? What if somebody wants to
have more than one of these things in their system?
Our plan to support multiple MA devices is to have them all connected
to the same virtual host controller, so only 1 would be needed.
Would you prefer we have 1 host controller instance per MA device?
We are definitely open to suggestions on how this should be architected.
+/**
+ * @maurb: Media agnostic structure with URB to release.
+ * @status:Status for URB that is getting released.
+ *
+ * Removes an URB from the queue, deletes the media agnostic information in
+ * the urb, and gives the URB back to the HCD. Caller must be holding the
+ * driver's spinlock.
+ */
+void mausb_unlink_giveback_urb(struct mausb_urb *maurb, int status)
+{
+ struct urb *urb;
+ struct usb_hcd *hcd;
+ struct api_context *ctx = NULL;
+ unsigned long irq_flags;
+
+ hcd = mausb_hcd_to_usb_hcd(mhcd);
+
+ spin_lock_irqsave(mhcd.giveback_lock, irq_flags);
Why do you need multiple spinlocks? Isn't one lock sufficient?
We will simplify the locking scheme before resubmitting.
I think it might be worthwhile to have a per-endpoint lock, see below.
+ if (!maurb) {
+ mausb_err(mhcd, %s: no maurb\n, __func__);
+ spin_unlock_irqrestore(mhcd.giveback_lock, irq_flags);
+ return;
+ } else {
+ urb = maurb-urb;
+ ctx = urb-context;
+ }
+
+ if (!urb) {
+ mausb_err(mhcd, %s: no urb\n, __func__);
+ mausb_internal_drop_maurb(maurb, mhcd);
+ spin_unlock_irqrestore(mhcd.giveback_lock, irq_flags);
+ return;
+ }
+
+ mausb_dbg(mhcd, %s: returning urb with status %i\n, __func__,
status);
+
+ usb_hcd_unlink_urb_from_ep(hcd, urb);
+ usb_hcd_giveback_urb(hcd, urb, status);
You must not call this function while holding any spinlocks. What happens
if the URB's completion routine tries to resubmit?
This works with our multi-lock scheme, but I will fix when we move to 1 lock.
+
+ /* remove the mausb-specific data */
+ mausb_internal_drop_maurb(maurb, mhcd);
+
+ spin_unlock_irqrestore(mhcd.giveback_lock, irq_flags);
+}
+
+/**
+ * Adds an URB to the endpoint queue then calls the URB handler. URB is
wrapped
+ * in media agnostic structure before being enqueued.
+ */
+static int mausb_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
+ gfp_t memflags)
+{
+ int ret = 0;
+ struct mausb_urb*maurb;
+ struct mausb_host_ep*ep;
+ unsigned long irq_flags;
+
+ if (!hcd || !urb) {
+ pr_err(%s: no %s\n, __func__, (hcd ? urb : USB hcd));
+ }
This can never happen. The USB core guarantees it; you don't need
to check.
I will remove this check (along with any other unnecessary checks for things
guaranteed from usbcore).
+ ep = usb_to_ma_endpoint(urb-ep);
+
+ if (!ep) {
+ mausb_err(mhcd, %s: no endpoint\n, __func__);
+ return -EINVAL;
+ }
+
+ if (urb-status != -EINPROGRESS) {
+ mausb_err(mhcd, %s: urb already unlinked, status is %i\n,
+ __func__, urb-status);
+ return urb-status;
+ }
You also don't need to check this.
Will remove.
+ /* If the endpoint isn't activated, we can't enqueue anything. */
+ if (MAUSB_EP_HANDLE_UNASSIGNED == ep-ep_handle_state) {
+ mausb_err(mhcd, %s: endpoint handle unassigned\n, __func__);
+ return -EPIPE;
+ }
+
+ if (USB_SPEED_FULL != urb-dev-speed) /* suppress checks */
+ ep-max_pkt = usb_endpoint_maxp(urb-ep-desc);
What happens to full-speed devices? Don't they have maxpacket values?
+
+ /* initialize the maurb */
+ maurb = mausb_alloc_maurb(ep, memflags);
+ if (!maurb) {
+ mausb_err(mhcd, could not allocate memory for MA USB urb\n);
+ return -ENOMEM;
+ }
+
+ /* set maurb member values */
+ maurb-urb = urb;
+ urb-hcpriv = maurb;
+
+ /* submit urb to hcd and add to endpoint queue */
+ ret = usb_hcd_link_urb_to_ep(hcd, urb);
Read the kerneldoc for this function. You must hold your private
spinlock when you call it.
Will fix this make sure we hold our lock.
+ if (ret 0) {
+ mausb_err(mhcd, urb enqueue failed: error %d\n, ret);
+ usb_hcd_unlink_urb_from_ep(hcd, urb);
Re: [V2 PATCH 03/10] added media agnostic (MA) data structures and handling
On Tue, Nov 11, 2014 at 01:38:21PM +0900, Greg KH wrote: On Mon, Nov 10, 2014 at 06:09:34PM -0800, Stephanie Wallick wrote: Intel has a whole group of very experienced Linux kernel developers who will review code before you sent it out publicly. Please take advantage of them and run this all through them before resending this out again. If you did run this code through that group, please let me know who it was specifically that allowed this stuff to get through, and why they didn't want their name on this code submission. I need to have a strong word with them... We submitted the patches for internal review and got no objections to release. We will be more aggressive in seeking out feedback (and approval) before resubmitting any code. Yes, I am holding you to a higher standard than staging code normally is, and yes, it is purely because of the company you work for. But I only do that because your company knows how to do this stuff right, and you have access to the resources and talent to help make this code right. Other people and companies do not have the kind of advantage that you do. We know we are fortunate to work for a company with so much talent and resources and we don't mind being held to a higher standard. We have been receiving multiple requests for our host driver and wanted to make it available as soon as possible for others to use. We thought putting our host driver into staging would be a good way to release it, but realize now that it was premature. Wasting community member's time (i.e. mine) by forcing _them_ to review stuff like this, is something that your company knows better than to do, as should you as well. I want to see some more senior Intel kernel developer's signed-off-by lines on this code before I will ever consider accepting it for the kernel. Please do not resend this code until that happens. greg k-h We apologize for wasting everyone's time and will certainly learn from this. We won't resubmit the driver until a senior kernel developer has signed off on it. Sincerely, Sean O. Stalley Stephanie S. Wallick ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
