linux-kernel - Re: [RFC PATCH net-next 5/6] microchip: lan865x: add driver support for Microchip's LAN865X MACPHY

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: <8e9fecd4-f9bc-46d0-b7bf-fbf7ac83cc80@lunn.ch>
Date:   Tue, 19 Sep 2023 14:50:43 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Parthiban.Veerasooran@...rochip.com
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        corbet@....net, Steen.Hegelund@...rochip.com,
        rdunlap@...radead.org, horms@...nel.org, casper.casan@...il.com,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        Horatiu.Vultur@...rochip.com, Woojung.Huh@...rochip.com,
        Nicolas.Ferre@...rochip.com, UNGLinuxDriver@...rochip.com,
        Thorsten.Kummermehr@...rochip.com
Subject: Re: [RFC PATCH net-next 5/6] microchip: lan865x: add driver support
 for Microchip's LAN865X MACPHY

> Sure, I can move this part to oa_tc6 lib. If I understand you correctly 
> you are talking about the Standard Capabilities Register (0x0002) 
> defined in the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface spec 
> right? If so, the 9th bit of this register tells about Indirect PHY 
> Register access Capability. Did you mean this bit? If so, this bit 
> describes the below,
> 
> IPRAC - Indirect PHY Register Access Capability. Indicates if PHY 
> registers are indirectly accessible through the MDIO/MDC registers MDIOACCn.

Yes. If the core relies on any functionality which is optional in the
standard, it should check if the capability bit is set, and do a
dev_erro() and return -ENODEV if a device does not actually have
it. That makes it clear the core needs extending to support a device.

If you are only using mandatory parts of the spec, then no test is
needed.

> > I would expect to see a call to phy_ethtool_ksettings_set()
> > here. phylib should be able to do some of the validation.
> Ah ok, doing the below will make the life easier.
> .set_link_ksettings	= phy_ethtool_set_link_ksettings,

Please do some testing and check that phy_ethtool_set_link_ksettings
doe actually reject all invalid setting. I cannot guarantee it does,
and if it does not, it might actually be a PHY driver bug.

> >> +static int lan865x_net_open(struct net_device *netdev)
> >> +{
> >> +     struct lan865x_priv *priv = netdev_priv(netdev);
> >> +     int ret;
> >> +
> >> +     if (!is_valid_ether_addr(netdev->dev_addr)) {
> >> +             if (netif_msg_ifup(priv))
> >> +                     netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
> >> +             return -EADDRNOTAVAIL;
> > 
> > Using a random MAC address is the normal workaround for not having a
> > valid MAC address via OTP flash etc.
> Ah ok, you mean to use eth_hw_addr_random(netdev) instead of returning 
> error.

Yes. And this is generally done earlier than open, as part of
probe. You want to avoid surprising userspace when the MAC address
suddenly changes at open time.

	 Andrew