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

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

[Maciej Żenczykowski]

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 <geert+renesas@glider.be>
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 <geert+renesas@glider.be>
    Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Link: https://lore.kernel.org/r/20210113144309.1384615-1-geert+renesas@glider.be
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 1e3719028780..fbbe78643631 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -631,7 +631,6 @@ config USB_NET_AQC111
 config USB_RTL8153_ECM
        tristate "RTL8153 ECM support"
        depends on USB_NET_CDCETHER && (USB_RTL8152 || USB_RTL8152=n)
-       default y
        help
          This option supports ECM mode for RTL8153 ethernet adapter, when
          CONFIG_USB_RTL8152 is not set, or the RTL8153 device is not

Maciej Żenczykowski, Kernel Networking Developer @ Google

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

[Jakub Kicinski]

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.

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

[Maciej Żenczykowski]

On Fri, Sep 17, 2021 at 8:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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.

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.

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

[Jakub Kicinski]

On Fri, 17 Sep 2021 21:05:55 +0200 Maciej Żenczykowski wrote:
> > 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.

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

[Geert Uytterhoeven]

Hi Jakub,

On Fri, Sep 17, 2021 at 8:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
> 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.

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

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

[Geert Uytterhoeven]

Hi Jakub,

On Fri, Sep 17, 2021 at 9:37 PM Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri, 17 Sep 2021 21:05:55 +0200 Maciej Żenczykowski wrote:
> > > 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"?

> > 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

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

[Jakub Kicinski]

On Sat, 18 Sep 2021 12:53:31 +0200 Geert Uytterhoeven wrote:
> > 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.

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

[Maciej Żenczykowski]

On Mon, Sep 20, 2021 at 9:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 18 Sep 2021 12:53:31 +0200 Geert Uytterhoeven wrote:
> > > 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'

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

[Geert Uytterhoeven]

Hi Maciej,

On Mon, Sep 20, 2021 at 10:20 PM Maciej Żenczykowski <maze@google.com> wrote:
> On Mon, Sep 20, 2021 at 9:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Sat, 18 Sep 2021 12:53:31 +0200 Geert Uytterhoeven wrote:
> > > > 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

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

[Jakub Kicinski]

On Tue, 21 Sep 2021 09:30:34 +0200 Geert Uytterhoeven wrote:
> > 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.