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?
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.
> @@ -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.
> + 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.
> +
> + /*
> + * 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.
> + 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?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
