[U-Boot] [UBOOT PATCH] usb: ehci-hcd: Fix crash when rootdev not initialized

[U-Boot] [UBOOT PATCH] usb: ehci-hcd: Fix crash when rootdev not initialized

[Siva Durga Prasad Paladugu]

This patch fixes the issue on zynq USB failure with DM
when rootdev is not initialized. This variable is initalized
to zero in non driver model case but not in DM.

Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
---
- Tested on Zynq ZC702 board with USB DM driver model patches
  sent by Simon.
---
 drivers/usb/host/ehci-hcd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 13aa70d..8adffa6 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1624,6 +1624,8 @@ int ehci_register(struct udevice *dev, struct ehci_hccr *hccr,
 			goto err;
 	}
 
+	ctrl->rootdev = 0;
+
 	ret = ehci_common_init(ctrl, tweaks);
 	if (ret)
 		goto err;
-- 
2.1.1

[U-Boot] [UBOOT PATCH] usb: ehci-hcd: Fix crash when rootdev not initialized

[Hans de Goede]

Hi,

On 27-06-16 10:59, Siva Durga Prasad Paladugu wrote:
> This patch fixes the issue on zynq USB failure with DM
> when rootdev is not initialized. This variable is initalized
> to zero in non driver model case but not in DM.
>
> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> ---
> - Tested on Zynq ZC702 board with USB DM driver model patches
>   sent by Simon.

This patch does not seem like the right fix, normally something
like the ehci_ctrl struct would be allocated by dm by setting
priv_auto_alloc_size in the driver description, and then the
data will get calloc-ed. If you're allocating the ehci_ctrl struct
differently and not memsetting it to 0 you may have other bugs
lurking, or may get new bugs when new members get added in the
future.

My advice would be to use priv_auto_alloc_size, see .e.g :
drivers/usb/host/ehci-sunxi.c

If you cannot use that for some reason, make sure to memset
struct ehci_Ctrl to 0 before calling ehci_register()

Regards,

Hans



> ---
>  drivers/usb/host/ehci-hcd.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 13aa70d..8adffa6 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -1624,6 +1624,8 @@ int ehci_register(struct udevice *dev, struct ehci_hccr *hccr,
>  			goto err;
>  	}
>
> +	ctrl->rootdev = 0;
> +
>  	ret = ehci_common_init(ctrl, tweaks);
>  	if (ret)
>  		goto err;
>

[U-Boot] [UBOOT PATCH] usb: ehci-hcd: Fix crash when rootdev not initialized

[Siva Durga Prasad Paladugu]

Hi,

> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede at redhat.com]
> Sent: Monday, June 27, 2016 4:35 PM
> To: Siva Durga Prasad Paladugu <sivadur@xilinx.com>; u-boot at lists.denx.de
> Cc: marex at denx.de; Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> Subject: Re: [U-Boot] [UBOOT PATCH] usb: ehci-hcd: Fix crash when rootdev not
> initialized
> 
> Hi,
> 
> On 27-06-16 10:59, Siva Durga Prasad Paladugu wrote:
> > This patch fixes the issue on zynq USB failure with DM when rootdev is
> > not initialized. This variable is initalized to zero in non driver
> > model case but not in DM.
> >
> > Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> > ---
> > - Tested on Zynq ZC702 board with USB DM driver model patches
> >   sent by Simon.
> 
> This patch does not seem like the right fix, normally something like the ehci_ctrl
> struct would be allocated by dm by setting priv_auto_alloc_size in the driver
> description, and then the data will get calloc-ed. If you're allocating the ehci_ctrl
> struct differently and not memsetting it to 0 you may have other bugs lurking, or
> may get new bugs when new members get added in the future.
This is also fine for me, no Problem.
I will reply to Simon on his patch of Zynq USB DM and will ask him to do so.

Thanks,
Siva

> 
> My advice would be to use priv_auto_alloc_size, see .e.g :
> drivers/usb/host/ehci-sunxi.c
> 
> If you cannot use that for some reason, make sure to memset struct ehci_Ctrl to
> 0 before calling ehci_register()
> 
> Regards,
> 
> Hans
> 
> 
> 
> > ---
> >  drivers/usb/host/ehci-hcd.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> > index 13aa70d..8adffa6 100644
> > --- a/drivers/usb/host/ehci-hcd.c
> > +++ b/drivers/usb/host/ehci-hcd.c
> > @@ -1624,6 +1624,8 @@ int ehci_register(struct udevice *dev, struct
> ehci_hccr *hccr,
> >  			goto err;
> >  	}
> >
> > +	ctrl->rootdev = 0;
> > +
> >  	ret = ehci_common_init(ctrl, tweaks);
> >  	if (ret)
> >  		goto err;
> >

[U-Boot] [UBOOT PATCH] usb: ehci-hcd: Fix crash when rootdev not initialized

[Simon Glass]

Hi,

On 27 June 2016 at 23:06, Siva Durga Prasad Paladugu
<siva.durga.paladugu@xilinx.com> wrote:
>
> Hi,
>
>> -----Original Message-----
>> From: Hans de Goede [mailto:hdegoede at redhat.com]
>> Sent: Monday, June 27, 2016 4:35 PM
>> To: Siva Durga Prasad Paladugu <sivadur@xilinx.com>; u-boot at lists.denx.de
>> Cc: marex at denx.de; Siva Durga Prasad Paladugu <sivadur@xilinx.com>
>> Subject: Re: [U-Boot] [UBOOT PATCH] usb: ehci-hcd: Fix crash when rootdev not
>> initialized
>>
>> Hi,
>>
>> On 27-06-16 10:59, Siva Durga Prasad Paladugu wrote:
>> > This patch fixes the issue on zynq USB failure with DM when rootdev is
>> > not initialized. This variable is initalized to zero in non driver
>> > model case but not in DM.
>> >
>> > Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
>> > ---
>> > - Tested on Zynq ZC702 board with USB DM driver model patches
>> >   sent by Simon.
>>
>> This patch does not seem like the right fix, normally something like the ehci_ctrl
>> struct would be allocated by dm by setting priv_auto_alloc_size in the driver
>> description, and then the data will get calloc-ed. If you're allocating the ehci_ctrl
>> struct differently and not memsetting it to 0 you may have other bugs lurking, or
>> may get new bugs when new members get added in the future.
> This is also fine for me, no Problem.
> I will reply to Simon on his patch of Zynq USB DM and will ask him to do so.
>
> Thanks,
> Siva
>
>>
>> My advice would be to use priv_auto_alloc_size, see .e.g :
>> drivers/usb/host/ehci-sunxi.c
>>
>> If you cannot use that for some reason, make sure to memset struct ehci_Ctrl to
>> 0 before calling ehci_register()

Agreed, but this is how EHCI works at present. Since EHCI is not a
uclass it cannot have its own private data. So the driver must
incorporate EHCI's private data in its own private data.

Yes this is error-prone. I'm not sure of a better solution at present,
but welcome ideas!

Reviewed-by: Simon Glass <sjg@chromium.org>

>>
>> Regards,
>>
>> Hans
>>
>>
>>
>> > ---
>> >  drivers/usb/host/ehci-hcd.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> > index 13aa70d..8adffa6 100644
>> > --- a/drivers/usb/host/ehci-hcd.c
>> > +++ b/drivers/usb/host/ehci-hcd.c
>> > @@ -1624,6 +1624,8 @@ int ehci_register(struct udevice *dev, struct
>> ehci_hccr *hccr,
>> >                     goto err;
>> >     }
>> >
>> > +   ctrl->rootdev = 0;
>> > +
>> >     ret = ehci_common_init(ctrl, tweaks);
>> >     if (ret)
>> >             goto err;
>> >