[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Marek Vasut]

In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will cause an
ugly kernel crash (NULL pointer dereference in pxa3xx_u2d_start_hc(), because
struct u2d is NULL and clk_enable() call will crash the kernel, trying to access
it).

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 arch/arm/mach-pxa/pxa3xx-ulpi.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c b/arch/arm/mach-pxa/pxa3xx-ulpi.c
index e57439e..ce7168b 100644
--- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
+++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
@@ -252,6 +252,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
 {
 	int err = 0;
 
+	/* In case the PXA3xx ULPI isn't used, do nothing. */
+	if (!u2d)
+		return 0;
+
 	clk_enable(u2d->clk);
 
 	if (cpu_is_pxa310()) {
@@ -264,6 +268,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
 
 void pxa3xx_u2d_stop_hc(struct usb_bus *host)
 {
+	/* In case the PXA3xx ULPI isn't used, do nothing. */
+	if (!u2d)
+		return;
+
 	if (cpu_is_pxa310())
 		pxa310_stop_otg_hc();
 
-- 
1.7.1

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Eric Miao]

On Sat, Sep 4, 2010 at 4:35 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will cause an
> ugly kernel crash (NULL pointer dereference in pxa3xx_u2d_start_hc(), because
> struct u2d is NULL and clk_enable() call will crash the kernel, trying to access
> it).
>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>

Applied to 'devel'.

> ---
> ?arch/arm/mach-pxa/pxa3xx-ulpi.c | ? ?8 ++++++++
> ?1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c b/arch/arm/mach-pxa/pxa3xx-ulpi.c
> index e57439e..ce7168b 100644
> --- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
> +++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
> @@ -252,6 +252,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
> ?{
> ? ? ? ?int err = 0;
>
> + ? ? ? /* In case the PXA3xx ULPI isn't used, do nothing. */
> + ? ? ? if (!u2d)
> + ? ? ? ? ? ? ? return 0;
> +
> ? ? ? ?clk_enable(u2d->clk);
>
> ? ? ? ?if (cpu_is_pxa310()) {
> @@ -264,6 +268,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
>
> ?void pxa3xx_u2d_stop_hc(struct usb_bus *host)
> ?{
> + ? ? ? /* In case the PXA3xx ULPI isn't used, do nothing. */
> + ? ? ? if (!u2d)
> + ? ? ? ? ? ? ? return;
> +
> ? ? ? ?if (cpu_is_pxa310())
> ? ? ? ? ? ? ? ?pxa310_stop_otg_hc();
>
> --
> 1.7.1
>
>

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Igor Grinberg]

 On 09/03/10 23:35, Marek Vasut wrote:
> In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will cause an
> ugly kernel crash (NULL pointer dereference in pxa3xx_u2d_start_hc(), because
> struct u2d is NULL and clk_enable() call will crash the kernel, trying to access
> it).

ohci code checks for pxa3xx cpu and only then runs start/stop hc.
pxa3xx_ulpi.c is compiled if CONFIG_PXA3xx is selected.
The device <-> driver binding should not be a problem, so the
pxa3xx_u2d_probe() will run.
The only case, I see, when struct u2d does not exist is failure of the
probe function. If this is the case, we are having an abnormal execution
and if your patch is dealing with this issue, shouldn't you comment it as such?

> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> ---
>  arch/arm/mach-pxa/pxa3xx-ulpi.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c b/arch/arm/mach-pxa/pxa3xx-ulpi.c
> index e57439e..ce7168b 100644
> --- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
> +++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
> @@ -252,6 +252,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
>  {
>  	int err = 0;
>  
> +	/* In case the PXA3xx ULPI isn't used, do nothing. */
> +	if (!u2d)
> +		return 0;
> +
>  	clk_enable(u2d->clk);
>  
>  	if (cpu_is_pxa310()) {
> @@ -264,6 +268,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
>  
>  void pxa3xx_u2d_stop_hc(struct usb_bus *host)
>  {
> +	/* In case the PXA3xx ULPI isn't used, do nothing. */
> +	if (!u2d)
> +		return;
> +
>  	if (cpu_is_pxa310())
>  		pxa310_stop_otg_hc();
>  

-- 
Regards,
Igor.

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Marek Vasut]

Dne Ne 5. z??? 2010 09:54:31 Igor Grinberg napsal(a):
>  On 09/03/10 23:35, Marek Vasut wrote:
> > In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will
> > cause an ugly kernel crash (NULL pointer dereference in
> > pxa3xx_u2d_start_hc(), because struct u2d is NULL and clk_enable() call
> > will crash the kernel, trying to access it).
> 
> ohci code checks for pxa3xx cpu and only then runs start/stop hc.

Exactly ... and in case "struct pxa3xx_u2d_ulpi *u2d" is NULL, clk_enable will 
crash the kernel.

> pxa3xx_ulpi.c is compiled if CONFIG_PXA3xx is selected.
> The device <-> driver binding should not be a problem, so the
> pxa3xx_u2d_probe() will run.
> The only case, I see, when struct u2d does not exist is failure of the
> probe function. If this is the case, we are having an abnormal execution
> and if your patch is dealing with this issue, shouldn't you comment it as
> such?

Not at all ... if the pxa3xx-u2d driver isn't loaded at all, the function 
(start/stop hc) is still called, but struct pxa3xx_u2d_ulpi *u2d is NULL. In 
this case, if you call clk_enable(u2d->clk), you crash the kernel (because u2d 
is NULL).

Good night, I'll be back in 8 hrs or so :)

> 
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > ---
> > 
> >  arch/arm/mach-pxa/pxa3xx-ulpi.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c
> > b/arch/arm/mach-pxa/pxa3xx-ulpi.c index e57439e..ce7168b 100644
> > --- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
> > +++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
> > @@ -252,6 +252,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
> > 
> >  {
> >  
> >  	int err = 0;
> > 
> > +	/* In case the PXA3xx ULPI isn't used, do nothing. */
> > +	if (!u2d)
> > +		return 0;
> > +
> > 
> >  	clk_enable(u2d->clk);
> >  	
> >  	if (cpu_is_pxa310()) {
> > 
> > @@ -264,6 +268,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
> > 
> >  void pxa3xx_u2d_stop_hc(struct usb_bus *host)
> >  {
> > 
> > +	/* In case the PXA3xx ULPI isn't used, do nothing. */
> > +	if (!u2d)
> > +		return;
> > +
> > 
> >  	if (cpu_is_pxa310())
> >  	
> >  		pxa310_stop_otg_hc();

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Igor Grinberg]

 On 09/05/10 11:01, Marek Vasut wrote:
> Dne Ne 5. z??? 2010 09:54:31 Igor Grinberg napsal(a):
>>  On 09/03/10 23:35, Marek Vasut wrote:
>>> In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will
>>> cause an ugly kernel crash (NULL pointer dereference in
>>> pxa3xx_u2d_start_hc(), because struct u2d is NULL and clk_enable() call
>>> will crash the kernel, trying to access it).
>> ohci code checks for pxa3xx cpu and only then runs start/stop hc.
> Exactly ... and in case "struct pxa3xx_u2d_ulpi *u2d" is NULL, clk_enable will 
> crash the kernel.
>
>> pxa3xx_ulpi.c is compiled if CONFIG_PXA3xx is selected.
>> The device <-> driver binding should not be a problem, so the
>> pxa3xx_u2d_probe() will run.
>> The only case, I see, when struct u2d does not exist is failure of the
>> probe function. If this is the case, we are having an abnormal execution
>> and if your patch is dealing with this issue, shouldn't you comment it as
>> such?
> Not at all ... if the pxa3xx-u2d driver isn't loaded at all, the function 
> (start/stop hc) is still called, but struct pxa3xx_u2d_ulpi *u2d is NULL. In 
> this case, if you call clk_enable(u2d->clk), you crash the kernel (because u2d 
> is NULL).

How, can it happen, that "pxa3xx-u2d driver isn't loaded at all"?
This can happen only if you rip out the device registration or hack a Makefile.
I don't see any other way... is there?

> Good night, I'll be back in 8 hrs or so :)
>
>>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>>> ---
>>>
>>>  arch/arm/mach-pxa/pxa3xx-ulpi.c |    8 ++++++++
>>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c
>>> b/arch/arm/mach-pxa/pxa3xx-ulpi.c index e57439e..ce7168b 100644
>>> --- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
>>> +++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
>>> @@ -252,6 +252,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
>>>
>>>  {
>>>  
>>>  	int err = 0;
>>>
>>> +	/* In case the PXA3xx ULPI isn't used, do nothing. */
>>> +	if (!u2d)
>>> +		return 0;
>>> +
>>>
>>>  	clk_enable(u2d->clk);
>>>  	
>>>  	if (cpu_is_pxa310()) {
>>>
>>> @@ -264,6 +268,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
>>>
>>>  void pxa3xx_u2d_stop_hc(struct usb_bus *host)
>>>  {
>>>
>>> +	/* In case the PXA3xx ULPI isn't used, do nothing. */
>>> +	if (!u2d)
>>> +		return;
>>> +
>>>
>>>  	if (cpu_is_pxa310())
>>>  	
>>>  		pxa310_stop_otg_hc();

-- 
Regards,
Igor.

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Marek Vasut]

Dne Ne 5. z??? 2010 10:16:48 Igor Grinberg napsal(a):
>  On 09/05/10 11:01, Marek Vasut wrote:
> > Dne Ne 5. z??? 2010 09:54:31 Igor Grinberg napsal(a):
> >>  On 09/03/10 23:35, Marek Vasut wrote:
> >>> In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will
> >>> cause an ugly kernel crash (NULL pointer dereference in
> >>> pxa3xx_u2d_start_hc(), because struct u2d is NULL and clk_enable() call
> >>> will crash the kernel, trying to access it).
> >> 
> >> ohci code checks for pxa3xx cpu and only then runs start/stop hc.
> > 
> > Exactly ... and in case "struct pxa3xx_u2d_ulpi *u2d" is NULL, clk_enable
> > will crash the kernel.
> > 
> >> pxa3xx_ulpi.c is compiled if CONFIG_PXA3xx is selected.
> >> The device <-> driver binding should not be a problem, so the
> >> pxa3xx_u2d_probe() will run.
> >> The only case, I see, when struct u2d does not exist is failure of the
> >> probe function. If this is the case, we are having an abnormal execution
> >> and if your patch is dealing with this issue, shouldn't you comment it
> >> as such?
> > 
> > Not at all ... if the pxa3xx-u2d driver isn't loaded at all, the function
> > (start/stop hc) is still called, but struct pxa3xx_u2d_ulpi *u2d is NULL.
> > In this case, if you call clk_enable(u2d->clk), you crash the kernel
> > (because u2d is NULL).
> 
> How, can it happen, that "pxa3xx-u2d driver isn't loaded at all"?
> This can happen only if you rip out the device registration or hack a
> Makefile. I don't see any other way... is there?

If you don't call pxa3xx_set_u2d_info() ?
> 
> > Good night, I'll be back in 8 hrs or so :)
> > 
> >>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> >>> ---
> >>> 
> >>>  arch/arm/mach-pxa/pxa3xx-ulpi.c |    8 ++++++++
> >>>  1 files changed, 8 insertions(+), 0 deletions(-)
> >>> 
> >>> diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c
> >>> b/arch/arm/mach-pxa/pxa3xx-ulpi.c index e57439e..ce7168b 100644
> >>> --- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
> >>> +++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
> >>> @@ -252,6 +252,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
> >>> 
> >>>  {
> >>>  
> >>>  	int err = 0;
> >>> 
> >>> +	/* In case the PXA3xx ULPI isn't used, do nothing. */
> >>> +	if (!u2d)
> >>> +		return 0;
> >>> +
> >>> 
> >>>  	clk_enable(u2d->clk);
> >>>  	
> >>>  	if (cpu_is_pxa310()) {
> >>> 
> >>> @@ -264,6 +268,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
> >>> 
> >>>  void pxa3xx_u2d_stop_hc(struct usb_bus *host)
> >>>  {
> >>> 
> >>> +	/* In case the PXA3xx ULPI isn't used, do nothing. */
> >>> +	if (!u2d)
> >>> +		return;
> >>> +
> >>> 
> >>>  	if (cpu_is_pxa310())
> >>>  	
> >>>  		pxa310_stop_otg_hc();

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Igor Grinberg]

 On 09/05/10 11:25, Marek Vasut wrote:
> Dne Ne 5. z??? 2010 10:16:48 Igor Grinberg napsal(a):
>>  On 09/05/10 11:01, Marek Vasut wrote:
>>> Dne Ne 5. z??? 2010 09:54:31 Igor Grinberg napsal(a):
>>>>  On 09/03/10 23:35, Marek Vasut wrote:
>>>>> In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will
>>>>> cause an ugly kernel crash (NULL pointer dereference in
>>>>> pxa3xx_u2d_start_hc(), because struct u2d is NULL and clk_enable() call
>>>>> will crash the kernel, trying to access it).
>>>> ohci code checks for pxa3xx cpu and only then runs start/stop hc.
>>> Exactly ... and in case "struct pxa3xx_u2d_ulpi *u2d" is NULL, clk_enable
>>> will crash the kernel.
>>>
>>>> pxa3xx_ulpi.c is compiled if CONFIG_PXA3xx is selected.
>>>> The device <-> driver binding should not be a problem, so the
>>>> pxa3xx_u2d_probe() will run.
>>>> The only case, I see, when struct u2d does not exist is failure of the
>>>> probe function. If this is the case, we are having an abnormal execution
>>>> and if your patch is dealing with this issue, shouldn't you comment it
>>>> as such?
>>> Not at all ... if the pxa3xx-u2d driver isn't loaded at all, the function
>>> (start/stop hc) is still called, but struct pxa3xx_u2d_ulpi *u2d is NULL.
>>> In this case, if you call clk_enable(u2d->clk), you crash the kernel
>>> (because u2d is NULL).
>> How, can it happen, that "pxa3xx-u2d driver isn't loaded at all"?
>> This can happen only if you rip out the device registration or hack a
>> Makefile. I don't see any other way... is there?
> If you don't call pxa3xx_set_u2d_info() ?

Oh... right.
I've added it this way, so boards can control u2d existence and forgot it is there...
Buggy me... :(

Thanks.

>>> Good night, I'll be back in 8 hrs or so :)

Good night indeed :)

>>>>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>>>>> ---
>>>>>
>>>>>  arch/arm/mach-pxa/pxa3xx-ulpi.c |    8 ++++++++
>>>>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c
>>>>> b/arch/arm/mach-pxa/pxa3xx-ulpi.c index e57439e..ce7168b 100644
>>>>> --- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
>>>>> +++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
>>>>> @@ -252,6 +252,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
>>>>>
>>>>>  {
>>>>>  
>>>>>  	int err = 0;
>>>>>
>>>>> +	/* In case the PXA3xx ULPI isn't used, do nothing. */
>>>>> +	if (!u2d)
>>>>> +		return 0;
>>>>> +
>>>>>
>>>>>  	clk_enable(u2d->clk);
>>>>>  	
>>>>>  	if (cpu_is_pxa310()) {
>>>>>
>>>>> @@ -264,6 +268,10 @@ int pxa3xx_u2d_start_hc(struct usb_bus *host)
>>>>>
>>>>>  void pxa3xx_u2d_stop_hc(struct usb_bus *host)
>>>>>  {
>>>>>
>>>>> +	/* In case the PXA3xx ULPI isn't used, do nothing. */
>>>>> +	if (!u2d)
>>>>> +		return;
>>>>> +
>>>>>
>>>>>  	if (cpu_is_pxa310())
>>>>>  	
>>>>>  		pxa310_stop_otg_hc();

-- 
Regards,
Igor.

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Eric Miao]

On Sun, Sep 5, 2010 at 4:35 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> ?On 09/05/10 11:25, Marek Vasut wrote:
>> Dne Ne 5. z??? 2010 10:16:48 Igor Grinberg napsal(a):
>>> ?On 09/05/10 11:01, Marek Vasut wrote:
>>>> Dne Ne 5. z??? 2010 09:54:31 Igor Grinberg napsal(a):
>>>>> ?On 09/03/10 23:35, Marek Vasut wrote:
>>>>>> In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will
>>>>>> cause an ugly kernel crash (NULL pointer dereference in
>>>>>> pxa3xx_u2d_start_hc(), because struct u2d is NULL and clk_enable() call
>>>>>> will crash the kernel, trying to access it).
>>>>> ohci code checks for pxa3xx cpu and only then runs start/stop hc.
>>>> Exactly ... and in case "struct pxa3xx_u2d_ulpi *u2d" is NULL, clk_enable
>>>> will crash the kernel.
>>>>
>>>>> pxa3xx_ulpi.c is compiled if CONFIG_PXA3xx is selected.
>>>>> The device <-> driver binding should not be a problem, so the
>>>>> pxa3xx_u2d_probe() will run.
>>>>> The only case, I see, when struct u2d does not exist is failure of the
>>>>> probe function. If this is the case, we are having an abnormal execution
>>>>> and if your patch is dealing with this issue, shouldn't you comment it
>>>>> as such?
>>>> Not at all ... if the pxa3xx-u2d driver isn't loaded at all, the function
>>>> (start/stop hc) is still called, but struct pxa3xx_u2d_ulpi *u2d is NULL.
>>>> In this case, if you call clk_enable(u2d->clk), you crash the kernel
>>>> (because u2d is NULL).
>>> How, can it happen, that "pxa3xx-u2d driver isn't loaded at all"?
>>> This can happen only if you rip out the device registration or hack a
>>> Makefile. I don't see any other way... is there?
>> If you don't call pxa3xx_set_u2d_info() ?
>
> Oh... right.
> I've added it this way, so boards can control u2d existence and forgot it is there...
> Buggy me... :(
>

Igor,

So do you this as a proper fix, or there is better way out?

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Igor Grinberg]

 On 09/05/10 13:43, Eric Miao wrote:
> On Sun, Sep 5, 2010 at 4:35 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>  On 09/05/10 11:25, Marek Vasut wrote:
>>> Dne Ne 5. z??? 2010 10:16:48 Igor Grinberg napsal(a):
>>>>  On 09/05/10 11:01, Marek Vasut wrote:
>>>>> Dne Ne 5. z??? 2010 09:54:31 Igor Grinberg napsal(a):
>>>>>>  On 09/03/10 23:35, Marek Vasut wrote:
>>>>>>> In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will
>>>>>>> cause an ugly kernel crash (NULL pointer dereference in
>>>>>>> pxa3xx_u2d_start_hc(), because struct u2d is NULL and clk_enable() call
>>>>>>> will crash the kernel, trying to access it).
>>>>>> ohci code checks for pxa3xx cpu and only then runs start/stop hc.
>>>>> Exactly ... and in case "struct pxa3xx_u2d_ulpi *u2d" is NULL, clk_enable
>>>>> will crash the kernel.
>>>>>
>>>>>> pxa3xx_ulpi.c is compiled if CONFIG_PXA3xx is selected.
>>>>>> The device <-> driver binding should not be a problem, so the
>>>>>> pxa3xx_u2d_probe() will run.
>>>>>> The only case, I see, when struct u2d does not exist is failure of the
>>>>>> probe function. If this is the case, we are having an abnormal execution
>>>>>> and if your patch is dealing with this issue, shouldn't you comment it
>>>>>> as such?
>>>>> Not at all ... if the pxa3xx-u2d driver isn't loaded at all, the function
>>>>> (start/stop hc) is still called, but struct pxa3xx_u2d_ulpi *u2d is NULL.
>>>>> In this case, if you call clk_enable(u2d->clk), you crash the kernel
>>>>> (because u2d is NULL).
>>>> How, can it happen, that "pxa3xx-u2d driver isn't loaded at all"?
>>>> This can happen only if you rip out the device registration or hack a
>>>> Makefile. I don't see any other way... is there?
>>> If you don't call pxa3xx_set_u2d_info() ?
>> Oh... right.
>> I've added it this way, so boards can control u2d existence and forgot it is there...
>> Buggy me... :(
>>
> Igor,
>
> So do you this as a proper fix, or there is better way out?

If we want to keep it the most straight-forward way so it is fine.

Another possible ways would be:

1) create a new flag, lets say PORT2_USE_U2DC in pxaohci_platform_data.
This is a relatively clean way of making ohci aware of
u2d existence at runtime and eliminates the calls to functions
of non-existing (not loaded) driver...

2) use something like:
struct u2d_hc_ops {
    int (*start_hc)(...);
    void (*stop_hc)(...);
}

in board init code register the ops structure via the pxa_ohci platform data.

This way adds some more pollution to the pxa_ohci glue with code
relevant only to pxa3xx, achieving the same as the 1st way, but also
can be useful for u2dc otg/peripheral driver (if it will come some day...).

I'm fine with both (Marek's patch or my proposal), so tell me what
looks better to you.

-- 
Regards,
Igor.

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Eric Miao]

On Sun, Sep 5, 2010 at 9:46 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> ?On 09/05/10 13:43, Eric Miao wrote:
>> On Sun, Sep 5, 2010 at 4:35 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>> ?On 09/05/10 11:25, Marek Vasut wrote:
>>>> Dne Ne 5. z??? 2010 10:16:48 Igor Grinberg napsal(a):
>>>>> ?On 09/05/10 11:01, Marek Vasut wrote:
>>>>>> Dne Ne 5. z??? 2010 09:54:31 Igor Grinberg napsal(a):
>>>>>>> ?On 09/03/10 23:35, Marek Vasut wrote:
>>>>>>>> In case the pxa3xx-u2d driver isn't used, probing of ohci-pxa27x will
>>>>>>>> cause an ugly kernel crash (NULL pointer dereference in
>>>>>>>> pxa3xx_u2d_start_hc(), because struct u2d is NULL and clk_enable() call
>>>>>>>> will crash the kernel, trying to access it).
>>>>>>> ohci code checks for pxa3xx cpu and only then runs start/stop hc.
>>>>>> Exactly ... and in case "struct pxa3xx_u2d_ulpi *u2d" is NULL, clk_enable
>>>>>> will crash the kernel.
>>>>>>
>>>>>>> pxa3xx_ulpi.c is compiled if CONFIG_PXA3xx is selected.
>>>>>>> The device <-> driver binding should not be a problem, so the
>>>>>>> pxa3xx_u2d_probe() will run.
>>>>>>> The only case, I see, when struct u2d does not exist is failure of the
>>>>>>> probe function. If this is the case, we are having an abnormal execution
>>>>>>> and if your patch is dealing with this issue, shouldn't you comment it
>>>>>>> as such?
>>>>>> Not at all ... if the pxa3xx-u2d driver isn't loaded at all, the function
>>>>>> (start/stop hc) is still called, but struct pxa3xx_u2d_ulpi *u2d is NULL.
>>>>>> In this case, if you call clk_enable(u2d->clk), you crash the kernel
>>>>>> (because u2d is NULL).
>>>>> How, can it happen, that "pxa3xx-u2d driver isn't loaded at all"?
>>>>> This can happen only if you rip out the device registration or hack a
>>>>> Makefile. I don't see any other way... is there?
>>>> If you don't call pxa3xx_set_u2d_info() ?
>>> Oh... right.
>>> I've added it this way, so boards can control u2d existence and forgot it is there...
>>> Buggy me... :(
>>>
>> Igor,
>>
>> So do you this as a proper fix, or there is better way out?
>
> If we want to keep it the most straight-forward way so it is fine.
>
> Another possible ways would be:
>
> 1) create a new flag, lets say PORT2_USE_U2DC in pxaohci_platform_data.
> This is a relatively clean way of making ohci aware of
> u2d existence at runtime and eliminates the calls to functions
> of non-existing (not loaded) driver...
>
> 2) use something like:
> struct u2d_hc_ops {
> ? ?int (*start_hc)(...);
> ? ?void (*stop_hc)(...);
> }
>
> in board init code register the ops structure via the pxa_ohci platform data.
>
> This way adds some more pollution to the pxa_ohci glue with code
> relevant only to pxa3xx, achieving the same as the 1st way, but also
> can be useful for u2dc otg/peripheral driver (if it will come some day...).
>
> I'm fine with both (Marek's patch or my proposal), so tell me what
> looks better to you.
>

That sounds to be a cleaner way, but may require more changes. What
if we get this patch into -rc phase as is first, and get a cleaner patch for
-next?

> --
> Regards,
> Igor.
>
>

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Igor Grinberg]

 On 09/05/10 16:58, Eric Miao wrote:
> On Sun, Sep 5, 2010 at 9:46 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>  On 09/05/10 13:43, Eric Miao wrote:
>>> On Sun, Sep 5, 2010 at 4:35 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>> Igor,
>>>
>>> So do you this as a proper fix, or there is better way out?
>> If we want to keep it the most straight-forward way so it is fine.
>>
>> Another possible ways would be:
>>
>> 1) create a new flag, lets say PORT2_USE_U2DC in pxaohci_platform_data.
>> This is a relatively clean way of making ohci aware of
>> u2d existence at runtime and eliminates the calls to functions
>> of non-existing (not loaded) driver...
>>
>> 2) use something like:
>> struct u2d_hc_ops {
>>    int (*start_hc)(...);
>>    void (*stop_hc)(...);
>> }
>>
>> in board init code register the ops structure via the pxa_ohci platform data.
>>
>> This way adds some more pollution to the pxa_ohci glue with code
>> relevant only to pxa3xx, achieving the same as the 1st way, but also
>> can be useful for u2dc otg/peripheral driver (if it will come some day...).
>>
>> I'm fine with both (Marek's patch or my proposal), so tell me what
>> looks better to you.
>>
> That sounds to be a cleaner way, but may require more changes. What
> if we get this patch into -rc phase as is first, and get a cleaner patch for
> -next?

Fine with me.

-- 
Regards,
Igor.

[PATCH] ARM: pxa: Fix pxa3xx-u2d crash when ULPI not used

[Marek Vasut]

Dne Ne 5. z??? 2010 16:31:53 Igor Grinberg napsal(a):
>  On 09/05/10 16:58, Eric Miao wrote:
> > On Sun, Sep 5, 2010 at 9:46 PM, Igor Grinberg <grinberg@compulab.co.il> 
wrote:
> >>  On 09/05/10 13:43, Eric Miao wrote:
> >>> On Sun, Sep 5, 2010 at 4:35 PM, Igor Grinberg <grinberg@compulab.co.il>
> >>> wrote: Igor,
> >>> 
> >>> So do you this as a proper fix, or there is better way out?
> >> 
> >> If we want to keep it the most straight-forward way so it is fine.
> >> 
> >> Another possible ways would be:
> >> 
> >> 1) create a new flag, lets say PORT2_USE_U2DC in pxaohci_platform_data.
> >> This is a relatively clean way of making ohci aware of
> >> u2d existence at runtime and eliminates the calls to functions
> >> of non-existing (not loaded) driver...
> >> 
> >> 2) use something like:
> >> struct u2d_hc_ops {
> >> 
> >>    int (*start_hc)(...);
> >>    void (*stop_hc)(...);
> >> 
> >> }
> >> 
> >> in board init code register the ops structure via the pxa_ohci platform
> >> data.
> >> 
> >> This way adds some more pollution to the pxa_ohci glue with code
> >> relevant only to pxa3xx, achieving the same as the 1st way, but also
> >> can be useful for u2dc otg/peripheral driver (if it will come some
> >> day...).
> >> 
> >> I'm fine with both (Marek's patch or my proposal), so tell me what
> >> looks better to you.
> > 
> > That sounds to be a cleaner way, but may require more changes. What
> > if we get this patch into -rc phase as is first, and get a cleaner patch
> > for -next?
> 
> Fine with me.

Morning (even if it's 9pm here).

I'd be for solution #2 as well. I might eventually look into it, but now it's 
really impossible for me. Just two more weeks until I'll be able to activelly 
participate on kernel development again.

Cheers