phy: Fix db410c crash issue during usb_stop

[Marek Vasut]

On 8/23/24 6:59 AM, JianfengA.Zhu at sony.com wrote:
> usb stop crash log
> ====================================================================
> dragonboard410c => usb start
> starting USB...
> Bus usb at 78d9000: USB EHCI 1.00
> scanning bus usb at 78d9000 for devices... 4 USB Device(s) found
>         scanning usb for storage devices... 0 Storage Device(s) found
> dragonboard410c => usb stop
> stopping USB..
> "Synchronous Abort" handler, esr 0x96000007, far 0x0
> elr: 000000008f636a28 lr : 000000008f636a24 (reloc)
> elr: 00000000bd97ea28 lr : 00000000bd97ea24
> x0 : 0000000000000000 x1 : 00000000bd1494d0
> x2 : 0000000000000000 x3 : 0000000000001f40
> x4 : 00000000bd131990 x5 : 00000000bd157b70
> x6 : 0000000000000041 x7 : 00000000bd163b80
> x8 : 00000000bd131db0 x9 : 000000000000b65c
> x10: 00000000bd13183c x11: 0000000000000006
> x12: 000000000001869f x13: 0000000000000061
> x14: 00000000ffffffff x15: 00000000bd1317b8
> x16: 00000000bd9b78ac x17: 0000000000000000
> x18: 00000000bd143d90 x19: 00000000bd1575d8
> x20: 00000000bd1575d8 x21: 00000000bd157440
> x22: 00000000bd1493b0 x23: 0000000000000002
> x24: 00000000bd9eb910 x25: 0000000000000000
> x26: 0000000000000000 x27: 0000000000000000
> x28: 00000000bd158540 x29: 00000000bd131980
> 
> Code: f9400000 b4000120 97ffdd1e aa0003e2 (f9400001)
> Resetting CPU ...
> 
> resetting ...
> ====================================================================
> 
> After commit ed8fbd2889fc ("dts: msm8916: replace with upstream DTS")
> msm8916_usbphy will be defined as a child device of usb at 78d9000. When
> calling usb stop, usb at 78d9000 will be removed. During the removal process,
> the sub-device msm8916_usbphy will be removed first. In the subsequent
> remove process, the uclass_priv_ of msm8916_usbphy will be accessed again.
> Because uclass_priv_ is NULL at this time, it will cause a crash.
> 
> Detailed calling process
> ====================================================================
> usb_stop (drivers/usb/host/usb-uclass.c)
> |-> device_remove(bus, DM_REMOVE_NORMAL); <== remove ehci_msm
> .|-> device_chld_remove <== remove msm8916_usbphy
> . |-> device_remove
> .  |-> device_free(dev);
> .   |-> dev_set_uclass_priv(dev, NULL);
> .    |-> dev->uclass_priv_ = uclass_priv; <== uclass_priv is NULL
> |-> drv->remove(dev);
>   |-> ehci_usb_remove
>    |-> generic_shutdown_phy
>     |-> generic_shutdown_phy
>      |-> phy_get_counts
>       |-> dev_get_uclass_priv
>        |-> dm_priv_to_rw(dev->uclass_priv_); <== NULL pointer access
> ====================================================================
> 
> Fix: move usb phy power off into msm8916_usbphy remove.
> 
> Signed-off-by: Jianfeng Zhu <JianfengA.Zhu at sony.com>
> Reviewed-by: Jacky Cao <Jacky.Cao at sony.com>
> Reviewed-by: Toyama, Yoshihiro <Yoshihiro.Toyama at sony.com>
> ---
>   drivers/phy/qcom/msm8916-usbh-phy.c | 20 ++++++++++++++++++++
>   drivers/usb/host/ehci-msm.c         |  4 ----
>   2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/phy/qcom/msm8916-usbh-phy.c b/drivers/phy/qcom/msm8916-usbh-phy.c
> index 4b435aa2a6..0ba520c93d 100644
> --- a/drivers/phy/qcom/msm8916-usbh-phy.c
> +++ b/drivers/phy/qcom/msm8916-usbh-phy.c
> @@ -46,6 +46,25 @@ static int msm_phy_power_off(struct phy *phy)
>   	return 0;
>   }
>   
> +static int msm_phy_remove(struct udevice *dev)
> +{
> +	struct phy phy;
> +	int ret;
> +
> +	ret = generic_phy_get_by_index(dev->parent, 0, &phy);
> +	if (ret)
> +		return ret;
> +
> +	/* To avoid NULL pointer access exception issue, move
> +	 * generic_shutdown_phy from ehci_usb_remove to here
> +	 */
> +	ret = generic_shutdown_phy(&phy);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>   static int msm_phy_reset(struct phy *phy)
>   {
>   	struct msm_phy_priv *p = dev_get_priv(phy->dev);
> @@ -105,5 +124,6 @@ U_BOOT_DRIVER(msm8916_usbphy) = {
>   	.of_match	= msm_phy_ids,
>   	.ops		= &msm_phy_ops,
>   	.probe		= msm_phy_probe,
> +	.remove		= msm_phy_remove,
>   	.priv_auto	= sizeof(struct msm_phy_priv),
>   };
> diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> index ff336082e3..565859e806 100644
> --- a/drivers/usb/host/ehci-msm.c
> +++ b/drivers/usb/host/ehci-msm.c
> @@ -114,10 +114,6 @@ static int ehci_usb_remove(struct udevice *dev)
>   	clk_disable_unprepare(&p->iface_clk);
>   	clk_disable_unprepare(&p->core_clk);
>   
> -	ret = generic_shutdown_phy(&p->phy);
> -	if (ret)
> -		return ret;
> -
>   	ret = board_usb_init(0, USB_INIT_DEVICE); /* Board specific hook */
>   	if (ret < 0)
>   		return ret;

The PHY management (=shutdown) should be done by the consumer (=the USB 
controller driver), so this patch is not correct. The issue is valid.

Caleb, any suggestion ?