On Mon, Sep 17, 2012 at 4:50 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
> On Sunday 16 September 2012 01:48:19 Ming Lei wrote:
>
>
>> +void usbnet_link_updated(struct usbnet *dev)
>> +{
>> + complete(&dev->link_update_completion);
>> +}
>> +EXPORT_SYMBOL_GPL(usbnet_link_updated);
>
> Isn't that a bit too trivial to get the _GPL version?
OK, will use the non GPL version.
>
>> +#define usbnet_link_suspend(dev) do { \
>> + dev_dbg(&dev->intf->dev, "%s:link suspend", __func__); \
>> + usb_autopm_put_interface_async(dev->intf); \
>> +} while(0)
>> +
>> +#define usbnet_link_resume(dev) do { \
>> + dev_dbg(&dev->intf->dev, "%s:link resume", __func__); \
>> + usb_autopm_get_interface_async(dev->intf); \
>> +} while(0)
>
> Why macros?
Just for easy debug by dumping the caller function name.
>
>
> [..]
>> +/* called by usbnet_open */
>> +static void enable_link_runtime_pm(struct usbnet *dev)
>> +{
>> + dev->link_rpm_enabled = 1;
>> +
>> + if (!dev->link_remote_wakeup) {
>> + dev->old_autosuspend_delay =
>> + dev->udev->dev.power.autosuspend_delay;
>> + pm_runtime_set_autosuspend_delay(&dev->udev->dev, 1);
>
> This is a problem. Suppose the user changes the autosuspend timeout.
> You cannot assume that the old value remains valid.
Good catch, I will fix it in -v1 by reading again the timeout value in
disable_link_runtime_pm().
>> + }
>> +
>> + if (!netif_carrier_ok(dev->net)) {
>> + dev->link_open_suspend = 1;
>> + usbnet_link_suspend(dev);
>> + }
>> +}
>
>> +static void update_link_state(struct usbnet *dev)
>> +{
>> + char *buf = NULL;
>> + unsigned pipe = 0;
>> + unsigned maxp;
>> + int ret, act_len, timeout;
>> + struct urb urb;
>> +
>> + pipe = usb_rcvintpipe(dev->udev,
>> + dev->status->desc.bEndpointAddress
>> + & USB_ENDPOINT_NUMBER_MASK);
>> + maxp = usb_maxpacket(dev->udev, pipe, 0);
>> +
>> + /*
>> + * Take default timeout as 2 times of period.
>> + * It is observed that asix device can update its link
>> + * state duing one period(128ms). Low level driver can set
>> + * its default update link time in bind() callback.
>> + */
>> + if (!dev->link_update_timeout) {
>> + timeout = max((int) dev->status->desc.bInterval,
>> + (dev->udev->speed == USB_SPEED_HIGH) ? 7 : 3);
>> + timeout = 1 << timeout;
>> + if (dev->udev->speed == USB_SPEED_HIGH)
>> + timeout /= 8;
>> + if (timeout < 128)
>> + timeout = 128;
>> + } else
>> + timeout = dev->link_update_timeout;
>> +
>> + buf = kmalloc(maxp, GFP_KERNEL);
>> + if (!buf)
>> + return;
>> +
>> + dev_dbg(&dev->intf->dev, "%s: timeout %dms\n", __func__, timeout);
>> + ret = usb_interrupt_msg(dev->udev, pipe, buf, maxp,
>> + &act_len, timeout);
>> + if (!ret) {
>> + urb.status = 0;
>> + urb.actual_length = act_len;
>> + urb.transfer_buffer = buf;
>> + urb.transfer_buffer_length = maxp;
>> + dev->driver_info->status(dev, &urb);
>> + if (dev->driver_info->flags &
>> + FLAG_LINK_UPDATE_BY_DRIVER)
>> + wait_for_completion(&dev->link_update_completion);
>
> If a driver calls usbnet_link_updated() from the same workqueue this
> will deadlock.
Good catch, I will fix it in -v1 by using system_freezable_wq.
>
>> + dev_dbg(&dev->intf->dev, "%s: link updated\n", __func__);
>> + } else
>> + dev_dbg(&dev->intf->dev, "%s: link update failed %d\n",
>> + __func__, ret);
>> + kfree(buf);
>> +}
>
> [..]
>> @@ -795,6 +977,9 @@ int usbnet_open (struct net_device *net)
>> if (retval < 0)
>> goto done_manage_power_error;
>> usb_autopm_put_interface(dev->intf);
>> + } else {
>> + if (need_link_runtime_pm(dev))
>> + enable_link_runtime_pm(dev);
>> }
>> return retval;
>>
>> @@ -1489,6 +1674,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>> if (dev->driver_info->flags & FLAG_LINK_INTR)
>> usbnet_link_change(dev, 0, 0);
>>
>> + init_link_rpm(dev);
>> +
>> return 0;
>>
>> out4:
>> @@ -1538,6 +1725,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
>> * wake the device
>> */
>> netif_device_attach (dev->net);
>> +
>> + if (PMSG_IS_AUTO(message))
>> + start_link_detect(dev);
>
> What happens if the device is autosuspended, then the system is suspended
> and the work is executed while the suspension is underway?
IMO we can avoid the problem by scheduling 'link_detect_work' on the
workqueue of 'system_freezable_wq', which will be introduced in -v1.
>
>> }
>> return 0;
>> }
>> @@ -1552,8 +1742,10 @@ int usbnet_resume (struct usb_interface *intf)
>>
>> if (!--dev->suspend_count) {
>> /* resume interrupt URBs */
>> - if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
>> - usb_submit_urb(dev->interrupt, GFP_NOIO);
>> + if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags)) {
>> + if (!dev->link_checking)
>
> That is impossible. If the device is resumed the interrupt URB must
> be restarted in every case (if it exists).
The interrupt URB will be submitted later in link_detect_work() under
the situation of being resumed by link detect work.
> You cannot assume that its only function is checking the link state.
> And it introduces a race with the workqueue.
Looks no race because usbnet_resume() will be run in workqueue
task context under the situation of being resumed by link detect work.
Thanks,
--
Ming Lei
--
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
