linux-kernel - Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1304251039210.1185-100000@iolanthe.rowland.org>
Date:	Thu, 25 Apr 2013 10:54:53 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	ZhenHua <zhen-hual@...com>
cc:	Greg KH <gregkh@...uxfoundation.org>, <linux-usb@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <tom.vaden@...com>
Subject: Re: [PATCH 1/1] driver,usb: Fix a warning in uhci-hcd driver

On Thu, 25 Apr 2013, ZhenHua wrote:

> >>> +#define UHCI_SUSPENDRH_RETRY_MAX      10
> >>> +#define UHCI_SUSPENDRH_RETRY_DELAY    100
> > Why is the delay set to 100 us?  Isn't that excessively large?  How
> > long does it take for this controller to go into suspend?
> This controller will take about 200~400 us, but I am not sure how long 
> other devices will take.
> I set interval to 100 us,  so it will save more time.

A 400-us delay is fairly long.  It would be better to avoid it
entirely.

> > Why are these variables u16?  Why not int?
> uhci_readw will return u16.

That's not a good reason, since u16 fits perfectly well inside an
int.  But never mind...

> > Anyway, a better approach would be not to add a delay loop at all.
> > Instead, change this test:
> >
> > 	if (!auto_stop && !(uhci_readw(uhci, USBSTS) & USBSTS_HCH)) {
> > 		uhci->rh_state = UHCI_RH_SUSPENDING;
> > 		spin_unlock_irq(&uhci->lock);
> > 		msleep(1);
> > 		spin_lock_irq(&uhci->lock);
> > 		if (uhci->dead)
> > 			return;
> > 	}
> >
> > When the iLo controller is present, make the "if" statement always
> > succeed.  Then you'll get a whole 1-ms delay.
> This will cause more operation and more time for other devices.

Actually what I wrote was wrong anyway.  I forgot that when auto_stop 
is set, the routine is not allowed to sleep.

A better way to solve your problem is to change uhci_hub_status_data().
In the UHCI_RH_RUNNING_NODEVS case, change the line that says

		else if (time_after_eq(jiffies, uhci->auto_stop_time))

to

		else if (time_after_eq(jiffies, uhci->auto_stop_time) &&
				!uhci->no_auto_stops)

where uhci->no_auto_stops is a new bitflag that you set inside
uhci_pci_init() if you detect that the controller is an iLo virtual
UHCI controller.

This way there will always be a 1-ms delay, so the slow controller will
suspend successfully.  And other types of host controllers won't be
affected, because the no_auto_stops flag won't get set for them.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/