[PATCH] otg-for-v3.9-v1: USB otg: fix usage of try_module_get() and module_put()

[PATCH] otg-for-v3.9-v1: USB otg: fix usage of try_module_get() and module_put()

[Marc Kleine-Budde]

Hello,

this patch is intended for v3.9 and applies to Greg's usb/master tree. If fixes
the use of try_module_get() and module_put() in all usb_get_phy functions.

regards,
Marc


The following changes since commit 74e1a2a39355b2d3ae8c60c78d8add162c6d7183:

  Merge tag 'usb-3.9-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb (2013-02-21 12:20:00 -0800)

are available in the git repository at:


  git://git.pengutronix.de/git/mkl/linux.git tags/otg-for-v3.9-v1

for you to fetch changes up to 6bef020b4aebd7886281fb7fb37c788d5a365eea:

  USB otg: use try_module_get in all usb_get_phy functions and add missing module_put (2013-02-27 12:53:15 +0100)

----------------------------------------------------------------
USB otg: add missing try_module_get and module_put

Add try_module_get() to usb_get_phy() and usb_get_phy_dev().  Further the
missing module_put() is added to usb_put_phy()

----------------------------------------------------------------
Marc Kleine-Budde (1):
      USB otg: use try_module_get in all usb_get_phy functions and add missing module_put

 drivers/usb/otg/otg.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

[PATCH] USB otg: use try_module_get in all usb_get_phy functions and add missing module_put

[Marc Kleine-Budde]

In patch "5d3c28b usb: otg: add device tree support to otg library"
devm_usb_get_phy_by_phandle() was added. It uses try_module_get() to lock the
phy driver in memory. The corresponding module_put() is missing in that patch.

This patch adds try_module_get() to usb_get_phy() and usb_get_phy_dev().
Further the missing module_put() is added to usb_put_phy().

Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/otg/otg.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/otg/otg.c b/drivers/usb/otg/otg.c
index e181439..2bd03d2 100644
--- a/drivers/usb/otg/otg.c
+++ b/drivers/usb/otg/otg.c
@@ -130,7 +130,7 @@ struct usb_phy *usb_get_phy(enum usb_phy_type type)
 	spin_lock_irqsave(&phy_lock, flags);
 
 	phy = __usb_find_phy(&phy_list, type);
-	if (IS_ERR(phy)) {
+	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
 		pr_err("unable to find transceiver of type %s\n",
 			usb_phy_type_string(type));
 		goto err0;
@@ -228,7 +228,7 @@ struct usb_phy *usb_get_phy_dev(struct device *dev, u8 index)
 	spin_lock_irqsave(&phy_lock, flags);
 
 	phy = __usb_find_phy_dev(dev, &phy_bind_list, index);
-	if (IS_ERR(phy)) {
+	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
 		pr_err("unable to find transceiver\n");
 		goto err0;
 	}
@@ -301,8 +301,12 @@ EXPORT_SYMBOL(devm_usb_put_phy);
  */
 void usb_put_phy(struct usb_phy *x)
 {
-	if (x)
+	if (x) {
+		struct module *owner = x->dev->driver->owner;
+
 		put_device(x->dev);
+		module_put(owner);
+	}
 }
 EXPORT_SYMBOL(usb_put_phy);
 
-- 
1.7.10.4

[PATCH] USB otg: use try_module_get in all usb_get_phy functions and add missing module_put

[Greg KH]

On Wed, Feb 27, 2013 at 03:11:13PM +0100, Marc Kleine-Budde wrote:
> In patch "5d3c28b usb: otg: add device tree support to otg library"
> devm_usb_get_phy_by_phandle() was added. It uses try_module_get() to lock the
> phy driver in memory. The corresponding module_put() is missing in that patch.
> 
> This patch adds try_module_get() to usb_get_phy() and usb_get_phy_dev().
> Further the missing module_put() is added to usb_put_phy().
> 
> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
> Acked-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/otg/otg.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/otg/otg.c b/drivers/usb/otg/otg.c
> index e181439..2bd03d2 100644
> --- a/drivers/usb/otg/otg.c
> +++ b/drivers/usb/otg/otg.c
> @@ -130,7 +130,7 @@ struct usb_phy *usb_get_phy(enum usb_phy_type type)
>  	spin_lock_irqsave(&phy_lock, flags);
>  
>  	phy = __usb_find_phy(&phy_list, type);
> -	if (IS_ERR(phy)) {
> +	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {

Ugh, really?  We really are trying this type of module locking mess?

Why?  What is it solving?  What's wrong with having the module be able
to be unloaded whenever it wants to?  No one should be doing that and
expecting that their hardware would still work properly, right?

I really don't like this type of thing, sorry.

greg k-h

[PATCH] USB otg: use try_module_get in all usb_get_phy functions and add missing module_put

[Marc Kleine-Budde]

On 02/27/2013 05:21 PM, Greg KH wrote:
> On Wed, Feb 27, 2013 at 03:11:13PM +0100, Marc Kleine-Budde wrote:
>> In patch "5d3c28b usb: otg: add device tree support to otg library"
>> devm_usb_get_phy_by_phandle() was added. It uses try_module_get() to lock the
>> phy driver in memory. The corresponding module_put() is missing in that patch.
>>
>> This patch adds try_module_get() to usb_get_phy() and usb_get_phy_dev().
>> Further the missing module_put() is added to usb_put_phy().
>>
>> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Acked-by: Felipe Balbi <balbi@ti.com>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  drivers/usb/otg/otg.c |   10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/otg/otg.c b/drivers/usb/otg/otg.c
>> index e181439..2bd03d2 100644
>> --- a/drivers/usb/otg/otg.c
>> +++ b/drivers/usb/otg/otg.c
>> @@ -130,7 +130,7 @@ struct usb_phy *usb_get_phy(enum usb_phy_type type)
>>  	spin_lock_irqsave(&phy_lock, flags);
>>  
>>  	phy = __usb_find_phy(&phy_list, type);
>> -	if (IS_ERR(phy)) {
>> +	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
> 
> Ugh, really?  We really are trying this type of module locking mess?
> 
> Why?  What is it solving?  What's wrong with having the module be able

Without this patch, you can unload the phy module, while it is in use.
As the phy framework doesn't use any existing abstraction to handle phy
<-> user of phy pairing/unpairing (it's all open coded), any subsequent
use of the phy's callback will result in deref'ing bogus pointers.

> to be unloaded whenever it wants to?  No one should be doing that and
> expecting that their hardware would still work properly, right?

Yes, but it should not result in an kernel oops.

> I really don't like this type of thing, sorry.

Can you point Kishon and me to a better implementation?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130227/6b5ac10f/attachment.sig>

[PATCH] USB otg: use try_module_get in all usb_get_phy functions and add missing module_put

[Greg KH]

On Wed, Feb 27, 2013 at 05:33:11PM +0100, Marc Kleine-Budde wrote:
> On 02/27/2013 05:21 PM, Greg KH wrote:
> > On Wed, Feb 27, 2013 at 03:11:13PM +0100, Marc Kleine-Budde wrote:
> >> In patch "5d3c28b usb: otg: add device tree support to otg library"
> >> devm_usb_get_phy_by_phandle() was added. It uses try_module_get() to lock the
> >> phy driver in memory. The corresponding module_put() is missing in that patch.
> >>
> >> This patch adds try_module_get() to usb_get_phy() and usb_get_phy_dev().
> >> Further the missing module_put() is added to usb_put_phy().
> >>
> >> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> Acked-by: Felipe Balbi <balbi@ti.com>
> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> >> ---
> >>  drivers/usb/otg/otg.c |   10 +++++++---
> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/otg/otg.c b/drivers/usb/otg/otg.c
> >> index e181439..2bd03d2 100644
> >> --- a/drivers/usb/otg/otg.c
> >> +++ b/drivers/usb/otg/otg.c
> >> @@ -130,7 +130,7 @@ struct usb_phy *usb_get_phy(enum usb_phy_type type)
> >>  	spin_lock_irqsave(&phy_lock, flags);
> >>  
> >>  	phy = __usb_find_phy(&phy_list, type);
> >> -	if (IS_ERR(phy)) {
> >> +	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
> > 
> > Ugh, really?  We really are trying this type of module locking mess?
> > 
> > Why?  What is it solving?  What's wrong with having the module be able
> 
> Without this patch, you can unload the phy module, while it is in use.
> As the phy framework doesn't use any existing abstraction to handle phy
> <-> user of phy pairing/unpairing (it's all open coded), any subsequent
> use of the phy's callback will result in deref'ing bogus pointers.

Then perhaps we should fix the open-coded-ness of the phy layer?

> > to be unloaded whenever it wants to?  No one should be doing that and
> > expecting that their hardware would still work properly, right?
> 
> Yes, but it should not result in an kernel oops.

Agreed.

> > I really don't like this type of thing, sorry.
> 
> Can you point Kishon and me to a better implementation?

Wasn't someone working on a "generic" phy layer for the kernel?  That
should probably resolve this issue there.

But in looking at this further, you are right, this is about the only
way this can be solved now.  It is a pretty minor problem though.

I'll wait for this to come in from Felipe into my trees.

thanks,

greg k-h

[PATCH] USB otg: use try_module_get in all usb_get_phy functions and add missing module_put

[kishon]

Hi,

On Thursday 28 February 2013 06:11 AM, Greg KH wrote:
> On Wed, Feb 27, 2013 at 05:33:11PM +0100, Marc Kleine-Budde wrote:
>> On 02/27/2013 05:21 PM, Greg KH wrote:
>>> On Wed, Feb 27, 2013 at 03:11:13PM +0100, Marc Kleine-Budde wrote:
>>>> In patch "5d3c28b usb: otg: add device tree support to otg library"
>>>> devm_usb_get_phy_by_phandle() was added. It uses try_module_get() to lock the
>>>> phy driver in memory. The corresponding module_put() is missing in that patch.
>>>>
>>>> This patch adds try_module_get() to usb_get_phy() and usb_get_phy_dev().
>>>> Further the missing module_put() is added to usb_put_phy().
>>>>
>>>> Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> Acked-by: Felipe Balbi <balbi@ti.com>
>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>> ---
>>>>   drivers/usb/otg/otg.c |   10 +++++++---
>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/otg/otg.c b/drivers/usb/otg/otg.c
>>>> index e181439..2bd03d2 100644
>>>> --- a/drivers/usb/otg/otg.c
>>>> +++ b/drivers/usb/otg/otg.c
>>>> @@ -130,7 +130,7 @@ struct usb_phy *usb_get_phy(enum usb_phy_type type)
>>>>   	spin_lock_irqsave(&phy_lock, flags);
>>>>
>>>>   	phy = __usb_find_phy(&phy_list, type);
>>>> -	if (IS_ERR(phy)) {
>>>> +	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
>>>
>>> Ugh, really?  We really are trying this type of module locking mess?
>>>
>>> Why?  What is it solving?  What's wrong with having the module be able
>>
>> Without this patch, you can unload the phy module, while it is in use.
>> As the phy framework doesn't use any existing abstraction to handle phy
>> <-> user of phy pairing/unpairing (it's all open coded), any subsequent
>> use of the phy's callback will result in deref'ing bogus pointers.
>
> Then perhaps we should fix the open-coded-ness of the phy layer?
>
>>> to be unloaded whenever it wants to?  No one should be doing that and
>>> expecting that their hardware would still work properly, right?
>>
>> Yes, but it should not result in an kernel oops.
>
> Agreed.
>
>>> I really don't like this type of thing, sorry.
>>
>> Can you point Kishon and me to a better implementation?
>
> Wasn't someone working on a "generic" phy layer for the kernel?  That
> should probably resolve this issue there.

Even in the generic PHY layer, we use *try_module_get* to avoid 
dereferencing bogus pointers.

Thanks
Kishon

[PATCH] otg-for-v3.9-v1: USB otg: fix usage of try_module_get() and module_put()

[Steffen Trumtrar]

On Wed, Feb 27, 2013 at 03:11:12PM +0100, Marc Kleine-Budde wrote:
> Hello,
> 
> this patch is intended for v3.9 and applies to Greg's usb/master tree. If fixes
> the use of try_module_get() and module_put() in all usb_get_phy functions.
> 
> regards,
> Marc
> 
> 
> The following changes since commit 74e1a2a39355b2d3ae8c60c78d8add162c6d7183:
> 
>   Merge tag 'usb-3.9-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb (2013-02-21 12:20:00 -0800)
> 
> are available in the git repository at:
> 
> 
>   git://git.pengutronix.de/git/mkl/linux.git tags/otg-for-v3.9-v1
> 
> for you to fetch changes up to 6bef020b4aebd7886281fb7fb37c788d5a365eea:
> 
>   USB otg: use try_module_get in all usb_get_phy functions and add missing module_put (2013-02-27 12:53:15 +0100)
> 
> ----------------------------------------------------------------
> USB otg: add missing try_module_get and module_put
> 
> Add try_module_get() to usb_get_phy() and usb_get_phy_dev().  Further the
> missing module_put() is added to usb_put_phy()
> 
> ----------------------------------------------------------------
> Marc Kleine-Budde (1):
>       USB otg: use try_module_get in all usb_get_phy functions and add missing module_put
> 
>  drivers/usb/otg/otg.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> 
> 
> 

Hi,

with this series plus
        otg-for-v3.10-v1: separate phy code and add DT helper
        chipidea-for-v3.10-v1 : USB chipidea: make use of DT helpers in chipidea driver improve driver
        v9: Add tested id switch and vbus connect detect support for Chipidea

and

diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 13b7053..1eeff3c 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -907,7 +907,8 @@
                        compatible = "fsl,imx28-usb", "fsl,imx27-usb";
                        reg = <0x80080000 0x10000>;
                        interrupts = <93>;
-                       clocks = <&clks 60>;
+                       clocks = <&clks 60>, <&clks 60>, <&clks 60>;
+                       clock-names = "ipg", "ahb", "per";
                        fsl,usbphy = <&usbphy0>;
                        status = "disabled";
                };
@@ -916,7 +917,8 @@
                        compatible = "fsl,imx28-usb", "fsl,imx27-usb";
                        reg = <0x80090000 0x10000>;
                        interrupts = <92>;
-                       clocks = <&clks 61>;
+                       clocks = <&clks 61>, <&clks 61>, <&clks 61>;
+                       clock-names = "ipg", "ahb", "per";
                        fsl,usbphy = <&usbphy1>;
                        status = "disabled";
                };

I have USB otg+host working on v3.8 on an i.MX28.

So,
        Tested-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Regards,
Steffen


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |