lkml.org 
[lkml]   [2014]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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 0000000..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 parameters*/
> > + struct usb_port_status port_status[MAUSB_ROOTHUB_NUM_PORTS];
> > +
> > + /* root hub state */
> > + enum mausb_rh_state rh_state;
> > +
> > +};
>
> HTH
> Oliver
>
> --
> Oliver Neukum <oneukum@suse.de>
>


\
 
 \ /
  Last update: 2014-11-12 21:01    [W:0.084 / U:1.456 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site