[PATCH] omap: usbhs: Fixed the crash during rmmod of ehci and ohci

[PATCH] omap: usbhs: Fixed the crash during rmmod of ehci and ohci

[Keshava Munegowda]

From: Keshava Munegowda <Keshava_mgowda-l0cyMroinI0@public.gmane.org>

the disabling of clocks and freeing GPIO are changed
to fix the occurence of the crash of rmmod of ehci and ohci
drivers

Signed-off-by: Keshava Munegowda <keshava_mgowda-l0cyMroinI0@public.gmane.org>
---
 drivers/mfd/omap-usb-host.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index 3ab9ffa..cda0797 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -939,6 +939,7 @@ static void usbhs_disable(struct device *dev)
 	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
 	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
 	unsigned long			flags = 0;
+	unsigned int			halt = 0;
 	unsigned long			timeout;
 
 	dev_dbg(dev, "stopping TI HSUSB Controller\n");
@@ -994,24 +995,31 @@ static void usbhs_disable(struct device *dev)
 			dev_dbg(dev, "operation timed out\n");
 	}
 
-	if (pdata->ehci_data->phy_reset) {
-		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
-			gpio_free(pdata->ehci_data->reset_gpio_port[0]);
-
-		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
-			gpio_free(pdata->ehci_data->reset_gpio_port[1]);
+	if (is_omap_usbhs_rev2(omap)) {
+		if (is_ehci_tll_mode(pdata->port_mode[0]))
+			clk_enable(omap->usbtll_p1_fck);
+		if (is_ehci_tll_mode(pdata->port_mode[1]))
+			clk_enable(omap->usbtll_p2_fck);
+		clk_disable(omap->utmi_p2_fck);
+		clk_disable(omap->utmi_p1_fck);
 	}
 
-	clk_disable(omap->utmi_p2_fck);
-	clk_disable(omap->utmi_p1_fck);
 	clk_disable(omap->usbtll_ick);
 	clk_disable(omap->usbtll_fck);
 	clk_disable(omap->usbhost_fs_fck);
 	clk_disable(omap->usbhost_hs_fck);
 	clk_disable(omap->usbhost_ick);
+	halt = 1;
 
 end_disble:
 	spin_unlock_irqrestore(&omap->lock, flags);
+	if (halt && pdata->ehci_data->phy_reset) {
+		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
+			gpio_free(pdata->ehci_data->reset_gpio_port[0]);
+
+		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
+			gpio_free(pdata->ehci_data->reset_gpio_port[1]);
+	}
 }
 
 int omap_usbhs_enable(struct device *dev)
-- 
1.6.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap: usbhs: Fixed the crash during rmmod of ehci and ohci

[Felipe Balbi]

Hi,

On Mon, May 09, 2011 at 02:40:03PM +0530, Keshava Munegowda wrote:
> From: Keshava Munegowda <Keshava_mgowda-l0cyMroinI0@public.gmane.org>
> 
> the disabling of clocks and freeing GPIO are changed
> to fix the occurence of the crash of rmmod of ehci and ohci
> drivers
> 
> Signed-off-by: Keshava Munegowda <keshava_mgowda-l0cyMroinI0@public.gmane.org>

NAK

> ---
>  drivers/mfd/omap-usb-host.c |   24 ++++++++++++++++--------
>  1 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
> index 3ab9ffa..cda0797 100644
> --- a/drivers/mfd/omap-usb-host.c
> +++ b/drivers/mfd/omap-usb-host.c
> @@ -939,6 +939,7 @@ static void usbhs_disable(struct device *dev)
>  	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
>  	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
>  	unsigned long			flags = 0;
> +	unsigned int			halt = 0;

you shouldn't need this.

> @@ -994,24 +995,31 @@ static void usbhs_disable(struct device *dev)
>  			dev_dbg(dev, "operation timed out\n");
>  	}
>  
> -	if (pdata->ehci_data->phy_reset) {
> -		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
> -			gpio_free(pdata->ehci_data->reset_gpio_port[0]);
> -
> -		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
> -			gpio_free(pdata->ehci_data->reset_gpio_port[1]);
> +	if (is_omap_usbhs_rev2(omap)) {
> +		if (is_ehci_tll_mode(pdata->port_mode[0]))
> +			clk_enable(omap->usbtll_p1_fck);
> +		if (is_ehci_tll_mode(pdata->port_mode[1]))
> +			clk_enable(omap->usbtll_p2_fck);
> +		clk_disable(omap->utmi_p2_fck);
> +		clk_disable(omap->utmi_p1_fck);
>  	}
>  
> -	clk_disable(omap->utmi_p2_fck);
> -	clk_disable(omap->utmi_p1_fck);
>  	clk_disable(omap->usbtll_ick);
>  	clk_disable(omap->usbtll_fck);
>  	clk_disable(omap->usbhost_fs_fck);
>  	clk_disable(omap->usbhost_hs_fck);
>  	clk_disable(omap->usbhost_ick);
> +	halt = 1;

at least from this diff, you're always reaching that part of the code
rendering this halt flag useless.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap: usbhs: Fixed the crash during rmmod of ehci and ohci

[Munegowda, Keshava]

On Mon, May 9, 2011 at 3:06 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, May 09, 2011 at 02:40:03PM +0530, Keshava Munegowda wrote:
>> From: Keshava Munegowda <Keshava_mgowda@ti.com>
>>
>> the disabling of clocks and freeing GPIO are changed
>> to fix the occurence of the crash of rmmod of ehci and ohci
>> drivers
>>
>> Signed-off-by: Keshava Munegowda <keshava_mgowda@ti.com>
>
> NAK
>
>> ---
>>  drivers/mfd/omap-usb-host.c |   24 ++++++++++++++++--------
>>  1 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
>> index 3ab9ffa..cda0797 100644
>> --- a/drivers/mfd/omap-usb-host.c
>> +++ b/drivers/mfd/omap-usb-host.c
>> @@ -939,6 +939,7 @@ static void usbhs_disable(struct device *dev)
>>       struct usbhs_hcd_omap           *omap = dev_get_drvdata(dev);
>>       struct usbhs_omap_platform_data *pdata = &omap->platdata;
>>       unsigned long                   flags = 0;
>> +     unsigned int                    halt = 0;
>
> you shouldn't need this.
>
>> @@ -994,24 +995,31 @@ static void usbhs_disable(struct device *dev)
>>                       dev_dbg(dev, "operation timed out\n");
>>       }
>>
>> -     if (pdata->ehci_data->phy_reset) {
>> -             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
>> -                     gpio_free(pdata->ehci_data->reset_gpio_port[0]);
>> -
>> -             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
>> -                     gpio_free(pdata->ehci_data->reset_gpio_port[1]);
>> +     if (is_omap_usbhs_rev2(omap)) {
>> +             if (is_ehci_tll_mode(pdata->port_mode[0]))
>> +                     clk_enable(omap->usbtll_p1_fck);
>> +             if (is_ehci_tll_mode(pdata->port_mode[1]))
>> +                     clk_enable(omap->usbtll_p2_fck);
>> +             clk_disable(omap->utmi_p2_fck);
>> +             clk_disable(omap->utmi_p1_fck);
>>       }
>>
>> -     clk_disable(omap->utmi_p2_fck);
>> -     clk_disable(omap->utmi_p1_fck);
>>       clk_disable(omap->usbtll_ick);
>>       clk_disable(omap->usbtll_fck);
>>       clk_disable(omap->usbhost_fs_fck);
>>       clk_disable(omap->usbhost_hs_fck);
>>       clk_disable(omap->usbhost_ick);
>> +     halt = 1;
>
> at least from this diff, you're always reaching that part of the code
> rendering this halt flag useless.

I you see only the patch ; its looks like variable halt is not needed;

If the code; it will be set only when the clocks are disabled;
Then after restoring irq, you will free the gpio based on this value.

The complete usbhs_disable code is follows:
static void usbhs_disable(struct device *dev)
{
	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
	unsigned long			flags = 0;
	unsigned int			halt = 0;
	unsigned long			timeout;

	dev_dbg(dev, "stopping TI HSUSB Controller\n");

	spin_lock_irqsave(&omap->lock, flags);

	if (omap->count == 0)
		goto end_disble;

	omap->count--;

	if (omap->count != 0)
		goto end_disble;

	/* Reset OMAP modules for insmod/rmmod to work */
	usbhs_write(omap->uhh_base, OMAP_UHH_SYSCONFIG,
			is_omap_usbhs_rev2(omap) ?
			OMAP4_UHH_SYSCONFIG_SOFTRESET :
			OMAP_UHH_SYSCONFIG_SOFTRESET);

	timeout = jiffies + msecs_to_jiffies(100);
	while (!(usbhs_read(omap->uhh_base, OMAP_UHH_SYSSTATUS)
				& (1 << 0))) {
		cpu_relax();

		if (time_after(jiffies, timeout))
			dev_dbg(dev, "operation timed out\n");
	}

	while (!(usbhs_read(omap->uhh_base, OMAP_UHH_SYSSTATUS)
				& (1 << 1))) {
		cpu_relax();

		if (time_after(jiffies, timeout))
			dev_dbg(dev, "operation timed out\n");
	}

	while (!(usbhs_read(omap->uhh_base, OMAP_UHH_SYSSTATUS)
				& (1 << 2))) {
		cpu_relax();

		if (time_after(jiffies, timeout))
			dev_dbg(dev, "operation timed out\n");
	}

	usbhs_write(omap->tll_base, OMAP_USBTLL_SYSCONFIG, (1 << 1));

	while (!(usbhs_read(omap->tll_base, OMAP_USBTLL_SYSSTATUS)
				& (1 << 0))) {
		cpu_relax();

		if (time_after(jiffies, timeout))
			dev_dbg(dev, "operation timed out\n");
	}

	if (is_omap_usbhs_rev2(omap)) {
		if (is_ehci_tll_mode(pdata->port_mode[0]))
			clk_enable(omap->usbtll_p1_fck);
		if (is_ehci_tll_mode(pdata->port_mode[1]))
			clk_enable(omap->usbtll_p2_fck);
		clk_disable(omap->utmi_p2_fck);
		clk_disable(omap->utmi_p1_fck);
	}

	clk_disable(omap->usbtll_ick);
	clk_disable(omap->usbtll_fck);
	clk_disable(omap->usbhost_fs_fck);
	clk_disable(omap->usbhost_hs_fck);
	clk_disable(omap->usbhost_ick);
	halt = 1;

end_disble:
	spin_unlock_irqrestore(&omap->lock, flags);
	if (halt && pdata->ehci_data->phy_reset) {
		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
			gpio_free(pdata->ehci_data->reset_gpio_port[0]);

		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
			gpio_free(pdata->ehci_data->reset_gpio_port[1]);
	}
}




>
> --
> balbi
>
--
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

Re: [PATCH] omap: usbhs: Fixed the crash during rmmod of ehci and ohci

[Felipe Balbi]

Hi,

On Mon, May 09, 2011 at 03:58:53PM +0530, Munegowda, Keshava wrote:
> I you see only the patch ; its looks like variable halt is not needed;
> 
> If the code; it will be set only when the clocks are disabled;
> Then after restoring irq, you will free the gpio based on this value.

that code is wrong in so many ways that it's difficult to reply, but in
short:

. get rid of the magic constants, define some macros
. instead of using only one error label, use several each one taking
	care of a different case (that will allow you to remove halt flag)
. timeout is never reset, meaning after the first loop, the time_after()
	will always be true.
. this omap->count is ugly.
. you shouldn't be accessing pdata on the exit path
. that save in spin_lock_irqsave() looks unnecessary. Wouldn't
	spin_lock_irq() be enough ?

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] omap: usbhs: Fixed the crash during rmmod of ehci and ohci

[Keshava Munegowda]

From: Keshava Munegowda <Keshava_mgowda@ti.com>

The disableing of clocks and freeing GPIO are changed
to fix the occurence of the crash of rmmod of ehci and ohci
drivers. The GPIOs should be freed after the spin locks are
unlocked.

Signed-off-by: Keshava Munegowda <keshava_mgowda@ti.com>
---
 drivers/mfd/omap-usb-host.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index 3ab9ffa..55c5d47 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -994,22 +994,33 @@ static void usbhs_disable(struct device *dev)
 			dev_dbg(dev, "operation timed out\n");
 	}
 
-	if (pdata->ehci_data->phy_reset) {
-		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
-			gpio_free(pdata->ehci_data->reset_gpio_port[0]);
-
-		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
-			gpio_free(pdata->ehci_data->reset_gpio_port[1]);
+	if (is_omap_usbhs_rev2(omap)) {
+		if (is_ehci_tll_mode(pdata->port_mode[0]))
+			clk_enable(omap->usbtll_p1_fck);
+		if (is_ehci_tll_mode(pdata->port_mode[1]))
+			clk_enable(omap->usbtll_p2_fck);
+		clk_disable(omap->utmi_p2_fck);
+		clk_disable(omap->utmi_p1_fck);
 	}
 
-	clk_disable(omap->utmi_p2_fck);
-	clk_disable(omap->utmi_p1_fck);
 	clk_disable(omap->usbtll_ick);
 	clk_disable(omap->usbtll_fck);
 	clk_disable(omap->usbhost_fs_fck);
 	clk_disable(omap->usbhost_hs_fck);
 	clk_disable(omap->usbhost_ick);
 
+	/* The gpio_free migh sleep; so unlock the spinlock */
+	spin_unlock_irqrestore(&omap->lock, flags);
+
+	if (pdata->ehci_data->phy_reset) {
+		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
+			gpio_free(pdata->ehci_data->reset_gpio_port[0]);
+
+		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
+			gpio_free(pdata->ehci_data->reset_gpio_port[1]);
+	}
+	return;
+
 end_disble:
 	spin_unlock_irqrestore(&omap->lock, flags);
 }
-- 
1.6.0.4