diff mbox series

[v2,1/1] net: phy: Add link between phy dev and mac dev

Message ID 20221125041206.1883833-2-xiaolei.wang@windriver.com
State New
Headers
Series net: phy: Add link between phy dev and mac dev |

Commit Message

xiaolei wang Nov. 25, 2022, 4:12 a.m. UTC 
  If the external phy used by current mac interface is
managed by another mac interface, it means that this
network port cannot work independently, especially
when the system suspend and resume, the following
trace may appear, so we should create a device link
between phy dev and mac dev.

  WARNING: CPU: 0 PID: 24 at drivers/net/phy/phy.c:983 phy_error+0x20/0x68
  Modules linked in:
  CPU: 0 PID: 24 Comm: kworker/0:2 Not tainted 6.1.0-rc3-00011-g5aaef24b5c6d-dirty #34
  Hardware name: Freescale i.MX6 SoloX (Device Tree)
  Workqueue: events_power_efficient phy_state_machine
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x68/0x90
  dump_stack_lvl from __warn+0xb4/0x24c
  __warn from warn_slowpath_fmt+0x5c/0xd8
  warn_slowpath_fmt from phy_error+0x20/0x68
  phy_error from phy_state_machine+0x22c/0x23c
  phy_state_machine from process_one_work+0x288/0x744
  process_one_work from worker_thread+0x3c/0x500
  worker_thread from kthread+0xf0/0x114
  kthread from ret_from_fork+0x14/0x28
  Exception stack(0xf0951fb0 to 0xf0951ff8)

Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
---
 drivers/net/phy/phy_device.c | 12 ++++++++++++
 include/linux/phy.h          |  2 ++
 2 files changed, 14 insertions(+)

Comments

Andrew Lunn Nov. 25, 2022, 9:43 p.m. UTC | #1 
On Fri, Nov 25, 2022 at 12:12:06PM +0800, Xiaolei Wang wrote:
> If the external phy used by current mac interface is
> managed by another mac interface, it means that this
> network port cannot work independently, especially
> when the system suspend and resume, the following
> trace may appear, so we should create a device link
> between phy dev and mac dev.
> 
>   WARNING: CPU: 0 PID: 24 at drivers/net/phy/phy.c:983 phy_error+0x20/0x68
>   Modules linked in:
>   CPU: 0 PID: 24 Comm: kworker/0:2 Not tainted 6.1.0-rc3-00011-g5aaef24b5c6d-dirty #34
>   Hardware name: Freescale i.MX6 SoloX (Device Tree)
>   Workqueue: events_power_efficient phy_state_machine
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x68/0x90
>   dump_stack_lvl from __warn+0xb4/0x24c
>   __warn from warn_slowpath_fmt+0x5c/0xd8
>   warn_slowpath_fmt from phy_error+0x20/0x68
>   phy_error from phy_state_machine+0x22c/0x23c
>   phy_state_machine from process_one_work+0x288/0x744
>   process_one_work from worker_thread+0x3c/0x500
>   worker_thread from kthread+0xf0/0x114
>   kthread from ret_from_fork+0x14/0x28
>   Exception stack(0xf0951fb0 to 0xf0951ff8)
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>

This needs Florians review, since for v1 he thinks it will cause
regressions.

	Andrew
xiaolei wang Nov. 26, 2022, 1:41 a.m. UTC | #2 
add Florian

thanks

xiaolei

On 11/26/2022 5:43 AM, Andrew Lunn wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On Fri, Nov 25, 2022 at 12:12:06PM +0800, Xiaolei Wang wrote:
>> If the external phy used by current mac interface is
>> managed by another mac interface, it means that this
>> network port cannot work independently, especially
>> when the system suspend and resume, the following
>> trace may appear, so we should create a device link
>> between phy dev and mac dev.
>>
>>    WARNING: CPU: 0 PID: 24 at drivers/net/phy/phy.c:983 phy_error+0x20/0x68
>>    Modules linked in:
>>    CPU: 0 PID: 24 Comm: kworker/0:2 Not tainted 6.1.0-rc3-00011-g5aaef24b5c6d-dirty #34
>>    Hardware name: Freescale i.MX6 SoloX (Device Tree)
>>    Workqueue: events_power_efficient phy_state_machine
>>    unwind_backtrace from show_stack+0x10/0x14
>>    show_stack from dump_stack_lvl+0x68/0x90
>>    dump_stack_lvl from __warn+0xb4/0x24c
>>    __warn from warn_slowpath_fmt+0x5c/0xd8
>>    warn_slowpath_fmt from phy_error+0x20/0x68
>>    phy_error from phy_state_machine+0x22c/0x23c
>>    phy_state_machine from process_one_work+0x288/0x744
>>    process_one_work from worker_thread+0x3c/0x500
>>    worker_thread from kthread+0xf0/0x114
>>    kthread from ret_from_fork+0x14/0x28
>>    Exception stack(0xf0951fb0 to 0xf0951ff8)
>>
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> This needs Florians review, since for v1 he thinks it will cause
> regressions.
>
>          Andrew
Florian Fainelli Nov. 27, 2022, 8:30 p.m. UTC | #3 
On 11/25/2022 5:41 PM, Wang, Xiaolei wrote:
> add Florian
> 
> thanks
> 
> xiaolei
> 
> On 11/26/2022 5:43 AM, Andrew Lunn wrote:
>> CAUTION: This email comes from a non Wind River email account!
>> Do not click links or open attachments unless you recognize the sender 
>> and know the content is safe.
>>
>> On Fri, Nov 25, 2022 at 12:12:06PM +0800, Xiaolei Wang wrote:
>>> If the external phy used by current mac interface is
>>> managed by another mac interface, it means that this
>>> network port cannot work independently, especially
>>> when the system suspend and resume, the following
>>> trace may appear, so we should create a device link
>>> between phy dev and mac dev.
>>>
>>>    WARNING: CPU: 0 PID: 24 at drivers/net/phy/phy.c:983 
>>> phy_error+0x20/0x68
>>>    Modules linked in:
>>>    CPU: 0 PID: 24 Comm: kworker/0:2 Not tainted 
>>> 6.1.0-rc3-00011-g5aaef24b5c6d-dirty #34
>>>    Hardware name: Freescale i.MX6 SoloX (Device Tree)
>>>    Workqueue: events_power_efficient phy_state_machine
>>>    unwind_backtrace from show_stack+0x10/0x14
>>>    show_stack from dump_stack_lvl+0x68/0x90
>>>    dump_stack_lvl from __warn+0xb4/0x24c
>>>    __warn from warn_slowpath_fmt+0x5c/0xd8
>>>    warn_slowpath_fmt from phy_error+0x20/0x68
>>>    phy_error from phy_state_machine+0x22c/0x23c
>>>    phy_state_machine from process_one_work+0x288/0x744
>>>    process_one_work from worker_thread+0x3c/0x500
>>>    worker_thread from kthread+0xf0/0x114
>>>    kthread from ret_from_fork+0x14/0x28
>>>    Exception stack(0xf0951fb0 to 0xf0951ff8)
>>>
>>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> This needs Florians review, since for v1 he thinks it will cause
>> regressions.

Please give me until Tuesday to give this patch some proper testing, thanks!
xiaolei wang Nov. 28, 2022, 3:18 a.m. UTC | #4 
On 11/28/2022 4:30 AM, Florian Fainelli wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender 
> and know the content is safe.
>
> On 11/25/2022 5:41 PM, Wang, Xiaolei wrote:
>> add Florian
>>
>> thanks
>>
>> xiaolei
>>
>> On 11/26/2022 5:43 AM, Andrew Lunn wrote:
>>> CAUTION: This email comes from a non Wind River email account!
>>> Do not click links or open attachments unless you recognize the sender
>>> and know the content is safe.
>>>
>>> On Fri, Nov 25, 2022 at 12:12:06PM +0800, Xiaolei Wang wrote:
>>>> If the external phy used by current mac interface is
>>>> managed by another mac interface, it means that this
>>>> network port cannot work independently, especially
>>>> when the system suspend and resume, the following
>>>> trace may appear, so we should create a device link
>>>> between phy dev and mac dev.
>>>>
>>>>    WARNING: CPU: 0 PID: 24 at drivers/net/phy/phy.c:983
>>>> phy_error+0x20/0x68
>>>>    Modules linked in:
>>>>    CPU: 0 PID: 24 Comm: kworker/0:2 Not tainted
>>>> 6.1.0-rc3-00011-g5aaef24b5c6d-dirty #34
>>>>    Hardware name: Freescale i.MX6 SoloX (Device Tree)
>>>>    Workqueue: events_power_efficient phy_state_machine
>>>>    unwind_backtrace from show_stack+0x10/0x14
>>>>    show_stack from dump_stack_lvl+0x68/0x90
>>>>    dump_stack_lvl from __warn+0xb4/0x24c
>>>>    __warn from warn_slowpath_fmt+0x5c/0xd8
>>>>    warn_slowpath_fmt from phy_error+0x20/0x68
>>>>    phy_error from phy_state_machine+0x22c/0x23c
>>>>    phy_state_machine from process_one_work+0x288/0x744
>>>>    process_one_work from worker_thread+0x3c/0x500
>>>>    worker_thread from kthread+0xf0/0x114
>>>>    kthread from ret_from_fork+0x14/0x28
>>>>    Exception stack(0xf0951fb0 to 0xf0951ff8)
>>>>
>>>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>>> This needs Florians review, since for v1 he thinks it will cause
>>> regressions.
>
> Please give me until Tuesday to give this patch some proper testing, 
> thanks!

Hi

Test on imx6sx
Before adding the patch:

ifconfig eth1 up
echo enabled > /sys/class/tty/ttymxc0/power/wakeup
echo mem > /sys/power/state

The following problems arise:

WARNING: CPU: 0 PID: 24 at drivers/net/phy/phy.c:983
phy_error+0x20/0x68
    Modules linked in:
    CPU: 0 PID: 24 Comm: kworker/0:2 Not tainted
6.1.0-rc3-00011-g5aaef24b5c6d-dirty #34
    Hardware name: Freescale i.MX6 SoloX (Device Tree)
    Workqueue: events_power_efficient phy_state_machine
    unwind_backtrace from show_stack+0x10/0x14
    show_stack from dump_stack_lvl+0x68/0x90
    dump_stack_lvl from __warn+0xb4/0x24c
    __warn from warn_slowpath_fmt+0x5c/0xd8
    warn_slowpath_fmt from phy_error+0x20/0x68
    phy_error from phy_state_machine+0x22c/0x23c
    phy_state_machine from process_one_work+0x288/0x744
    process_one_work from worker_thread+0x3c/0x500
    worker_thread from kthread+0xf0/0x114
    kthread from ret_from_fork+0x14/0x28
    Exception stack (0xf0951fb0 to 0xf0951ff8)

After applying the patch:

ifconfig eth1 up

echo enabled > /sys/class/tty/ttymxc0/power/wakeup
echo mem > /sys/power/state

eth1 will link normally

Since I don't have more boards here, I haven't tested and analyzed the 
different situations of other boards. If you need more test records, 
please wait, I will collect some hardware to verify different situations.

thanks

xiaolei


> -- 
> Florian
Florian Fainelli Nov. 28, 2022, 9:05 p.m. UTC | #5 
On 11/24/22 20:12, Xiaolei Wang wrote:
> If the external phy used by current mac interface is
> managed by another mac interface, it means that this
> network port cannot work independently, especially
> when the system suspend and resume, the following
> trace may appear, so we should create a device link
> between phy dev and mac dev.
> 
>    WARNING: CPU: 0 PID: 24 at drivers/net/phy/phy.c:983 phy_error+0x20/0x68
>    Modules linked in:
>    CPU: 0 PID: 24 Comm: kworker/0:2 Not tainted 6.1.0-rc3-00011-g5aaef24b5c6d-dirty #34
>    Hardware name: Freescale i.MX6 SoloX (Device Tree)
>    Workqueue: events_power_efficient phy_state_machine
>    unwind_backtrace from show_stack+0x10/0x14
>    show_stack from dump_stack_lvl+0x68/0x90
>    dump_stack_lvl from __warn+0xb4/0x24c
>    __warn from warn_slowpath_fmt+0x5c/0xd8
>    warn_slowpath_fmt from phy_error+0x20/0x68
>    phy_error from phy_state_machine+0x22c/0x23c
>    phy_state_machine from process_one_work+0x288/0x744
>    process_one_work from worker_thread+0x3c/0x500
>    worker_thread from kthread+0xf0/0x114
>    kthread from ret_from_fork+0x14/0x28
>    Exception stack(0xf0951fb0 to 0xf0951ff8)
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Tested with bcmgenet and bcmsysport/bcm_sf2:

- suspend/resume using rtcwake w/ Wake-on-LAN disabled
- suspend/resume using rtcwale w/ Wake-on-LAN enabled
- reboot -f (which does exercise the shutdown path which has ties with 
device_links)
- binding/unbinding PHY driver

There was no change to the ordering for GENET, however there was a 
change of ordering for the DSA (bcmsysport/bcm_sf2) combination but it 
seemed to make more sense the way it was, in that we suspended the 
switch first and later the Ethernet controller attached to the switch.

Thanks for your patience.
Jakub Kicinski Nov. 29, 2022, 3 a.m. UTC | #6 
On Mon, 28 Nov 2022 13:05:09 -0800 Florian Fainelli wrote:
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks! Is this for next or for net?
Florian Fainelli Nov. 29, 2022, 3:02 a.m. UTC | #7 
On 11/28/2022 7:00 PM, Jakub Kicinski wrote:
> On Mon, 28 Nov 2022 13:05:09 -0800 Florian Fainelli wrote:
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>> Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Thanks! Is this for next or for net?

I would play it safe and schedule it for net-next so we can be made 
aware of possible regressions, if any. Platforms affected like the one 
Xiaolei worked on would likely get this back ported into $vendor tree.
Paolo Abeni Nov. 29, 2022, 11:50 a.m. UTC | #8 
On Fri, 2022-11-25 at 12:12 +0800, Xiaolei Wang wrote:
> If the external phy used by current mac interface is
> managed by another mac interface, it means that this
> network port cannot work independently, especially
> when the system suspend and resume, the following
> trace may appear, so we should create a device link
> between phy dev and mac dev.
> 
>   WARNING: CPU: 0 PID: 24 at drivers/net/phy/phy.c:983 phy_error+0x20/0x68
>   Modules linked in:
>   CPU: 0 PID: 24 Comm: kworker/0:2 Not tainted 6.1.0-rc3-00011-g5aaef24b5c6d-dirty #34
>   Hardware name: Freescale i.MX6 SoloX (Device Tree)
>   Workqueue: events_power_efficient phy_state_machine
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x68/0x90
>   dump_stack_lvl from __warn+0xb4/0x24c
>   __warn from warn_slowpath_fmt+0x5c/0xd8
>   warn_slowpath_fmt from phy_error+0x20/0x68
>   phy_error from phy_state_machine+0x22c/0x23c
>   phy_state_machine from process_one_work+0x288/0x744
>   process_one_work from worker_thread+0x3c/0x500
>   worker_thread from kthread+0xf0/0x114
>   kthread from ret_from_fork+0x14/0x28
>   Exception stack(0xf0951fb0 to 0xf0951ff8)
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
>  drivers/net/phy/phy_device.c | 12 ++++++++++++
>  include/linux/phy.h          |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 57849ac0384e..ca6d12f37066 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1511,6 +1511,15 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	phy_resume(phydev);
>  	phy_led_triggers_register(phydev);
>  
> +	/**
> +	 * If the external phy used by current mac interface is managed by
> +	 * another mac interface, so we should create a device link between
> +	 * phy dev and mac dev.
> +	 */
> +	if (phydev->mdio.bus->parent && dev->dev.parent != phydev->mdio.bus->parent)
> +		phydev->devlink = device_link_add(dev->dev.parent, &phydev->mdio.dev,
> +						  DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
> +
>  	return err;
>  
>  error:
> @@ -1748,6 +1757,9 @@ void phy_detach(struct phy_device *phydev)
>  	struct module *ndev_owner = NULL;
>  	struct mii_bus *bus;
>  
> +	if (phydev->devlink)
> +		device_link_del(phydev->devlink);
> +
>  	if (phydev->sysfs_links) {
>  		if (dev)
>  			sysfs_remove_link(&dev->dev.kobj, "phydev");
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index ddf66198f751..f7f8b909fed0 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -617,6 +617,8 @@ struct phy_device {
>  	/* And management functions */
>  	struct phy_driver *drv;
>  
> +	struct device_link *devlink;

Sorry for the late nit picking, but could you please add the kdoc
documentation for this new field?

Also, please specify explicitly the net-next target tree on repost, as
per Florian's request.

Thanks,

Paolo
diff mbox series

Patch

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 57849ac0384e..ca6d12f37066 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1511,6 +1511,15 @@  int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	phy_resume(phydev);
 	phy_led_triggers_register(phydev);
 
+	/**
+	 * If the external phy used by current mac interface is managed by
+	 * another mac interface, so we should create a device link between
+	 * phy dev and mac dev.
+	 */
+	if (phydev->mdio.bus->parent && dev->dev.parent != phydev->mdio.bus->parent)
+		phydev->devlink = device_link_add(dev->dev.parent, &phydev->mdio.dev,
+						  DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
+
 	return err;
 
 error:
@@ -1748,6 +1757,9 @@  void phy_detach(struct phy_device *phydev)
 	struct module *ndev_owner = NULL;
 	struct mii_bus *bus;
 
+	if (phydev->devlink)
+		device_link_del(phydev->devlink);
+
 	if (phydev->sysfs_links) {
 		if (dev)
 			sysfs_remove_link(&dev->dev.kobj, "phydev");
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ddf66198f751..f7f8b909fed0 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -617,6 +617,8 @@  struct phy_device {
 	/* And management functions */
 	struct phy_driver *drv;
 
+	struct device_link *devlink;
+
 	u32 phy_id;
 
 	struct phy_c45_device_ids c45_ids;