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
