Date:   Thu, 18 Jun 2020 17:34:49 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        "michael@...le.cc" <michael@...le.cc>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>
Subject: RE: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

> Subject: Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module
> 
> On Thu, Jun 18, 2020 at 04:17:56PM +0000, Ioana Ciornei wrote:
> > > > +struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device
> > > > +*mdio_dev) {
> > > > +	struct mdio_lynx_pcs *pcs;
> > > > +
> > > > +	if (WARN_ON(!mdio_dev))
> > > > +		return NULL;
> > > > +
> > > > +	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
> > > > +	if (!pcs)
> > > > +		return NULL;
> > > > +
> > > > +	pcs->dev = mdio_dev;
> > > > +	pcs->an_restart = lynx_pcs_an_restart;
> > > > +	pcs->get_state = lynx_pcs_get_state;
> > > > +	pcs->link_up = lynx_pcs_link_up;
> > > > +	pcs->config = lynx_pcs_config;
> > >
> > > We really should not have these private structure interfaces.
> > > Private structure- based driver specific interfaces really don't
> > > constitute a sane approach to interface design.
> > >
> > > Would it work if there was a "struct mdio_device" add to the
> > > phylink_config structure, and then you could have the
> > > phylink_pcs_ops embedded into this driver?
> >
> > I think that would restrict too much the usage.
> > I am afraid there will be instances where the PCS is not recognizable
> > by PHY_ID, thus no way of knowing which driver to probe which mdio_device.
> > Also, I would leave to the driver the choice of using (or not) the
> > functions exported by Lynx.
> 
> I think you've taken my point way too far.  What I'm complaining about is the
> indirection of lynx_pcs_an_restart() et.al. through a driver- private structure.
> 
> In order to access lynx_pcs_an_restart(), we need to dereference struct
> mdio_lynx_pcs, which is a structure specific to this lynx PCS driver.  What this
> leads to is users doing this:
> 
> 	if (pcs_is_lynx) {
> 		struct mdio_lynx_pcs *pcs = foo->bar;
> 
> 		pcs->an_restart(...);
> 	} else if (pcs_is_something_else) {
> 		struct mdio_somethingelse_pcs *pcs = foo->bar;
> 
> 		pcs->an_restart(...);
> 	}
> 
> which really does not scale.  A step forward would be:
> 
> 	if (pcs_is_lynx) {
> 		lynx_pcs_an_restart(...);
> 	} else if (pcs_is_something_else) {
> 		something_else_pcs_an_restart(...);
> 	}
> 
> but that also scales horribly.

This is what I was proposing. I can of course take the indirection away
and just export the functions.

Are there really instances where the ethernet driver has to manage multiple
different types of PCSs? I am not sure this type of snippet of code is really
going to occur.

> 
> Even better would be to have a generic set of operations for PCS devices that
> can be declared in the lynx PCS driver and used externally... like, maybe struct
> phylink_pcs_ops, which is made globally visible for MAC drivers to use with
> phylink_add_pcs().
> 
> Or maybe a function in this lynx PCS driver that calls phylink_add_pcs() itself with
> its own PCS operations, and maybe also sets a field in struct phylink_config for
> the PCS mdio device?
>

I am not sure how this would work with Felix and DSA drivers in general since the
DSA core is hiding the phylink_pcs_ops from the actual switch driver.

> Or something like that - just some a way that doesn't force us down a path that
> we end up forcing people into code that has to decide what sort of PCS we have
> at runtime in all these method paths.

I get what you are saying but I do not know of any drivers that actually need this
distinction at runtime.

Ioana

> 
> > What if we directly export the helper functions directly as symbols
> > which can be used by the driver without any mdio_lynx_pcs in the
> > middle (just the mdio_device passed to the function).
> > This would be exactly as phylink_mii_c22_pcs_[an_restart/config] are
> > currently used.
> 
> The difference is that phylink_mii_c22_pcs_* are designed as library functions -
> functions that are very likely to be re-used for multiple different PCS (because
> the format, location, and access method of these registers is defined by IEEE
> 802.3).  It's a bit like phylib's configuration of autoneg - we don't have all the
> individual drivers doing that, we have core code that does that for us in the form
> of helpers.
> 

Powered by blists - more mailing lists