[U-Boot,1/1] usb: musb-new: sunxi: Fix null pointer access
diff mbox series

Message ID 20181205124945.22813-1-stefan@olimex.com
State Accepted
Commit 46a3f276549f3e5720b6e80278cda354c7fa859f
Delegated to: Marek Vasut
Headers show
Series
  • [U-Boot,1/1] usb: musb-new: sunxi: Fix null pointer access
Related show

Commit Message

Stefan Mavrodiev Dec. 5, 2018, 12:49 p.m. UTC
When the device is in peripheral mode there is no
struct usb_bus_priv allocated pointer, as the uclass driver
("usb_dev_generic") doesn't call per_device_auto_alloc_size.

This results in writing to the internal SDRAM at
	priv->desc_before_addr = true;

Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
---
 drivers/usb/musb-new/sunxi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Marek Vasut Dec. 5, 2018, 12:57 p.m. UTC | #1
On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
> When the device is in peripheral mode

Can you have two devices, one in peripheral mode and one in host mode,
on the same system ?

> there is no
> struct usb_bus_priv allocated pointer, as the uclass driver
> ("usb_dev_generic") doesn't call per_device_auto_alloc_size.
> 
> This results in writing to the internal SDRAM at
> 	priv->desc_before_addr = true;
> 
> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> ---
>  drivers/usb/musb-new/sunxi.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> index 6cf9826cda..f3deb9bc66 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -435,11 +435,14 @@ static int musb_usb_probe(struct udevice *dev)
>  {
>  	struct sunxi_glue *glue = dev_get_priv(dev);
>  	struct musb_host_data *host = &glue->mdata;
> -	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>  	struct musb_hdrc_platform_data pdata;
>  	void *base = dev_read_addr_ptr(dev);
>  	int ret;
>  
> +#ifdef CONFIG_USB_MUSB_HOST
> +	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
> +#endif
> +
>  	if (!base)
>  		return -EINVAL;
>  
> @@ -459,7 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>  		return ret;
>  	}
>  
> -	priv->desc_before_addr = true;

See my question at the beginning, and if that can be the case, the fix
is to check if priv is not null here, eg.
if (priv)
 priv->...

Still, why is the priv data not allocated for device ?

>  	memset(&pdata, 0, sizeof(pdata));
>  	pdata.power = 250;
> @@ -467,6 +469,8 @@ static int musb_usb_probe(struct udevice *dev)
>  	pdata.config = glue->cfg->config;
>  
>  #ifdef CONFIG_USB_MUSB_HOST
> +	priv->desc_before_addr = true;
> +
>  	pdata.mode = MUSB_HOST;
>  	host->host = musb_init_controller(&pdata, &glue->dev, base);
>  	if (!host->host)
>
Stefan Mavrodiev Dec. 5, 2018, 1:06 p.m. UTC | #2
On 12/5/18 2:57 PM, Marek Vasut wrote:
> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
>> When the device is in peripheral mode
> Can you have two devices, one in peripheral mode and one in host mode,
> on the same system ?

Not 100% sure, but I'm thinking there is only one OTG port for
all sunxi boards. The operation is decided in the Kconfig.

>
>> there is no
>> struct usb_bus_priv allocated pointer, as the uclass driver
>> ("usb_dev_generic") doesn't call per_device_auto_alloc_size.
>>
>> This results in writing to the internal SDRAM at
>> 	priv->desc_before_addr = true;
>>
>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>> ---
>>   drivers/usb/musb-new/sunxi.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
>> index 6cf9826cda..f3deb9bc66 100644
>> --- a/drivers/usb/musb-new/sunxi.c
>> +++ b/drivers/usb/musb-new/sunxi.c
>> @@ -435,11 +435,14 @@ static int musb_usb_probe(struct udevice *dev)
>>   {
>>   	struct sunxi_glue *glue = dev_get_priv(dev);
>>   	struct musb_host_data *host = &glue->mdata;
>> -	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>   	struct musb_hdrc_platform_data pdata;
>>   	void *base = dev_read_addr_ptr(dev);
>>   	int ret;
>>   
>> +#ifdef CONFIG_USB_MUSB_HOST
>> +	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>> +#endif
>> +
>>   	if (!base)
>>   		return -EINVAL;
>>   
>> @@ -459,7 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>>   		return ret;
>>   	}
>>   
>> -	priv->desc_before_addr = true;
> See my question at the beginning, and if that can be the case, the fix
> is to check if priv is not null here, eg.
> if (priv)
>   priv->...
>
> Still, why is the priv data not allocated for device ?

Depending on configuration, the device is registered ether as
UCLASS_USB_DEV_GENERIC or UCLASS_USB. There is no

    .per_device_auto_alloc_size = sizeof(struct usb_bus_priv),

for the second. (As seen in drivers/usb/host/usb-uclass.c)

>
>>   	memset(&pdata, 0, sizeof(pdata));
>>   	pdata.power = 250;
>> @@ -467,6 +469,8 @@ static int musb_usb_probe(struct udevice *dev)
>>   	pdata.config = glue->cfg->config;
>>   
>>   #ifdef CONFIG_USB_MUSB_HOST
>> +	priv->desc_before_addr = true;
>> +
>>   	pdata.mode = MUSB_HOST;
>>   	host->host = musb_init_controller(&pdata, &glue->dev, base);
>>   	if (!host->host)
>>
>
Maxime Ripard Dec. 5, 2018, 1:11 p.m. UTC | #3
On Wed, Dec 05, 2018 at 01:57:14PM +0100, Marek Vasut wrote:
> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
> > When the device is in peripheral mode
> 
> Can you have two devices, one in peripheral mode and one in host mode,
> on the same system ?

No, or at least, on all of the SoCs that Allwinner ever produced,
there's only a single musb controller.

Maxime
Marek Vasut Dec. 5, 2018, 1:16 p.m. UTC | #4
On 12/05/2018 02:06 PM, Stefan Mavrodiev wrote:
> 
> On 12/5/18 2:57 PM, Marek Vasut wrote:
>> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
>>> When the device is in peripheral mode
>> Can you have two devices, one in peripheral mode and one in host mode,
>> on the same system ?
> 
> Not 100% sure, but I'm thinking there is only one OTG port for
> all sunxi boards. The operation is decided in the Kconfig.

I'm rather sure I saw sunxi boards with more than one USB port.

>>> there is no
>>> struct usb_bus_priv allocated pointer, as the uclass driver
>>> ("usb_dev_generic") doesn't call per_device_auto_alloc_size.
>>>
>>> This results in writing to the internal SDRAM at
>>>     priv->desc_before_addr = true;
>>>
>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>>> ---
>>>   drivers/usb/musb-new/sunxi.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
>>> index 6cf9826cda..f3deb9bc66 100644
>>> --- a/drivers/usb/musb-new/sunxi.c
>>> +++ b/drivers/usb/musb-new/sunxi.c
>>> @@ -435,11 +435,14 @@ static int musb_usb_probe(struct udevice *dev)
>>>   {
>>>       struct sunxi_glue *glue = dev_get_priv(dev);
>>>       struct musb_host_data *host = &glue->mdata;
>>> -    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>>       struct musb_hdrc_platform_data pdata;
>>>       void *base = dev_read_addr_ptr(dev);
>>>       int ret;
>>>   +#ifdef CONFIG_USB_MUSB_HOST
>>> +    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>> +#endif
>>> +
>>>       if (!base)
>>>           return -EINVAL;
>>>   @@ -459,7 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>>>           return ret;
>>>       }
>>>   -    priv->desc_before_addr = true;
>> See my question at the beginning, and if that can be the case, the fix
>> is to check if priv is not null here, eg.
>> if (priv)
>>   priv->...
>>
>> Still, why is the priv data not allocated for device ?
> 
> Depending on configuration, the device is registered ether as
> UCLASS_USB_DEV_GENERIC or UCLASS_USB. There is no
> 
>    .per_device_auto_alloc_size = sizeof(struct usb_bus_priv),
> 
> for the second. (As seen in drivers/usb/host/usb-uclass.c)

I see the code is rather horrible. I'd expect all that configuration to
come from DT otg-mode property instead of being hard-wired into the
code. Sigh.

Jagan, A-B ? I'd like to pick this .

>>
>>>       memset(&pdata, 0, sizeof(pdata));
>>>       pdata.power = 250;
>>> @@ -467,6 +469,8 @@ static int musb_usb_probe(struct udevice *dev)
>>>       pdata.config = glue->cfg->config;
>>>     #ifdef CONFIG_USB_MUSB_HOST
>>> +    priv->desc_before_addr = true;
>>> +
>>>       pdata.mode = MUSB_HOST;
>>>       host->host = musb_init_controller(&pdata, &glue->dev, base);
>>>       if (!host->host)
>>>
>>
Stefan Mavrodiev Dec. 13, 2018, 7:14 a.m. UTC | #5
On 12/5/18 3:16 PM, Marek Vasut wrote:
> On 12/05/2018 02:06 PM, Stefan Mavrodiev wrote:
>> On 12/5/18 2:57 PM, Marek Vasut wrote:
>>> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
>>>> When the device is in peripheral mode
>>> Can you have two devices, one in peripheral mode and one in host mode,
>>> on the same system ?
>> Not 100% sure, but I'm thinking there is only one OTG port for
>> all sunxi boards. The operation is decided in the Kconfig.
> I'm rather sure I saw sunxi boards with more than one USB port.
>
>>>> there is no
>>>> struct usb_bus_priv allocated pointer, as the uclass driver
>>>> ("usb_dev_generic") doesn't call per_device_auto_alloc_size.
>>>>
>>>> This results in writing to the internal SDRAM at
>>>>      priv->desc_before_addr = true;
>>>>
>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>>>> ---
>>>>    drivers/usb/musb-new/sunxi.c | 8 ++++++--
>>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
>>>> index 6cf9826cda..f3deb9bc66 100644
>>>> --- a/drivers/usb/musb-new/sunxi.c
>>>> +++ b/drivers/usb/musb-new/sunxi.c
>>>> @@ -435,11 +435,14 @@ static int musb_usb_probe(struct udevice *dev)
>>>>    {
>>>>        struct sunxi_glue *glue = dev_get_priv(dev);
>>>>        struct musb_host_data *host = &glue->mdata;
>>>> -    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>>>        struct musb_hdrc_platform_data pdata;
>>>>        void *base = dev_read_addr_ptr(dev);
>>>>        int ret;
>>>>    +#ifdef CONFIG_USB_MUSB_HOST
>>>> +    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>>> +#endif
>>>> +
>>>>        if (!base)
>>>>            return -EINVAL;
>>>>    @@ -459,7 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>>>>            return ret;
>>>>        }
>>>>    -    priv->desc_before_addr = true;
>>> See my question at the beginning, and if that can be the case, the fix
>>> is to check if priv is not null here, eg.
>>> if (priv)
>>>    priv->...
>>>
>>> Still, why is the priv data not allocated for device ?
>> Depending on configuration, the device is registered ether as
>> UCLASS_USB_DEV_GENERIC or UCLASS_USB. There is no
>>
>>     .per_device_auto_alloc_size = sizeof(struct usb_bus_priv),
>>
>> for the second. (As seen in drivers/usb/host/usb-uclass.c)
> I see the code is rather horrible. I'd expect all that configuration to
> come from DT otg-mode property instead of being hard-wired into the
> code. Sigh.
>
> Jagan, A-B ? I'd like to pick this .
>
>>>>        memset(&pdata, 0, sizeof(pdata));
>>>>        pdata.power = 250;
>>>> @@ -467,6 +469,8 @@ static int musb_usb_probe(struct udevice *dev)
>>>>        pdata.config = glue->cfg->config;
>>>>      #ifdef CONFIG_USB_MUSB_HOST
>>>> +    priv->desc_before_addr = true;
>>>> +
>>>>        pdata.mode = MUSB_HOST;
>>>>        host->host = musb_init_controller(&pdata, &glue->dev, base);
>>>>        if (!host->host)
>>>>
>
Any further comments?
Marek Vasut Dec. 13, 2018, 1:28 p.m. UTC | #6
On 12/13/2018 08:14 AM, Stefan Mavrodiev wrote:
> 
> On 12/5/18 3:16 PM, Marek Vasut wrote:
>> On 12/05/2018 02:06 PM, Stefan Mavrodiev wrote:
>>> On 12/5/18 2:57 PM, Marek Vasut wrote:
>>>> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
>>>>> When the device is in peripheral mode
>>>> Can you have two devices, one in peripheral mode and one in host mode,
>>>> on the same system ?
>>> Not 100% sure, but I'm thinking there is only one OTG port for
>>> all sunxi boards. The operation is decided in the Kconfig.
>> I'm rather sure I saw sunxi boards with more than one USB port.
>>
>>>>> there is no
>>>>> struct usb_bus_priv allocated pointer, as the uclass driver
>>>>> ("usb_dev_generic") doesn't call per_device_auto_alloc_size.
>>>>>
>>>>> This results in writing to the internal SDRAM at
>>>>>      priv->desc_before_addr = true;
>>>>>
>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>>>>> ---
>>>>>    drivers/usb/musb-new/sunxi.c | 8 ++++++--
>>>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/musb-new/sunxi.c
>>>>> b/drivers/usb/musb-new/sunxi.c
>>>>> index 6cf9826cda..f3deb9bc66 100644
>>>>> --- a/drivers/usb/musb-new/sunxi.c
>>>>> +++ b/drivers/usb/musb-new/sunxi.c
>>>>> @@ -435,11 +435,14 @@ static int musb_usb_probe(struct udevice *dev)
>>>>>    {
>>>>>        struct sunxi_glue *glue = dev_get_priv(dev);
>>>>>        struct musb_host_data *host = &glue->mdata;
>>>>> -    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>>>>        struct musb_hdrc_platform_data pdata;
>>>>>        void *base = dev_read_addr_ptr(dev);
>>>>>        int ret;
>>>>>    +#ifdef CONFIG_USB_MUSB_HOST
>>>>> +    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>>>> +#endif
>>>>> +
>>>>>        if (!base)
>>>>>            return -EINVAL;
>>>>>    @@ -459,7 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>>>>>            return ret;
>>>>>        }
>>>>>    -    priv->desc_before_addr = true;
>>>> See my question at the beginning, and if that can be the case, the fix
>>>> is to check if priv is not null here, eg.
>>>> if (priv)
>>>>    priv->...
>>>>
>>>> Still, why is the priv data not allocated for device ?
>>> Depending on configuration, the device is registered ether as
>>> UCLASS_USB_DEV_GENERIC or UCLASS_USB. There is no
>>>
>>>     .per_device_auto_alloc_size = sizeof(struct usb_bus_priv),
>>>
>>> for the second. (As seen in drivers/usb/host/usb-uclass.c)
>> I see the code is rather horrible. I'd expect all that configuration to
>> come from DT otg-mode property instead of being hard-wired into the
>> code. Sigh.
>>
>> Jagan, A-B ? I'd like to pick this .
>>
>>>>>        memset(&pdata, 0, sizeof(pdata));
>>>>>        pdata.power = 250;
>>>>> @@ -467,6 +469,8 @@ static int musb_usb_probe(struct udevice *dev)
>>>>>        pdata.config = glue->cfg->config;
>>>>>      #ifdef CONFIG_USB_MUSB_HOST
>>>>> +    priv->desc_before_addr = true;
>>>>> +
>>>>>        pdata.mode = MUSB_HOST;
>>>>>        host->host = musb_init_controller(&pdata, &glue->dev, base);
>>>>>        if (!host->host)
>>>>>
>>
> Any further comments?

As Jagan is inactive, applied.
Jean-Jacques Hiblot Dec. 13, 2018, 2:03 p.m. UTC | #7
On 05/12/2018 13:57, Marek Vasut wrote:
> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
>> When the device is in peripheral mode
> Can you have two devices, one in peripheral mode and one in host mode,
> on the same system ?

It is possible with the musb-new. Using DM_USB and DM_USB_GADGET, I did 
it with on a beaglebone black.

JJ

>> there is no
>> struct usb_bus_priv allocated pointer, as the uclass driver
>> ("usb_dev_generic") doesn't call per_device_auto_alloc_size.
>>
>> This results in writing to the internal SDRAM at
>> 	priv->desc_before_addr = true;
>>
>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>> ---
>>   drivers/usb/musb-new/sunxi.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
>> index 6cf9826cda..f3deb9bc66 100644
>> --- a/drivers/usb/musb-new/sunxi.c
>> +++ b/drivers/usb/musb-new/sunxi.c
>> @@ -435,11 +435,14 @@ static int musb_usb_probe(struct udevice *dev)
>>   {
>>   	struct sunxi_glue *glue = dev_get_priv(dev);
>>   	struct musb_host_data *host = &glue->mdata;
>> -	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>   	struct musb_hdrc_platform_data pdata;
>>   	void *base = dev_read_addr_ptr(dev);
>>   	int ret;
>>   
>> +#ifdef CONFIG_USB_MUSB_HOST
>> +	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>> +#endif
>> +
>>   	if (!base)
>>   		return -EINVAL;
>>   
>> @@ -459,7 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>>   		return ret;
>>   	}
>>   
>> -	priv->desc_before_addr = true;
> See my question at the beginning, and if that can be the case, the fix
> is to check if priv is not null here, eg.
> if (priv)
>   priv->...
>
> Still, why is the priv data not allocated for device ?
>
>>   	memset(&pdata, 0, sizeof(pdata));
>>   	pdata.power = 250;
>> @@ -467,6 +469,8 @@ static int musb_usb_probe(struct udevice *dev)
>>   	pdata.config = glue->cfg->config;
>>   
>>   #ifdef CONFIG_USB_MUSB_HOST
>> +	priv->desc_before_addr = true;
>> +
>>   	pdata.mode = MUSB_HOST;
>>   	host->host = musb_init_controller(&pdata, &glue->dev, base);
>>   	if (!host->host)
>>
>
Marek Vasut Dec. 13, 2018, 2:05 p.m. UTC | #8
On 12/13/2018 03:03 PM, Jean-Jacques Hiblot wrote:
> 
> On 05/12/2018 13:57, Marek Vasut wrote:
>> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
>>> When the device is in peripheral mode
>> Can you have two devices, one in peripheral mode and one in host mode,
>> on the same system ?
> 
> It is possible with the musb-new. Using DM_USB and DM_USB_GADGET, I did
> it with on a beaglebone black.

Does this patch break it ? And if so, can you send a proper fix ?

> JJ
> 
>>> there is no
>>> struct usb_bus_priv allocated pointer, as the uclass driver
>>> ("usb_dev_generic") doesn't call per_device_auto_alloc_size.
>>>
>>> This results in writing to the internal SDRAM at
>>>     priv->desc_before_addr = true;
>>>
>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>>> ---
>>>   drivers/usb/musb-new/sunxi.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
>>> index 6cf9826cda..f3deb9bc66 100644
>>> --- a/drivers/usb/musb-new/sunxi.c
>>> +++ b/drivers/usb/musb-new/sunxi.c
>>> @@ -435,11 +435,14 @@ static int musb_usb_probe(struct udevice *dev)
>>>   {
>>>       struct sunxi_glue *glue = dev_get_priv(dev);
>>>       struct musb_host_data *host = &glue->mdata;
>>> -    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>>       struct musb_hdrc_platform_data pdata;
>>>       void *base = dev_read_addr_ptr(dev);
>>>       int ret;
>>>   +#ifdef CONFIG_USB_MUSB_HOST
>>> +    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>> +#endif
>>> +
>>>       if (!base)
>>>           return -EINVAL;
>>>   @@ -459,7 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>>>           return ret;
>>>       }
>>>   -    priv->desc_before_addr = true;
>> See my question at the beginning, and if that can be the case, the fix
>> is to check if priv is not null here, eg.
>> if (priv)
>>   priv->...
>>
>> Still, why is the priv data not allocated for device ?
>>
>>>       memset(&pdata, 0, sizeof(pdata));
>>>       pdata.power = 250;
>>> @@ -467,6 +469,8 @@ static int musb_usb_probe(struct udevice *dev)
>>>       pdata.config = glue->cfg->config;
>>>     #ifdef CONFIG_USB_MUSB_HOST
>>> +    priv->desc_before_addr = true;
>>> +
>>>       pdata.mode = MUSB_HOST;
>>>       host->host = musb_init_controller(&pdata, &glue->dev, base);
>>>       if (!host->host)
>>>
>>
Jean-Jacques Hiblot Dec. 13, 2018, 3:40 p.m. UTC | #9
On 13/12/2018 15:05, Marek Vasut wrote:
> On 12/13/2018 03:03 PM, Jean-Jacques Hiblot wrote:
>> On 05/12/2018 13:57, Marek Vasut wrote:
>>> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
>>>> When the device is in peripheral mode
>>> Can you have two devices, one in peripheral mode and one in host mode,
>>> on the same system ?
>> It is possible with the musb-new. Using DM_USB and DM_USB_GADGET, I did
>> it with on a beaglebone black.
> Does this patch break it ? And if so, can you send a proper fix ?

This file is not compiled when building for beaglebone, so it won't 
break anything.

BTW this driver could be adapted, with little work, to support a dynamic 
selection of the role based on "dr-mode" in the DT.

Not knowing the boards, I don't know if this work has any added value 
though.

JJ


>
>> JJ
>>
>>>> there is no
>>>> struct usb_bus_priv allocated pointer, as the uclass driver
>>>> ("usb_dev_generic") doesn't call per_device_auto_alloc_size.
>>>>
>>>> This results in writing to the internal SDRAM at
>>>>      priv->desc_before_addr = true;
>>>>
>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>>>> ---
>>>>    drivers/usb/musb-new/sunxi.c | 8 ++++++--
>>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
>>>> index 6cf9826cda..f3deb9bc66 100644
>>>> --- a/drivers/usb/musb-new/sunxi.c
>>>> +++ b/drivers/usb/musb-new/sunxi.c
>>>> @@ -435,11 +435,14 @@ static int musb_usb_probe(struct udevice *dev)
>>>>    {
>>>>        struct sunxi_glue *glue = dev_get_priv(dev);
>>>>        struct musb_host_data *host = &glue->mdata;
>>>> -    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>>>        struct musb_hdrc_platform_data pdata;
>>>>        void *base = dev_read_addr_ptr(dev);
>>>>        int ret;
>>>>    +#ifdef CONFIG_USB_MUSB_HOST
>>>> +    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>>>> +#endif
>>>> +
>>>>        if (!base)
>>>>            return -EINVAL;
>>>>    @@ -459,7 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>>>>            return ret;
>>>>        }
>>>>    -    priv->desc_before_addr = true;
>>> See my question at the beginning, and if that can be the case, the fix
>>> is to check if priv is not null here, eg.
>>> if (priv)
>>>    priv->...
>>>
>>> Still, why is the priv data not allocated for device ?
>>>
>>>>        memset(&pdata, 0, sizeof(pdata));
>>>>        pdata.power = 250;
>>>> @@ -467,6 +469,8 @@ static int musb_usb_probe(struct udevice *dev)
>>>>        pdata.config = glue->cfg->config;
>>>>      #ifdef CONFIG_USB_MUSB_HOST
>>>> +    priv->desc_before_addr = true;
>>>> +
>>>>        pdata.mode = MUSB_HOST;
>>>>        host->host = musb_init_controller(&pdata, &glue->dev, base);
>>>>        if (!host->host)
>>>>
>
Marek Vasut Dec. 13, 2018, 3:49 p.m. UTC | #10
On 12/13/2018 04:40 PM, Jean-Jacques Hiblot wrote:
> 
> On 13/12/2018 15:05, Marek Vasut wrote:
>> On 12/13/2018 03:03 PM, Jean-Jacques Hiblot wrote:
>>> On 05/12/2018 13:57, Marek Vasut wrote:
>>>> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
>>>>> When the device is in peripheral mode
>>>> Can you have two devices, one in peripheral mode and one in host mode,
>>>> on the same system ?
>>> It is possible with the musb-new. Using DM_USB and DM_USB_GADGET, I did
>>> it with on a beaglebone black.
>> Does this patch break it ? And if so, can you send a proper fix ?
> 
> This file is not compiled when building for beaglebone, so it won't
> break anything.

Ah, sunxi, right.

> BTW this driver could be adapted, with little work, to support a dynamic
> selection of the role based on "dr-mode" in the DT.
> 
> Not knowing the boards, I don't know if this work has any added value
> though.

It would, as it would remove ifdeffery.
Jagan Teki Dec. 14, 2018, 10:14 a.m. UTC | #11
On Thu, Dec 13, 2018 at 7:00 PM Marek Vasut <marex@denx.de> wrote:
>
> On 12/13/2018 08:14 AM, Stefan Mavrodiev wrote:
> >
> > On 12/5/18 3:16 PM, Marek Vasut wrote:
> >> On 12/05/2018 02:06 PM, Stefan Mavrodiev wrote:
> >>> On 12/5/18 2:57 PM, Marek Vasut wrote:
> >>>> On 12/05/2018 01:49 PM, Stefan Mavrodiev wrote:
> >>>>> When the device is in peripheral mode
> >>>> Can you have two devices, one in peripheral mode and one in host mode,
> >>>> on the same system ?
> >>> Not 100% sure, but I'm thinking there is only one OTG port for
> >>> all sunxi boards. The operation is decided in the Kconfig.
> >> I'm rather sure I saw sunxi boards with more than one USB port.
> >>
> >>>>> there is no
> >>>>> struct usb_bus_priv allocated pointer, as the uclass driver
> >>>>> ("usb_dev_generic") doesn't call per_device_auto_alloc_size.
> >>>>>
> >>>>> This results in writing to the internal SDRAM at
> >>>>>      priv->desc_before_addr = true;
> >>>>>
> >>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
> >>>>> ---
> >>>>>    drivers/usb/musb-new/sunxi.c | 8 ++++++--
> >>>>>    1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/usb/musb-new/sunxi.c
> >>>>> b/drivers/usb/musb-new/sunxi.c
> >>>>> index 6cf9826cda..f3deb9bc66 100644
> >>>>> --- a/drivers/usb/musb-new/sunxi.c
> >>>>> +++ b/drivers/usb/musb-new/sunxi.c
> >>>>> @@ -435,11 +435,14 @@ static int musb_usb_probe(struct udevice *dev)
> >>>>>    {
> >>>>>        struct sunxi_glue *glue = dev_get_priv(dev);
> >>>>>        struct musb_host_data *host = &glue->mdata;
> >>>>> -    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
> >>>>>        struct musb_hdrc_platform_data pdata;
> >>>>>        void *base = dev_read_addr_ptr(dev);
> >>>>>        int ret;
> >>>>>    +#ifdef CONFIG_USB_MUSB_HOST
> >>>>> +    struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
> >>>>> +#endif
> >>>>> +
> >>>>>        if (!base)
> >>>>>            return -EINVAL;
> >>>>>    @@ -459,7 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
> >>>>>            return ret;
> >>>>>        }
> >>>>>    -    priv->desc_before_addr = true;
> >>>> See my question at the beginning, and if that can be the case, the fix
> >>>> is to check if priv is not null here, eg.
> >>>> if (priv)
> >>>>    priv->...
> >>>>
> >>>> Still, why is the priv data not allocated for device ?
> >>> Depending on configuration, the device is registered ether as
> >>> UCLASS_USB_DEV_GENERIC or UCLASS_USB. There is no
> >>>
> >>>     .per_device_auto_alloc_size = sizeof(struct usb_bus_priv),
> >>>
> >>> for the second. (As seen in drivers/usb/host/usb-uclass.c)
> >> I see the code is rather horrible. I'd expect all that configuration to
> >> come from DT otg-mode property instead of being hard-wired into the
> >> code. Sigh.
> >>
> >> Jagan, A-B ? I'd like to pick this .
> >>
> >>>>>        memset(&pdata, 0, sizeof(pdata));
> >>>>>        pdata.power = 250;
> >>>>> @@ -467,6 +469,8 @@ static int musb_usb_probe(struct udevice *dev)
> >>>>>        pdata.config = glue->cfg->config;
> >>>>>      #ifdef CONFIG_USB_MUSB_HOST
> >>>>> +    priv->desc_before_addr = true;
> >>>>> +
> >>>>>        pdata.mode = MUSB_HOST;
> >>>>>        host->host = musb_init_controller(&pdata, &glue->dev, base);
> >>>>>        if (!host->host)
> >>>>>
> >>
> > Any further comments?
>
> As Jagan is inactive, applied.

Sorry, I was in travel in multiple locations couldn't find this patch.
Thanks for applying on your side.

Patch
diff mbox series

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
index 6cf9826cda..f3deb9bc66 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -435,11 +435,14 @@  static int musb_usb_probe(struct udevice *dev)
 {
 	struct sunxi_glue *glue = dev_get_priv(dev);
 	struct musb_host_data *host = &glue->mdata;
-	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
 	struct musb_hdrc_platform_data pdata;
 	void *base = dev_read_addr_ptr(dev);
 	int ret;
 
+#ifdef CONFIG_USB_MUSB_HOST
+	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
+#endif
+
 	if (!base)
 		return -EINVAL;
 
@@ -459,7 +462,6 @@  static int musb_usb_probe(struct udevice *dev)
 		return ret;
 	}
 
-	priv->desc_before_addr = true;
 
 	memset(&pdata, 0, sizeof(pdata));
 	pdata.power = 250;
@@ -467,6 +469,8 @@  static int musb_usb_probe(struct udevice *dev)
 	pdata.config = glue->cfg->config;
 
 #ifdef CONFIG_USB_MUSB_HOST
+	priv->desc_before_addr = true;
+
 	pdata.mode = MUSB_HOST;
 	host->host = musb_init_controller(&pdata, &glue->dev, base);
 	if (!host->host)