On imx6sx, there are two fec interfaces, but the external
phys can only be configured by fec0 mii_bus. That means
the fec1 can't work independently, it only work when the
fec0 is active. It is alright in the normal boot since the
fec0 will be probed first. But then the fec0 maybe moved
behind of fec1 in the dpm_list due to various device link.
So in system suspend and resume, we would get the following
warning when configuring the external phy of fec1 via the
fec0 mii_bus due to the inactive of fec0. In order to fix
this issue, we create a device link between phy dev and fec0.
This will make sure that fec0 is always active when fec1
is in active mode.
Therefore, in order to solve this kind of problem, an interface
function phy_mac_link_add is added to establish a link relationship
between phy dev and mac dev.
Xiaolei Wang (2):
net: phy: Add netdev link when multiple phys share a mdio
net: fec: net: fec: Create device link between fec0 and fec1
drivers/net/ethernet/freescale/fec_main.c | 4 +++-
drivers/net/phy/phy.c | 21 +++++++++++++++++++++
include/linux/phy.h | 1 +
3 files changed, 25 insertions(+), 1 deletion(-)
--
2.25.1
On imx6sx, there are two fec interfaces, but the external
phys can only be configured by fec0 mii_bus. That means
the fec1 can't work independently, it only work when the
fec0 is active. It is alright in the normal boot since the
fec0 will be probed first. But then the fec0 maybe moved
behind of fec1 in the dpm_list due to various device link.
So in system suspend and resume, we would get the following
warning when configuring the external phy of fec1 via the
fec0 mii_bus due to the inactive of fec0. In order to fix
this issue, we create a device link between phy dev and fec0.
This will make sure that fec0 is always active when fec1
is in active mode.
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)
Fixes: 2f664823a470 ("net: phy: at803x: add device tree binding")
Signed-off-by: Xiaolei Wang <[email protected]>
---
drivers/net/ethernet/freescale/fec_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f623c12eaf95..036e1bbafdd2 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3963,6 +3963,9 @@ fec_probe(struct platform_device *pdev)
goto failed_stop_mode;
phy_node = of_parse_phandle(np, "phy-handle", 0);
+ if (phy_node) {
+ phy_mac_link_add(phy_node, ndev);
+ }
if (!phy_node && of_phy_is_fixed_link(np)) {
ret = of_phy_register_fixed_link(np);
if (ret < 0) {
--
2.25.1
On Wed, Nov 16, 2022 at 10:43:05PM +0800, Xiaolei Wang wrote:
> On imx6sx, there are two fec interfaces, but the external
> phys can only be configured by fec0 mii_bus. That means
> the fec1 can't work independently, it only work when the
> fec0 is active. It is alright in the normal boot since the
> fec0 will be probed first. But then the fec0 maybe moved
> behind of fec1 in the dpm_list due to various device link.
> So in system suspend and resume, we would get the following
> warning when configuring the external phy of fec1 via the
> fec0 mii_bus due to the inactive of fec0. In order to fix
> this issue, we create a device link between phy dev and fec0.
> This will make sure that fec0 is always active when fec1
> is in active mode.
>
> 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)
Please add an explanation why you only do this for the FEC? Is this
not a problem for any board which has the PHY on an MDIO bus not
direct child of the MAC?
Andrew
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.
Signed-off-by: Xiaolei Wang <[email protected]>
---
drivers/net/phy/phy.c | 20 ++++++++++++++++++++
include/linux/phy.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e741d8aebffe..0ef6b69026c7 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -35,6 +35,7 @@
#include <net/netlink.h>
#include <net/genetlink.h>
#include <net/sock.h>
+#include <linux/of_mdio.h>
#define PHY_STATE_TIME HZ
@@ -1535,3 +1536,22 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
return phy_restart_aneg(phydev);
}
EXPORT_SYMBOL(phy_ethtool_nway_reset);
+
+/**
+ * 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.
+ */
+void phy_mac_link_add(struct device_node *phy_np, struct net_device *ndev)
+{
+ struct phy_device *phy_dev = of_phy_find_device(phy_np);
+ struct device *dev = phy_dev ? &phy_dev->mdio.dev : NULL;
+
+ if (dev && ndev->dev.parent != dev)
+ device_link_add(ndev->dev.parent, dev,
+ DL_FLAG_PM_RUNTIME);
+
+ if (phy_dev)
+ put_device(&phy_dev->mdio.dev);
+}
+EXPORT_SYMBOL(phy_mac_link_add);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ddf66198f751..11cdfbd81153 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1758,6 +1758,7 @@ int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size);
void phy_package_leave(struct phy_device *phydev);
int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
int addr, size_t priv_size);
+void phy_mac_link_add(struct device_node *phy_np, struct net_device *ndev);
#if IS_ENABLED(CONFIG_PHYLIB)
int __init mdio_bus_init(void);
--
2.25.1
Hi Xiaolei,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master v6.1-rc5 next-20221116]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Xiaolei-Wang/Add-link-between-phy-dev-and-mac-dev/20221116-224621
patch link: https://lore.kernel.org/r/20221116144305.2317573-2-xiaolei.wang%40windriver.com
patch subject: [PATCH 1/2] net: phy: Add link between phy dev and mac dev
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a5e03a412891ff1a482e9a53794432817fd9e84e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Xiaolei-Wang/Add-link-between-phy-dev-and-mac-dev/20221116-224621
git checkout a5e03a412891ff1a482e9a53794432817fd9e84e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/net/phy/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/net/phy/phy.c:1542: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* The external phy used by current mac interface is managed by
vim +1542 drivers/net/phy/phy.c
1540
1541 /**
> 1542 * The external phy used by current mac interface is managed by
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On 11/16/22 06:43, Xiaolei Wang wrote:
> 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.
>
> Signed-off-by: Xiaolei Wang <[email protected]>
> ---
> drivers/net/phy/phy.c | 20 ++++++++++++++++++++
> include/linux/phy.h | 1 +
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index e741d8aebffe..0ef6b69026c7 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -35,6 +35,7 @@
> #include <net/netlink.h>
> #include <net/genetlink.h>
> #include <net/sock.h>
> +#include <linux/of_mdio.h>
>
> #define PHY_STATE_TIME HZ
>
> @@ -1535,3 +1536,22 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
> return phy_restart_aneg(phydev);
> }
> EXPORT_SYMBOL(phy_ethtool_nway_reset);
> +
> +/**
> + * 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.
> + */
> +void phy_mac_link_add(struct device_node *phy_np, struct net_device *ndev)
> +{
> + struct phy_device *phy_dev = of_phy_find_device(phy_np);
> + struct device *dev = phy_dev ? &phy_dev->mdio.dev : NULL;
> +
> + if (dev && ndev->dev.parent != dev)
> + device_link_add(ndev->dev.parent, dev,
> + DL_FLAG_PM_RUNTIME);
Where is the matching device_link_del()?
--
Florian
On 11/16/22 07:07, Andrew Lunn wrote:
> On Wed, Nov 16, 2022 at 10:43:05PM +0800, Xiaolei Wang wrote:
>> On imx6sx, there are two fec interfaces, but the external
>> phys can only be configured by fec0 mii_bus. That means
>> the fec1 can't work independently, it only work when the
>> fec0 is active. It is alright in the normal boot since the
>> fec0 will be probed first. But then the fec0 maybe moved
>> behind of fec1 in the dpm_list due to various device link.
Humm, but if FEC1 depends upon its PHY to be available by the FEC0 MDIO
bus provider, then surely we will need to make sure that FEC0's MDIO bus
is always functional, and that includes surviving re-ordering as well as
any sort of run-time power management that can occur.
>> So in system suspend and resume, we would get the following
>> warning when configuring the external phy of fec1 via the
>> fec0 mii_bus due to the inactive of fec0. In order to fix
>> this issue, we create a device link between phy dev and fec0.
>> This will make sure that fec0 is always active when fec1
>> is in active mode.
Still not clear to me how the proposed fix works, let alone how it does
not leak device links since there is no device_link_del(), also you are
going to be creating guaranteed regressions by putting that change in
the PHY library.
It seems to me that you need to address a more fundamental issue within
the FEC driver and how it registers its internal MDIO busses.
The device link between the PHY and MAC does have the MDIO bus in
between as a device.
Have you verified your patch is still needed even with
557d5dc83f6831b4e54d141e9b121850406f9a60 ("net: fec: use mac-managed PHY
PM")?
--
Florian
On Wed, Nov 16, 2022 at 03:27:39PM -0800, Florian Fainelli wrote:
> On 11/16/22 07:07, Andrew Lunn wrote:
> > On Wed, Nov 16, 2022 at 10:43:05PM +0800, Xiaolei Wang wrote:
> > > On imx6sx, there are two fec interfaces, but the external
> > > phys can only be configured by fec0 mii_bus. That means
> > > the fec1 can't work independently, it only work when the
> > > fec0 is active. It is alright in the normal boot since the
> > > fec0 will be probed first. But then the fec0 maybe moved
> > > behind of fec1 in the dpm_list due to various device link.
>
> Humm, but if FEC1 depends upon its PHY to be available by the FEC0 MDIO bus
> provider, then surely we will need to make sure that FEC0's MDIO bus is
> always functional, and that includes surviving re-ordering as well as any
> sort of run-time power management that can occur.
Runtime PM is solved for the FECs MDIO bus, because there are switches
hanging off it, which have their own life cycle independent of the
MAC. This is something i had to fix many moons ago, when the FEC would
power off the MDIO bus when the interface is admin down, stopping
access to the switch. The mdio read and write functions now do run
time pm get and put as needed.
I've never done suspend/resume with a switch, it is not something
needed in the use cases i've covered.
> > > So in system suspend and resume, we would get the following
> > > warning when configuring the external phy of fec1 via the
> > > fec0 mii_bus due to the inactive of fec0. In order to fix
> > > this issue, we create a device link between phy dev and fec0.
> > > This will make sure that fec0 is always active when fec1
> > > is in active mode.
>
> Still not clear to me how the proposed fix works, let alone how it does not
> leak device links since there is no device_link_del(), also you are going to
> be creating guaranteed regressions by putting that change in the PHY
> library.
The reference leak is bad, but i think phylib is the correct place to
fix this general issue. It is not specific to the FEC. There are other
boards with dual MAC SoCs and they save a couple of pins by putting
both PHYs on one MDIO bus. Having the link should help better
represent the device tree so that suspend/resume can do stuff in the
right order.
Andrew
On 11/16/22 15:57, Andrew Lunn wrote:
> On Wed, Nov 16, 2022 at 03:27:39PM -0800, Florian Fainelli wrote:
>> On 11/16/22 07:07, Andrew Lunn wrote:
>>> On Wed, Nov 16, 2022 at 10:43:05PM +0800, Xiaolei Wang wrote:
>>>> On imx6sx, there are two fec interfaces, but the external
>>>> phys can only be configured by fec0 mii_bus. That means
>>>> the fec1 can't work independently, it only work when the
>>>> fec0 is active. It is alright in the normal boot since the
>>>> fec0 will be probed first. But then the fec0 maybe moved
>>>> behind of fec1 in the dpm_list due to various device link.
>>
>> Humm, but if FEC1 depends upon its PHY to be available by the FEC0 MDIO bus
>> provider, then surely we will need to make sure that FEC0's MDIO bus is
>> always functional, and that includes surviving re-ordering as well as any
>> sort of run-time power management that can occur.
>
> Runtime PM is solved for the FECs MDIO bus, because there are switches
> hanging off it, which have their own life cycle independent of the
> MAC. This is something i had to fix many moons ago, when the FEC would
> power off the MDIO bus when the interface is admin down, stopping
> access to the switch. The mdio read and write functions now do run
> time pm get and put as needed.
>
> I've never done suspend/resume with a switch, it is not something
> needed in the use cases i've covered.
All of the systems with integrated I worked on had to support
suspend/resume both with HW maintaining the state, and with HW losing
the state because of being powered off. The whole thing is IMHO still
not quite well supported upstream if you have applied some configuration
more complicated than a bridge or standalone ports. Anyway, this is a
topic for another 10 years to come :)
>
>>>> So in system suspend and resume, we would get the following
>>>> warning when configuring the external phy of fec1 via the
>>>> fec0 mii_bus due to the inactive of fec0. In order to fix
>>>> this issue, we create a device link between phy dev and fec0.
>>>> This will make sure that fec0 is always active when fec1
>>>> is in active mode.
>>
>> Still not clear to me how the proposed fix works, let alone how it does not
>> leak device links since there is no device_link_del(), also you are going to
>> be creating guaranteed regressions by putting that change in the PHY
>> library.
>
> The reference leak is bad, but i think phylib is the correct place to
> fix this general issue. It is not specific to the FEC. There are other
> boards with dual MAC SoCs and they save a couple of pins by putting
> both PHYs on one MDIO bus. Having the link should help better
> represent the device tree so that suspend/resume can do stuff in the
> right order.
My concern is that we already have had a hard time solving the proper
suspend/resume sequence whether the MAC suspends the PHY or the MDIO bus
suspends the PHY and throwing device links will either change the
ordering in subtle ways, or hopefully just provide the same piece of
information we already have via mac_managed_pm.
It seems like in Xiaolei's case, the MDIO bus should suspend the PHY and
that ought to take care of all dependencies, one would think.
--
Florian
On 11/17/2022 7:22 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/16/22 06:43, Xiaolei Wang wrote:
>> 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.
>>
>> Signed-off-by: Xiaolei Wang <[email protected]>
>> ---
>> drivers/net/phy/phy.c | 20 ++++++++++++++++++++
>> include/linux/phy.h | 1 +
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index e741d8aebffe..0ef6b69026c7 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -35,6 +35,7 @@
>> #include <net/netlink.h>
>> #include <net/genetlink.h>
>> #include <net/sock.h>
>> +#include <linux/of_mdio.h>
>>
>> #define PHY_STATE_TIME HZ
>>
>> @@ -1535,3 +1536,22 @@ int phy_ethtool_nway_reset(struct net_device
>> *ndev)
>> return phy_restart_aneg(phydev);
>> }
>> EXPORT_SYMBOL(phy_ethtool_nway_reset);
>> +
>> +/**
>> + * 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.
>> + */
>> +void phy_mac_link_add(struct device_node *phy_np, struct net_device
>> *ndev)
>> +{
>> + struct phy_device *phy_dev = of_phy_find_device(phy_np);
>> + struct device *dev = phy_dev ? &phy_dev->mdio.dev : NULL;
>> +
>> + if (dev && ndev->dev.parent != dev)
>> + device_link_add(ndev->dev.parent, dev,
>> + DL_FLAG_PM_RUNTIME);
>
> Where is the matching device_link_del()?
Hi
Oh, what do you mean when some modules are uninstalled, should we delete
the link?
My original idea was to wait for the dev (consumer or supplier) to be
unregistered and automatically deleted, I read the comment of
device_link_add, if DL_FLAG_STATELESS is not set, the caller of this
function will completely hand over the management of the link to the
driver core, then The link will remain until one of the devices it
points to (consumer or provider) is unregistered.
thanks
xiaolei
> --
> Florian
>
On 11/16/2022 11:07 PM, 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 Wed, Nov 16, 2022 at 10:43:05PM +0800, Xiaolei Wang wrote:
>> On imx6sx, there are two fec interfaces, but the external
>> phys can only be configured by fec0 mii_bus. That means
>> the fec1 can't work independently, it only work when the
>> fec0 is active. It is alright in the normal boot since the
>> fec0 will be probed first. But then the fec0 maybe moved
>> behind of fec1 in the dpm_list due to various device link.
>> So in system suspend and resume, we would get the following
>> warning when configuring the external phy of fec1 via the
>> fec0 mii_bus due to the inactive of fec0. In order to fix
>> this issue, we create a device link between phy dev and fec0.
>> This will make sure that fec0 is always active when fec1
>> is in active mode.
>>
>> 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)
> Please add an explanation why you only do this for the FEC? Is this
> not a problem for any board which has the PHY on an MDIO bus not
> direct child of the MAC?
Hi
Oh, my initial idea is to provide such an interface, we can add links
manually in such cases, if it is to solve this type of problem, my idea
is to create a link in phy_connect_direct, or is there any better
suggestion?
thanks
xiaolei
>
> Andrew
On 11/17/2022 8:21 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/16/22 15:57, Andrew Lunn wrote:
>> On Wed, Nov 16, 2022 at 03:27:39PM -0800, Florian Fainelli wrote:
>>> On 11/16/22 07:07, Andrew Lunn wrote:
>>>> On Wed, Nov 16, 2022 at 10:43:05PM +0800, Xiaolei Wang wrote:
>>>>> On imx6sx, there are two fec interfaces, but the external
>>>>> phys can only be configured by fec0 mii_bus. That means
>>>>> the fec1 can't work independently, it only work when the
>>>>> fec0 is active. It is alright in the normal boot since the
>>>>> fec0 will be probed first. But then the fec0 maybe moved
>>>>> behind of fec1 in the dpm_list due to various device link.
>>>
>>> Humm, but if FEC1 depends upon its PHY to be available by the FEC0
>>> MDIO bus
>>> provider, then surely we will need to make sure that FEC0's MDIO bus is
>>> always functional, and that includes surviving re-ordering as well
>>> as any
>>> sort of run-time power management that can occur.
>>
>> Runtime PM is solved for the FECs MDIO bus, because there are switches
>> hanging off it, which have their own life cycle independent of the
>> MAC. This is something i had to fix many moons ago, when the FEC would
>> power off the MDIO bus when the interface is admin down, stopping
>> access to the switch. The mdio read and write functions now do run
>> time pm get and put as needed.
>>
>> I've never done suspend/resume with a switch, it is not something
>> needed in the use cases i've covered.
>
> All of the systems with integrated I worked on had to support
> suspend/resume both with HW maintaining the state, and with HW losing
> the state because of being powered off. The whole thing is IMHO still
> not quite well supported upstream if you have applied some configuration
> more complicated than a bridge or standalone ports. Anyway, this is a
> topic for another 10 years to come :)
>
>>
>>>>> So in system suspend and resume, we would get the following
>>>>> warning when configuring the external phy of fec1 via the
>>>>> fec0 mii_bus due to the inactive of fec0. In order to fix
>>>>> this issue, we create a device link between phy dev and fec0.
>>>>> This will make sure that fec0 is always active when fec1
>>>>> is in active mode.
>>>
>>> Still not clear to me how the proposed fix works, let alone how it
>>> does not
>>> leak device links since there is no device_link_del(), also you are
>>> going to
>>> be creating guaranteed regressions by putting that change in the PHY
>>> library.
>>
>> The reference leak is bad, but i think phylib is the correct place to
>> fix this general issue. It is not specific to the FEC. There are other
>> boards with dual MAC SoCs and they save a couple of pins by putting
>> both PHYs on one MDIO bus. Having the link should help better
>> represent the device tree so that suspend/resume can do stuff in the
>> right order.
>
> My concern is that we already have had a hard time solving the proper
> suspend/resume sequence whether the MAC suspends the PHY or the MDIO bus
> suspends the PHY and throwing device links will either change the
> ordering in subtle ways, or hopefully just provide the same piece of
> information we already have via mac_managed_pm.
>
> It seems like in Xiaolei's case, the MDIO bus should suspend the PHY and
> that ought to take care of all dependencies, one would think.
Hi
mac_managed_pm solves the soft reset triggered during aeg. If you modify
it back to MDIO bus to suspend phy, you still need to solve the problem
of auto-negotiation,
thanks
xiaolei
> --
> Florian
>
Hi Xiaolei,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master v6.1-rc5 next-20221117]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Xiaolei-Wang/Add-link-between-phy-dev-and-mac-dev/20221116-224621
patch link: https://lore.kernel.org/r/20221116144305.2317573-2-xiaolei.wang%40windriver.com
patch subject: [PATCH 1/2] net: phy: Add link between phy dev and mac dev
config: x86_64-randconfig-a016
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a5e03a412891ff1a482e9a53794432817fd9e84e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Xiaolei-Wang/Add-link-between-phy-dev-and-mac-dev/20221116-224621
git checkout a5e03a412891ff1a482e9a53794432817fd9e84e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/phy/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/net/phy/phy.c:1542: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* The external phy used by current mac interface is managed by
vim +1542 drivers/net/phy/phy.c
1540
1541 /**
> 1542 * The external phy used by current mac interface is managed by
--
0-DAY CI Kernel Test Service
https://01.org/lkp