netdev - Re: [net-next] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240503133033.GJ3927860@ragnatech.se>
Date: Fri, 3 May 2024 15:30:33 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Andrew Lunn <andrew@...n.ch>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next] net: ethernet: rtsn: Add support for Renesas
 Ethernet-TSN

On 2024-05-03 13:59:52 +0200, Andrew Lunn wrote:
> > > > +static int rtsn_mii_register(struct rtsn_private *priv)
> > > > +{
> > > > +	struct platform_device *pdev = priv->pdev;
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct device_node *mdio_node;
> > > > +	struct mii_bus *mii;
> > > > +	int ret;
> > > > +
> > > > +	mii = mdiobus_alloc();
> > > > +	if (!mii)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	mdio_node = of_get_child_by_name(dev->of_node, "mdio");
> > > > +	if (!mdio_node) {
> > > > +		ret = -ENODEV;
> > > > +		goto out_free_bus;
> > > > +	};
> > > > +
> > > > +	mii->name = "rtsn_mii";
> > > > +	sprintf(mii->id, "%s-%x", pdev->name, pdev->id);
> > > > +	mii->priv = priv;
> > > > +	mii->read = rtsn_mii_read;
> > > > +	mii->write = rtsn_mii_write;
> > > > +	mii->read_c45 = rtsn_mii_read_c45;
> > > > +	mii->write_c45 = rtsn_mii_write_c45;
> > > 
> > > Just leave these two empty, and the core will do C45 over C22 for you.
> > 
> > Does this not require the bus to be created/allocated with an 
> > implementation that support this, for example mdio_i2c_alloc() or 
> > alloc_mdio_bitbang()?  This bus is allocated with mdiobus_alloc() which 
> > do not implement this. Removing the C45 functions here result in 
> > __mdiobus_c45_read() returning -EOPNOTSUPP as bus->read_c45 is not set.
> 
> phy_read_mmd():
>   __phy_read_mmd():
>       mmd_phy_read():
> 
> So is is_c45 is true?

Not sure, I never get that far. The function __mdiobus_c45_read() is 
called directly from of_mdiobus_register() call-chain.

The call chain is:

  rtsn_open()
    of_mdiobus_register() <- This fails and RTSN can't talk to the PHY
      __of_mdiobus_register()
        __of_mdiobus_parse_phys()
          of_mdiobus_register_phy()
            fwnode_mdiobus_register_phy() <- See [1]
              get_phy_device()
                get_phy_c45_ids()
                  mdiobus_c45_read()
                    __mdiobus_c45_read() <- Returns -EOPNOTSUPP [2]

1. Here is_c45 is set as it checks the compatible value is checked.

     is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");

2. The struct mii_bus have no read_c45() callback and returns  
   -EOPNOTSUPP.

> 
> I would expect it to be false, so that it then uses
> 
> 	mmd_phy_indirect(bus, phy_addr, devad, regnum);
> 	/* Write the data into MMD's selected register */
> 	return __mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);

Cool, I was not aware of that code-path I was only looking at the above 
when trying to remove the implementation in RTSN driver.

> 
> which is C45 over C22.
> 
> If is_c45 is true, what is setting it true?
> 
>       Andrew

-- 
Kind Regards,
Niklas Söderlund