From: Maciej Żenczykowski <hidden> Date: 2021-09-17 17:59:30
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...
Cheers,
- Maciej
--- --- ---
commit 7da17624e7948d5d9660b910f8079d26d26ce453
Author: Geert Uytterhoeven [off-list ref]
Date: Wed Jan 13 15:43:09 2021 +0100
nt: usb: USB_RTL8153_ECM should not default to y
In general, device drivers should not be enabled by default.
Fixes: 657bc1d10bfc23ac ("r8153_ecm: avoid to be prior to r8152 driver")
Signed-off-by: Geert Uytterhoeven [off-list ref]
Reviewed-by: Greg Kroah-Hartman [off-list ref]
Link: https://lore.kernel.org/r/20210113144309.1384615-1-geert+renesas@glider.be
Signed-off-by: Jakub Kicinski [off-list ref]
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-09-17 18:49:28
On Fri, 17 Sep 2021 19:59:15 +0200 Maciej Żenczykowski wrote:
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/
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.
From: Maciej Żenczykowski <hidden> Date: 2021-09-17 19:07:17
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/
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.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-09-17 19:37:16
On Fri, 17 Sep 2021 21:05:55 +0200 Maciej Żenczykowski wrote:
quoted
Yeah.. more context here:
https://lore.kernel.org/all/7fd014f2-c9a5-e7ec-f1c6-b3e4bb0f6eb6@samsung.com/
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.
Right, it was _technically_ correct but run afoul of the "drivers
should default to 'n'" policy. If it should not be treated as a driver
but more of a feature of an existing driver which user has already
selected we should refine the name of the option to make that clear.
Hi Jakub,
On Fri, Sep 17, 2021 at 9:37 PM Jakub Kicinski [off-list ref] wrote:
On Fri, 17 Sep 2021 21:05:55 +0200 Maciej Żenczykowski wrote:
quoted
quoted
Yeah.. more context here:
https://lore.kernel.org/all/7fd014f2-c9a5-e7ec-f1c6-b3e4bb0f6eb6@samsung.com/
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.
What is the expected behavior if USB_NET_CDCETHER=y, and USB_RTL8152=n?
In that case, the old "default y" made USB_RTL8153_ECM builtin.
That was the issue I was fixing.
If USB_RTL8153_ECM is somewhat critical, perhaps you want
"default m if USB_NET_CDCETHER=m"?
quoted
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.
Right, it was _technically_ correct but run afoul of the "drivers
should default to 'n'" policy. If it should not be treated as a driver
but more of a feature of an existing driver which user has already
selected we should refine the name of the option to make that clear.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Jakub,
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/
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.
If it can be a module (tristate), it must be a separate (sub)driver, right?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-09-20 19:17:27
On Sat, 18 Sep 2021 12:53:31 +0200 Geert Uytterhoeven wrote:
quoted
Yeah.. more context here:
https://lore.kernel.org/all/7fd014f2-c9a5-e7ec-f1c6-b3e4bb0f6eb6@samsung.com/
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.
If it can be a module (tristate), it must be a separate (sub)driver, right?
From: Maciej Żenczykowski <hidden> Date: 2021-09-21 02:37:36
On Mon, Sep 20, 2021 at 9:15 PM Jakub Kicinski [off-list ref] wrote:
On Sat, 18 Sep 2021 12:53:31 +0200 Geert Uytterhoeven wrote:
quoted
quoted
Yeah.. more context here:
https://lore.kernel.org/all/7fd014f2-c9a5-e7ec-f1c6-b3e4bb0f6eb6@samsung.com/
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.
If it can be a module (tristate), it must be a separate (sub)driver, right?
Fair point.
The problem is CDCETHER (ECM) tries to be a generic driver that just
works for USB standards compliant generic hardware...
(similarly the EEM/NCM drivers)
There shouldn't be a 'subdriver'
Hi Maciej,
On Mon, Sep 20, 2021 at 10:20 PM Maciej Żenczykowski [off-list ref] wrote:
On Mon, Sep 20, 2021 at 9:15 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Sat, 18 Sep 2021 12:53:31 +0200 Geert Uytterhoeven wrote:
quoted
quoted
Yeah.. more context here:
https://lore.kernel.org/all/7fd014f2-c9a5-e7ec-f1c6-b3e4bb0f6eb6@samsung.com/
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.
If it can be a module (tristate), it must be a separate (sub)driver, right?
Fair point.
The problem is CDCETHER (ECM) tries to be a generic driver that just
works for USB standards compliant generic hardware...
(similarly the EEM/NCM drivers)
There shouldn't be a 'subdriver'
If it does not make any sense to disable USB_RTL8153_ECM if CDCETHER
is enabled, perhaps the option should just be removed?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-09-21 21:28:09
On Tue, 21 Sep 2021 09:30:34 +0200 Geert Uytterhoeven wrote:
quoted
The problem is CDCETHER (ECM) tries to be a generic driver that just
works for USB standards compliant generic hardware...
(similarly the EEM/NCM drivers)
There shouldn't be a 'subdriver'
If it does not make any sense to disable USB_RTL8153_ECM if CDCETHER
is enabled, perhaps the option should just be removed?
And when we say "removed" we can just hide it from what's prompted
to the user (whatever such internal options are called)? I believe
this way we don't bring back Marek's complaint.