netdev - Re: [PATCH net-next 5/6] net: dsa: use swnode fixed-link if using default params

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: <YtHd3f22AtrIzZ1K@shell.armlinux.org.uk>
Date:   Fri, 15 Jul 2022 22:36:29 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Alvin __ipraga <alsi@...g-olufsen.dk>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Daniel Scally <djrscally@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        DENG Qingfang <dqfext@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        George McCollister <george.mccollister@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        Matthias Brugger <matthias.bgg@...il.com>,
        netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Sean Wang <sean.wang@...iatek.com>,
        UNGLinuxDriver@...rochip.com,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Marek Behún <kabel@...nel.org>
Subject: Re: [PATCH net-next 5/6] net: dsa: use swnode fixed-link if using
 default params

On Fri, Jul 15, 2022 at 11:11:18PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 15, 2022 at 05:01:48PM +0100, Russell King (Oracle) wrote:
> > Create and use a swnode fixed-link specification for phylink if no
> > parameters are given in DT for a fixed-link. This allows phylink to
> > be used for "default" cases for DSA and CPU ports. Enable the use
> > of phylink in all cases for DSA and CPU ports.
> 
> > Co-developed by Vladimir Oltean and myself.
> 
> Why not to use
> 
>   Co-developed-by: Vladimir Oltean <vladimir.oltean@....com>

Ah, that's an official thing. Thanks.

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > Reviewed-by: Marek Behún <kabel@...nel.org>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> 
> ...
> 
> > +static struct {
> > +	unsigned long mask;
> > +	int speed;
> > +	int duplex;
> > +} phylink_caps_params[] = {
> > +	{ MAC_400000FD, SPEED_400000, DUPLEX_FULL },
> > +	{ MAC_200000FD, SPEED_200000, DUPLEX_FULL },
> > +	{ MAC_100000FD, SPEED_100000, DUPLEX_FULL },
> > +	{ MAC_56000FD,  SPEED_56000,  DUPLEX_FULL },
> > +	{ MAC_50000FD,  SPEED_50000,  DUPLEX_FULL },
> > +	{ MAC_40000FD,  SPEED_40000,  DUPLEX_FULL },
> > +	{ MAC_25000FD,  SPEED_25000,  DUPLEX_FULL },
> > +	{ MAC_20000FD,  SPEED_20000,  DUPLEX_FULL },
> > +	{ MAC_10000FD,  SPEED_10000,  DUPLEX_FULL },
> > +	{ MAC_5000FD,   SPEED_5000,   DUPLEX_FULL },
> > +	{ MAC_2500FD,   SPEED_2500,   DUPLEX_FULL },
> > +	{ MAC_1000FD,   SPEED_1000,   DUPLEX_FULL },
> > +	{ MAC_100FD,    SPEED_100,    DUPLEX_FULL },
> > +	{ MAC_10FD,     SPEED_10,     DUPLEX_FULL },
> > +	{ MAC_1000HD,   SPEED_1000,   DUPLEX_HALF },
> > +	{ MAC_100HD,    SPEED_100,    DUPLEX_HALF },
> > +	{ MAC_10HD,     SPEED_10,     DUPLEX_HALF },
> > +};
> > +
> > +static int dsa_port_find_max_speed(unsigned long caps, int *speed, int *duplex)
> > +{
> > +	int i;
> > +
> > +	*speed = SPEED_UNKNOWN;
> > +	*duplex = DUPLEX_UNKNOWN;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(phylink_caps_params); i++) {
> > +		if (caps & phylink_caps_params[i].mask) {
> > +			*speed = phylink_caps_params[i].speed;
> > +			*duplex = phylink_caps_params[i].duplex;
> 
> > +			break;
> 
> With the below check it's way too protective programming.
> 
> 			return 0;
> 
> > +		}
> > +	}
> > +
> > +	return *speed == SPEED_UNKNOWN ? -EINVAL : 0;
> 
> 	return -EINVAL;

Ok.

> > +static struct fwnode_handle *dsa_port_get_fwnode(struct dsa_port *dp,
> > +						 phy_interface_t mode)
> > +{
> 
> > +	struct property_entry fixed_link_props[3] = { };
> > +	struct property_entry port_props[3] = {};
> 
> A bit of consistency in the assignments?
> 
> Also it seems you are using up to 2 for the first one and only 1 in the second
> one. IIUC it requires a terminator entry, so it means 3 and 2. Do we really
> need 3 in the second case?

Probably not - that came from Vladimir's patch, and I removed the "reg"
property without fixing this up. Thanks for spotting.

> > +	struct fwnode_handle *fixed_link_fwnode;
> > +	struct fwnode_handle *new_port_fwnode;
> > +	struct device_node *dn = dp->dn;
> > +	struct device_node *phy_node;
> > +	int err, speed, duplex;
> > +	unsigned long caps;
> > +
> > +	phy_node = of_parse_phandle(dn, "phy-handle", 0);
> 
> fwnode in the name, why not to use fwnode APIs?
> 
> 	fwnode_find_reference();

Marcin has a series converting DSA to use fwnode things - currently DSA
does not support ACPI, so converting it to fwnode doesn't make that much
sese until the proper ACPI patches get merged, which have now been
rebased on this series by Marcin in the expectation that these patches
would be merged... so I don't want to tred on Marcin's feet on that.

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