linux-kernel - Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 19 Jul 2022 01:32:26 +0200
From:   Christian Marangi <ansuelsmth@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jens Axboe <axboe@...nel.dk>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop
 qca8k_read/write/rmw for regmap variant

On Tue, Jul 19, 2022 at 02:43:58AM +0300, Vladimir Oltean wrote:
> On Mon, Jul 18, 2022 at 11:54:44PM +0200, Christian Marangi wrote:
> > On Mon, Jul 18, 2022 at 11:30:42PM +0300, Vladimir Oltean wrote:
> > > On Mon, Jul 18, 2022 at 09:30:58PM +0200, Christian Marangi wrote:
> > > > Tell me if I got this wrong.
> > > > 
> > > > The suggestion was to move the struct dsa_switch_ops to qca8k.h and add
> > > > in the specific code probe the needed ops to add to the generic
> > > > struct...
> > > 
> > > The declaration yes; the definition to qca8k-common.c. See for example
> > > where felix_switch_ops is, relative to felix_vsc9959.c, seville_vsc9953.c
> > > (users), felix.h (declaration), and felix.c (definition). Or how
> > > mv88e6xxx_switch_ops does things and still supports a gazillion of switches.
> > 
> > Mhh I checked the example and they doesn't seems to be useful from my
> > problem. But I think it's better to discuss this to the patch directly
> > so you can better understand whay I intended with having dsa_switch_ops
> > set to const.
> 
> So you don't modify the common dsa_switch_ops from the switch-specific
> probe path, but rather, from the common dsa_switch_ops method, you call
> a second function pointer.
> 
> static void felix_phylink_validate(struct dsa_switch *ds, int port,
> 				   unsigned long *supported,
> 				   struct phylink_link_state *state)
> {
> 	struct ocelot *ocelot = ds->priv;
> 	struct felix *felix = ocelot_to_felix(ocelot);
> 
> 	if (felix->info->phylink_validate)
> 		felix->info->phylink_validate(ocelot, port, supported, state);
> }

Ohhh ok now it makes sense.

If the ops is not supported should I return -ENOSUPP?
Example some ops won't be supported like the get_phy_flags or
connect_tag_protocol for example.

Anyway the series is ready, I was just pushing it... At the end it's 23
patch big... (I know you will hate me but at least it's reviewable)

My solution currently was this...

	ops = devm_kzalloc(&mdiodev->dev, sizeof(*ops), GFP_KERNEL);
	if (!ops)
		return -ENOMEM;

	/* Copy common ops */
	memcpy(ops, &qca8k_switch_ops, sizeof(*ops));

	/* Setup specific ops */
	ops->get_tag_protocol = qca8k_get_tag_protocol;
	ops->setup = qca8k_setup;
	ops->phylink_get_caps = qca8k_phylink_get_caps;
	ops->phylink_mac_select_pcs = qca8k_phylink_mac_select_pcs;
	ops->phylink_mac_config = qca8k_phylink_mac_config;
	ops->phylink_mac_link_down = qca8k_phylink_mac_link_down;
	ops->phylink_mac_link_up = qca8k_phylink_mac_link_up;
	ops->get_phy_flags = qca8k_get_phy_flags;
	ops->master_state_change = qca8k_master_change;
	ops->connect_tag_protocol = qca8k_connect_tag_protocol;

	/* Assign the final ops */
	priv->ds->ops = ops;

Will wait your response on how to hanle ops that are not supported.
(I assume dsa checks if an ops is declared and not if it does return
ENOSUPP, so this is my concern your example)

-- 
	Ansuel