* Re: [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed
2013-11-05 20:33 [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed Valentine Barshak
@ 2013-11-06 15:45 ` Felipe Balbi
2013-11-06 16:36 ` Alan Stern
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Felipe Balbi @ 2013-11-06 15:45 UTC (permalink / raw)
To: linux-sh
[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]
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 ?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed
2013-11-05 20:33 [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed Valentine Barshak
2013-11-06 15:45 ` Felipe Balbi
@ 2013-11-06 16:36 ` Alan Stern
2013-11-06 16:43 ` Valentine
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2013-11-06 16:36 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed
2013-11-05 20:33 [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed Valentine Barshak
2013-11-06 15:45 ` Felipe Balbi
2013-11-06 16:36 ` Alan Stern
@ 2013-11-06 16:43 ` Valentine
2013-11-06 17:04 ` Felipe Balbi
2013-11-06 17:17 ` Valentine
4 siblings, 0 replies; 6+ messages in thread
From: Valentine @ 2013-11-06 16:43 UTC (permalink / raw)
To: linux-sh
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed
2013-11-05 20:33 [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed Valentine Barshak
` (2 preceding siblings ...)
2013-11-06 16:43 ` Valentine
@ 2013-11-06 17:04 ` Felipe Balbi
2013-11-06 17:17 ` Valentine
4 siblings, 0 replies; 6+ messages in thread
From: Felipe Balbi @ 2013-11-06 17:04 UTC (permalink / raw)
To: linux-sh
[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]
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 ;-)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed
2013-11-05 20:33 [RFC PATCH 1/2] usb: hcd: Remove USB phy if needed Valentine Barshak
` (3 preceding siblings ...)
2013-11-06 17:04 ` Felipe Balbi
@ 2013-11-06 17:17 ` Valentine
4 siblings, 0 replies; 6+ messages in thread
From: Valentine @ 2013-11-06 17:17 UTC (permalink / raw)
To: linux-sh
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.
^ permalink raw reply [flat|nested] 6+ messages in thread