| Message ID | 1383683607-28119-2-git-send-email-valentine.barshak@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Wed, Nov 06, 2013 at 12:33:26AM +0400, Valentine Barshak wrote: > This adds remove_phy flag to the HCD structure. If the flag is > set and if hcd->phy is valid, the phy is shutdown and released > whenever usb_add_hcd fails or usb_hcd_remove is called. > This can be used by the HCD drivers to auto-remove > the external USB phy when it is no longer needed. > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > --- > drivers/usb/core/hcd.c | 14 +++++++++++++- > include/linux/usb/hcd.h | 1 + > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index d6a8d23..d939521 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -43,6 +43,7 @@ > > #include <linux/usb.h> > #include <linux/usb/hcd.h> > +#include <linux/usb/phy.h> > > #include "usb.h" > > @@ -2611,7 +2612,7 @@ int usb_add_hcd(struct usb_hcd *hcd, > */ > if ((retval = hcd_buffer_create(hcd)) != 0) { > dev_dbg(hcd->self.controller, "pool alloc failed\n"); > - return retval; > + goto err_remove_phy; > } > > if ((retval = usb_register_bus(&hcd->self)) < 0) > @@ -2742,6 +2743,12 @@ err_allocate_root_hub: > usb_deregister_bus(&hcd->self); > err_register_bus: > hcd_buffer_destroy(hcd); > +err_remove_phy: > + if (hcd->remove_phy && hcd->phy) { > + usb_phy_shutdown(hcd->phy); > + usb_put_phy(hcd->phy); > + hcd->phy = NULL; > + } why do you need the flag at all ? If hcd->phy is valid, just casll usb_phy_shutdown() followed by usb_put_phy(). Did you find any issues with that ?
On Wed, 6 Nov 2013, Felipe Balbi wrote: > Hi, > > On Wed, Nov 06, 2013 at 12:33:26AM +0400, Valentine Barshak wrote: > > This adds remove_phy flag to the HCD structure. If the flag is > > set and if hcd->phy is valid, the phy is shutdown and released > > whenever usb_add_hcd fails or usb_hcd_remove is called. > > This can be used by the HCD drivers to auto-remove > > the external USB phy when it is no longer needed. > > > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > > --- > > drivers/usb/core/hcd.c | 14 +++++++++++++- > > include/linux/usb/hcd.h | 1 + > > 2 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > > index d6a8d23..d939521 100644 > > --- a/drivers/usb/core/hcd.c > > +++ b/drivers/usb/core/hcd.c > > @@ -43,6 +43,7 @@ > > > > #include <linux/usb.h> > > #include <linux/usb/hcd.h> > > +#include <linux/usb/phy.h> > > > > #include "usb.h" > > > > @@ -2611,7 +2612,7 @@ int usb_add_hcd(struct usb_hcd *hcd, > > */ > > if ((retval = hcd_buffer_create(hcd)) != 0) { > > dev_dbg(hcd->self.controller, "pool alloc failed\n"); > > - return retval; > > + goto err_remove_phy; > > } > > > > if ((retval = usb_register_bus(&hcd->self)) < 0) > > @@ -2742,6 +2743,12 @@ err_allocate_root_hub: > > usb_deregister_bus(&hcd->self); > > err_register_bus: > > hcd_buffer_destroy(hcd); > > +err_remove_phy: > > + if (hcd->remove_phy && hcd->phy) { > > + usb_phy_shutdown(hcd->phy); > > + usb_put_phy(hcd->phy); > > + hcd->phy = NULL; > > + } > > why do you need the flag at all ? If hcd->phy is valid, just casll > usb_phy_shutdown() followed by usb_put_phy(). Did you find any issues > with that ? That's a reasonable thing to do, but it means that a few other HC drivers would have to be updated (they shouldn't call usb_phy_shutdown or usb_put_phy any more). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/06/2013 07:45 PM, Felipe Balbi wrote: > Hi, > > On Wed, Nov 06, 2013 at 12:33:26AM +0400, Valentine Barshak wrote: >> This adds remove_phy flag to the HCD structure. If the flag is >> set and if hcd->phy is valid, the phy is shutdown and released >> whenever usb_add_hcd fails or usb_hcd_remove is called. >> This can be used by the HCD drivers to auto-remove >> the external USB phy when it is no longer needed. >> >> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> >> --- >> drivers/usb/core/hcd.c | 14 +++++++++++++- >> include/linux/usb/hcd.h | 1 + >> 2 files changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c >> index d6a8d23..d939521 100644 >> --- a/drivers/usb/core/hcd.c >> +++ b/drivers/usb/core/hcd.c >> @@ -43,6 +43,7 @@ >> >> #include <linux/usb.h> >> #include <linux/usb/hcd.h> >> +#include <linux/usb/phy.h> >> >> #include "usb.h" >> >> @@ -2611,7 +2612,7 @@ int usb_add_hcd(struct usb_hcd *hcd, >> */ >> if ((retval = hcd_buffer_create(hcd)) != 0) { >> dev_dbg(hcd->self.controller, "pool alloc failed\n"); >> - return retval; >> + goto err_remove_phy; >> } >> >> if ((retval = usb_register_bus(&hcd->self)) < 0) >> @@ -2742,6 +2743,12 @@ err_allocate_root_hub: >> usb_deregister_bus(&hcd->self); >> err_register_bus: >> hcd_buffer_destroy(hcd); >> +err_remove_phy: >> + if (hcd->remove_phy && hcd->phy) { >> + usb_phy_shutdown(hcd->phy); >> + usb_put_phy(hcd->phy); >> + hcd->phy = NULL; >> + } > > why do you need the flag at all ? If hcd->phy is valid, just casll > usb_phy_shutdown() followed by usb_put_phy(). Did you find any issues > with that ? > I haven't been able to test it with other platforms. However, some drivers use devm_usb_get_phy_dev() and we may face refcounter issues if we call usb_put_phy unconditionally. Adding this flag seems safe enough and we doesn't affect other drivers. Thanks, Val. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Nov 06, 2013 at 08:43:36PM +0400, Valentine wrote: > On 11/06/2013 07:45 PM, Felipe Balbi wrote: > >Hi, > > > >On Wed, Nov 06, 2013 at 12:33:26AM +0400, Valentine Barshak wrote: > >>This adds remove_phy flag to the HCD structure. If the flag is > >>set and if hcd->phy is valid, the phy is shutdown and released > >>whenever usb_add_hcd fails or usb_hcd_remove is called. > >>This can be used by the HCD drivers to auto-remove > >>the external USB phy when it is no longer needed. > >> > >>Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > >>--- > >> drivers/usb/core/hcd.c | 14 +++++++++++++- > >> include/linux/usb/hcd.h | 1 + > >> 2 files changed, 14 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > >>index d6a8d23..d939521 100644 > >>--- a/drivers/usb/core/hcd.c > >>+++ b/drivers/usb/core/hcd.c > >>@@ -43,6 +43,7 @@ > >> > >> #include <linux/usb.h> > >> #include <linux/usb/hcd.h> > >>+#include <linux/usb/phy.h> > >> > >> #include "usb.h" > >> > >>@@ -2611,7 +2612,7 @@ int usb_add_hcd(struct usb_hcd *hcd, > >> */ > >> if ((retval = hcd_buffer_create(hcd)) != 0) { > >> dev_dbg(hcd->self.controller, "pool alloc failed\n"); > >>- return retval; > >>+ goto err_remove_phy; > >> } > >> > >> if ((retval = usb_register_bus(&hcd->self)) < 0) > >>@@ -2742,6 +2743,12 @@ err_allocate_root_hub: > >> usb_deregister_bus(&hcd->self); > >> err_register_bus: > >> hcd_buffer_destroy(hcd); > >>+err_remove_phy: > >>+ if (hcd->remove_phy && hcd->phy) { > >>+ usb_phy_shutdown(hcd->phy); > >>+ usb_put_phy(hcd->phy); > >>+ hcd->phy = NULL; > >>+ } > > > >why do you need the flag at all ? If hcd->phy is valid, just casll > >usb_phy_shutdown() followed by usb_put_phy(). Did you find any issues > >with that ? > > > > I haven't been able to test it with other platforms. > However, some drivers use devm_usb_get_phy_dev() and we may face refcounter issues > if we call usb_put_phy unconditionally. > Adding this flag seems safe enough and we doesn't affect other drivers. then use devm_usb_get_phy_dev() here and remove the call from all other drivers ;-)
On 11/06/2013 09:04 PM, Felipe Balbi wrote: > Hi, > > On Wed, Nov 06, 2013 at 08:43:36PM +0400, Valentine wrote: >> On 11/06/2013 07:45 PM, Felipe Balbi wrote: >>> Hi, >>> >>> On Wed, Nov 06, 2013 at 12:33:26AM +0400, Valentine Barshak wrote: >>>> This adds remove_phy flag to the HCD structure. If the flag is >>>> set and if hcd->phy is valid, the phy is shutdown and released >>>> whenever usb_add_hcd fails or usb_hcd_remove is called. >>>> This can be used by the HCD drivers to auto-remove >>>> the external USB phy when it is no longer needed. >>>> >>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> >>>> --- >>>> drivers/usb/core/hcd.c | 14 +++++++++++++- >>>> include/linux/usb/hcd.h | 1 + >>>> 2 files changed, 14 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c >>>> index d6a8d23..d939521 100644 >>>> --- a/drivers/usb/core/hcd.c >>>> +++ b/drivers/usb/core/hcd.c >>>> @@ -43,6 +43,7 @@ >>>> >>>> #include <linux/usb.h> >>>> #include <linux/usb/hcd.h> >>>> +#include <linux/usb/phy.h> >>>> >>>> #include "usb.h" >>>> >>>> @@ -2611,7 +2612,7 @@ int usb_add_hcd(struct usb_hcd *hcd, >>>> */ >>>> if ((retval = hcd_buffer_create(hcd)) != 0) { >>>> dev_dbg(hcd->self.controller, "pool alloc failed\n"); >>>> - return retval; >>>> + goto err_remove_phy; >>>> } >>>> >>>> if ((retval = usb_register_bus(&hcd->self)) < 0) >>>> @@ -2742,6 +2743,12 @@ err_allocate_root_hub: >>>> usb_deregister_bus(&hcd->self); >>>> err_register_bus: >>>> hcd_buffer_destroy(hcd); >>>> +err_remove_phy: >>>> + if (hcd->remove_phy && hcd->phy) { >>>> + usb_phy_shutdown(hcd->phy); >>>> + usb_put_phy(hcd->phy); >>>> + hcd->phy = NULL; >>>> + } >>> >>> why do you need the flag at all ? If hcd->phy is valid, just casll >>> usb_phy_shutdown() followed by usb_put_phy(). Did you find any issues >>> with that ? >>> >> >> I haven't been able to test it with other platforms. >> However, some drivers use devm_usb_get_phy_dev() and we may face refcounter issues >> if we call usb_put_phy unconditionally. >> Adding this flag seems safe enough and we doesn't affect other drivers. > > then use devm_usb_get_phy_dev() here and remove the call from all other > drivers ;-) > The glue drivers may use different ways of getting the USB phy: usb_get_phy; usb_get_phy_dev; devm_usb_get_phy_dev. We can't consolidate phy getting here since usb_get_phy and usb_get_phy_dev are not equivalent. So we let the glue drivers decide which way of getting/putting USB phy to use. Thanks, Val. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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/core/hcd.c b/drivers/usb/core/hcd.c index d6a8d23..d939521 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -43,6 +43,7 @@ #include <linux/usb.h> #include <linux/usb/hcd.h> +#include <linux/usb/phy.h> #include "usb.h" @@ -2611,7 +2612,7 @@ int usb_add_hcd(struct usb_hcd *hcd, */ if ((retval = hcd_buffer_create(hcd)) != 0) { dev_dbg(hcd->self.controller, "pool alloc failed\n"); - return retval; + goto err_remove_phy; } if ((retval = usb_register_bus(&hcd->self)) < 0) @@ -2742,6 +2743,12 @@ err_allocate_root_hub: usb_deregister_bus(&hcd->self); err_register_bus: hcd_buffer_destroy(hcd); +err_remove_phy: + if (hcd->remove_phy && hcd->phy) { + usb_phy_shutdown(hcd->phy); + usb_put_phy(hcd->phy); + hcd->phy = NULL; + } return retval; } EXPORT_SYMBOL_GPL(usb_add_hcd); @@ -2814,6 +2821,11 @@ void usb_remove_hcd(struct usb_hcd *hcd) usb_put_dev(hcd->self.root_hub); usb_deregister_bus(&hcd->self); hcd_buffer_destroy(hcd); + if (hcd->remove_phy && hcd->phy) { + usb_phy_shutdown(hcd->phy); + usb_put_phy(hcd->phy); + hcd->phy = NULL; + } } EXPORT_SYMBOL_GPL(usb_remove_hcd); diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h index 75efc45..9c2907b 100644 --- a/include/linux/usb/hcd.h +++ b/include/linux/usb/hcd.h @@ -133,6 +133,7 @@ struct usb_hcd { unsigned rh_registered:1;/* is root hub registered? */ unsigned rh_pollable:1; /* may we poll the root hub? */ unsigned msix_enabled:1; /* driver has MSI-X enabled? */ + unsigned remove_phy:1; /* auto-remove USB phy */ /* The next flag is a stopgap, to be removed when all the HCDs * support the new root-hub polling mechanism. */
This adds remove_phy flag to the HCD structure. If the flag is set and if hcd->phy is valid, the phy is shutdown and released whenever usb_add_hcd fails or usb_hcd_remove is called. This can be used by the HCD drivers to auto-remove the external USB phy when it is no longer needed. Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> --- drivers/usb/core/hcd.c | 14 +++++++++++++- include/linux/usb/hcd.h | 1 + 2 files changed, 14 insertions(+), 1 deletion(-)