Re: [PATCH v7 11/16] usb: refactor port handling in hub_events()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 26, 2014 at 12:44 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 19 Mar 2014, Dan Williams wrote:
>
>> In preparation for synchronizing port handling with pm_runtime
>> transitions refactor port handling into its own subroutine.
>>
>> We expect that clearing some status flags will be required regardless of
>> the port state, so handle those first and group all non-trivial actions
>> at the bottom of the routine.
>>
>> This also splits off the bottom half of hub_port_connect_change() into
>> hub_port_reconnect() in prepartion for introducing a port->status_lock.
>> hub_port_reconnect() will expect the port lock to not be held while
>> hub_port_connect_change() expects to enter with it held.
>>
>> Other cleanups include:
>> 1/ reflowing to 80 columns
>> 2/ replacing redundant usages of 'hub->hdev' with 'hdev'
>> 3/ consolidate clearing of ->change_bits() in hub_port_connect_change
>>
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> Although this is basically good, I think it might be worthwhile to
> separate the various minor cleanups from the code movement and
> reflowing.  Also, there is one more cleanup we could attempt.
>
>> @@ -4390,66 +4390,15 @@ hub_power_remaining (struct usb_hub *hub)
>>       return remaining;
>>  }
>>
>> -/* Handle physical or logical connection change events.
>> - * This routine is called when:
>> - *   a port connection-change occurs;
>> - *   a port enable-change occurs (often caused by EMI);
>> - *   usb_reset_and_verify_device() encounters changed descriptors (as from
>> - *           a firmware download)
>> - * caller already locked the hub
>> - */
>> -static void hub_port_connect_change(struct usb_hub *hub, int port1,
>> -                                     u16 portstatus, u16 portchange)
>> +static void hub_port_reconnect(struct usb_hub *hub, int port1, u16 portstatus,
>> +             u16 portchange)
>>  {
>> +     int status, i;
>> +     unsigned unit_load;
>>       struct usb_device *hdev = hub->hdev;
>>       struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
>>       struct usb_port *port_dev = hub->ports[port1 - 1];
>> -     struct usb_device *udev;
>> -     int status, i;
>> -     unsigned unit_load;
>
> Not that it makes any difference, but why did you move the declarations
> of status, i, and unit_load to the top?

Commit 5a0e3ad6af86 "include cleanup: Update gfp.h and slab.h includes
to prepare for breaking implicit slab.h"  introduced me to the concept
of "christmas-tree" or "reverse christmas-tree" declaration ordering.
I wasn't even aware there was such a thing previously, but now I can't
un-learn it and my OCD compels me to "christmas tree all the
declarations!".

> Here's a second question.  I don't know if there's any definitive
> answer.  What do you think of passing values like hdev, hcd, and
> port_dev as arguments, as opposed to re-deriving them from the other
> values?
>
> In theory, it could result in slightly smaller object code,
> particularly in cases (like here) where the whole routine can be
> inlined.  Also, it might reduce slightly the chances for copy/pasting
> errors.
>
> This isn't a big deal either way.  But people seem to prefer passing
> fewer arguments, and I have often wondered why.

80 columns?

We could do something like:

struct usb_port_context {
  struct usb_port port_dev;
  struct usb_hub *hub;
  struct usb_device *hdev;
  int port1;
  u16 portstatus, portchange;
}

...and pass that around.  But I think that is a cleanup for another
patch series.

>
>> @@ -4682,6 +4690,125 @@ static int hub_handle_remote_wakeup(struct usb_hub *hub, unsigned int port,
>>       return connect_change;
>>  }
>>
>> +static void port_event(struct usb_hub *hub, int port1)
>> +{
>> +     struct usb_port *port_dev = hub->ports[port1 - 1];
>> +     struct usb_device *udev = port_dev->child;
>> +     struct usb_device *hdev = hub->hdev;
>> +     int connect_change, wakeup_change;
>> +     u16 portstatus, portchange;
>
> Another example where passing additional arguments could be beneficial.
>
>> +
>> +     connect_change = test_bit(port1, hub->change_bits);
>> +     wakeup_change = test_and_clear_bit(port1, hub->wakeup_bits);
>> +
>> +     if (hub_port_status(hub, port1, &portstatus, &portchange) < 0)
>> +             return;
>> +
>> +     if (portchange & USB_PORT_STAT_C_CONNECTION) {
>> +             usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION);
>> +             connect_change = 1;
>> +     }
>> +
>> +     if (portchange & USB_PORT_STAT_C_ENABLE) {
>> +             if (!connect_change)
>> +                     dev_dbg(&port_dev->dev, "enable change, status %08x\n",
>> +                                     portstatus);
>> +             usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE);
>> +
>> +             /*
>> +              * EM interference sometimes causes badly shielded USB devices
>> +              * to be shutdown by the hub, this hack enables them again.
>> +              * Works at least with mouse driver.
>> +              */
>> +             if (!(portstatus & USB_PORT_STAT_ENABLE)
>> +                 && !connect_change && udev) {
>> +                     dev_err(&port_dev->dev, "disabled by hub (EMI?), re-enabling...\n");
>> +                     connect_change = 1;
>> +             }
>> +     }
>> +
>> +     if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
>> +             u16 status = 0, unused;
>> +
>> +             dev_dbg(&port_dev->dev, " over-current change\n");
>
> Extra ' ' at the start of the string.

fixed.

>
>> +             usb_clear_port_feature(hdev, port1,
>> +                             USB_PORT_FEAT_C_OVER_CURRENT);
>> +             msleep(100);    /* Cool down */
>> +             hub_power_on(hub, true);
>> +             hub_port_status(hub, port1, &status, &unused);
>> +             if (status & USB_PORT_STAT_OVERCURRENT)
>> +                     dev_err(&port_dev->dev, "over-current condition\n");
>> +     }
>> +
>> +     if (portchange & USB_PORT_STAT_C_RESET) {
>> +             dev_dbg(&port_dev->dev, "reset change\n");
>> +             usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_RESET);
>> +     }
>> +     if ((portchange & USB_PORT_STAT_C_BH_RESET)
>> +         && hub_is_superspeed(hdev)) {
>> +             dev_dbg(&port_dev->dev, "warm reset change\n");
>> +             usb_clear_port_feature(hdev, port1,
>> +                             USB_PORT_FEAT_C_BH_PORT_RESET);
>> +     }
>> +     if (portchange & USB_PORT_STAT_C_LINK_STATE) {
>> +             dev_dbg(&port_dev->dev, "link state change\n");
>> +             usb_clear_port_feature(hdev, port1,
>> +                             USB_PORT_FEAT_C_PORT_LINK_STATE);
>> +     }
>> +     if (portchange & USB_PORT_STAT_C_CONFIG_ERROR) {
>> +             dev_warn(&port_dev->dev, "config error\n");
>> +             usb_clear_port_feature(hdev, port1,
>> +                             USB_PORT_FEAT_C_PORT_CONFIG_ERROR);
>> +     }
>> +
>> +     if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange))
>> +             connect_change = 1;
>
> Moving this little portion is a candidate for the cleanup patch.

Moving it where?  Sorry I've cache flushed the context for this bit.

>
>> +
>> +     /*
>> +      * Warm reset a USB3 protocol port if it's in
>> +      * SS.Inactive state.
>> +      */
>> +     if (hub_port_warm_reset_required(hub, portstatus)) {
>> +             int status;
>> +
>> +             dev_dbg(&port_dev->dev, "do warm reset\n");
>> +             if (!udev || !(portstatus & USB_PORT_STAT_CONNECTION)
>> +                             || udev->state == USB_STATE_NOTATTACHED) {
>> +                     status = hub_port_reset(hub, port1, NULL,
>> +                                     HUB_BH_RESET_TIME, true);
>> +                     if (status < 0)
>> +                             hub_port_disable(hub, port1, 1);
>> +             } else {
>> +                     usb_lock_device(udev);
>> +                     status = usb_reset_device(udev);
>
> Another cleanup candidate: status doesn't get used.

Went ahead and removed it.

>
>> +                     usb_unlock_device(udev);
>> +                     connect_change = 0;
>> +             }
>> +     /*
>> +      * On disconnect USB3 protocol ports transit from U0 to
>> +      * SS.Inactive to Rx.Detect. If this happens a warm-
>> +      * reset is not needed, but a (re)connect may happen
>> +      * before khubd runs and sees the disconnect, and the
>> +      * device may be an unknown state.
>> +      *
>> +      * If the port went through SS.Inactive without khubd
>> +      * seeing it the C_LINK_STATE change flag will be set,
>> +      * and we reset the dev to put it in a known state.
>> +      */
>> +     } else if (udev && hub_is_superspeed(hub->hdev) &&
>> +                     (portchange & USB_PORT_STAT_C_LINK_STATE) &&
>> +                     (portstatus & USB_PORT_STAT_CONNECTION)) {
>> +             usb_lock_device(udev);
>> +             usb_reset_device(udev);
>> +             usb_unlock_device(udev);
>> +             connect_change = 0;
>> +     }
>
> This is almost identical to the code just above.  The big difference
> is in how the USB_OPRT_STAT_C_LINK_STATE bit in portchange is used.
> Can you figure out how to combine these two pieces?

Done.

Attached for review, but will be resubmitted with a refresh the whole patch set

Attachment: usb-synchronize-port-poweroff
Description: Binary data


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux