Re: nt: usb: USB_RTL8153_ECM should not default to y

On Fri, Sep 17, 2021 at 8:49 PM Jakub Kicinski [off-list ref] wrote:

On Fri, 17 Sep 2021 19:59:15 +0200 Maciej Żenczykowski wrote:

quoted

I've been browsing some usb ethernet dongle related stuff in the
kernel (trying to figure out which options to enable in Android 13
5.~15 kernels), and I've come across the following patch (see topic,
full patch quoted below).

Doesn't it entirely defeat the purpose of the patch it claims to fix
(and the patch that fixed)?
Certainly the reasoning provided (in general device drivers should not
be enabled by default) doesn't jive with me.
The device driver is CDC_ETHER and AFAICT this is just a compatibility
option for it.

Shouldn't it be reverted (ie. the 'default y' line be re-added) ?

AFAICT the logic should be:
  if we have CDC ETHER (aka. ECM), but we don't have R8152 then we
need to have R8153_ECM.

Alternatively, maybe there shouldn't be a config option for this at all?

Instead r8153_ecm should simply be part of cdc_ether.ko iff r8152=n

I'm not knowledgeable enough about Kconfig syntax to know how to
phrase the logic...
Maybe there shouldn't be a Kconfig option at all, and just some Makefile if'ery.

Something like:

obj-$(CONFIG_USB_RTL8152) += r8152.o
obj-$(CONFIG_USB_NET_CDCETHER) += cdc_ether.o obj-
ifndef CONFIG_USB_RTL8152
obj-$(CONFIG_USB_NET_CDCETHER) += r8153_ecm.o
endif

Though it certainly would be nice to use 8153 devices with the
CDCETHER driver even with the r8152 driver enabled...

Yeah.. more context here:

https://lore.kernel.org/all/7fd014f2-c9a5-e7ec-f1c6-b3e4bb0f6eb6@samsung.com/ (local)

default !USB_RTL8152 would be my favorite but that probably doesn't
compute in kconfig land. Or perhaps bring back the 'y' but more clearly
mark it as a sub-option of CDCETHER? It's hard to blame people for
expecting drivers to default to n, we should make it clearer that this
is more of a "make driver X support variation Y", 'cause now it sounds
like a completely standalone driver from the Kconfig wording. At least
to a lay person like myself.

I think:
        depends on USB_NET_CDCETHER && (USB_RTL8152 || USB_RTL8152=n)
        default y
accomplished exactly what was wanted.

USB_NET_CDCETHER is a dependency, hence:

USB_NET_CDCETHER=n forces it off - as it should - it's an addon to cdcether.

USB_NET_CDCETHER=m disallows 'y' - module implies addon must be module.

similarly USB_RTL8152 is a dependency, so it being a module disallows 'y'.
This is desired, because if CDCETHER is builtin, so this addon could
be builtin, then RTL8152 would fail to bind it by default.
ie. CDCETHER=y && RTL8152=m must force RTL8153_ECM != y  (this is the bugfix)

basically the funky 'USB_RTL8152 || USB_RTL8152=n' --> disallows 'y'
iff RTL8152=m

'default y' enables it by default as 'y' if possible, as 'm' if not,
and disables it if impossible.

So I believe this had the exact right default behaviour - and allowed
all the valid options.