| Message ID | 1341673008-29808-7-git-send-email-richard.zhao@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 07, 2012 at 10:56:45PM +0800, Richard Zhao wrote: > Phy may need to change settings when port connect change. > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > Tested-by: Subodh Nijsure <snijsure@grid-net.com> > --- > drivers/usb/core/hub.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 4cc8dc9..2ba9d84 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -20,6 +20,7 @@ > #include <linux/usb.h> > #include <linux/usbdevice_fs.h> > #include <linux/usb/hcd.h> > +#include <linux/usb/otg.h> > #include <linux/usb/quirks.h> > #include <linux/kthread.h> > #include <linux/mutex.h> > @@ -4037,6 +4038,13 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, > } > } > > + if (unlikely(hcd->phy && !hdev->parent)) { Why is this "unlikely"? And why mark it as such, is this a "fast path" that needs the compiler to know this hint to optimize things here? Please don't use likely() or unlikely() except in places it really is needed, _and_ you have measured the difference. Have you done so in this place? thanks, greg k-h
On Mon, Jul 09, 2012 at 09:57:57AM -0700, Greg KH wrote: > On Sat, Jul 07, 2012 at 10:56:45PM +0800, Richard Zhao wrote: > > Phy may need to change settings when port connect change. > > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > > Tested-by: Subodh Nijsure <snijsure@grid-net.com> > > --- > > drivers/usb/core/hub.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index 4cc8dc9..2ba9d84 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -20,6 +20,7 @@ > > #include <linux/usb.h> > > #include <linux/usbdevice_fs.h> > > #include <linux/usb/hcd.h> > > +#include <linux/usb/otg.h> > > #include <linux/usb/quirks.h> > > #include <linux/kthread.h> > > #include <linux/mutex.h> > > @@ -4037,6 +4038,13 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, > > } > > } > > > > + if (unlikely(hcd->phy && !hdev->parent)) { > > Why is this "unlikely"? And why mark it as such, is this a "fast path" > that needs the compiler to know this hint to optimize things here? > > Please don't use likely() or unlikely() except in places it really is > needed, _and_ you have measured the difference. Have you done so in > this place? It's from a comment by Alan Stern. http://www.spinics.net/lists/linux-usb/msg64987.html Actually, for my board, it's not unlikely. But for others which don't have notify_connect/disconnect, it's unlikely. Because it's not unlikely for all boards, I prefer remove "unlikely". Thanks Richard > > thanks, > > greg k-h >
On Tue, 10 Jul 2012, Richard Zhao wrote: > > > @@ -4037,6 +4038,13 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, > > > } > > > } > > > > > > + if (unlikely(hcd->phy && !hdev->parent)) { > > > > Why is this "unlikely"? And why mark it as such, is this a "fast path" > > that needs the compiler to know this hint to optimize things here? > > > > Please don't use likely() or unlikely() except in places it really is > > needed, _and_ you have measured the difference. Have you done so in > > this place? > It's from a comment by Alan Stern. > http://www.spinics.net/lists/linux-usb/msg64987.html That comment was made in a somewhat different context. At the time the code was part of an interrupt handler; now it isn't. > Actually, for my board, it's not unlikely. But for others which don't > have notify_connect/disconnect, it's unlikely. > > Because it's not unlikely for all boards, I prefer remove "unlikely". It's no longer a big deal one way or another. I don't care about the "unlikely" because it's on a cold path running in process context. Go ahead and remove it. Alan Stern
On Tue, Jul 10, 2012 at 10:24:07AM -0400, Alan Stern wrote: > On Tue, 10 Jul 2012, Richard Zhao wrote: > > > > > @@ -4037,6 +4038,13 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, > > > > } > > > > } > > > > > > > > + if (unlikely(hcd->phy && !hdev->parent)) { > > > > > > Why is this "unlikely"? And why mark it as such, is this a "fast path" > > > that needs the compiler to know this hint to optimize things here? > > > > > > Please don't use likely() or unlikely() except in places it really is > > > needed, _and_ you have measured the difference. Have you done so in > > > this place? > > It's from a comment by Alan Stern. > > http://www.spinics.net/lists/linux-usb/msg64987.html > > That comment was made in a somewhat different context. At the time > the code was part of an interrupt handler; now it isn't. > > > Actually, for my board, it's not unlikely. But for others which don't > > have notify_connect/disconnect, it's unlikely. > > > > Because it's not unlikely for all boards, I prefer remove "unlikely". > > It's no longer a big deal one way or another. I don't care about the > "unlikely" because it's on a cold path running in process context. Go > ahead and remove it. Ok. I'll remove it. Thanks Richard > > Alan Stern >
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 4cc8dc9..2ba9d84 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -20,6 +20,7 @@ #include <linux/usb.h> #include <linux/usbdevice_fs.h> #include <linux/usb/hcd.h> +#include <linux/usb/otg.h> #include <linux/usb/quirks.h> #include <linux/kthread.h> #include <linux/mutex.h> @@ -4037,6 +4038,13 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, } } + if (unlikely(hcd->phy && !hdev->parent)) { + if (portstatus & USB_PORT_STAT_CONNECTION) + usb_phy_notify_connect(hcd->phy, port1); + else + usb_phy_notify_disconnect(hcd->phy, port1); + } + /* Return now if debouncing failed or nothing is connected or * the device was "removed". */