Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Nov 01, 2023 at 04:55:19PM -0300, Luiz Angelo Daros de Luca wrote:
> Hi Vladimir,
> 
> > realtek-mdio is an MDIO driver while realtek-smi is a platform driver
> > implementing a bitbang protocol. They might never be used together in
> > a system to share RAM and not even installed together in small
> > systems. If I do need to share the code, I would just link it twice.
> > Would something like this be acceptable?
> >
> > drivers/net/dsa/realtek/Makefile
> > -obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o
> > -obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o
> > +obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o realtek_common.o
> > +obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o realtek_common.o

You cannot link realtek_common.o into multiple .ko files. It generates
build warnings and it is being phased out.
https://patchwork.kernel.org/project/netdevbpf/cover/20221119225650.1044591-1-alobakin@pm.me/

> Just a follow up.
> 
> It is not that simple to include a .c file into an existing single
> file module. It looks like you need to rename the original file as all
> linked objects must not conflict with the module name. The kernel
> build seems to create a new object file for each module. Is there a
> clearer way? I think #include a common .c file would not be
> acceptable.
> 
> I tested your shared module suggestion. It is the clearest one but it
> also increased the overall size quite a bit. Even linking two objects
> seems to use the double of space used by the functions alone. These
> are some values (mips)
> 
> drivers/net/dsa/realtek/realtek_common.o  5744  without exports
> drivers/net/dsa/realtek/realtek_common.o 33472  exporting the two reset functions (assert/deassert)
> 
> drivers/net/dsa/realtek/realtek-mdio.o   18756  without the reset funcs (to be used as a module)
> drivers/net/dsa/realtek/realtek-mdio.o   20480  including the realtek_common.c (#include <realtek_common.c>)
> drivers/net/dsa/realtek/realtek-mdio.o   22696  linking the realtek_common.o
> 
> drivers/net/dsa/realtek/realtek-smi.o    30712  without the reset funcs (to be used as a module)
> drivers/net/dsa/realtek/realtek-smi.o    34604  linking the realtek_common.o
> 
> drivers/net/dsa/realtek/realtek-mdio.ko  28800  without the reset funcs (it will use realtek_common.ko)
> drivers/net/dsa/realtek/realtek-mdio.ko  32736  linking the realtek_common.o
> 
> drivers/net/dsa/realtek/realtek-smi.ko   40708  without the reset funcs (it will use realtek_common.ko)
> drivers/net/dsa/realtek/realtek-smi.ko   44612  linking the realtek_common.o
> 
> In summary, we get about 1.5kb of code with the extra functions,
> almost 4kb if we link a common object containing the functions and
> 33kb if we use a module for those two functions.
> 
> I can go with any option. I just need to know which one would be
> accepted to update my patches.
> 1) keep duplicated functions on each file

Disadvantage: need to update the same functions twice, it becomes
possible for them to diverge, increases maintenance difficulty.

> 2) share the code including the .c on both

Including a .c file with a preprocessor #include is fragile, has to be
placed very carefully, etc. So it is also a time bomb from a maintenance
PoV. Maybe a header with static inline function definitions would be
worth considering, although I don't think it's common practice to do
this.

> 3) share the code linking a common object in both modules (and
> renaming existing .c files)

Sharing object files is being phased out.

> 4) create a new module used by both modules.

I am suspicious of the numbers you provided above. What needs to be
compared is the reduction in size of realtek_mdio.ko and realtek_smi.ko,
compared to the size of the new realtek_common.ko. Also, this starts
being more and more worthwhile as more code goes into realtek_common.ko,
and I also mentioned a common probe/remove/shutdown as being viable
candidates. Looking at your figures, I'm not sure at which ones I need
to look in order to compute that metric.

> The devices that would use this driver have very restricted storage
> space. Every kbyte counts.

Well, in that case you could compile the code as built into the kernel,
and the module overhead would go away.