Potential issue with PHYs connected to the same MDIO bus and different MACs

Potential issue with PHYs connected to the same MDIO bus and different MACs

[piergiorgio.beruto]

Hello,
I am struggling with a problem and I would kindly ask if someone could shed
some light on this potential issue.

I'm using Linux 5.19.12 on the Altera socfpga ARMv7 platform (custom board).
My HW setup comprises three identical Ethernet PHYs, all attached to the
same MDIO bus belonging to one of the stmmac MACs embedded in the SoC.
Using the proper DTS configuration I was able to connect two PHYs each to
its own stmmac (the SoC has two embedded MACs), sharing the same MDIO bus.
Additionally, I'm able to communicate via MDIO with all three PHYs using the
proper ioctls, therefore I assume my HW is OK.

Now, to use the third PHY I added a MAC IP in the FPGA (Altera Triple Speed
MAC) for which a Linux driver already exists (altera_tse). However, unless I
connect the PHY to the dedicated TSE mdio bus (which requires HW
modifications), I get an error saying that the driver could not connect to
the PHY. I assumed this could be a conflict between the phylink interface
(used by STMMAC) and the "plain" PHY of APIs used by the TSE driver (which
seems a bit old).

I then decided to use a different MAC IP for which I'm writing a driver
using the phylink interface.
I got stuck at a point where the function "phy_attach_direct" in
phy_device.c gives an Oops (see log below).

Doing some debug, it seems that the NULL pointer comes from
dev->dev.parent->driver.
The "parent" pointer seems to reference the SoC BUS which the MAC IP belongs
to.

Any clue of what I might be doing wrong? I also think it is possible that
the problem I saw with the altera_tse driver could have something in common
with this? The MAC would be located inside the bus as well.

My DTS looks like this (just listing the relevant parts):

{
   SoC {
        gmac1 {
           // This is the stmmac which has all the PHYs attached.
           phy-handle=<&phy1>
            ...
           mdio {
              phy1 {
                 ...
              }
            
              phy2 {
                 ...
              }

              phy3 {
                 .
              }
         } // mdio
      } // gmac1

      gmac0 {
         phy-handle=<&phy2>
         ...
      }

      bridge {
          ...
         myMAC {
            phy-handle=<&phy3>
            ...
         }
      }
   } // Soc
}

One more information: I can clearly see from the log my PHYs being scanned
and enumerated. So again, I don't think the problem relates to the PHYs.

This is the Oops log.

8<--- cut here ---
[ 36.823689] Unable to handle kernel NULL pointer dereference at virtual
address 00000008
[ 36.832018] [00000008] *pgd=00000000
[ 36.835783] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[ 36.841090] Modules linked in: onsemi_tmac(O)
[ 36.845452] CPU: 0 PID: 240 Comm: ifconfig Tainted: G O
5.19.12-centurion3-1.0.3.0 #10
[ 36.854646] Hardware name: Altera SOCFPGA
[ 36.858644] PC is at phy_attach_direct+0x98/0x328
[ 36.863357] LR is at phy_attach_direct+0x8c/0x328
[ 36.868057] pc : [<c051f218>] lr : [<c051f20c>] psr: 600d0013
[ 36.874304] sp : d0eedd58 ip : 00000000 fp : d0eede78
[ 36.879513] r10: c162e000 r9 : 00000000 r8 : 00000000
[ 36.884722] r7 : 00000002 r6 : c0d58520 r5 : cfdf3730 r4 : c1752000
[ 36.891230] r3 : 00000000 r2 : 0f5a3000 r1 : 00000000 r0 : c06e186e
[ 36.897738] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 36.904855] Control: 10c5387d Table: 015d404a DAC: 00000055
[ 36.910583] Register r0 information: non-slab/vmalloc memory
[ 36.916238] Register r1 information: NULL pointer
[ 36.920933] Register r2 information: non-paged memory
[ 36.925974] Register r3 information: NULL pointer
[ 36.930667] Register r4 information: slab kmalloc-1k start c1752000 pointer
offset 0 size 1024
[ 36.939278] Register r5 information: non-slab/vmalloc memory
[ 36.944925] Register r6 information: slab kmalloc-512 start c0d58400
pointer offset 288 size 512
[ 36.953705] Register r7 information: non-paged memory
[ 36.958746] Register r8 information: NULL pointer
[ 36.963440] Register r9 information: NULL pointer
[ 36.968133] Register r10 information: slab kmalloc-4k start c162e000
pointer offset 0 size 4096
[ 36.976827] Register r11 information: 2-page vmalloc region starting at
0xd0eec000 allocated at kernel_clone+0x8c/0x1bc
[ 36.987595] Register r12 information: NULL pointer
[ 36.992376] Process ifconfig (pid: 240, stack limit = 0xa94bafcf)
[ 36.998455] Stack: (0xd0eedd58 to 0xd0eee000)
[ 37.002807] dd40: c0f4d300 cfdf3730
[ 37.010964] dd60: c1752000 00000000 bf001200 00000000 00008914 c039d9d0
c1633800 00000000
[ 37.019120] dd80: 00000000 c1633824 bf001200 bf000378 c1633800 c1633800
00000003 c1633800
[ 37.027276] dda0: 00000000 c044ec54 c1633800 00000000 c1633800 00000000
c1633800 00001043
[ 37.035431] ddc0: 00000000 00001002 00000041 c044ef58 c1633800 00001043
00000000 00008914
[ 37.043578] dde0: c1633800 d0eede68 00000000 00001002 c1730a0c c044ef80
00000000 d0eede68
[ 37.051734] de00: c1633800 c1730a00 c1730a0c c04db608 000000bf 00000100
ffffffff 00000014
[ 37.059890] de20: d0eede68 d0eede68 00000003 004c1043 004cb0c8 bea1be94
b6f05d28 65cdff6b
[ 37.068045] de40: 00000000 00008914 bea1bca4 bea1bca4 00008914 00000003
c168e4c0 c166a6c0
[ 37.076200] de60: 00000000 c04ddd7c 31687465 00000000 00000000 00000000
004c1043 004cb0c8
[ 37.084356] de80: bea1be94 b6f05d28 c0943580 c04453c0 00008913 d0eedee4
c0943580 c0445448
[ 37.092511] dea0: 00008913 bea1bca4 00000003 c168e4c0 bea1bca4 00000020
00000000 65cdff6b
[ 37.100668] dec0: 00000000 c12ded00 00008914 c04301a0 d0eededf c168e4c0
00000254 01000080
[ 37.108823] dee0: 004cb794 31687465 00000000 00000000 00000000 004c1002
004cb0c8 bea1be94
[ 37.116979] df00: b6f05d28 65cdff6b c12ded00 c166a6c0 bea1bca4 c01e0268
c12ded00 c01e0c00
[ 37.125134] df20: 00000000 c15c85fc c1744ffc 00000000 c0100218 d0eedfb0
b6d72340 c168e4c0
[ 37.133290] df40: c1745000 c01544fc d0eedfb0 c010bb90 00000000 00000003
c092d63c 00000004
[ 37.141445] df60: 00000003 00000007 b6d72340 d0eedfb0 c0908fa0 c082f044
bea1be94 65cdff6b
[ 37.149601] df80: b6f06000 004d4c53 bea1bca4 004ff078 00000036 c0100218
c168e4c0 00000036
[ 37.157756] dfa0: 00000000 c0100040 004d4c53 bea1bca4 00000003 00008914
bea1bca4 bea1bc50
[ 37.165911] dfc0: 004d4c53 bea1bca4 004ff078 00000036 bea1be8c bea1bf7a
00000015 00000000
[ 37.174065] dfe0: 005001f0 bea1bc38 00437484 b6db7d74 200a0010 00000003
00000000 00000000
[ 37.182221] phy_attach_direct from phylink_fwnode_phy_connect+0xa4/0xdc
[ 37.188932] phylink_fwnode_phy_connect from tmac_open+0x44/0x9c
[onsemi_tmac]
[ 37.196160] tmac_open [onsemi_tmac] from __dev_open+0x110/0x128
[ 37.202180] __dev_open from __dev_change_flags+0x168/0x17c
[ 37.207758] __dev_change_flags from dev_change_flags+0x14/0x44
[ 37.213680] dev_change_flags from devinet_ioctl+0x2ac/0x5fc
[ 37.219349] devinet_ioctl from inet_ioctl+0x1ec/0x214
[ 37.224494] inet_ioctl from sock_ioctl+0x14c/0x3c4
[ 37.229383] sock_ioctl from vfs_ioctl+0x20/0x38
[ 37.234013] vfs_ioctl from sys_ioctl+0x24c/0x840
[ 37.238727] sys_ioctl from ret_fast_syscall+0x0/0x4c
[ 37.243782] Exception stack(0xd0eedfa8 to 0xd0eedff0)
[ 37.248830] dfa0: 004d4c53 bea1bca4 00000003 00008914 bea1bca4 bea1bc50
[ 37.256987] dfc0: 004d4c53 bea1bca4 004ff078 00000036 bea1be8c bea1bf7a
00000015 00000000
[ 37.265140] dfe0: 005001f0 bea1bc38 00437484 b6db7d74
[ 37.270186] Code: ebfff67a e5963314 e59f0264 e5933038 (e5931008)
[ 37.281284] ---[ end trace 0000000000000000 ]---

Thank you in advance for any help you could provide!

Kind Regards,
Piergiorgio

Re: Potential issue with PHYs connected to the same MDIO bus and different MACs

[Vladimir Oltean]

Hi Piergiorgio,

On Sat, Nov 05, 2022 at 01:23:36PM +0100, piergiorgio.beruto@gmail.com wrote:
> Now, to use the third PHY I added a MAC IP in the FPGA (Altera Triple Speed
> MAC) for which a Linux driver already exists (altera_tse). However, unless I
> connect the PHY to the dedicated TSE mdio bus (which requires HW
> modifications), I get an error saying that the driver could not connect to
> the PHY. I assumed this could be a conflict between the phylink interface
> (used by STMMAC) and the "plain" PHY of APIs used by the TSE driver (which
> seems a bit old).

Can you share exactly what error message you get?
Also, btw, the tse driver was also converted to phylink in net-next, see
commit fef2998203e1 ("net: altera: tse: convert to phylink"). Maybe it
would be a good idea to retry with that.

> I then decided to use a different MAC IP for which I'm writing a driver
> using the phylink interface.
> I got stuck at a point where the function "phy_attach_direct" in
> phy_device.c gives an Oops (see log below).
>
> Doing some debug, it seems that the NULL pointer comes from
> dev->dev.parent->driver.
> The "parent" pointer seems to reference the SoC BUS which the MAC IP belongs
> to.
>
> Any clue of what I might be doing wrong? I also think it is possible that
> the problem I saw with the altera_tse driver could have something in common
> with this?

Not clear what problem you were seeing with the altera_tse driver...

> The MAC would be located inside the bus as well.
>
> My DTS looks like this (just listing the relevant parts):
>
> {
>    SoC {
>         gmac1 {
>            // This is the stmmac which has all the PHYs attached.
>            phy-handle=<&phy1>
>             ...
>            mdio {
>               phy1 {
>                  ...
>               }
>
>               phy2 {
>                  ...
>               }
>
>               phy3 {
>                  .
>               }
>          } // mdio
>       } // gmac1
>
>       gmac0 {
>          phy-handle=<&phy2>
>          ...
>       }
>
>       bridge {
>           ...
>          myMAC {
>             phy-handle=<&phy3>
>             ...
>          }
>       }
>    } // Soc
> }

Device tree looks more or less okay (not sure what's the "bridge"
though, this is potentially irrelevant).

>
> One more information: I can clearly see from the log my PHYs being scanned
> and enumerated. So again, I don't think the problem relates to the PHYs.
>
> This is the Oops log.
>
> 8<--- cut here ---
> [ 36.823689] Unable to handle kernel NULL pointer dereference at virtual address 00000008
> [ 36.835783] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> [ 36.841090] Modules linked in: onsemi_tmac(O)
> [ 36.845452] CPU: 0 PID: 240 Comm: ifconfig Tainted: G O 5.19.12-centurion3-1.0.3.0 #10
> [ 36.854646] Hardware name: Altera SOCFPGA
> [ 36.858644] PC is at phy_attach_direct+0x98/0x328
> [ 36.863357] LR is at phy_attach_direct+0x8c/0x328
> [ 36.868057] pc : [<c051f218>] lr : [<c051f20c>] psr: 600d0013
> [ 36.874304] sp : d0eedd58 ip : 00000000 fp : d0eede78
> [ 36.992376] Process ifconfig (pid: 240, stack limit = 0xa94bafcf)
> [ 36.998455] Stack: (0xd0eedd58 to 0xd0eee000)
> [ 37.182221] phy_attach_direct from phylink_fwnode_phy_connect+0xa4/0xdc
> [ 37.188932] phylink_fwnode_phy_connect from tmac_open+0x44/0x9c [onsemi_tmac]
> [ 37.196160] tmac_open [onsemi_tmac] from __dev_open+0x110/0x128
> [ 37.202180] __dev_open from __dev_change_flags+0x168/0x17c
> [ 37.207758] __dev_change_flags from dev_change_flags+0x14/0x44
> [ 37.213680] dev_change_flags from devinet_ioctl+0x2ac/0x5fc
> [ 37.219349] devinet_ioctl from inet_ioctl+0x1ec/0x214

In phy_attach_direct() we currently have this in net-next (didn't check 5.19):

int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
		      u32 flags, phy_interface_t interface)
{
	/* For Ethernet device drivers that register their own MDIO bus, we
	 * will have bus->owner match ndev_mod, so we do not want to increment
	 * our own module->refcnt here, otherwise we would not be able to
	 * unload later on.
	 */
	if (dev)
		ndev_owner = dev->dev.parent->driver->owner;
	if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
		phydev_err(phydev, "failed to get the bus module\n");
		return -EIO;
	}

I think the (out-of-tree?!) driver you're writing needs to make a call
to SET_NETDEV_DEV() in order for the dev->dev.parent to be set. Also, a
bunch of other stuff relies on this. Is your driver making that call?

RE: Potential issue with PHYs connected to the same MDIO bus and different MACs

[piergiorgio.beruto]

Hello Vladimir,
Thank you very much for your kind reply.

It'll take me some time to reproduce the problem with the altera_tse but I
will certainly try the new version as you suggested.
In the meantime, I wish to continue writing my own driver. It is in fact out
of tree for the only reason that the MAC IP has not been released yet and it
is still experimental. The plan is to merge it into the main tree as soon as
it is stable.

This is what my driver does (I'm omitting the details, just focusing on the
PHY attach).

static int tmac_probe(struct platform_device *pdev) {
   struct device_node *np = pdev->dev.of_node;
   struct phylink *phylink;
   struct net_device *ndev;
   struct tmac_info *priv;
   
   phy_interface_t phy_mode;
   int ret = 0;

   // allocate device and related private data
   ndev = alloc_etherdev(sizeof(struct tmac_info));

   // .... check for errors ...   

   SET_NETDEV_DEV(ndev, &pdev->dev);

   // .. initialize private data, map registers and IRQs, configure DMAs...

   ret = of_get_phy_mode(np, &phy_mode);
   if (ret) {
      dev_err(priv->dev, "incorrect phy-mode\n");
      goto err_no_phy;
   }

   priv->phylink_config.dev = &pdev->dev;
   priv->phylink_config.type = PHYLINK_NETDEV;

   phylink = phylink_create(&priv->phylink_config, pdev->dev.fwnode,
phy_mode, &tmac_phylink_mac_ops);
   
   // ... check for errors ....

   priv->phylink = phylink;

   // ... initialize the MAC registers ...

   ndev->netdev_ops = &tmac_netdev_ops;
   ndev->ethtool_ops = &tmac_ethtool_ops;

   platform_set_drvdata(pdev, ndev);

   ret = register_netdev(ndev);
   if(ret) {
      dev_err(&pdev->dev, "failed to register the T-MAC device\n");
      ret = -ENODEV;
      
      goto err_netdev_register;
   }

   // success
   return 0;
}

static int tmac_open(struct net_device *dev) {
   struct tmac_info *priv = netdev_priv(dev);
   struct device_node *np; 
   int ret;

   if(request_irq(dev->irq, &tmac_interrupt, 0, dev->name, dev))
      return -EAGAIN;

   // this is the function that gives that Oops
   ret = phylink_of_phy_connect(priv->phylink, np, 0);

   // other initialization stuff
}

Do you see anything wrong with this?

Thanks,
Piergiorgio

-----Original Message-----
From: Vladimir Oltean <olteanv@gmail.com> 
Sent: 5 November, 2022 14:35
To: piergiorgio.beruto@gmail.com
Cc: netdev@vger.kernel.org
Subject: Re: Potential issue with PHYs connected to the same MDIO bus and
different MACs

Hi Piergiorgio,

On Sat, Nov 05, 2022 at 01:23:36PM +0100, piergiorgio.beruto@gmail.com
wrote:
> Now, to use the third PHY I added a MAC IP in the FPGA (Altera Triple 
> Speed
> MAC) for which a Linux driver already exists (altera_tse). However, 
> unless I connect the PHY to the dedicated TSE mdio bus (which requires 
> HW modifications), I get an error saying that the driver could not 
> connect to the PHY. I assumed this could be a conflict between the 
> phylink interface (used by STMMAC) and the "plain" PHY of APIs used by 
> the TSE driver (which seems a bit old).

Can you share exactly what error message you get?
Also, btw, the tse driver was also converted to phylink in net-next, see
commit fef2998203e1 ("net: altera: tse: convert to phylink"). Maybe it would
be a good idea to retry with that.

> I then decided to use a different MAC IP for which I'm writing a 
> driver using the phylink interface.
> I got stuck at a point where the function "phy_attach_direct" in 
> phy_device.c gives an Oops (see log below).
>
> Doing some debug, it seems that the NULL pointer comes from
> dev->dev.parent->driver.
> The "parent" pointer seems to reference the SoC BUS which the MAC IP 
> belongs to.
>
> Any clue of what I might be doing wrong? I also think it is possible 
> that the problem I saw with the altera_tse driver could have something 
> in common with this?

Not clear what problem you were seeing with the altera_tse driver...

> The MAC would be located inside the bus as well.
>
> My DTS looks like this (just listing the relevant parts):
>
> {
>    SoC {
>         gmac1 {
>            // This is the stmmac which has all the PHYs attached.
>            phy-handle=<&phy1>
>             ...
>            mdio {
>               phy1 {
>                  ...
>               }
>
>               phy2 {
>                  ...
>               }
>
>               phy3 {
>                  .
>               }
>          } // mdio
>       } // gmac1
>
>       gmac0 {
>          phy-handle=<&phy2>
>          ...
>       }
>
>       bridge {
>           ...
>          myMAC {
>             phy-handle=<&phy3>
>             ...
>          }
>       }
>    } // Soc
> }

Device tree looks more or less okay (not sure what's the "bridge"
though, this is potentially irrelevant).

>
> One more information: I can clearly see from the log my PHYs being 
> scanned and enumerated. So again, I don't think the problem relates to the
PHYs.
>
> This is the Oops log.
>
> 8<--- cut here ---
> [ 36.823689] Unable to handle kernel NULL pointer dereference at 
> virtual address 00000008 [ 36.835783] Internal error: Oops: 5 [#1] 
> PREEMPT SMP ARM [ 36.841090] Modules linked in: onsemi_tmac(O) [ 
> 36.845452] CPU: 0 PID: 240 Comm: ifconfig Tainted: G O 
> 5.19.12-centurion3-1.0.3.0 #10 [ 36.854646] Hardware name: Altera 
> SOCFPGA [ 36.858644] PC is at phy_attach_direct+0x98/0x328 [ 
> 36.863357] LR is at phy_attach_direct+0x8c/0x328 [ 36.868057] pc : 
> [<c051f218>] lr : [<c051f20c>] psr: 600d0013 [ 36.874304] sp : 
> d0eedd58 ip : 00000000 fp : d0eede78 [ 36.992376] Process ifconfig 
> (pid: 240, stack limit = 0xa94bafcf) [ 36.998455] Stack: (0xd0eedd58 
> to 0xd0eee000) [ 37.182221] phy_attach_direct from 
> phylink_fwnode_phy_connect+0xa4/0xdc
> [ 37.188932] phylink_fwnode_phy_connect from tmac_open+0x44/0x9c 
> [onsemi_tmac] [ 37.196160] tmac_open [onsemi_tmac] from 
> __dev_open+0x110/0x128 [ 37.202180] __dev_open from 
> __dev_change_flags+0x168/0x17c [ 37.207758] __dev_change_flags from 
> dev_change_flags+0x14/0x44 [ 37.213680] dev_change_flags from 
> devinet_ioctl+0x2ac/0x5fc [ 37.219349] devinet_ioctl from 
> inet_ioctl+0x1ec/0x214

In phy_attach_direct() we currently have this in net-next (didn't check
5.19):

int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
		      u32 flags, phy_interface_t interface) {
	/* For Ethernet device drivers that register their own MDIO bus, we
	 * will have bus->owner match ndev_mod, so we do not want to
increment
	 * our own module->refcnt here, otherwise we would not be able to
	 * unload later on.
	 */
	if (dev)
		ndev_owner = dev->dev.parent->driver->owner;
	if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
		phydev_err(phydev, "failed to get the bus module\n");
		return -EIO;
	}

I think the (out-of-tree?!) driver you're writing needs to make a call to
SET_NETDEV_DEV() in order for the dev->dev.parent to be set. Also, a bunch
of other stuff relies on this. Is your driver making that call?

RE: Potential issue with PHYs connected to the same MDIO bus and different MACs

[piergiorgio.beruto]

Ah, sorry! I copied & paste the wrong code for tmac_open (that was just a
test).
This is the actual code:

static int tmac_open(struct net_device *dev) {
   struct tmac_info *priv = netdev_priv(dev);
   int ret;

   if(request_irq(dev->irq, &tmac_interrupt, 0, dev->name, dev))
      return -EAGAIN;

   // this is the function that gives that Oops
   ret = phylink_of_phy_connect(priv->phylink, priv->dev->of_node, 0);
   
   // other initialization stuff
}

Regards,
Piergiorgio

-----Original Message-----
From: piergiorgio.beruto@gmail.com <piergiorgio.beruto@gmail.com> 
Sent: 5 November, 2022 15:39
To: 'Vladimir Oltean' <olteanv@gmail.com>
Cc: netdev@vger.kernel.org
Subject: RE: Potential issue with PHYs connected to the same MDIO bus and
different MACs

Hello Vladimir,
Thank you very much for your kind reply.

It'll take me some time to reproduce the problem with the altera_tse but I
will certainly try the new version as you suggested.
In the meantime, I wish to continue writing my own driver. It is in fact out
of tree for the only reason that the MAC IP has not been released yet and it
is still experimental. The plan is to merge it into the main tree as soon as
it is stable.

This is what my driver does (I'm omitting the details, just focusing on the
PHY attach).

static int tmac_probe(struct platform_device *pdev) {
   struct device_node *np = pdev->dev.of_node;
   struct phylink *phylink;
   struct net_device *ndev;
   struct tmac_info *priv;
   
   phy_interface_t phy_mode;
   int ret = 0;

   // allocate device and related private data
   ndev = alloc_etherdev(sizeof(struct tmac_info));

   // .... check for errors ...   

   SET_NETDEV_DEV(ndev, &pdev->dev);

   // .. initialize private data, map registers and IRQs, configure DMAs...

   ret = of_get_phy_mode(np, &phy_mode);
   if (ret) {
      dev_err(priv->dev, "incorrect phy-mode\n");
      goto err_no_phy;
   }

   priv->phylink_config.dev = &pdev->dev;
   priv->phylink_config.type = PHYLINK_NETDEV;

   phylink = phylink_create(&priv->phylink_config, pdev->dev.fwnode,
phy_mode, &tmac_phylink_mac_ops);
   
   // ... check for errors ....

   priv->phylink = phylink;

   // ... initialize the MAC registers ...

   ndev->netdev_ops = &tmac_netdev_ops;
   ndev->ethtool_ops = &tmac_ethtool_ops;

   platform_set_drvdata(pdev, ndev);

   ret = register_netdev(ndev);
   if(ret) {
      dev_err(&pdev->dev, "failed to register the T-MAC device\n");
      ret = -ENODEV;
      
      goto err_netdev_register;
   }

   // success
   return 0;
}

static int tmac_open(struct net_device *dev) {
   struct tmac_info *priv = netdev_priv(dev);
   struct device_node *np; 
   int ret;

   if(request_irq(dev->irq, &tmac_interrupt, 0, dev->name, dev))
      return -EAGAIN;

   // this is the function that gives that Oops
   ret = phylink_of_phy_connect(priv->phylink, np, 0);

   // other initialization stuff
}

Do you see anything wrong with this?

Thanks,
Piergiorgio

-----Original Message-----
From: Vladimir Oltean <olteanv@gmail.com>
Sent: 5 November, 2022 14:35
To: piergiorgio.beruto@gmail.com
Cc: netdev@vger.kernel.org
Subject: Re: Potential issue with PHYs connected to the same MDIO bus and
different MACs

Hi Piergiorgio,

On Sat, Nov 05, 2022 at 01:23:36PM +0100, piergiorgio.beruto@gmail.com
wrote:
> Now, to use the third PHY I added a MAC IP in the FPGA (Altera Triple 
> Speed
> MAC) for which a Linux driver already exists (altera_tse). However, 
> unless I connect the PHY to the dedicated TSE mdio bus (which requires 
> HW modifications), I get an error saying that the driver could not 
> connect to the PHY. I assumed this could be a conflict between the 
> phylink interface (used by STMMAC) and the "plain" PHY of APIs used by 
> the TSE driver (which seems a bit old).

Can you share exactly what error message you get?
Also, btw, the tse driver was also converted to phylink in net-next, see
commit fef2998203e1 ("net: altera: tse: convert to phylink"). Maybe it would
be a good idea to retry with that.

> I then decided to use a different MAC IP for which I'm writing a 
> driver using the phylink interface.
> I got stuck at a point where the function "phy_attach_direct" in 
> phy_device.c gives an Oops (see log below).
>
> Doing some debug, it seems that the NULL pointer comes from
> dev->dev.parent->driver.
> The "parent" pointer seems to reference the SoC BUS which the MAC IP 
> belongs to.
>
> Any clue of what I might be doing wrong? I also think it is possible 
> that the problem I saw with the altera_tse driver could have something 
> in common with this?

Not clear what problem you were seeing with the altera_tse driver...

> The MAC would be located inside the bus as well.
>
> My DTS looks like this (just listing the relevant parts):
>
> {
>    SoC {
>         gmac1 {
>            // This is the stmmac which has all the PHYs attached.
>            phy-handle=<&phy1>
>             ...
>            mdio {
>               phy1 {
>                  ...
>               }
>
>               phy2 {
>                  ...
>               }
>
>               phy3 {
>                  .
>               }
>          } // mdio
>       } // gmac1
>
>       gmac0 {
>          phy-handle=<&phy2>
>          ...
>       }
>
>       bridge {
>           ...
>          myMAC {
>             phy-handle=<&phy3>
>             ...
>          }
>       }
>    } // Soc
> }

Device tree looks more or less okay (not sure what's the "bridge"
though, this is potentially irrelevant).

>
> One more information: I can clearly see from the log my PHYs being 
> scanned and enumerated. So again, I don't think the problem relates to 
> the
PHYs.
>
> This is the Oops log.
>
> 8<--- cut here ---
> [ 36.823689] Unable to handle kernel NULL pointer dereference at 
> virtual address 00000008 [ 36.835783] Internal error: Oops: 5 [#1] 
> PREEMPT SMP ARM [ 36.841090] Modules linked in: onsemi_tmac(O) [ 
> 36.845452] CPU: 0 PID: 240 Comm: ifconfig Tainted: G O
> 5.19.12-centurion3-1.0.3.0 #10 [ 36.854646] Hardware name: Altera 
> SOCFPGA [ 36.858644] PC is at phy_attach_direct+0x98/0x328 [ 
> 36.863357] LR is at phy_attach_direct+0x8c/0x328 [ 36.868057] pc :
> [<c051f218>] lr : [<c051f20c>] psr: 600d0013 [ 36.874304] sp : 
> d0eedd58 ip : 00000000 fp : d0eede78 [ 36.992376] Process ifconfig
> (pid: 240, stack limit = 0xa94bafcf) [ 36.998455] Stack: (0xd0eedd58 
> to 0xd0eee000) [ 37.182221] phy_attach_direct from 
> phylink_fwnode_phy_connect+0xa4/0xdc
> [ 37.188932] phylink_fwnode_phy_connect from tmac_open+0x44/0x9c 
> [onsemi_tmac] [ 37.196160] tmac_open [onsemi_tmac] from
> __dev_open+0x110/0x128 [ 37.202180] __dev_open from 
> __dev_change_flags+0x168/0x17c [ 37.207758] __dev_change_flags from
> dev_change_flags+0x14/0x44 [ 37.213680] dev_change_flags from 
> devinet_ioctl+0x2ac/0x5fc [ 37.219349] devinet_ioctl from
> inet_ioctl+0x1ec/0x214

In phy_attach_direct() we currently have this in net-next (didn't check
5.19):

int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
		      u32 flags, phy_interface_t interface) {
	/* For Ethernet device drivers that register their own MDIO bus, we
	 * will have bus->owner match ndev_mod, so we do not want to
increment
	 * our own module->refcnt here, otherwise we would not be able to
	 * unload later on.
	 */
	if (dev)
		ndev_owner = dev->dev.parent->driver->owner;
	if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
		phydev_err(phydev, "failed to get the bus module\n");
		return -EIO;
	}

I think the (out-of-tree?!) driver you're writing needs to make a call to
SET_NETDEV_DEV() in order for the dev->dev.parent to be set. Also, a bunch
of other stuff relies on this. Is your driver making that call?

Re: Potential issue with PHYs connected to the same MDIO bus and different MACs

[Vladimir Oltean]

On Sat, Nov 05, 2022 at 03:39:07PM +0100, piergiorgio.beruto@gmail.com wrote:
>    priv->phylink_config.dev = &pdev->dev;
>    priv->phylink_config.type = PHYLINK_NETDEV;

The problem is here. You think that &pdev->dev is the same as
&ndev->dev, but it isn't (and it's SET_NETDEV_DEV that makes the linkage
between the 2). Use &ndev->dev here, and check out how phylink uses the
"dev" field:

#define to_net_dev(d) container_of(d, struct net_device, dev)

	if (config->type == PHYLINK_NETDEV) {
		pl->netdev = to_net_dev(config->dev);

RE: Potential issue with PHYs connected to the same MDIO bus and different MACs

[piergiorgio.beruto]

Oh, my!
Yes, this is so obvious but I could not see it...!
I really thank you for your time, and I apologize for overlooking this.

Kind Regards,
Piergiorgio

-----Original Message-----
From: Vladimir Oltean <olteanv@gmail.com> 
Sent: 5 November, 2022 17:38
To: piergiorgio.beruto@gmail.com
Cc: netdev@vger.kernel.org
Subject: Re: Potential issue with PHYs connected to the same MDIO bus and
different MACs

On Sat, Nov 05, 2022 at 03:39:07PM +0100, piergiorgio.beruto@gmail.com
wrote:
>    priv->phylink_config.dev = &pdev->dev;
>    priv->phylink_config.type = PHYLINK_NETDEV;

The problem is here. You think that &pdev->dev is the same as &ndev->dev,
but it isn't (and it's SET_NETDEV_DEV that makes the linkage between the 2).
Use &ndev->dev here, and check out how phylink uses the "dev" field:

#define to_net_dev(d) container_of(d, struct net_device, dev)

	if (config->type == PHYLINK_NETDEV) {
		pl->netdev = to_net_dev(config->dev);

RE: Potential issue with PHYs connected to the same MDIO bus and different MACs

[piergiorgio.beruto]

Hello Vladimir,
Sorry for sending more e-mails, but based on your observations I made some
progress and I think this might help in understanding the issue.

I've tried to make the following change to phy_device.c:

diff -urNa linux-5.19.12.orig/drivers/net/phy/phy_device.c
linux-5.19.12/drivers/net/phy/phy_device.c
--- linux-5.19.12.orig/drivers/net/phy/phy_device.c	2022-11-05
16:19:45.049827674 +0100
+++ linux-5.19.12/drivers/net/phy/phy_device.c	2022-11-05
16:21:27.181831665 +0100
@@ -1402,7 +1402,7 @@
 	 * our own module->refcnt here, otherwise we would not be able to
 	 * unload later on.
 	 */
-	if (dev)
+	if (dev && dev->dev.parent->driver)
 		ndev_owner = dev->dev.parent->driver->owner;
 	if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
 		phydev_err(phydev, "failed to get the bus module\n");
@@ -1785,7 +1785,7 @@
 	bus = phydev->mdio.bus;
 
 	put_device(&phydev->mdio.dev);
-	if (dev)
+	if (dev && dev->dev.parent->driver)
 		ndev_owner = dev->dev.parent->driver->owner;
 	if (ndev_owner != bus->owner)
 		module_put(bus->owner);

The result is that now it WORKS! I can access the PHY and the MAC seems to
function properly.
Although, I suspect this is not really a fix, rather a workaround... this is
what I see when I bringup my network interface:

/root # insmod onsemi-tmac.ko
[   23.181201] onsemi_tmac: loading out-of-tree module taints kernel.
[   23.189175] onsemi_tmac ff200380.tmac: using random MAC address
a6:c1:c7:0d:15:bd

/root # ifconfig eth1 up
[   29.131266] platform c0000000.bridge (unnamed net_device)
(uninitialized): PHY [stmmac-0:09] driver [NCN26000] (irq=50)
[   29.143316] platform c0000000.bridge (unnamed net_device)
(uninitialized): configuring for phy/mii link mode
[   29.153536] platform c0000000.bridge (unnamed net_device)
(uninitialized): Link is Up - 10Mbps/Half - flow control off

The "platform c0000000.bridge" and the " unnamed net_device" makes me think
there is something wrong still.

Looking in the DTS, the c0000000.bridge is the container of the network
device:

soc {
      hps_lw_bridge: bridge@c0000000 {
         #address-cells = <2>;
         #size-cells = <1>;

         compatible = "altr,bridge-21.1", "simple-bus";
         reg = <0xc0000000 0x20000000>,
               <0xff200000 0x00200000>;
         
         reg-names = "axi_h2f", "axi_h2f_lw";
			
         ranges = <0x00000001 0x00000080 0xff200200 0x00000200>,
 	          <0x00000001 0x00000000 0xff200000 0x00000008>,
                         <0x00000001 0x00000010 0xff200010 0x00000010>,
                         <0x00000001 0x00000020 0xff200020 0x00000020>;

        tmac0: tmac@100000200 {
      	compatible = "onsemi,tmac-1.0", "onsemi,tmac";
      	reg = <0x00000001 0x00000200 0x00000200>;
      	interrupt-parent = <&intc>;
      	interrupts = <0 40 4>;
      	status = "okay";

      	phy-handle = <&phy3>;
      	phy-mode = "mii";
         };
      }; //end bridge@0xc0000000 (hps_bridges)
   };

I've tried to debug further but I'm getting lost at some point.
I was wondering if this rings a bell to you?

Thanks,
Piergiorgio

-----Original Message-----
From: Vladimir Oltean <olteanv@gmail.com> 
Sent: 5 November, 2022 14:35
To: piergiorgio.beruto@gmail.com
Cc: netdev@vger.kernel.org
Subject: Re: Potential issue with PHYs connected to the same MDIO bus and
different MACs

Hi Piergiorgio,

On Sat, Nov 05, 2022 at 01:23:36PM +0100, piergiorgio.beruto@gmail.com
wrote:
> Now, to use the third PHY I added a MAC IP in the FPGA (Altera Triple 
> Speed
> MAC) for which a Linux driver already exists (altera_tse). However, 
> unless I connect the PHY to the dedicated TSE mdio bus (which requires 
> HW modifications), I get an error saying that the driver could not 
> connect to the PHY. I assumed this could be a conflict between the 
> phylink interface (used by STMMAC) and the "plain" PHY of APIs used by 
> the TSE driver (which seems a bit old).

Can you share exactly what error message you get?
Also, btw, the tse driver was also converted to phylink in net-next, see
commit fef2998203e1 ("net: altera: tse: convert to phylink"). Maybe it would
be a good idea to retry with that.

> I then decided to use a different MAC IP for which I'm writing a 
> driver using the phylink interface.
> I got stuck at a point where the function "phy_attach_direct" in 
> phy_device.c gives an Oops (see log below).
>
> Doing some debug, it seems that the NULL pointer comes from
> dev->dev.parent->driver.
> The "parent" pointer seems to reference the SoC BUS which the MAC IP 
> belongs to.
>
> Any clue of what I might be doing wrong? I also think it is possible 
> that the problem I saw with the altera_tse driver could have something 
> in common with this?

Not clear what problem you were seeing with the altera_tse driver...

> The MAC would be located inside the bus as well.
>
> My DTS looks like this (just listing the relevant parts):
>
> {
>    SoC {
>         gmac1 {
>            // This is the stmmac which has all the PHYs attached.
>            phy-handle=<&phy1>
>             ...
>            mdio {
>               phy1 {
>                  ...
>               }
>
>               phy2 {
>                  ...
>               }
>
>               phy3 {
>                  .
>               }
>          } // mdio
>       } // gmac1
>
>       gmac0 {
>          phy-handle=<&phy2>
>          ...
>       }
>
>       bridge {
>           ...
>          myMAC {
>             phy-handle=<&phy3>
>             ...
>          }
>       }
>    } // Soc
> }

Device tree looks more or less okay (not sure what's the "bridge"
though, this is potentially irrelevant).

>
> One more information: I can clearly see from the log my PHYs being 
> scanned and enumerated. So again, I don't think the problem relates to the
PHYs.
>
> This is the Oops log.
>
> 8<--- cut here ---
> [ 36.823689] Unable to handle kernel NULL pointer dereference at 
> virtual address 00000008 [ 36.835783] Internal error: Oops: 5 [#1] 
> PREEMPT SMP ARM [ 36.841090] Modules linked in: onsemi_tmac(O) [ 
> 36.845452] CPU: 0 PID: 240 Comm: ifconfig Tainted: G O 
> 5.19.12-centurion3-1.0.3.0 #10 [ 36.854646] Hardware name: Altera 
> SOCFPGA [ 36.858644] PC is at phy_attach_direct+0x98/0x328 [ 
> 36.863357] LR is at phy_attach_direct+0x8c/0x328 [ 36.868057] pc : 
> [<c051f218>] lr : [<c051f20c>] psr: 600d0013 [ 36.874304] sp : 
> d0eedd58 ip : 00000000 fp : d0eede78 [ 36.992376] Process ifconfig 
> (pid: 240, stack limit = 0xa94bafcf) [ 36.998455] Stack: (0xd0eedd58 
> to 0xd0eee000) [ 37.182221] phy_attach_direct from 
> phylink_fwnode_phy_connect+0xa4/0xdc
> [ 37.188932] phylink_fwnode_phy_connect from tmac_open+0x44/0x9c 
> [onsemi_tmac] [ 37.196160] tmac_open [onsemi_tmac] from 
> __dev_open+0x110/0x128 [ 37.202180] __dev_open from 
> __dev_change_flags+0x168/0x17c [ 37.207758] __dev_change_flags from 
> dev_change_flags+0x14/0x44 [ 37.213680] dev_change_flags from 
> devinet_ioctl+0x2ac/0x5fc [ 37.219349] devinet_ioctl from 
> inet_ioctl+0x1ec/0x214

In phy_attach_direct() we currently have this in net-next (didn't check
5.19):

int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
		      u32 flags, phy_interface_t interface) {
	/* For Ethernet device drivers that register their own MDIO bus, we
	 * will have bus->owner match ndev_mod, so we do not want to
increment
	 * our own module->refcnt here, otherwise we would not be able to
	 * unload later on.
	 */
	if (dev)
		ndev_owner = dev->dev.parent->driver->owner;
	if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
		phydev_err(phydev, "failed to get the bus module\n");
		return -EIO;
	}

I think the (out-of-tree?!) driver you're writing needs to make a call to
SET_NETDEV_DEV() in order for the dev->dev.parent to be set. Also, a bunch
of other stuff relies on this. Is your driver making that call?