Date: Thu, 23 Nov 2023 02:05:21 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
CC: Krzysztof Kozlowski <krzk@...nel.org>, "linus.walleij@...aro.org"
	<linus.walleij@...aro.org>, Vladimir Oltean <olteanv@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "andrew@...n.ch"
	<andrew@...n.ch>, "f.fainelli@...il.com" <f.fainelli@...il.com>,
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "arinc.unal@...nc9.com"
	<arinc.unal@...nc9.com>
Subject: Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

On Wed, Nov 22, 2023 at 07:44:44PM -0300, Luiz Angelo Daros de Luca wrote:
> For the driver structure, we have a few options:
> 
> 1) Merge everything into a single realtek-switch, declaring the
> compatible strings once and implementing it as both a platform and an
> mdio driver.
> 2) Have a module for each interface (realtek-smi and realtek-mdio),
> both repeating the compatible strings for all families, and load the
> family as an interface dependency (the current approach).
> 3) Introduce a realtek-common module, a module for each interface,
> both repeating the compatible strings for all families, and load the
> family as an interface dependency (the first patch in this series).

As you say below, this is too much for your test device. Skip.

> 4) Introduce a realtek-common module, a module for each interface,
> both repeating the compatible strings for all families, and expect the
> family module to be already loaded.

The kernel should be able to autoload. Skip.

> 5) Introduce a realtek-common module, a module for each interface,
> both repeating the compatible strings for all families, and request
> the family module to be loaded (the last patch in this series).

I had reservations before about this approach and I think I am not
the only one. Let's consider the other options.

> 6) Introduce a realtek-common module, merging everything from
> realtek-smi and realtek-mdio but the driver declarations and implement
> each family as a single module that works both as a platform and an
> mdio driver.
> 7) Introduce a realtek-common module, merging everything from
> realtek-smi and realtek-mdio but the driver declarations and create
> two modules for each family as real drivers, repeating the compatible
> strings on each family (e.g., {rtl8366rb, rtl8365mb}_{smi,mdio}.ko).

Yeah, something like this. Roughly I think we want:

- generic boilerplate probe/remove and regmap setup code in
  realtek-common.c
- interface code in realtek-{smi,mdio}.c
- chip variant code in rtl8365mb.c and rtl8366rb.c

The module dependencies only need to go upwards, i.e.:

       realtek-common
         ^       ^
         |       |  use symbols realtek_common_probe()
         |       |              realtek_common_remove()
         |       |
 realtek-smi   realtek-mdio 
      ^   ^     ^   ^
      |   |     |   |  use symbols realtek_mdio_probe()
      |    \   /    |              realtek_mdio_remove()
      |     \ /     |
      |      X      |  or symbols realtek_smi_probe()
      |     / \     |             realtek_smi_remove()
      |    /   \    |
      |   /     \   |  or both
      |  /       \  |
  rtl8365mb     rtl8366rb

We already see what realtek_common_probe() etc. look like in your patch
series here. If an interface driver needs to do something specific and
it can't do that before or after calling realtek_common_probe(), then it
can be abstracted away into some realtek_common_info struct and the
interface driver can populate this and pass it along.

The interface driver also ought to provide its own probe function that
the chip variant driver can call. Again, if there is something specific
that needs doing in the middle of this process it can call into some
realtek_{smi,mdio}_info function pointer.

I made similar points in my review of your last series so you can look
there for specific instances where I think this is valuable.

After that, the chip variant driver can do some #if ENABLED() logic to
selectively register itself as either a platform driver (SMI), MDIO
driver (MDIO), or both. The MODULE_DEVICE_TABLE is just used by the bus
driver in question (platform or MDIO) to match with the appropriate
driver. Compatible strings are the same, but it's the same chip after
all. In fact, I don't think you even need two MODULE_DEVICE_TABLEs. You
can pass the same one to both the mdio_driver and platform_driver
structs.

I think one needs to look critically at the data being passed around
between the constituent modules at the moment. Some observations:

- for MDIO, the only thing the chip variant driver needs to give it is a
  detect function and its dsa_switch_ops, so why not:

    struct realtek_mdio_info {
      int (*detect)(struct realtek_priv *priv);
      const struct dsa_switch_ops *ds_ops;
    }

- for SMI, some more info is needed:

    struct realtek_smi_info {
      int (*detect)(struct realtek_priv *priv);
      int (*phy_read)(struct realtek_priv *priv, int phy, int regnum);
      int (*phy_write)(struct realtek_priv *priv, int phy, int regnum,
                       u16 val);
      const struct dsa_switch_ops *ds_ops;
    }

  Yes, there is a lot more in struct realtek_ops, but all of that is
  used by rtl8366-core.c. I think this is trivial to disentangle but it
  might result in a fairly large diff if one tries to remove this from
  struct realtek_ops. My point being you don't need to actually populate
  this for the interface driver itself.

With that we can define reasonable signatures for the interface
drivers' probe functions:

  int realtek_smi_probe(struct platform_device *pdev,
                        const struct realtek_smi_info *info);
  int realtek_mdio_probe(struct mdio_device *mdiodev,
                         const struct realtek_mdio_info *info);
  EXPORT_SYMBOL_GPL(both of these functions);

The remove functions can just take the pdev or mdiodev pointers to keep
things balanced.

Now the interface drivers can populate realtek_priv sufficiently for the
realtek_common_probe() to do its job. 

  struct realtek_interface_info {
      static const struct regmap_config *regmap_config;
      static const struct regmap_config *regmap_config_nolock;
      int (*detect)(struct realtek_priv *priv);
      int (*setup_interface)(struct dsa_switch *ds);
      const struct dsa_switch_ops *ds_ops;
  };

The regmap configs are self explanatory.

The setup_interface function pointer is only needed for SMI, and
currently the chip interface drivers are the ones calling it. It is kind
of ugly that this gets passed all the way up, copied into realtek_priv,
and then called all the way down again... there is probably a more
elegant solution. I am mainly trying to illustrate how to handle the
module knot you are trying to unpick.

Now I recall that Vladimir suggested something more along the lines of
realtek_common_probe_pre() and post, and ditto for remove(). You might
prefer that approach. Something to experiment with. You might need
something like that because there are those GPIOs that SMI wants to
fiddle with, and they need to be ready before calling
dsa_register_switch(), and the priv datastructure is not allocated
before calling the common probe function.

Anyway, with the above suggestion I think one can move the vast majority
of common code into realtek-common while keeping the chip variant
drivers mostly unmodified. All that's needed is to move the
MODULE_DEVICE_TABLES into those files and create custom module init/exit
functions which conditionally register themselves as platform and mdio
drivers. This is what the module_platform_driver() and
mdio_module_driver() macros do - they just make the boilerplate
init/exit for you. But nothing stops you from registering both a struct
platform_driver and a struct mdio_driver. Just guard everything with
#ifs to ensure clean compilation.

The relevant probe functions will look like this (rtl8365mb example):

  #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI)
  static const struct realtek_smi_info rtl8365mb_smi_info = {
    .detect = rtl8365mb_detect,
    .phy_read = rtl8365mb_phy_read,
    .phy_write = rtl8365mb_phy_write,
    .ds_ops = rtl8365mb_switch_ops,
  };
  
  int rtl8365mb_smi_probe(struct platform_device *pdev) {
    return realtek_smi_probe(pdev, &rtl8365mb_smi_info);
  }
  #endif
  
  #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO)
  static const struct realtek_mdio_info rtl8365mb_mdio_info = {
    .detect = rtl8365mb_detect,
    .ds_ops = rtl8365mb_switch_ops,
  };
  
  int rtl8365mb_mdio_probe(struct mdio_device *mdiodev) {
    return realtek_mdio_probe(mdiodev, &rtl8365mb_mdio_info);
  }
  #endif

One thing I dislike is that all the driver private data is getting
allocated way up in the common driver. It would be nicer if the chip
variant or interface drivers could also allocate their own stuff and
just put it in a priv pointer of the parent. Likewise I was never a fan
of the fact that the chip variant drivers always receiving realtek_priv
pointers rather than pointers to their driver private data. You can see
a lot of diving into chip_data especially in rtl8365mb for this
reason. I regret not addressing that before I added the new driver.

> 
> Solutions 1, 2, and 3 may use more resources than needed. My test
> device, for example, cannot handle them. Solution 7 is similar to the
> examples I found in the kernel. Solutions 1 and 6 are the only ones
> not repeating the same compatible strings in different modules, if
> that's a problem.

You will have noticed that with the above solution, the chip variant
modules will invariably require both interface modules to be loaded if
the kernel is built with both REALTEK_SMI and REALTEK_MDIO. I hope that
your resource-constrained system has enough headroom to accommodate
that. If not then I am afraid you might simply be out of luck.

Kind regards,
Alvin

Powered by blists - more mailing lists