Date:   Tue, 19 Jul 2022 02:17:20 +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 03:18:11AM +0300, Vladimir Oltean wrote:
> On Tue, Jul 19, 2022 at 01:32:26AM +0200, Christian Marangi wrote:
> > 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.
> 
> That's a slight disadvantage of this approach, that DSA sometimes checks
> for the presence of a certain function pointer as an indication of
> whether a feature is supported or not. However that doesn't work in all
> cases, and then, it is actually necessary to call and see if it returns
> -EOPNOTSUPP or not. For example, commit 1054457006d4 ("net: phy:
> phylink: fix DSA mac_select_pcs() introduction") had to do just that
> in phylink because of DSA.
> 
> However, you need to check how each specific DSA operation is handled.
> For example, the no-op implementation of get_phy_flags is to return 0
> (meaning "no special flags, thank you"). The no-op implementation for
> connect_tag_protocol is to return success (0) for the tagging protocol
> you support, and -EOPNOTSUPP for everything else. Here -EOPNOTSUPP isn't
> a special code, it is an actual hard error that denies a certain tag
> protocol from attaching.
> 
> The advantage is that your driver-private ops don't have to map 1:1 with
> the dsa_switch_ops, so there is more potential for code reuse than if
> you had to reimplement an entire (*setup) function for example. You can
> have ops for small things like regmap creation, things like that.
> 
> > 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)
> 
> Please optimize the patches for a reviewer with average intelligence and
> the attention span of a fish. 23 patches sounds like the series would
> fail on the attention span count.
> 
> > 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;
> 
> Answered above.
> 
> > 	ops->setup = qca8k_setup;
> 
> Separate sub-operation, although this is a sub-optimal short-term
> solution that kind of undermines the approach with a single
> dsa_switch_ops in the long run.
> 
> > 	ops->phylink_get_caps = qca8k_phylink_get_caps;
> 
> Not sure what's going to be common and what's going to be different, but
> you can take other drivers as an example, some parts will be common and
> some hidden behind priv->info->mac_port_get_caps().
> 
> static void mt753x_phylink_get_caps(struct dsa_switch *ds, int port,
> 				    struct phylink_config *config)
> {
> 	struct mt7530_priv *priv = ds->priv;
> 
> 	/* This switch only supports full-duplex at 1Gbps */
> 	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> 				   MAC_10 | MAC_100 | MAC_1000FD;
> 
> 	/* This driver does not make use of the speed, duplex, pause or the
> 	 * advertisement in its mac_config, so it is safe to mark this driver
> 	 * as non-legacy.
> 	 */
> 	config->legacy_pre_march2020 = false;
> 
> 	priv->info->mac_port_get_caps(ds, port, config);
> }
> 
> > 	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;
> 
> Hard to comment for these phylink ops how to organize the switch
> differences in the best way, since I don't actually know what those
> differences are. Again, other drivers may be useful.
> 
> > 	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)
> 
> Maybe it's best to think this conversion through and not rush a patch set.
> I don't want you to blindly follow my advice to have a single dsa_switch_ops,
> then half-ass it. This kind of thing needs to be done with care and
> forethought.

Wonder if a good idea would be leave things as is for now and work of a
single dsa_switch_ops on another series.

With "leave things as is" I mean that function will get migrated to
qca8k-common.c and exposed with the header file.

And the dsa_switch_ops is defined in qca8k specific code.

The warn about the 23 patch was scary so considering this series is
already a bit big and I can squash only a few patch, putting extra logic
to correctly handle each would make this even bigger.

Think the right thing to do is handling the changes for single
dsa_switch_ops to a separate series and at the same time also get some
info on ipq4019 and what can be generalized.

What do you think?

-- 
	Ansuel

Powered by blists - more mailing lists