3:39 a.m.
Hi Edward,
I love your patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url:
https://github.com/0day-ci/linux/commits/Edward-Cree/sfc_ef100-driver-for...
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
59632b220f2d61df274ed3a14a204e941051fdad
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
All errors (new ones prefixed by >>):
mips-linux-ld: drivers/net/ethernet/sfc/ef100_nic.o: in function
`efx_mcdi_sensor_event':
> ef100_nic.c:(.text.efx_mcdi_sensor_event+0x0): multiple
definition of `efx_mcdi_sensor_event';
drivers/net/ethernet/sfc/mcdi_mon.o:mcdi_mon.c:(.text.efx_mcdi_sensor_event+0x0): first
defined here
> mips-linux-ld:
drivers/net/ethernet/sfc/ef100_netdev.o:(.bss.efx_separate_tx_channels+0x0): multiple
definition of `efx_separate_tx_channels';
drivers/net/ethernet/sfc/efx.o:(.bss.efx_separate_tx_channels+0x0): first defined here
> mips-linux-ld: drivers/net/ethernet/sfc/ef100_ethtool.o:(.data.efx_driver_name+0x0):
multiple definition of `efx_driver_name';
drivers/net/ethernet/sfc/ethtool.o:(.data.efx_driver_name+0x0): first defined here
mips-linux-ld: drivers/net/ethernet/sfc/ef100_rx.o: in function `__efx_rx_packet':
> ef100_rx.c:(.text.__efx_rx_packet+0x0): multiple definition of
`__efx_rx_packet'; drivers/net/ethernet/sfc/rx.o:rx.c:(.text.__efx_rx_packet+0x0):
first defined here
mips-linux-ld: drivers/net/ethernet/sfc/ef100_tx.o: in
function `efx_enqueue_skb_tso':
> ef100_tx.c:(.text.efx_enqueue_skb_tso+0x0): multiple definition
of `efx_enqueue_skb_tso';
drivers/net/ethernet/sfc/tx_tso.o:tx_tso.c:(.text.efx_enqueue_skb_tso+0x0): first defined
here
mips-linux-ld: drivers/net/ethernet/sfc/ef100_tx.o: in function
`efx_enqueue_skb':
> ef100_tx.c:(.text.efx_enqueue_skb+0x0): multiple definition of
`efx_enqueue_skb'; drivers/net/ethernet/sfc/tx.o:tx.c:(.text.efx_enqueue_skb+0x0):
first defined here
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
12:48 a.m.
New subject: [PATCH v3 net-next 04/16] sfc_ef100: skeleton EF100 PF driver
On 16/07/2020 18:39, kernel test robot wrote:
[auto build test ERROR on net-next/master]
...
config: mips-allyesconfig (attached as .config)
...
mips-linux-ld: drivers/net/ethernet/sfc/ef100_nic.o: in function
`efx_mcdi_sensor_event':
>> ef100_nic.c:(.text.efx_mcdi_sensor_event+0x0): multiple definition of
`efx_mcdi_sensor_event';
drivers/net/ethernet/sfc/mcdi_mon.o:mcdi_mon.c:(.text.efx_mcdi_sensor_event+0x0): first
defined here
Well, this holes us below the waterline.
When the sfc team was originally developing this driver, we
didn't anticipate this problem (in fact we tested a build with
both drivers 'y' and it apparently worked. It doesn't now, I
can reproduce this problem locally by just setting CONFIG_SFC=y
CONFIG_SFC_EF100=y in my normal .config, no specific need for
mips-allyesconfig). So we leaned pretty heavily on the 'use
the linker to select NIC-specific functions in calls from
common code' trick.
Some of these can be replaced with function pointers in struct
efx_nic_type (like this sensor-event handler above). But:
mips-linux-ld: drivers/net/ethernet/sfc/ef100_rx.o: in function
`__efx_rx_packet':
>> ef100_rx.c:(.text.__efx_rx_packet+0x0): multiple definition of
`__efx_rx_packet'; drivers/net/ethernet/sfc/rx.o:rx.c:(.text.__efx_rx_packet+0x0):
first defined here
mips-linux-ld: drivers/net/ethernet/sfc/ef100_tx.o: in function
`efx_enqueue_skb':
>> ef100_tx.c:(.text.efx_enqueue_skb+0x0): multiple definition of
`efx_enqueue_skb'; drivers/net/ethernet/sfc/tx.o:tx.c:(.text.efx_enqueue_skb+0x0):
first defined here
These two functions are right on the data path, where we really
don't want indirect calls and retpoline overhead.
I wondered if there were a way to deploy INDIRECT_CALLABLE, but
I don't see how to make it deal with all the cases:
* both 'y': both symbols reachable from the common code, so a
straightforward use of INDIRECT_CALLABLE to speed them up.
* both 'm': each time the common is linked, only the symbol
from the current module is reachable. The current link trick
handles this.
* one 'y' and the other 'm': from the built-in link, only the
y-module's symbol is reachable, but from the module, both are.
(Also, I get a lot of "drivers/net/ethernet/sfc/efx_common.o:
(__param+0x8): undefined reference to `__this_module'" and I
don't really understand why.)
And while in principle this should be fixable with a lot of
#if IS_REACHABLE() and #ifdef MODULE... the common code is only
built once AIUI, which is why I had to move stuff like
efx_driver_name in the first place! We would need a different
(e.g.) efx_common.o to link into each of a built-in and a
modular driver.
Aaaaargh; does anyone have any bright ideas?
-ed
2:45 a.m.
New subject: [PATCH v3 net-next 04/16] sfc_ef100: skeleton EF100 PF driver
On Tue, 21 Jul 2020 15:48:00 +0100 Edward Cree wrote:
Aaaaargh; does anyone have any bright ideas?
No bright ideas. Why do you want the driver to be modular in the first
place? This is a high performance adapter, not a embedded system. Does
this ~1MB of memory use really matter? Folks who care can rebuild their
kernel and disable the device support with a boolean flag.
Maybe I'm wrong, but I've never seen a reason to break up vendor drivers
for high performance NICs into multiple modules.
3:09 a.m.
New subject: [PATCH v3 net-next 04/16] sfc_ef100: skeleton EF100 PF driver
On 21/07/2020 17:45, Jakub Kicinski wrote:
On Tue, 21 Jul 2020 15:48:00 +0100 Edward Cree wrote:
> Aaaaargh; does anyone have any bright ideas?
No bright ideas. Why do you want the driver to be modular in the first
place?
Well, 'sfc' already is, and I'm not sure changing that is an
option
(wouldn't it break users' scripts?). And I find development is a lot
easier if you can just rebuild a module and reload it rather than
having to wait for LD to put together a whole new vmlinux.
Maybe I'm wrong, but I've never seen a reason to break up
vendor drivers
for high performance NICs into multiple modules.
So, what are you suggesting?
1) both drivers are builtin-only
2) a single module containing both drivers
3) something else?
Both (1) and (2) would allow replacing the linker trick with an if()
on efx->revision or an efx->type-> function with INDIRECT_CALLABLE.
I don't know for sure but I suspect we made the drivers separate
modules simply because we could (or so we thought) and we didn't
know for certain no-one would ever want the extra flexibility.
I'll ask around and see if there's any reason we can't do (2).
-ed
3:15 a.m.
New subject: [PATCH v3 net-next 04/16] sfc_ef100: skeleton EF100 PF driver
On Tue, 21 Jul 2020 18:09:13 +0100 Edward Cree wrote:
On 21/07/2020 17:45, Jakub Kicinski wrote:
> On Tue, 21 Jul 2020 15:48:00 +0100 Edward Cree wrote:
>> Aaaaargh; does anyone have any bright ideas?
> No bright ideas. Why do you want the driver to be modular in the first
> place?
Well, 'sfc' already is, and I'm not sure changing that is an option
(wouldn't it break users' scripts?). And I find development is a lot
easier if you can just rebuild a module and reload it rather than
having to wait for LD to put together a whole new vmlinux.
> Maybe I'm wrong, but I've never seen a reason to break up vendor drivers
> for high performance NICs into multiple modules.
So, what are you suggesting?
I was talking about option 2 below, yes.
1) both drivers are builtin-only
2) a single module containing both drivers
3) something else?
Both (1) and (2) would allow replacing the linker trick with an if()
on efx->revision or an efx->type-> function with INDIRECT_CALLABLE.
I don't know for sure but I suspect we made the drivers separate
modules simply because we could (or so we thought) and we didn't
know for certain no-one would ever want the extra flexibility.
I'll ask around and see if there's any reason we can't do (2).
-ed
791
days inactive
796
days old
4 comments
3 participants
participants (3)
-
Edward Cree -
Jakub Kicinski -
kernel test robot