| Message ID | 1429008120-5395-7-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 14, 2015 at 01:41:53PM +0300, Roger Quadros wrote: > The existing usb_add/remove_hcd() functionality > remains unchanged for non-OTG devices. For OTG > devices they only register the HCD with the OTG core. > > Introduce _usb_add/remove_hcd() for use by OTG core. > These functions actually add/remove the HCD. Would you please explain why additional _usb_add/remove_hcd are needed? Peter > > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > drivers/usb/common/usb-otg.c | 14 +++++++------- > drivers/usb/core/hcd.c | 24 ++++++++++++++++++++++-- > include/linux/usb/hcd.h | 3 +++ > 3 files changed, 32 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c > index e848e08..860e2e7 100644 > --- a/drivers/usb/common/usb-otg.c > +++ b/drivers/usb/common/usb-otg.c > @@ -198,18 +198,18 @@ static int usb_otg_start_host(struct otg_fsm *fsm, int on) > otgd->start_host(fsm, on); > > /* start host */ > - usb_add_hcd(otgd->primary_hcd.hcd, otgd->primary_hcd.irqnum, > - otgd->primary_hcd.irqflags); > + _usb_add_hcd(otgd->primary_hcd.hcd, otgd->primary_hcd.irqnum, > + otgd->primary_hcd.irqflags); > if (otgd->shared_hcd.hcd) { > - usb_add_hcd(otgd->shared_hcd.hcd, > - otgd->shared_hcd.irqnum, > - otgd->shared_hcd.irqflags); > + _usb_add_hcd(otgd->shared_hcd.hcd, > + otgd->shared_hcd.irqnum, > + otgd->shared_hcd.irqflags); > } > } else { > /* stop host */ > if (otgd->shared_hcd.hcd) > - usb_remove_hcd(otgd->shared_hcd.hcd); > - usb_remove_hcd(otgd->primary_hcd.hcd); > + _usb_remove_hcd(otgd->shared_hcd.hcd); > + _usb_remove_hcd(otgd->primary_hcd.hcd); > > /* OTG device operations */ > if (otgd->start_host) > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 45a915c..9a9c0f7 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -46,6 +46,7 @@ > #include <linux/usb.h> > #include <linux/usb/hcd.h> > #include <linux/usb/phy.h> > +#include <linux/usb/usb-otg.h> > > #include "usb.h" > > @@ -2622,7 +2623,7 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd) > * buffers of consistent memory, register the bus, request the IRQ line, > * and call the driver's reset() and start() routines. > */ > -int usb_add_hcd(struct usb_hcd *hcd, > +int _usb_add_hcd(struct usb_hcd *hcd, > unsigned int irqnum, unsigned long irqflags) > { > int retval; > @@ -2827,6 +2828,17 @@ err_phy: > } > return retval; > } > +EXPORT_SYMBOL_GPL(_usb_add_hcd); > + > +int usb_add_hcd(struct usb_hcd *hcd, > + unsigned int irqnum, unsigned long irqflags) > +{ > + /* If OTG device, OTG core takes care of adding HCD */ > + if (usb_otg_register_hcd(hcd, irqnum, irqflags)) > + return _usb_add_hcd(hcd, irqnum, irqflags); > + > + return 0; > +} > EXPORT_SYMBOL_GPL(usb_add_hcd); > > /** > @@ -2837,7 +2849,7 @@ EXPORT_SYMBOL_GPL(usb_add_hcd); > * Disconnects the root hub, then reverses the effects of usb_add_hcd(), > * invoking the HCD's stop() method. > */ > -void usb_remove_hcd(struct usb_hcd *hcd) > +void _usb_remove_hcd(struct usb_hcd *hcd) > { > struct usb_device *rhdev = hcd->self.root_hub; > > @@ -2911,6 +2923,14 @@ void usb_remove_hcd(struct usb_hcd *hcd) > > usb_put_invalidate_rhdev(hcd); > } > +EXPORT_SYMBOL_GPL(_usb_remove_hcd); > + > +void usb_remove_hcd(struct usb_hcd *hcd) > +{ > + /* If OTG device, OTG core takes care of stopping HCD */ > + if (usb_otg_unregister_hcd(hcd)) > + _usb_remove_hcd(hcd); > +} > EXPORT_SYMBOL_GPL(usb_remove_hcd); > > void > diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h > index 68b1e83..7993ae7 100644 > --- a/include/linux/usb/hcd.h > +++ b/include/linux/usb/hcd.h > @@ -433,7 +433,10 @@ extern void usb_put_hcd(struct usb_hcd *hcd); > extern int usb_hcd_is_primary_hcd(struct usb_hcd *hcd); > extern int usb_add_hcd(struct usb_hcd *hcd, > unsigned int irqnum, unsigned long irqflags); > +extern int _usb_add_hcd(struct usb_hcd *hcd, > + unsigned int irqnum, unsigned long irqflags); > extern void usb_remove_hcd(struct usb_hcd *hcd); > +extern void _usb_remove_hcd(struct usb_hcd *hcd); > extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1); > > struct platform_device; > -- > 2.1.0 >
On 17/04/15 05:18, Peter Chen wrote: > On Tue, Apr 14, 2015 at 01:41:53PM +0300, Roger Quadros wrote: >> The existing usb_add/remove_hcd() functionality >> remains unchanged for non-OTG devices. For OTG >> devices they only register the HCD with the OTG core. >> >> Introduce _usb_add/remove_hcd() for use by OTG core. >> These functions actually add/remove the HCD. > > Would you please explain why additional _usb_add/remove_hcd are needed? It is to differentiate if the add/remove_hcd was called by the HCD drivers or by the OTG core as we want to behave a bit differently in both cases. When called by HCD drivers, we want to defer the add/remove if it is an OTG device. When called by OTG core, we don't want to defer the add/remove. HCD drivers use usb_add/remove_hcd() OTG core uses _usb_add/remove_hcd() cheers, -roger > > Peter >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> drivers/usb/common/usb-otg.c | 14 +++++++------- >> drivers/usb/core/hcd.c | 24 ++++++++++++++++++++++-- >> include/linux/usb/hcd.h | 3 +++ >> 3 files changed, 32 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c >> index e848e08..860e2e7 100644 >> --- a/drivers/usb/common/usb-otg.c >> +++ b/drivers/usb/common/usb-otg.c >> @@ -198,18 +198,18 @@ static int usb_otg_start_host(struct otg_fsm *fsm, int on) >> otgd->start_host(fsm, on); >> >> /* start host */ >> - usb_add_hcd(otgd->primary_hcd.hcd, otgd->primary_hcd.irqnum, >> - otgd->primary_hcd.irqflags); >> + _usb_add_hcd(otgd->primary_hcd.hcd, otgd->primary_hcd.irqnum, >> + otgd->primary_hcd.irqflags); >> if (otgd->shared_hcd.hcd) { >> - usb_add_hcd(otgd->shared_hcd.hcd, >> - otgd->shared_hcd.irqnum, >> - otgd->shared_hcd.irqflags); >> + _usb_add_hcd(otgd->shared_hcd.hcd, >> + otgd->shared_hcd.irqnum, >> + otgd->shared_hcd.irqflags); >> } >> } else { >> /* stop host */ >> if (otgd->shared_hcd.hcd) >> - usb_remove_hcd(otgd->shared_hcd.hcd); >> - usb_remove_hcd(otgd->primary_hcd.hcd); >> + _usb_remove_hcd(otgd->shared_hcd.hcd); >> + _usb_remove_hcd(otgd->primary_hcd.hcd); >> >> /* OTG device operations */ >> if (otgd->start_host) >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c >> index 45a915c..9a9c0f7 100644 >> --- a/drivers/usb/core/hcd.c >> +++ b/drivers/usb/core/hcd.c >> @@ -46,6 +46,7 @@ >> #include <linux/usb.h> >> #include <linux/usb/hcd.h> >> #include <linux/usb/phy.h> >> +#include <linux/usb/usb-otg.h> >> >> #include "usb.h" >> >> @@ -2622,7 +2623,7 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd) >> * buffers of consistent memory, register the bus, request the IRQ line, >> * and call the driver's reset() and start() routines. >> */ >> -int usb_add_hcd(struct usb_hcd *hcd, >> +int _usb_add_hcd(struct usb_hcd *hcd, >> unsigned int irqnum, unsigned long irqflags) >> { >> int retval; >> @@ -2827,6 +2828,17 @@ err_phy: >> } >> return retval; >> } >> +EXPORT_SYMBOL_GPL(_usb_add_hcd); >> + >> +int usb_add_hcd(struct usb_hcd *hcd, >> + unsigned int irqnum, unsigned long irqflags) >> +{ >> + /* If OTG device, OTG core takes care of adding HCD */ >> + if (usb_otg_register_hcd(hcd, irqnum, irqflags)) >> + return _usb_add_hcd(hcd, irqnum, irqflags); >> + >> + return 0; >> +} >> EXPORT_SYMBOL_GPL(usb_add_hcd); >> >> /** >> @@ -2837,7 +2849,7 @@ EXPORT_SYMBOL_GPL(usb_add_hcd); >> * Disconnects the root hub, then reverses the effects of usb_add_hcd(), >> * invoking the HCD's stop() method. >> */ >> -void usb_remove_hcd(struct usb_hcd *hcd) >> +void _usb_remove_hcd(struct usb_hcd *hcd) >> { >> struct usb_device *rhdev = hcd->self.root_hub; >> >> @@ -2911,6 +2923,14 @@ void usb_remove_hcd(struct usb_hcd *hcd) >> >> usb_put_invalidate_rhdev(hcd); >> } >> +EXPORT_SYMBOL_GPL(_usb_remove_hcd); >> + >> +void usb_remove_hcd(struct usb_hcd *hcd) >> +{ >> + /* If OTG device, OTG core takes care of stopping HCD */ >> + if (usb_otg_unregister_hcd(hcd)) >> + _usb_remove_hcd(hcd); >> +} >> EXPORT_SYMBOL_GPL(usb_remove_hcd); >> >> void >> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h >> index 68b1e83..7993ae7 100644 >> --- a/include/linux/usb/hcd.h >> +++ b/include/linux/usb/hcd.h >> @@ -433,7 +433,10 @@ extern void usb_put_hcd(struct usb_hcd *hcd); >> extern int usb_hcd_is_primary_hcd(struct usb_hcd *hcd); >> extern int usb_add_hcd(struct usb_hcd *hcd, >> unsigned int irqnum, unsigned long irqflags); >> +extern int _usb_add_hcd(struct usb_hcd *hcd, >> + unsigned int irqnum, unsigned long irqflags); >> extern void usb_remove_hcd(struct usb_hcd *hcd); >> +extern void _usb_remove_hcd(struct usb_hcd *hcd); >> extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1); >> >> struct platform_device; >> -- >> 2.1.0 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 17 Apr 2015, Roger Quadros wrote: > On 17/04/15 05:18, Peter Chen wrote: > > On Tue, Apr 14, 2015 at 01:41:53PM +0300, Roger Quadros wrote: > >> The existing usb_add/remove_hcd() functionality > >> remains unchanged for non-OTG devices. For OTG > >> devices they only register the HCD with the OTG core. > >> > >> Introduce _usb_add/remove_hcd() for use by OTG core. > >> These functions actually add/remove the HCD. > > > > Would you please explain why additional _usb_add/remove_hcd are needed? > > It is to differentiate if the add/remove_hcd was called by the > HCD drivers or by the OTG core as we want to behave a bit differently > in both cases. > When called by HCD drivers, we want to defer the add/remove if it is an > OTG device. When called by OTG core, we don't want to defer the add/remove. I don't understand this. Why do you want to defer the add/remove if the device is OTG? Don't host controller drivers expect these things to execute synchronously? For example, what happens if you rmmod the HCD? If the remove call gets deferred, then when it finally runs it will try to call back into HCD code that has already been unloaded from memory! > HCD drivers use usb_add/remove_hcd() > OTG core uses _usb_add/remove_hcd() How about a more explicit naming scheme? HC drivers use usb_add/remove_hcd() OTG core uses usb_otg_add/remove_hcd() Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/04/15 17:03, Alan Stern wrote: > On Fri, 17 Apr 2015, Roger Quadros wrote: > >> On 17/04/15 05:18, Peter Chen wrote: >>> On Tue, Apr 14, 2015 at 01:41:53PM +0300, Roger Quadros wrote: >>>> The existing usb_add/remove_hcd() functionality >>>> remains unchanged for non-OTG devices. For OTG >>>> devices they only register the HCD with the OTG core. >>>> >>>> Introduce _usb_add/remove_hcd() for use by OTG core. >>>> These functions actually add/remove the HCD. >>> >>> Would you please explain why additional _usb_add/remove_hcd are needed? >> >> It is to differentiate if the add/remove_hcd was called by the >> HCD drivers or by the OTG core as we want to behave a bit differently >> in both cases. >> When called by HCD drivers, we want to defer the add/remove if it is an >> OTG device. When called by OTG core, we don't want to defer the add/remove. > > I don't understand this. Why do you want to defer the add/remove if > the device is OTG? Don't host controller drivers expect these things > to execute synchronously? Sorry for the wrong information. We actually defer only the add as the OTG state machine might not yet be in Host ready mode. The remove is always synchronous and we ensure that the HCD is removed when usb_otg_unregister_hcd() returns. > > For example, what happens if you rmmod the HCD? If the remove call > gets deferred, then when it finally runs it will try to call back into > HCD code that has already been unloaded from memory! > >> HCD drivers use usb_add/remove_hcd() >> OTG core uses _usb_add/remove_hcd() > > How about a more explicit naming scheme? > > HC drivers use usb_add/remove_hcd() > OTG core uses usb_otg_add/remove_hcd() Yes, this is better. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 20 Apr 2015, Roger Quadros wrote: > > I don't understand this. Why do you want to defer the add/remove if > > the device is OTG? Don't host controller drivers expect these things > > to execute synchronously? > > Sorry for the wrong information. We actually defer only the add as the > OTG state machine might not yet be in Host ready mode. > The remove is always synchronous and we ensure that the HCD is removed > when usb_otg_unregister_hcd() returns. That's okay then, but it does leave one potential hole. What happens if usb_add_hcd() is deferred for so long that usb_remove_hcd() gets called before the add can complete? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/04/15 16:56, Alan Stern wrote: > On Mon, 20 Apr 2015, Roger Quadros wrote: > >>> I don't understand this. Why do you want to defer the add/remove if >>> the device is OTG? Don't host controller drivers expect these things >>> to execute synchronously? >> >> Sorry for the wrong information. We actually defer only the add as the >> OTG state machine might not yet be in Host ready mode. >> The remove is always synchronous and we ensure that the HCD is removed >> when usb_otg_unregister_hcd() returns. > > That's okay then, but it does leave one potential hole. What happens > if usb_add_hcd() is deferred for so long that usb_remove_hcd() gets > called before the add can complete? We keep track of the HCD state in the OTG state machine and if it was not yet added then _usb_remove_hcd() is not called during usb_otg_unregister_hcd() in the usb_remove_hcd() call. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c index e848e08..860e2e7 100644 --- a/drivers/usb/common/usb-otg.c +++ b/drivers/usb/common/usb-otg.c @@ -198,18 +198,18 @@ static int usb_otg_start_host(struct otg_fsm *fsm, int on) otgd->start_host(fsm, on); /* start host */ - usb_add_hcd(otgd->primary_hcd.hcd, otgd->primary_hcd.irqnum, - otgd->primary_hcd.irqflags); + _usb_add_hcd(otgd->primary_hcd.hcd, otgd->primary_hcd.irqnum, + otgd->primary_hcd.irqflags); if (otgd->shared_hcd.hcd) { - usb_add_hcd(otgd->shared_hcd.hcd, - otgd->shared_hcd.irqnum, - otgd->shared_hcd.irqflags); + _usb_add_hcd(otgd->shared_hcd.hcd, + otgd->shared_hcd.irqnum, + otgd->shared_hcd.irqflags); } } else { /* stop host */ if (otgd->shared_hcd.hcd) - usb_remove_hcd(otgd->shared_hcd.hcd); - usb_remove_hcd(otgd->primary_hcd.hcd); + _usb_remove_hcd(otgd->shared_hcd.hcd); + _usb_remove_hcd(otgd->primary_hcd.hcd); /* OTG device operations */ if (otgd->start_host) diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 45a915c..9a9c0f7 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -46,6 +46,7 @@ #include <linux/usb.h> #include <linux/usb/hcd.h> #include <linux/usb/phy.h> +#include <linux/usb/usb-otg.h> #include "usb.h" @@ -2622,7 +2623,7 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd) * buffers of consistent memory, register the bus, request the IRQ line, * and call the driver's reset() and start() routines. */ -int usb_add_hcd(struct usb_hcd *hcd, +int _usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqflags) { int retval; @@ -2827,6 +2828,17 @@ err_phy: } return retval; } +EXPORT_SYMBOL_GPL(_usb_add_hcd); + +int usb_add_hcd(struct usb_hcd *hcd, + unsigned int irqnum, unsigned long irqflags) +{ + /* If OTG device, OTG core takes care of adding HCD */ + if (usb_otg_register_hcd(hcd, irqnum, irqflags)) + return _usb_add_hcd(hcd, irqnum, irqflags); + + return 0; +} EXPORT_SYMBOL_GPL(usb_add_hcd); /** @@ -2837,7 +2849,7 @@ EXPORT_SYMBOL_GPL(usb_add_hcd); * Disconnects the root hub, then reverses the effects of usb_add_hcd(), * invoking the HCD's stop() method. */ -void usb_remove_hcd(struct usb_hcd *hcd) +void _usb_remove_hcd(struct usb_hcd *hcd) { struct usb_device *rhdev = hcd->self.root_hub; @@ -2911,6 +2923,14 @@ void usb_remove_hcd(struct usb_hcd *hcd) usb_put_invalidate_rhdev(hcd); } +EXPORT_SYMBOL_GPL(_usb_remove_hcd); + +void usb_remove_hcd(struct usb_hcd *hcd) +{ + /* If OTG device, OTG core takes care of stopping HCD */ + if (usb_otg_unregister_hcd(hcd)) + _usb_remove_hcd(hcd); +} EXPORT_SYMBOL_GPL(usb_remove_hcd); void diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h index 68b1e83..7993ae7 100644 --- a/include/linux/usb/hcd.h +++ b/include/linux/usb/hcd.h @@ -433,7 +433,10 @@ extern void usb_put_hcd(struct usb_hcd *hcd); extern int usb_hcd_is_primary_hcd(struct usb_hcd *hcd); extern int usb_add_hcd(struct usb_hcd *hcd, unsigned int irqnum, unsigned long irqflags); +extern int _usb_add_hcd(struct usb_hcd *hcd, + unsigned int irqnum, unsigned long irqflags); extern void usb_remove_hcd(struct usb_hcd *hcd); +extern void _usb_remove_hcd(struct usb_hcd *hcd); extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1); struct platform_device;
The existing usb_add/remove_hcd() functionality remains unchanged for non-OTG devices. For OTG devices they only register the HCD with the OTG core. Introduce _usb_add/remove_hcd() for use by OTG core. These functions actually add/remove the HCD. Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/common/usb-otg.c | 14 +++++++------- drivers/usb/core/hcd.c | 24 ++++++++++++++++++++++-- include/linux/usb/hcd.h | 3 +++ 3 files changed, 32 insertions(+), 9 deletions(-)