| 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
| ||
|
Date: Sun, 17 May 2020 22:44:56 +0200
From: Roelof Berg <rberg@...g-solutions.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: Bryan Whitehead <bryan.whitehead@...rochip.com>,
Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lan743x: Added fixed link support
To Everyone: I need a test hardware recommendation for a lan7431/0 NIC in normal mode (not fixed-link mode). In prior patches this was not necessary, because I was able to ensure 100% backwards compatibility by careful coding alone. But I might soon come to a point where I need to test phy-connected devices as well.
Hi Andrew,
thanks for commenting on my patch.
> Am 17.05.2020 um 20:37 schrieb Andrew Lunn <andrew@...n.ch>:
>
>> @@ -946,6 +949,9 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
>> {
>> struct lan743x_adapter *adapter = netdev_priv(netdev);
>> struct phy_device *phydev = netdev->phydev;
>> + struct device_node *phynode;
>> + phy_interface_t phyifc = PHY_INTERFACE_MODE_GMII;
>> + u32 data;
>>
>> phy_print_status(phydev);
>> if (phydev->state == PHY_RUNNING) {
>> @@ -953,6 +959,48 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
>> int remote_advertisement = 0;
>> int local_advertisement = 0;
>>
>> + /* check if a fixed-link is defined in device-tree */
>> + phynode = of_node_get(adapter->pdev->dev.of_node);
>> + if (phynode && of_phy_is_fixed_link(phynode)) {
>
> Hi Roelof
>
> The whole point for fixed link is that it looks like a PHY. You should
> not need to care if it is a real PHY or a fixed link.
>
Ok, I can try to remove the additional speed and baud configuration, when the PHY simulation should lead to the same result. Understood, thanks, I’ll test this and remove the overhead.
>
>> + /* Configure MAC to fixed link parameters */
>> + data = lan743x_csr_read(adapter, MAC_CR);
>> + /* Disable auto negotiation */
>> + data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_);
>
> Why does the MAC care about autoneg? In general, all the MAC needs to
> know is the speed and duplex.
>
My assumption is, that in fixed-link mode we should switch off the autonegotiation between MAC and remote peer (e.g. a switch). I didn’t test, if it would also wun with the hardware doing auto-negotiation, however it feels cleaner to me to prevent the hardware from initiating any auto-negotiation in fixed-link mode.
>> + /* Set duplex mode */
>> + if (phydev->duplex)
>> + data |= MAC_CR_DPX_;
>> + else
>> + data &= ~MAC_CR_DPX_;
>> + /* Set bus speed */
>> + switch (phydev->speed) {
>> + case 10:
>> + data &= ~MAC_CR_CFG_H_;
>> + data &= ~MAC_CR_CFG_L_;
>> + break;
>> + case 100:
>> + data &= ~MAC_CR_CFG_H_;
>> + data |= MAC_CR_CFG_L_;
>> + break;
>> + case 1000:
>> + data |= MAC_CR_CFG_H_;
>> + data |= MAC_CR_CFG_L_;
>> + break;
>> + }
>
> The current code is unusual, in that it uses
> phy_ethtool_get_link_ksettings(). That should do the right thing with
> a fixed-link PHY, although i don't know if anybody uses it like
> this. So in theory, the current code should take care of duplex, flow
> control, and speed for you. Just watch out for bug/missing features in
> fixed link.
Ok, I test and report if it works. Now I understand the concept.
>
>
>> + /* Set interface mode */
>> + of_get_phy_mode(phynode, &phyifc);
>> + if (phyifc == PHY_INTERFACE_MODE_RGMII ||
>> + phyifc == PHY_INTERFACE_MODE_RGMII_ID ||
>> + phyifc == PHY_INTERFACE_MODE_RGMII_RXID ||
>> + phyifc == PHY_INTERFACE_MODE_RGMII_TXID)
>> + /* RGMII */
>> + data &= ~MAC_CR_MII_EN_;
>> + else
>> + /* GMII */
>> + data |= MAC_CR_MII_EN_;
>> + lan743x_csr_write(adapter, MAC_CR, data);
>> + }
>> + of_node_put(phynode);
>
> It is normal to do of_get_phy_mode when connecting to the PHY, and
> store the value in the private structure. This is also not specific to
> fixed link.
>
> There is also a helper you can use phy_interface_mode_is_rgmii().
Thanks for pointing to the method is_rgmii, very handy.
Using get_phy_mode() in all cases is not possible on a PC as it returns SGMII on a standard PC, but using GMII is today’s driver behavior. So what I basically did (on two places) is:
if(fixed-link)
Use get_phy_mode()’s result in of_phy_connect() and in the lan7431 register configuration.
else
Keep the prior behavior for backwards compatibility (i.e. ignoring the wrong interface mode config on a PC and use GMII constant)
The method is_rgmii you mention can lessen the pain here, thanks, and lead to:
if(is_rgmii()
use RGMII
else
use GMII
I need to think about this, because NOT passing get_phy_mode’s result directly into of_phy_connect or phy_connect_direct (and instead use above's (is_rgmii() ? RGMII : GMII) code) could have side effects.
However I don’t dare to pass get_phy_mode’s result directly into of_phy_connect or phy_connect_direct on a PC because then I might change the behavior of all standard PC NIC drivers. I haven’t researched yet why on a PC SGMII is returned by get_phy_mode(), does someone know ?.
>
>> +
>> memset(&ksettings, 0, sizeof(ksettings));
>> phy_ethtool_get_link_ksettings(netdev, &ksettings);
>> local_advertisement =
>> @@ -974,6 +1022,8 @@ static void lan743x_phy_close(struct lan743x_adapter *adapter)
>>
>> phy_stop(netdev->phydev);
>> phy_disconnect(netdev->phydev);
>> + if (of_phy_is_fixed_link(adapter->pdev->dev.of_node))
>> + of_phy_deregister_fixed_link(adapter->pdev->dev.of_node);
>> netdev->phydev = NULL;
>> }
>>
>> @@ -982,18 +1032,44 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
>> struct lan743x_phy *phy = &adapter->phy;
>> struct phy_device *phydev;
>> struct net_device *netdev;
>> + struct device_node *phynode = NULL;
>> + phy_interface_t phyifc = PHY_INTERFACE_MODE_GMII;
>> int ret = -EIO;
>
> netdev uses reverse christmas tree, meaning the lines should be
> sorted, longest first, getting shorter.
Ok
>
>>
>> netdev = adapter->netdev;
>> - phydev = phy_find_first(adapter->mdiobus);
>> - if (!phydev)
>> - goto return_error;
>>
>> - ret = phy_connect_direct(netdev, phydev,
>> - lan743x_phy_link_status_change,
>> - PHY_INTERFACE_MODE_GMII);
>> - if (ret)
>> - goto return_error;
>> + /* check if a fixed-link is defined in device-tree */
>> + phynode = of_node_get(adapter->pdev->dev.of_node);
>> + if (phynode && of_phy_is_fixed_link(phynode)) {
>> + netdev_dbg(netdev, "fixed-link detected\n");
>
> This is something which is useful during debug. But once it works can
> be removed.
Ok
>
>> + ret = of_phy_register_fixed_link(phynode);
>> + if (ret) {
>> + netdev_err(netdev, "cannot register fixed PHY\n");
>> + goto return_error;
>> + }
>> +
>> + of_get_phy_mode(phynode, &phyifc);
>> + phydev = of_phy_connect(netdev, phynode,
>> + lan743x_phy_link_status_change,
>> + 0, phyifc);
>> + if (!phydev)
>> + goto return_error;
>> + } else {
>> + phydev = phy_find_first(adapter->mdiobus);
>> + if (!phydev)
>> + goto return_error;
>> +
>> + ret = phy_connect_direct(netdev, phydev,
>> + lan743x_phy_link_status_change,
>> + PHY_INTERFACE_MODE_GMII);
>> + /* Note: We cannot use phyifc here because this would be SGMII
>> + * on a standard PC.
>> + */
>
> I don't understand this comment.
>
See above the lengthy section. On a PC SGMII is returned when I call of_get_phy_mode(phynode, &phyifc); but the original driver is using PHY_INTERFACE_MODE_GMII; and I don’t dare to change this behavior. Which I would do when I would pass on the result of of_get_phy_mode(). That’s what I meant by the comment.
Thanks a lot directing me to the proper way,
Roelof
Powered by blists - more mailing lists