phy: core: Add consumer device link support

Commit Message

Alexandre TORGUE Nov. 4, 2019, 2:37 p.m. UTC

In order to enforce suspend/resume ordering, this commit creates link
between phy consumers and phy devices. This link avoids to suspend phy
before phy consumers.

Signed-off-by: Alexandre Torgue <[e-mail]>

---

Hi,

To manage device_link in phy-core I had to "balance" get and put APIs a bit
more. Fot this reason, you'll find updates in Renesas usbhs rcar and rza drivers
as phy API changes.

Regards
Alex

Comments

Alexandre TORGUE Dec. 4, 2019, 3:33 p.m. UTC | #1

Hi,

Gentle ping, in case you missed this.

Regards
Alex

On 11/4/19 3:37 PM, Alexandre Torgue wrote:
> In order to enforce suspend/resume ordering, this commit creates link
> between phy consumers and phy devices. This link avoids to suspend phy
> before phy consumers.
> 
> Signed-off-by: Alexandre Torgue <[e-mail]>
> 
> ---
> 
> Hi,
> 
> To manage device_link in phy-core I had to "balance" get and put APIs a bit
> more. Fot this reason, you'll find updates in Renesas usbhs rcar and rza drivers
> as phy API changes.
> 
> Regards
> Alex
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index b04f4fe85ac2..8dfb4868c8c3 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -29,7 +29,7 @@ static void devm_phy_release(struct device *dev, void *res)
>   {
>   	struct phy *phy = *(struct phy **)res;
>   
> -	phy_put(phy);
> +	phy_put(dev, phy);
>   }
>   
>   static void devm_phy_provider_release(struct device *dev, void *res)
> @@ -566,12 +566,12 @@ struct phy *of_phy_get(struct device_node *np, const char *con_id)
>   EXPORT_SYMBOL_GPL(of_phy_get);
>   
>   /**
> - * phy_put() - release the PHY
> - * @phy: the phy returned by phy_get()
> + * of_phy_put() - release the PHY
> + * @phy: the phy returned by of_phy_get()
>    *
> - * Releases a refcount the caller received from phy_get().
> + * Releases a refcount the caller received from of_phy_get().
>    */
> -void phy_put(struct phy *phy)
> +void of_phy_put(struct phy *phy)
>   {
>   	if (!phy || IS_ERR(phy))
>   		return;
> @@ -584,6 +584,20 @@ void phy_put(struct phy *phy)
>   	module_put(phy->ops->owner);
>   	put_device(&phy->dev);
>   }
> +EXPORT_SYMBOL_GPL(of_phy_put);
> +
> +/**
> + * phy_put() - release the PHY
> + * @dev: device that wants to release this phy
> + * @phy: the phy returned by phy_get()
> + *
> + * Releases a refcount the caller received from phy_get().
> + */
> +void phy_put(struct device *dev, struct phy *phy)
> +{
> +	device_link_remove(dev, &phy->dev);
> +	of_phy_put(phy);
> +}
>   EXPORT_SYMBOL_GPL(phy_put);
>   
>   /**
> @@ -651,6 +665,7 @@ struct phy *phy_get(struct device *dev, const char *string)
>   {
>   	int index = 0;
>   	struct phy *phy;
> +	struct device_link *link;
>   
>   	if (string == NULL) {
>   		dev_WARN(dev, "missing string\n");
> @@ -672,6 +687,13 @@ struct phy *phy_get(struct device *dev, const char *string)
>   
>   	get_device(&phy->dev);
>   
> +	link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> +	if (!link) {
> +		dev_err(dev, "failed to create device link to %s\n",
> +			dev_name(phy->dev.parent));
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>   	return phy;
>   }
>   EXPORT_SYMBOL_GPL(phy_get);
> @@ -765,6 +787,7 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>   			    const char *con_id)
>   {
>   	struct phy **ptr, *phy;
> +	struct device_link *link;
>   
>   	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>   	if (!ptr)
> @@ -778,6 +801,13 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>   		devres_free(ptr);
>   	}
>   
> +	link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> +	if (!link) {
> +		dev_err(dev, "failed to create device link to %s\n",
> +			dev_name(phy->dev.parent));
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>   	return phy;
>   }
>   EXPORT_SYMBOL_GPL(devm_of_phy_get);
> @@ -798,6 +828,7 @@ struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
>   				     int index)
>   {
>   	struct phy **ptr, *phy;
> +	struct device_link *link;
>   
>   	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>   	if (!ptr)
> @@ -819,6 +850,13 @@ struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
>   	*ptr = phy;
>   	devres_add(dev, ptr);
>   
> +	link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> +	if (!link) {
> +		dev_err(dev, "failed to create device link to %s\n",
> +			dev_name(phy->dev.parent));
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>   	return phy;
>   }
>   EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
> diff --git a/drivers/usb/renesas_usbhs/rcar2.c b/drivers/usb/renesas_usbhs/rcar2.c
> index 440d213e1749..791908f8cf73 100644
> --- a/drivers/usb/renesas_usbhs/rcar2.c
> +++ b/drivers/usb/renesas_usbhs/rcar2.c
> @@ -34,7 +34,7 @@ static int usbhs_rcar2_hardware_exit(struct platform_device *pdev)
>   	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
>   
>   	if (priv->phy) {
> -		phy_put(priv->phy);
> +		phy_put(&pdev->dev, priv->phy);
>   		priv->phy = NULL;
>   	}
>   
> diff --git a/drivers/usb/renesas_usbhs/rza2.c b/drivers/usb/renesas_usbhs/rza2.c
> index 021749594389..3eed3334a17f 100644
> --- a/drivers/usb/renesas_usbhs/rza2.c
> +++ b/drivers/usb/renesas_usbhs/rza2.c
> @@ -29,7 +29,7 @@ static int usbhs_rza2_hardware_exit(struct platform_device *pdev)
>   {
>   	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
>   
> -	phy_put(priv->phy);
> +	phy_put(&pdev->dev, priv->phy);
>   	priv->phy = NULL;
>   
>   	return 0;
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 56d3a100006a..19eddd64c8f6 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -234,7 +234,8 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>   			    const char *con_id);
>   struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
>   				     int index);
> -void phy_put(struct phy *phy);
> +void of_phy_put(struct phy *phy);
> +void phy_put(struct device *dev, struct phy *phy);
>   void devm_phy_put(struct device *dev, struct phy *phy);
>   struct phy *of_phy_get(struct device_node *np, const char *con_id);
>   struct phy *of_phy_simple_xlate(struct device *dev,
> @@ -419,7 +420,11 @@ static inline struct phy *devm_of_phy_get_by_index(struct device *dev,
>   	return ERR_PTR(-ENOSYS);
>   }
>   
> -static inline void phy_put(struct phy *phy)
> +static inline void of_phy_put(struct phy *phy)
> +{
> +}
> +
> +static inline void phy_put(struct device *dev, struct phy *phy)
>   {
>   }
>   
>

Greg KH Dec. 4, 2019, 5:19 p.m. UTC | #2

On Wed, Dec 04, 2019 at 04:33:14PM +0100, Alexandre Torgue wrote:
> Hi,
> 
> Gentle ping, in case you missed this.

It's the middle of the merge window, we can't add anything to our trees
at this point in time, sorry.

greg k-h

Jon Hunter Jan. 7, 2020, 11:51 a.m. UTC | #3

On 04/11/2019 14:37, Alexandre Torgue wrote:
> In order to enforce suspend/resume ordering, this commit creates link
> between phy consumers and phy devices. This link avoids to suspend phy
> before phy consumers.
> 
> Signed-off-by: Alexandre Torgue <[e-mail]>

With next-20200106 we are seeing a boot regression on Tegra124 Jetson
TK1 board. Bisect is pointing to this commit and reverting this on top
of -next fixes the problem.

The bootlog is showing the following crash on boot ...

[    1.730024] 8<--- cut here ---
[    1.733079] Unable to handle kernel paging request at virtual address fffffe7f
[    1.740318] pgd = (ptrval)
[    1.743021] [fffffe7f] *pgd=affff841, *pte=00000000, *ppte=00000000
[    1.749304] Internal error: Oops: 27 [#1] SMP ARM
[    1.754001] Modules linked in:
[    1.757057] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.5.0-rc4-next-20200106-g9eb1b48ca4ce #1
[    1.765654] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    1.771919] PC is at device_link_add+0x68/0x4d4
[    1.776444] LR is at device_link_add+0x68/0x4d4
[    1.780967] pc : [<c09832e4>]    lr : [<c09832e4>]    psr: 60000013
[    1.787223] sp : ee0e1d60  ip : 60000013  fp : 00000005
[    1.792439] r10: 00000000  r9 : 00000000  r8 : eefedd88
[    1.797657] r7 : ee269c10  r6 : fffffdfb  r5 : 00000001  r4 : 00000001
[    1.804173] r3 : ee0d8000  r2 : 00000000  r1 : 00000000  r0 : c1858f88
[    1.810691] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    1.817815] Control: 10c5387d  Table: 8020406a  DAC: 00000051
[    1.823552] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[    1.829549] Stack: (0xee0e1d60 to 0xee0e2000)
[    1.833904] 1d60: eefedd88 00000040 c07087a0 fffffdfb ee269c10 ee737640 00000000 eefedd88
[    1.842073] 1d80: 00000000 00000000 00000005 c0707d34 00000000 ee3c8a00 ee7375c0 ee269c10
[    1.850242] 1da0: eefedd88 c0a0bd2c c1704e48 ee269c10 ee269c00 ee3c8a00 00000000 c0a0c4a8
[    1.858409] 1dc0: ee269c10 c1704e48 c186603c 00000000 c186603c 00000000 00000000 bc98ab22
[    1.866577] 1de0: ffffffff ee269c10 00000000 c186603c ee269c00 c186603c 00000000 00000000
[    1.874744] 1e00: c1656690 c0a0ffe0 00000000 bc98ab22 ee269c10 ee269c10 00000000 c186603c
[    1.882913] 1e20: 00000000 c186603c 00000000 c09887e0 c18ff9dc ee269c10 c18ff9e0 c0986860
[    1.891082] 1e40: ee269c10 c186603c c186603c c1704e48 00000000 c15003f0 c15c3854 c0986af0
[    1.899249] 1e60: c15c3854 c0d128b4 c10e48ec ee269c10 00000000 c186603c c1704e48 00000000
[    1.907416] 1e80: c15003f0 c15c3854 c1656690 c0986da0 00000000 c186603c ee269c10 c0986e28
[    1.915583] 1ea0: 00000000 c186603c c0986da8 c0984ba0 c15003f0 ee20c058 ee242334 bc98ab22
[    1.923752] 1ec0: c18588c8 c186603c ee737200 c18588c8 00000000 c0985b94 c133ef10 ffffe000
[    1.931919] 1ee0: c186603c c186603c c18aaf80 ffffe000 c158b72c c09878ac c1704e48 c18aaf80
[    1.940088] 1f00: ffffe000 c0302f80 00000168 c0367d84 c143e5b4 c1371000 00000000 00000006
[    1.948255] 1f20: 00000006 c125b1b4 00000000 c1704e48 c126f324 c125b228 00000000 efffec88
[    1.956424] 1f40: 00000000 bc98ab22 00000000 c18b6bc0 c18b6bc0 bc98ab22 c18b6bc0 c18b6bc0
[    1.964591] 1f60: 00000007 c15c3834 00000169 c1500f28 00000006 00000006 00000000 c15003f0
[    1.972758] 1f80: 00000000 00000000 c0ef1cdc 00000000 00000000 00000000 00000000 00000000
[    1.980924] 1fa0: 00000000 c0ef1ce4 00000000 c03010e8 00000000 00000000 00000000 00000000
[    1.989092] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    1.997260] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[    2.005440] [<c09832e4>] (device_link_add) from [<c0707d34>] (devm_of_phy_get+0x6c/0xb0)
[    2.013528] [<c0707d34>] (devm_of_phy_get) from [<c0a0bd2c>] (ahci_platform_get_phy+0x28/0xd0)
[    2.022134] [<c0a0bd2c>] (ahci_platform_get_phy) from [<c0a0c4a8>] (ahci_platform_get_resources+0x384/0x468)
[    2.031952] [<c0a0c4a8>] (ahci_platform_get_resources) from [<c0a0ffe0>] (tegra_ahci_probe+0x14/0x650)
[    2.041254] [<c0a0ffe0>] (tegra_ahci_probe) from [<c09887e0>] (platform_drv_probe+0x48/0x98)
[    2.049686] [<c09887e0>] (platform_drv_probe) from [<c0986860>] (really_probe+0x234/0x34c)
[    2.057944] [<c0986860>] (really_probe) from [<c0986af0>] (driver_probe_device+0x60/0x168)
[    2.066202] [<c0986af0>] (driver_probe_device) from [<c0986da0>] (device_driver_attach+0x58/0x60)
[    2.075064] [<c0986da0>] (device_driver_attach) from [<c0986e28>] (__driver_attach+0x80/0xbc)
[    2.083582] [<c0986e28>] (__driver_attach) from [<c0984ba0>] (bus_for_each_dev+0x74/0xb4)
[    2.091751] [<c0984ba0>] (bus_for_each_dev) from [<c0985b94>] (bus_add_driver+0x164/0x1e8)
[    2.100008] [<c0985b94>] (bus_add_driver) from [<c09878ac>] (driver_register+0x7c/0x114)
[    2.108094] [<c09878ac>] (driver_register) from [<c0302f80>] (do_one_initcall+0x54/0x22c)
[    2.116271] [<c0302f80>] (do_one_initcall) from [<c1500f28>] (kernel_init_freeable+0x14c/0x1b0)
[    2.124967] [<c1500f28>] (kernel_init_freeable) from [<c0ef1ce4>] (kernel_init+0x8/0x10c)
[    2.133139] [<c0ef1ce4>] (kernel_init) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
[    2.140697] Exception stack(0xee0e1fb0 to 0xee0e1ff8)
[    2.145743] 1fa0:                                     00000000 00000000 00000000 00000000
[    2.153910] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.162076] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    2.168686] Code: e59f0470 03844040 eb15cb16 eb004c8a (e5d63084) 
[    2.174824] ---[ end trace fddbf111e88ec722 ]---


I believe that there is a bug in this patch and the following fixed it for me ...

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 8dfb4868c8c3..2eb28cc2d2dc 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -799,6 +799,7 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
                devres_add(dev, ptr);
        } else {
                devres_free(ptr);
+               return phy;
        }
 
        link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);

Cheers
Jon

Kishon Vijay Abraham I Jan. 8, 2020, 7:29 a.m. UTC | #4

Hi Jon,

On 07/01/20 5:21 PM, Jon Hunter wrote:
> 
> On 04/11/2019 14:37, Alexandre Torgue wrote:
>> In order to enforce suspend/resume ordering, this commit creates link
>> between phy consumers and phy devices. This link avoids to suspend phy
>> before phy consumers.
>>
>> Signed-off-by: Alexandre Torgue <[e-mail]>
> 
> With next-20200106 we are seeing a boot regression on Tegra124 Jetson
> TK1 board. Bisect is pointing to this commit and reverting this on top
> of -next fixes the problem.
> 
> The bootlog is showing the following crash on boot ...
> 
> [    1.730024] 8<--- cut here ---
> [    1.733079] Unable to handle kernel paging request at virtual address fffffe7f
> [    1.740318] pgd = (ptrval)
> [    1.743021] [fffffe7f] *pgd=affff841, *pte=00000000, *ppte=00000000
> [    1.749304] Internal error: Oops: 27 [#1] SMP ARM
> [    1.754001] Modules linked in:
> [    1.757057] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.5.0-rc4-next-20200106-g9eb1b48ca4ce #1
> [    1.765654] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [    1.771919] PC is at device_link_add+0x68/0x4d4
> [    1.776444] LR is at device_link_add+0x68/0x4d4
> [    1.780967] pc : [<c09832e4>]    lr : [<c09832e4>]    psr: 60000013
> [    1.787223] sp : ee0e1d60  ip : 60000013  fp : 00000005
> [    1.792439] r10: 00000000  r9 : 00000000  r8 : eefedd88
> [    1.797657] r7 : ee269c10  r6 : fffffdfb  r5 : 00000001  r4 : 00000001
> [    1.804173] r3 : ee0d8000  r2 : 00000000  r1 : 00000000  r0 : c1858f88
> [    1.810691] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [    1.817815] Control: 10c5387d  Table: 8020406a  DAC: 00000051
> [    1.823552] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
> [    1.829549] Stack: (0xee0e1d60 to 0xee0e2000)
> [    1.833904] 1d60: eefedd88 00000040 c07087a0 fffffdfb ee269c10 ee737640 00000000 eefedd88
> [    1.842073] 1d80: 00000000 00000000 00000005 c0707d34 00000000 ee3c8a00 ee7375c0 ee269c10
> [    1.850242] 1da0: eefedd88 c0a0bd2c c1704e48 ee269c10 ee269c00 ee3c8a00 00000000 c0a0c4a8
> [    1.858409] 1dc0: ee269c10 c1704e48 c186603c 00000000 c186603c 00000000 00000000 bc98ab22
> [    1.866577] 1de0: ffffffff ee269c10 00000000 c186603c ee269c00 c186603c 00000000 00000000
> [    1.874744] 1e00: c1656690 c0a0ffe0 00000000 bc98ab22 ee269c10 ee269c10 00000000 c186603c
> [    1.882913] 1e20: 00000000 c186603c 00000000 c09887e0 c18ff9dc ee269c10 c18ff9e0 c0986860
> [    1.891082] 1e40: ee269c10 c186603c c186603c c1704e48 00000000 c15003f0 c15c3854 c0986af0
> [    1.899249] 1e60: c15c3854 c0d128b4 c10e48ec ee269c10 00000000 c186603c c1704e48 00000000
> [    1.907416] 1e80: c15003f0 c15c3854 c1656690 c0986da0 00000000 c186603c ee269c10 c0986e28
> [    1.915583] 1ea0: 00000000 c186603c c0986da8 c0984ba0 c15003f0 ee20c058 ee242334 bc98ab22
> [    1.923752] 1ec0: c18588c8 c186603c ee737200 c18588c8 00000000 c0985b94 c133ef10 ffffe000
> [    1.931919] 1ee0: c186603c c186603c c18aaf80 ffffe000 c158b72c c09878ac c1704e48 c18aaf80
> [    1.940088] 1f00: ffffe000 c0302f80 00000168 c0367d84 c143e5b4 c1371000 00000000 00000006
> [    1.948255] 1f20: 00000006 c125b1b4 00000000 c1704e48 c126f324 c125b228 00000000 efffec88
> [    1.956424] 1f40: 00000000 bc98ab22 00000000 c18b6bc0 c18b6bc0 bc98ab22 c18b6bc0 c18b6bc0
> [    1.964591] 1f60: 00000007 c15c3834 00000169 c1500f28 00000006 00000006 00000000 c15003f0
> [    1.972758] 1f80: 00000000 00000000 c0ef1cdc 00000000 00000000 00000000 00000000 00000000
> [    1.980924] 1fa0: 00000000 c0ef1ce4 00000000 c03010e8 00000000 00000000 00000000 00000000
> [    1.989092] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    1.997260] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [    2.005440] [<c09832e4>] (device_link_add) from [<c0707d34>] (devm_of_phy_get+0x6c/0xb0)
> [    2.013528] [<c0707d34>] (devm_of_phy_get) from [<c0a0bd2c>] (ahci_platform_get_phy+0x28/0xd0)
> [    2.022134] [<c0a0bd2c>] (ahci_platform_get_phy) from [<c0a0c4a8>] (ahci_platform_get_resources+0x384/0x468)
> [    2.031952] [<c0a0c4a8>] (ahci_platform_get_resources) from [<c0a0ffe0>] (tegra_ahci_probe+0x14/0x650)
> [    2.041254] [<c0a0ffe0>] (tegra_ahci_probe) from [<c09887e0>] (platform_drv_probe+0x48/0x98)
> [    2.049686] [<c09887e0>] (platform_drv_probe) from [<c0986860>] (really_probe+0x234/0x34c)
> [    2.057944] [<c0986860>] (really_probe) from [<c0986af0>] (driver_probe_device+0x60/0x168)
> [    2.066202] [<c0986af0>] (driver_probe_device) from [<c0986da0>] (device_driver_attach+0x58/0x60)
> [    2.075064] [<c0986da0>] (device_driver_attach) from [<c0986e28>] (__driver_attach+0x80/0xbc)
> [    2.083582] [<c0986e28>] (__driver_attach) from [<c0984ba0>] (bus_for_each_dev+0x74/0xb4)
> [    2.091751] [<c0984ba0>] (bus_for_each_dev) from [<c0985b94>] (bus_add_driver+0x164/0x1e8)
> [    2.100008] [<c0985b94>] (bus_add_driver) from [<c09878ac>] (driver_register+0x7c/0x114)
> [    2.108094] [<c09878ac>] (driver_register) from [<c0302f80>] (do_one_initcall+0x54/0x22c)
> [    2.116271] [<c0302f80>] (do_one_initcall) from [<c1500f28>] (kernel_init_freeable+0x14c/0x1b0)
> [    2.124967] [<c1500f28>] (kernel_init_freeable) from [<c0ef1ce4>] (kernel_init+0x8/0x10c)
> [    2.133139] [<c0ef1ce4>] (kernel_init) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
> [    2.140697] Exception stack(0xee0e1fb0 to 0xee0e1ff8)
> [    2.145743] 1fa0:                                     00000000 00000000 00000000 00000000
> [    2.153910] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    2.162076] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    2.168686] Code: e59f0470 03844040 eb15cb16 eb004c8a (e5d63084) 
> [    2.174824] ---[ end trace fddbf111e88ec722 ]---
> 
> 
> I believe that there is a bug in this patch and the following fixed it for me ...
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 8dfb4868c8c3..2eb28cc2d2dc 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -799,6 +799,7 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>                 devres_add(dev, ptr);
>         } else {
>                 devres_free(ptr);
> +               return phy;
>         }
>  
>         link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);

Thank you for spotting this. I've included the fix now.

Thanks
Kishon

Alexandre TORGUE Jan. 8, 2020, 8:37 a.m. UTC | #5

On 1/8/20 8:29 AM, Kishon Vijay Abraham I wrote:
> Hi Jon,
> 
> On 07/01/20 5:21 PM, Jon Hunter wrote:
>>
>> On 04/11/2019 14:37, Alexandre Torgue wrote:
>>> In order to enforce suspend/resume ordering, this commit creates link
>>> between phy consumers and phy devices. This link avoids to suspend phy
>>> before phy consumers.
>>>
>>> Signed-off-by: Alexandre Torgue <[e-mail]>
>>
>> With next-20200106 we are seeing a boot regression on Tegra124 Jetson
>> TK1 board. Bisect is pointing to this commit and reverting this on top
>> of -next fixes the problem.
>>
>> The bootlog is showing the following crash on boot ...
>>
>> [    1.730024] 8<--- cut here ---
>> [    1.733079] Unable to handle kernel paging request at virtual address fffffe7f
>> [    1.740318] pgd = (ptrval)
>> [    1.743021] [fffffe7f] *pgd=affff841, *pte=00000000, *ppte=00000000
>> [    1.749304] Internal error: Oops: 27 [#1] SMP ARM
>> [    1.754001] Modules linked in:
>> [    1.757057] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.5.0-rc4-next-20200106-g9eb1b48ca4ce #1
>> [    1.765654] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>> [    1.771919] PC is at device_link_add+0x68/0x4d4
>> [    1.776444] LR is at device_link_add+0x68/0x4d4
>> [    1.780967] pc : [<c09832e4>]    lr : [<c09832e4>]    psr: 60000013
>> [    1.787223] sp : ee0e1d60  ip : 60000013  fp : 00000005
>> [    1.792439] r10: 00000000  r9 : 00000000  r8 : eefedd88
>> [    1.797657] r7 : ee269c10  r6 : fffffdfb  r5 : 00000001  r4 : 00000001
>> [    1.804173] r3 : ee0d8000  r2 : 00000000  r1 : 00000000  r0 : c1858f88
>> [    1.810691] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>> [    1.817815] Control: 10c5387d  Table: 8020406a  DAC: 00000051
>> [    1.823552] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
>> [    1.829549] Stack: (0xee0e1d60 to 0xee0e2000)
>> [    1.833904] 1d60: eefedd88 00000040 c07087a0 fffffdfb ee269c10 ee737640 00000000 eefedd88
>> [    1.842073] 1d80: 00000000 00000000 00000005 c0707d34 00000000 ee3c8a00 ee7375c0 ee269c10
>> [    1.850242] 1da0: eefedd88 c0a0bd2c c1704e48 ee269c10 ee269c00 ee3c8a00 00000000 c0a0c4a8
>> [    1.858409] 1dc0: ee269c10 c1704e48 c186603c 00000000 c186603c 00000000 00000000 bc98ab22
>> [    1.866577] 1de0: ffffffff ee269c10 00000000 c186603c ee269c00 c186603c 00000000 00000000
>> [    1.874744] 1e00: c1656690 c0a0ffe0 00000000 bc98ab22 ee269c10 ee269c10 00000000 c186603c
>> [    1.882913] 1e20: 00000000 c186603c 00000000 c09887e0 c18ff9dc ee269c10 c18ff9e0 c0986860
>> [    1.891082] 1e40: ee269c10 c186603c c186603c c1704e48 00000000 c15003f0 c15c3854 c0986af0
>> [    1.899249] 1e60: c15c3854 c0d128b4 c10e48ec ee269c10 00000000 c186603c c1704e48 00000000
>> [    1.907416] 1e80: c15003f0 c15c3854 c1656690 c0986da0 00000000 c186603c ee269c10 c0986e28
>> [    1.915583] 1ea0: 00000000 c186603c c0986da8 c0984ba0 c15003f0 ee20c058 ee242334 bc98ab22
>> [    1.923752] 1ec0: c18588c8 c186603c ee737200 c18588c8 00000000 c0985b94 c133ef10 ffffe000
>> [    1.931919] 1ee0: c186603c c186603c c18aaf80 ffffe000 c158b72c c09878ac c1704e48 c18aaf80
>> [    1.940088] 1f00: ffffe000 c0302f80 00000168 c0367d84 c143e5b4 c1371000 00000000 00000006
>> [    1.948255] 1f20: 00000006 c125b1b4 00000000 c1704e48 c126f324 c125b228 00000000 efffec88
>> [    1.956424] 1f40: 00000000 bc98ab22 00000000 c18b6bc0 c18b6bc0 bc98ab22 c18b6bc0 c18b6bc0
>> [    1.964591] 1f60: 00000007 c15c3834 00000169 c1500f28 00000006 00000006 00000000 c15003f0
>> [    1.972758] 1f80: 00000000 00000000 c0ef1cdc 00000000 00000000 00000000 00000000 00000000
>> [    1.980924] 1fa0: 00000000 c0ef1ce4 00000000 c03010e8 00000000 00000000 00000000 00000000
>> [    1.989092] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [    1.997260] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
>> [    2.005440] [<c09832e4>] (device_link_add) from [<c0707d34>] (devm_of_phy_get+0x6c/0xb0)
>> [    2.013528] [<c0707d34>] (devm_of_phy_get) from [<c0a0bd2c>] (ahci_platform_get_phy+0x28/0xd0)
>> [    2.022134] [<c0a0bd2c>] (ahci_platform_get_phy) from [<c0a0c4a8>] (ahci_platform_get_resources+0x384/0x468)
>> [    2.031952] [<c0a0c4a8>] (ahci_platform_get_resources) from [<c0a0ffe0>] (tegra_ahci_probe+0x14/0x650)
>> [    2.041254] [<c0a0ffe0>] (tegra_ahci_probe) from [<c09887e0>] (platform_drv_probe+0x48/0x98)
>> [    2.049686] [<c09887e0>] (platform_drv_probe) from [<c0986860>] (really_probe+0x234/0x34c)
>> [    2.057944] [<c0986860>] (really_probe) from [<c0986af0>] (driver_probe_device+0x60/0x168)
>> [    2.066202] [<c0986af0>] (driver_probe_device) from [<c0986da0>] (device_driver_attach+0x58/0x60)
>> [    2.075064] [<c0986da0>] (device_driver_attach) from [<c0986e28>] (__driver_attach+0x80/0xbc)
>> [    2.083582] [<c0986e28>] (__driver_attach) from [<c0984ba0>] (bus_for_each_dev+0x74/0xb4)
>> [    2.091751] [<c0984ba0>] (bus_for_each_dev) from [<c0985b94>] (bus_add_driver+0x164/0x1e8)
>> [    2.100008] [<c0985b94>] (bus_add_driver) from [<c09878ac>] (driver_register+0x7c/0x114)
>> [    2.108094] [<c09878ac>] (driver_register) from [<c0302f80>] (do_one_initcall+0x54/0x22c)
>> [    2.116271] [<c0302f80>] (do_one_initcall) from [<c1500f28>] (kernel_init_freeable+0x14c/0x1b0)
>> [    2.124967] [<c1500f28>] (kernel_init_freeable) from [<c0ef1ce4>] (kernel_init+0x8/0x10c)
>> [    2.133139] [<c0ef1ce4>] (kernel_init) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
>> [    2.140697] Exception stack(0xee0e1fb0 to 0xee0e1ff8)
>> [    2.145743] 1fa0:                                     00000000 00000000 00000000 00000000
>> [    2.153910] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [    2.162076] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> [    2.168686] Code: e59f0470 03844040 eb15cb16 eb004c8a (e5d63084)
>> [    2.174824] ---[ end trace fddbf111e88ec722 ]---
>>
>>
>> I believe that there is a bug in this patch and the following fixed it for me ...
>>
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> index 8dfb4868c8c3..2eb28cc2d2dc 100644
>> --- a/drivers/phy/phy-core.c
>> +++ b/drivers/phy/phy-core.c
>> @@ -799,6 +799,7 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>>                  devres_add(dev, ptr);
>>          } else {
>>                  devres_free(ptr);
>> +               return phy;
>>          }
>>   
>>          link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> 
> Thank you for spotting this. I've included the fix now.

Thanks guys, and sorry for this regression.

alex

> 
> Thanks
> Kishon
>

youling 257 Feb. 6, 2020, 1:39 p.m. UTC | #6

This patch cause "dwc3 dwc3.3.auto: failed to create device link to dwc3.3.auto.ulpi" problem.
https://bugzilla.kernel.org/show_bug.cgi?id=206435

Kishon Vijay Abraham I Feb. 7, 2020, 5:16 a.m. UTC | #7

Hi,

On 06/02/20 7:09 PM, youling257 wrote:
> This patch cause "dwc3 dwc3.3.auto: failed to create device link to dwc3.3.auto.ulpi" problem.
> https://bugzilla.kernel.org/show_bug.cgi?id=206435

I'm suspecting there is some sort of reverse dependency with dwc3 ULPI.
Can you try the following diff?

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 2eb28cc2d2dc..397311dcb116 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char
*string)

        get_device(&phy->dev);

-       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
+       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
        if (!link) {
                dev_err(dev, "failed to create device link to %s\n",
                        dev_name(phy->dev.parent));
@@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
struct device_node *np,
                return phy;
        }

-       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
+       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
        if (!link) {
                dev_err(dev, "failed to create device link to %s\n",
                        dev_name(phy->dev.parent));
@@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device
*dev, struct device_node *np,
        *ptr = phy;
        devres_add(dev, ptr);

-       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
+       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
        if (!link) {
                dev_err(dev, "failed to create device link to %s\n",
                        dev_name(phy->dev.parent));

Thanks
Kishon

youling 257 Feb. 7, 2020, 6:57 a.m. UTC | #8

test this diff, dwc3 work for my device, thanks.

2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <[e-mail]>:
> Hi,
>
> On 06/02/20 7:09 PM, youling257 wrote:
>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to
>> dwc3.3.auto.ulpi" problem.
>> https://bugzilla.kernel.org/show_bug.cgi?id=206435
>
> I'm suspecting there is some sort of reverse dependency with dwc3 ULPI.
> Can you try the following diff?
>
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 2eb28cc2d2dc..397311dcb116 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char
> *string)
>
>         get_device(&phy->dev);
>
> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>         if (!link) {
>                 dev_err(dev, "failed to create device link to %s\n",
>                         dev_name(phy->dev.parent));
> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
> struct device_node *np,
>                 return phy;
>         }
>
> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>         if (!link) {
>                 dev_err(dev, "failed to create device link to %s\n",
>                         dev_name(phy->dev.parent));
> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device
> *dev, struct device_node *np,
>         *ptr = phy;
>         devres_add(dev, ptr);
>
> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>         if (!link) {
>                 dev_err(dev, "failed to create device link to %s\n",
>                         dev_name(phy->dev.parent));
>
> Thanks
> Kishon
>

Kishon Vijay Abraham I Feb. 10, 2020, 8:08 a.m. UTC | #9

Hi Alexandre,

On 07/02/20 12:27 PM, youling 257 wrote:
> test this diff, dwc3 work for my device, thanks.
> 
> 2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <[e-mail]>:
>> Hi,
>>
>> On 06/02/20 7:09 PM, youling257 wrote:
>>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to
>>> dwc3.3.auto.ulpi" problem.
>>> https://bugzilla.kernel.org/show_bug.cgi?id=206435
>>
>> I'm suspecting there is some sort of reverse dependency with dwc3 ULPI.
>> Can you try the following diff?
>>
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> index 2eb28cc2d2dc..397311dcb116 100644
>> --- a/drivers/phy/phy-core.c
>> +++ b/drivers/phy/phy-core.c
>> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char
>> *string)
>>
>>         get_device(&phy->dev);
>>
>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>>         if (!link) {
>>                 dev_err(dev, "failed to create device link to %s\n",
>>                         dev_name(phy->dev.parent));
>> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
>> struct device_node *np,
>>                 return phy;
>>         }
>>
>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>>         if (!link) {
>>                 dev_err(dev, "failed to create device link to %s\n",
>>                         dev_name(phy->dev.parent));
>> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device
>> *dev, struct device_node *np,
>>         *ptr = phy;
>>         devres_add(dev, ptr);
>>
>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>>         if (!link) {
>>                 dev_err(dev, "failed to create device link to %s\n",
>>                         dev_name(phy->dev.parent));Parent

Can you check if this doesn't affect the suspend/resume ordering?

Thanks
Kishon

Alexandre TORGUE Feb. 10, 2020, 11:30 a.m. UTC | #10

Hi Kishon,

On 2/10/20 9:08 AM, Kishon Vijay Abraham I wrote:
> Hi Alexandre,
> 
> On 07/02/20 12:27 PM, youling 257 wrote:
>> test this diff, dwc3 work for my device, thanks.
>>
>> 2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <[e-mail]>:
>>> Hi,
>>>
>>> On 06/02/20 7:09 PM, youling257 wrote:
>>>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to
>>>> dwc3.3.auto.ulpi" problem.
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=206435
>>>
>>> I'm suspecting there is some sort of reverse dependency with dwc3 ULPI.
>>> Can you try the following diff?
>>>
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index 2eb28cc2d2dc..397311dcb116 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char
>>> *string)
>>>
>>>          get_device(&phy->dev);
>>>
>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>>>          if (!link) {
>>>                  dev_err(dev, "failed to create device link to %s\n",
>>>                          dev_name(phy->dev.parent));
>>> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
>>> struct device_node *np,
>>>                  return phy;
>>>          }
>>>
>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>>>          if (!link) {
>>>                  dev_err(dev, "failed to create device link to %s\n",
>>>                          dev_name(phy->dev.parent));
>>> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device
>>> *dev, struct device_node *np,
>>>          *ptr = phy;
>>>          devres_add(dev, ptr);
>>>
>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>>>          if (!link) {
>>>                  dev_err(dev, "failed to create device link to %s\n",
>>>                          dev_name(phy->dev.parent));Parent
> 
> Can you check if this doesn't affect the suspend/resume ordering?

With this fix, suspend/resume ordering is broken on my side. What do you 
think to keep the STATELESS flag and to only display a warn if 
"device_link_add" returns an error ? It's not "smart" but it could 
solved our issue.

As a lot of improvements have been recently done on device link topic by 
Saravana, we could check with him what is the way to follow.

Regards
Alex

> 
> Thanks
> Kishon
>

Kishon Vijay Abraham I Feb. 10, 2020, 12:18 p.m. UTC | #11

Hi,

On 10/02/20 5:00 PM, Alexandre Torgue wrote:
> Hi Kishon,
> 
> On 2/10/20 9:08 AM, Kishon Vijay Abraham I wrote:
>> Hi Alexandre,
>>
>> On 07/02/20 12:27 PM, youling 257 wrote:
>>> test this diff, dwc3 work for my device, thanks.
>>>
>>> 2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <[e-mail]>:
>>>> Hi,
>>>>
>>>> On 06/02/20 7:09 PM, youling257 wrote:
>>>>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to
>>>>> dwc3.3.auto.ulpi" problem.
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=206435
>>>>
>>>> I'm suspecting there is some sort of reverse dependency with dwc3 ULPI.
>>>> Can you try the following diff?
>>>>
>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>> index 2eb28cc2d2dc..397311dcb116 100644
>>>> --- a/drivers/phy/phy-core.c
>>>> +++ b/drivers/phy/phy-core.c
>>>> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char
>>>> *string)
>>>>
>>>>          get_device(&phy->dev);
>>>>
>>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>>> +       link = device_link_add(dev, &phy->dev,
>>>> DL_FLAG_SYNC_STATE_ONLY);
>>>>          if (!link) {
>>>>                  dev_err(dev, "failed to create device link to %s\n",
>>>>                          dev_name(phy->dev.parent));
>>>> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
>>>> struct device_node *np,
>>>>                  return phy;
>>>>          }
>>>>
>>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>>> +       link = device_link_add(dev, &phy->dev,
>>>> DL_FLAG_SYNC_STATE_ONLY);
>>>>          if (!link) {
>>>>                  dev_err(dev, "failed to create device link to %s\n",
>>>>                          dev_name(phy->dev.parent));
>>>> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device
>>>> *dev, struct device_node *np,
>>>>          *ptr = phy;
>>>>          devres_add(dev, ptr);
>>>>
>>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>>> +       link = device_link_add(dev, &phy->dev,
>>>> DL_FLAG_SYNC_STATE_ONLY);
>>>>          if (!link) {
>>>>                  dev_err(dev, "failed to create device link to %s\n",
>>>>                          dev_name(phy->dev.parent));Parent
>>
>> Can you check if this doesn't affect the suspend/resume ordering?
> 
> With this fix, suspend/resume ordering is broken on my side. What do you
> think to keep the STATELESS flag and to only display a warn if
> "device_link_add" returns an error ? It's not "smart" but it could
> solved our issue.

yeah, that sounds reasonable for now. Can you find out the dependencies
between dwc3 and ulpi and what exactly breaks. That would help Saravana
to suggest a fix?
> 
> As a lot of improvements have been recently done on device link topic by
> Saravana, we could check with him what is the way to follow.
Thanks
Kishon

Saravana Kannan Feb. 11, 2020, 6:17 a.m. UTC | #12

On Mon, Feb 10, 2020 at 4:14 AM Kishon Vijay Abraham I <[e-mail]> wrote:
>
> Hi,
>
> On 10/02/20 5:00 PM, Alexandre Torgue wrote:
> > Hi Kishon,
> >
> > On 2/10/20 9:08 AM, Kishon Vijay Abraham I wrote:
> >> Hi Alexandre,
> >>
> >> On 07/02/20 12:27 PM, youling 257 wrote:
> >>> test this diff, dwc3 work for my device, thanks.
> >>>
> >>> 2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <[e-mail]>:
> >>>> Hi,
> >>>>
> >>>> On 06/02/20 7:09 PM, youling257 wrote:
> >>>>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to
> >>>>> dwc3.3.auto.ulpi" problem.
> >>>>> https://bugzilla.kernel.org/show_bug.cgi?id=206435
> >>>>
> >>>> I'm suspecting there is some sort of reverse dependency with dwc3 ULPI.
> >>>> Can you try the following diff?
> >>>>
> >>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> >>>> index 2eb28cc2d2dc..397311dcb116 100644
> >>>> --- a/drivers/phy/phy-core.c
> >>>> +++ b/drivers/phy/phy-core.c
> >>>> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char
> >>>> *string)
> >>>>
> >>>>          get_device(&phy->dev);
> >>>>
> >>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> >>>> +       link = device_link_add(dev, &phy->dev,
> >>>> DL_FLAG_SYNC_STATE_ONLY);
> >>>>          if (!link) {
> >>>>                  dev_err(dev, "failed to create device link to %s\n",
> >>>>                          dev_name(phy->dev.parent));
> >>>> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
> >>>> struct device_node *np,
> >>>>                  return phy;
> >>>>          }
> >>>>
> >>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> >>>> +       link = device_link_add(dev, &phy->dev,
> >>>> DL_FLAG_SYNC_STATE_ONLY);

Definitely don't use SYNC_STATE_ONLY.

To give some context, drivers themselves are only expected to use
STATELESS links. Only the driver core is supposed to use MANAGED (if
you don't use STATELESS, it's MANAGED by default) links. And
SYNC_STATE_ONLY makes sense only for MANAGED links.

Also, as the SYNC_STATE_ONLY documentation says, it only affect
sync_state() behavior and doesn't affect suspend/resume in any way.

> >>>>          if (!link) {
> >>>>                  dev_err(dev, "failed to create device link to %s\n",
> >>>>                          dev_name(phy->dev.parent));
> >>>> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device
> >>>> *dev, struct device_node *np,
> >>>>          *ptr = phy;
> >>>>          devres_add(dev, ptr);
> >>>>
> >>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> >>>> +       link = device_link_add(dev, &phy->dev,
> >>>> DL_FLAG_SYNC_STATE_ONLY);
> >>>>          if (!link) {
> >>>>                  dev_err(dev, "failed to create device link to %s\n",
> >>>>                          dev_name(phy->dev.parent));Parent
> >>
> >> Can you check if this doesn't affect the suspend/resume ordering?
> >
> > With this fix, suspend/resume ordering is broken on my side. What do you
> > think to keep the STATELESS flag and to only display a warn if
> > "device_link_add" returns an error ? It's not "smart" but it could
> > solved our issue.
>
> yeah, that sounds reasonable for now. Can you find out the dependencies
> between dwc3 and ulpi and what exactly breaks. That would help Saravana
> to suggest a fix?

Yup, I don't have enough context of the dependencies here to suggest a
good fix. But if device_link_add() is failing with STATELESS and not
failing with SYNC_STATE_ONLY, I'm pretty sure you have a cyclic
dependency between these devices when you create this link. Keep in
mind that it can be a cycle involving more than 2 devices -- A -> B ->
C -> A. And cycles don't make sense when you are trying to order
suspend/resume. Looks like the new link is wrong or an existing link
is wrong. If the dependencies are actually correct in hardware, then
maybe your hardware device needs to be split into multiple devices so
that you don't have cycles and can suspend in a meaningful way?

Hope that helps.

Thanks,
Saravana

Andy Shevchenko Feb. 14, 2020, 6:46 p.m. UTC | #13

On Mon, Feb 10, 2020 at 1:32 PM Alexandre Torgue
<[e-mail]> wrote:
> On 2/10/20 9:08 AM, Kishon Vijay Abraham I wrote:
> > On 07/02/20 12:27 PM, youling 257 wrote:
> >> test this diff, dwc3 work for my device, thanks.
> >>
> >> 2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <[e-mail]>:
> >>> On 06/02/20 7:09 PM, youling257 wrote:
> >>>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to
> >>>> dwc3.3.auto.ulpi" problem.
> >>>> https://bugzilla.kernel.org/show_bug.cgi?id=206435

+1 to the report.
Please revert for v5.6 or provide a fix ASAP!

> >>>
> >>> I'm suspecting there is some sort of reverse dependency with dwc3 ULPI.
> >>> Can you try the following diff?
> >>>
> >>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> >>> index 2eb28cc2d2dc..397311dcb116 100644
> >>> --- a/drivers/phy/phy-core.c
> >>> +++ b/drivers/phy/phy-core.c
> >>> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char
> >>> *string)
> >>>
> >>>          get_device(&phy->dev);
> >>>
> >>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> >>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
> >>>          if (!link) {
> >>>                  dev_err(dev, "failed to create device link to %s\n",
> >>>                          dev_name(phy->dev.parent));
> >>> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
> >>> struct device_node *np,
> >>>                  return phy;
> >>>          }
> >>>
> >>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> >>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
> >>>          if (!link) {
> >>>                  dev_err(dev, "failed to create device link to %s\n",
> >>>                          dev_name(phy->dev.parent));
> >>> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device
> >>> *dev, struct device_node *np,
> >>>          *ptr = phy;
> >>>          devres_add(dev, ptr);
> >>>
> >>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
> >>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
> >>>          if (!link) {
> >>>                  dev_err(dev, "failed to create device link to %s\n",
> >>>                          dev_name(phy->dev.parent));Parent
> >
> > Can you check if this doesn't affect the suspend/resume ordering?
>
> With this fix, suspend/resume ordering is broken on my side. What do you
> think to keep the STATELESS flag and to only display a warn if
> "device_link_add" returns an error ? It's not "smart" but it could
> solved our issue.
>
> As a lot of improvements have been recently done on device link topic by
> Saravana, we could check with him what is the way to follow.
>
> Regards
> Alex
>
> >
> > Thanks
> > Kishon
> >

Alexandre TORGUE Feb. 17, 2020, 8:35 a.m. UTC | #14

Hi,

On 2/14/20 7:46 PM, Andy Shevchenko wrote:
> On Mon, Feb 10, 2020 at 1:32 PM Alexandre Torgue
> <[e-mail]> wrote:
>> On 2/10/20 9:08 AM, Kishon Vijay Abraham I wrote:
>>> On 07/02/20 12:27 PM, youling 257 wrote:
>>>> test this diff, dwc3 work for my device, thanks.
>>>>
>>>> 2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <[e-mail]>:
>>>>> On 06/02/20 7:09 PM, youling257 wrote:
>>>>>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to
>>>>>> dwc3.3.auto.ulpi" problem.
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=206435
> 
> +1 to the report.
> Please revert for v5.6 or provide a fix ASAP!
> 

Kishon, do you plan to do the fix? If not, I'll send it quickly.

Thanks
Alex

>>>>>
>>>>> I'm suspecting there is some sort of reverse dependency with dwc3 ULPI.
>>>>> Can you try the following diff?
>>>>>
>>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>>> index 2eb28cc2d2dc..397311dcb116 100644
>>>>> --- a/drivers/phy/phy-core.c
>>>>> +++ b/drivers/phy/phy-core.c
>>>>> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char
>>>>> *string)
>>>>>
>>>>>           get_device(&phy->dev);
>>>>>
>>>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>>>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>>>>>           if (!link) {
>>>>>                   dev_err(dev, "failed to create device link to %s\n",
>>>>>                           dev_name(phy->dev.parent));
>>>>> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
>>>>> struct device_node *np,
>>>>>                   return phy;
>>>>>           }
>>>>>
>>>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>>>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>>>>>           if (!link) {
>>>>>                   dev_err(dev, "failed to create device link to %s\n",
>>>>>                           dev_name(phy->dev.parent));
>>>>> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device
>>>>> *dev, struct device_node *np,
>>>>>           *ptr = phy;
>>>>>           devres_add(dev, ptr);
>>>>>
>>>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>>>> +       link = device_link_add(dev, &phy->dev, DL_FLAG_SYNC_STATE_ONLY);
>>>>>           if (!link) {
>>>>>                   dev_err(dev, "failed to create device link to %s\n",
>>>>>                           dev_name(phy->dev.parent));Parent
>>>
>>> Can you check if this doesn't affect the suspend/resume ordering?
>>
>> With this fix, suspend/resume ordering is broken on my side. What do you
>> think to keep the STATELESS flag and to only display a warn if
>> "device_link_add" returns an error ? It's not "smart" but it could
>> solved our issue.
>>
>> As a lot of improvements have been recently done on device link topic by
>> Saravana, we could check with him what is the way to follow.
>>
>> Regards
>> Alex
>>
>>>
>>> Thanks
>>> Kishon
>>>
> 
> 
>

Kishon Vijay Abraham I Feb. 17, 2020, 8:40 a.m. UTC | #15

Hi,

On 17/02/20 2:05 pm, Alexandre Torgue wrote:
> Hi,
> 
> On 2/14/20 7:46 PM, Andy Shevchenko wrote:
>> On Mon, Feb 10, 2020 at 1:32 PM Alexandre Torgue
>> <[e-mail]> wrote:
>>> On 2/10/20 9:08 AM, Kishon Vijay Abraham I wrote:
>>>> On 07/02/20 12:27 PM, youling 257 wrote:
>>>>> test this diff, dwc3 work for my device, thanks.
>>>>>
>>>>> 2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <[e-mail]>:
>>>>>> On 06/02/20 7:09 PM, youling257 wrote:
>>>>>>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to
>>>>>>> dwc3.3.auto.ulpi" problem.
>>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=206435
>>
>> +1 to the report.
>> Please revert for v5.6 or provide a fix ASAP!
>>
> 
> Kishon, do you plan to do the fix? If not, I'll send it quickly.

Please send a fix for it.

Thanks
Kishon

> 
> Thanks
> Alex
> 
>>>>>>
>>>>>> I'm suspecting there is some sort of reverse dependency with dwc3
>>>>>> ULPI.
>>>>>> Can you try the following diff?
>>>>>>
>>>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>>>> index 2eb28cc2d2dc..397311dcb116 100644
>>>>>> --- a/drivers/phy/phy-core.c
>>>>>> +++ b/drivers/phy/phy-core.c
>>>>>> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const
>>>>>> char
>>>>>> *string)
>>>>>>
>>>>>>           get_device(&phy->dev);
>>>>>>
>>>>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>>>>> +       link = device_link_add(dev, &phy->dev,
>>>>>> DL_FLAG_SYNC_STATE_ONLY);
>>>>>>           if (!link) {
>>>>>>                   dev_err(dev, "failed to create device link to
>>>>>> %s\n",
>>>>>>                           dev_name(phy->dev.parent));
>>>>>> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev,
>>>>>> struct device_node *np,
>>>>>>                   return phy;
>>>>>>           }
>>>>>>
>>>>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>>>>> +       link = device_link_add(dev, &phy->dev,
>>>>>> DL_FLAG_SYNC_STATE_ONLY);
>>>>>>           if (!link) {
>>>>>>                   dev_err(dev, "failed to create device link to
>>>>>> %s\n",
>>>>>>                           dev_name(phy->dev.parent));
>>>>>> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct
>>>>>> device
>>>>>> *dev, struct device_node *np,
>>>>>>           *ptr = phy;
>>>>>>           devres_add(dev, ptr);
>>>>>>
>>>>>> -       link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
>>>>>> +       link = device_link_add(dev, &phy->dev,
>>>>>> DL_FLAG_SYNC_STATE_ONLY);
>>>>>>           if (!link) {
>>>>>>                   dev_err(dev, "failed to create device link to
>>>>>> %s\n",
>>>>>>                           dev_name(phy->dev.parent));Parent
>>>>
>>>> Can you check if this doesn't affect the suspend/resume ordering?
>>>
>>> With this fix, suspend/resume ordering is broken on my side. What do you
>>> think to keep the STATELESS flag and to only display a warn if
>>> "device_link_add" returns an error ? It's not "smart" but it could
>>> solved our issue.
>>>
>>> As a lot of improvements have been recently done on device link topic by
>>> Saravana, we could check with him what is the way to follow.
>>>
>>> Regards
>>> Alex
>>>
>>>>
>>>> Thanks
>>>> Kishon
>>>>
>>
>>
>>

Patch

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index b04f4fe85ac2..8dfb4868c8c3 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -29,7 +29,7 @@  static void devm_phy_release(struct device *dev, void *res)
 {
 	struct phy *phy = *(struct phy **)res;
 
-	phy_put(phy);
+	phy_put(dev, phy);
 }
 
 static void devm_phy_provider_release(struct device *dev, void *res)
@@ -566,12 +566,12 @@  struct phy *of_phy_get(struct device_node *np, const char *con_id)
 EXPORT_SYMBOL_GPL(of_phy_get);
 
 /**
- * phy_put() - release the PHY
- * @phy: the phy returned by phy_get()
+ * of_phy_put() - release the PHY
+ * @phy: the phy returned by of_phy_get()
  *
- * Releases a refcount the caller received from phy_get().
+ * Releases a refcount the caller received from of_phy_get().
  */
-void phy_put(struct phy *phy)
+void of_phy_put(struct phy *phy)
 {
 	if (!phy || IS_ERR(phy))
 		return;
@@ -584,6 +584,20 @@  void phy_put(struct phy *phy)
 	module_put(phy->ops->owner);
 	put_device(&phy->dev);
 }
+EXPORT_SYMBOL_GPL(of_phy_put);
+
+/**
+ * phy_put() - release the PHY
+ * @dev: device that wants to release this phy
+ * @phy: the phy returned by phy_get()
+ *
+ * Releases a refcount the caller received from phy_get().
+ */
+void phy_put(struct device *dev, struct phy *phy)
+{
+	device_link_remove(dev, &phy->dev);
+	of_phy_put(phy);
+}
 EXPORT_SYMBOL_GPL(phy_put);
 
 /**
@@ -651,6 +665,7 @@  struct phy *phy_get(struct device *dev, const char *string)
 {
 	int index = 0;
 	struct phy *phy;
+	struct device_link *link;
 
 	if (string == NULL) {
 		dev_WARN(dev, "missing string\n");
@@ -672,6 +687,13 @@  struct phy *phy_get(struct device *dev, const char *string)
 
 	get_device(&phy->dev);
 
+	link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
+	if (!link) {
+		dev_err(dev, "failed to create device link to %s\n",
+			dev_name(phy->dev.parent));
+		return ERR_PTR(-EINVAL);
+	}
+
 	return phy;
 }
 EXPORT_SYMBOL_GPL(phy_get);
@@ -765,6 +787,7 @@  struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
 			    const char *con_id)
 {
 	struct phy **ptr, *phy;
+	struct device_link *link;
 
 	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
 	if (!ptr)
@@ -778,6 +801,13 @@  struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
 		devres_free(ptr);
 	}
 
+	link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
+	if (!link) {
+		dev_err(dev, "failed to create device link to %s\n",
+			dev_name(phy->dev.parent));
+		return ERR_PTR(-EINVAL);
+	}
+
 	return phy;
 }
 EXPORT_SYMBOL_GPL(devm_of_phy_get);
@@ -798,6 +828,7 @@  struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
 				     int index)
 {
 	struct phy **ptr, *phy;
+	struct device_link *link;
 
 	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
 	if (!ptr)
@@ -819,6 +850,13 @@  struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
 	*ptr = phy;
 	devres_add(dev, ptr);
 
+	link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
+	if (!link) {
+		dev_err(dev, "failed to create device link to %s\n",
+			dev_name(phy->dev.parent));
+		return ERR_PTR(-EINVAL);
+	}
+
 	return phy;
 }
 EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
diff --git a/drivers/usb/renesas_usbhs/rcar2.c b/drivers/usb/renesas_usbhs/rcar2.c
index 440d213e1749..791908f8cf73 100644
--- a/drivers/usb/renesas_usbhs/rcar2.c
+++ b/drivers/usb/renesas_usbhs/rcar2.c
@@ -34,7 +34,7 @@  static int usbhs_rcar2_hardware_exit(struct platform_device *pdev)
 	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
 
 	if (priv->phy) {
-		phy_put(priv->phy);
+		phy_put(&pdev->dev, priv->phy);
 		priv->phy = NULL;
 	}
 
diff --git a/drivers/usb/renesas_usbhs/rza2.c b/drivers/usb/renesas_usbhs/rza2.c
index 021749594389..3eed3334a17f 100644
--- a/drivers/usb/renesas_usbhs/rza2.c
+++ b/drivers/usb/renesas_usbhs/rza2.c
@@ -29,7 +29,7 @@  static int usbhs_rza2_hardware_exit(struct platform_device *pdev)
 {
 	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
 
-	phy_put(priv->phy);
+	phy_put(&pdev->dev, priv->phy);
 	priv->phy = NULL;
 
 	return 0;
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 56d3a100006a..19eddd64c8f6 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -234,7 +234,8 @@  struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
 			    const char *con_id);
 struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
 				     int index);
-void phy_put(struct phy *phy);
+void of_phy_put(struct phy *phy);
+void phy_put(struct device *dev, struct phy *phy);
 void devm_phy_put(struct device *dev, struct phy *phy);
 struct phy *of_phy_get(struct device_node *np, const char *con_id);
 struct phy *of_phy_simple_xlate(struct device *dev,
@@ -419,7 +420,11 @@  static inline struct phy *devm_of_phy_get_by_index(struct device *dev,
 	return ERR_PTR(-ENOSYS);
 }
 
-static inline void phy_put(struct phy *phy)
+static inline void of_phy_put(struct phy *phy)
+{
+}
+
+static inline void phy_put(struct device *dev, struct phy *phy)
 {
 }