linux-kernel - Re: [PATCH net-next 18/20] net: ethernet: qualcomm: Add PPE MAC support for phylink

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] [day] [month] [year] [list]

Date: Mon, 22 Jan 2024 17:36:44 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Lei Wei <quic_leiwei@...cinc.com>
Cc: Luo Jie <quic_luoj@...cinc.com>, agross@...nel.org,
	andersson@...nel.org, konrad.dybcio@...aro.org, 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, catalin.marinas@....com,
	will@...nel.org, p.zabel@...gutronix.de, shannon.nelson@....com,
	anthony.l.nguyen@...el.com, jasowang@...hat.com,
	brett.creeley@....com, rrameshbabu@...dia.com,
	joshua.a.hay@...el.com, arnd@...db.de, geert+renesas@...der.be,
	neil.armstrong@...aro.org, dmitry.baryshkov@...aro.org,
	nfraprado@...labora.com, m.szyprowski@...sung.com, u-kumar1@...com,
	jacob.e.keller@...el.com, andrew@...n.ch, netdev@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, ryazanov.s.a@...il.com,
	ansuelsmth@...il.com, quic_kkumarcs@...cinc.com,
	quic_suruchia@...cinc.com, quic_soni@...cinc.com,
	quic_pavir@...cinc.com, quic_souravp@...cinc.com,
	quic_linchen@...cinc.com
Subject: Re: [PATCH net-next 18/20] net: ethernet: qualcomm: Add PPE MAC
 support for phylink

On Mon, Jan 22, 2024 at 11:01:26PM +0800, Lei Wei wrote:
> 
> 
> On 1/10/2024 8:18 PM, Russell King (Oracle) wrote:
> > On Wed, Jan 10, 2024 at 07:40:30PM +0800, Luo Jie wrote:
> > > @@ -352,6 +1230,12 @@ static int ppe_port_maxframe_set(struct ppe_device *ppe_dev,
> > >   }
> > >   static struct ppe_device_ops qcom_ppe_ops = {
> > > +	.phylink_setup = ppe_phylink_setup,
> > > +	.phylink_destroy = ppe_phylink_destroy,
> > > +	.phylink_mac_config = ppe_phylink_mac_config,
> > > +	.phylink_mac_link_up = ppe_phylink_mac_link_up,
> > > +	.phylink_mac_link_down = ppe_phylink_mac_link_down,
> > > +	.phylink_mac_select_pcs = ppe_phylink_mac_select_pcs,
> > >   	.set_maxframe = ppe_port_maxframe_set,
> > >   };
> > 
> > Why this extra layer of abstraction? If you need separate phylink
> > operations, why not implement separate phylink_mac_ops structures?
> > 
> 
> This PPE driver will serve as the base driver for higher level drivers
> such as the ethernet DMA (EDMA) driver and the DSA switch driver.

Why not have the higher level drivers provide a pointer to the
appropriate phylink_mac_ops structure? Having extra levels of
indirection makes my future maintenance of phylink harder (I'm already
bugged by DSA doing this, and it's a right pain.)

For example, if one of your higher level drivers needs the mac_prepare
or mac_finish functionality, you have to add a shim, extra function
pointers and so on.

If I need to add an extra parameter to a method, then I have to fix
up your shim layer _as well_ as all the called methods - in other
words, it adds extra maintenance burden.

It also makes detecting whether an implementation provides something
or not harder - see the problems when mac_select_pcs() was introduced
and rather than testing to see whether the method is populated, we
have to call the method with a dummy value to discover whether the
sub-driver implements it or not. Honestly, I would really like to get
rid of DSA's phylink_mac_ops shim layer.

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