Date:   Sat, 5 Nov 2022 15:42:37 +0100
From:   <piergiorgio.beruto@...il.com>
To:     "'Vladimir Oltean'" <olteanv@...il.com>
Cc:     <netdev@...r.kernel.org>
Subject: RE: Potential issue with PHYs connected to the same MDIO bus and different MACs

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@...il.com <piergiorgio.beruto@...il.com> 
Sent: 5 November, 2022 15:39
To: 'Vladimir Oltean' <olteanv@...il.com>
Cc: netdev@...r.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@...il.com>
Sent: 5 November, 2022 14:35
To: piergiorgio.beruto@...il.com
Cc: netdev@...r.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@...il.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?

Powered by blists - more mailing lists