netdev - Re: [PATCH net-next v3 7/8] dpaa2-mac: configure the SerDes phy on a protocol change

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]

Date:   Thu, 10 Mar 2022 15:05:50 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Ioana Ciornei <ioana.ciornei@....com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        kishon@...com, vkoul@...nel.org, robh+dt@...nel.org,
        leoyang.li@....com, linux-phy@...ts.infradead.org,
        devicetree@...r.kernel.org, shawnguo@...nel.org,
        hongxing.zhu@....com
Subject: Re: [PATCH net-next v3 7/8] dpaa2-mac: configure the SerDes phy on a
 protocol change

On Thu, Mar 10, 2022 at 04:51:59PM +0200, Ioana Ciornei wrote:
> This patch integrates the dpaa2-eth driver with the generic PHY
> infrastructure in order to search, find and reconfigure the SerDes lanes
> in case of a protocol change.
> 
> On the .mac_config() callback, the phy_set_mode_ext() API is called so
> that the Lynx 28G SerDes PHY driver can change the lane's configuration.
> In the same phylink callback the MC firmware is called so that it
> reconfigures the MAC side to run using the new protocol.
> 
> The consumer drivers - dpaa2-eth and dpaa2-switch - are updated to call
> the dpaa2_mac_start/stop functions newly added which will
> power_on/power_off the associated SerDes lane.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>

Looks better, there's a minor thing that I missed, sorry:

> +	if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE &&
> +	    !phy_interface_mode_is_rgmii(mac->if_mode) &&
> +	    is_of_node(dpmac_node)) {
> +		serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL);
> +
> +		if (IS_ERR(serdes_phy)) {
> +			if (PTR_ERR(serdes_phy) == -ENODEV)
> +				serdes_phy = NULL;
> +			else
> +				return PTR_ERR(serdes_phy);
> +		} else {
> +			phy_init(serdes_phy);
> +		}

Would:
		if (PTR_ERR(serdes_phy) == -ENODEV)
			serdes_phy = NULL;
		else if (IS_ERR(serdes_phy))
			return PTR_ERR(serdes_phy);
		else
			phy_init(serdes_phy);

be neater? There is no need to check IS_ERR() before testing PTR_ERR().
One may also prefer the pointer-comparison approach:

		if (serdes_phy == ERR_PTR(-ENODEV))

to remove any question about PTR_ERR(p) on a !IS_ERR(p) value too, but
it really doesn't make any difference.

I suspect this is just a code formatting issue, I'd think the compiler
would generate reasonable code either way, so as I said above, it's
quite minor.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!