Re: [PATCH 08/17] can: add ISO 15765-2:2016 transport protocol

On 10.10.20 17:44, Jakub Kicinski wrote:

On Sat, 10 Oct 2020 16:29:06 +0200 Oliver Hartkopp wrote:

quoted

quoted

quoted

diff --git a/net/can/Kconfig b/net/can/Kconfig
index 25436a715db3..021fe03a8ed6 100644
--- a/net/can/Kconfig
+++ b/net/can/Kconfig

@@ -55,6 +55,19 @@ config CAN_GW
   
   source "net/can/j1939/Kconfig"
   
+config CAN_ISOTP
+	tristate "ISO 15765-2:2016 CAN transport protocol"
+	default y

default should not be y unless there is a very good reason.
I don't see such reason here. This is new functionality, users
can enable it if they need it.

Yes. I agree. But there is a good reason for it.
The ISO 15765-2 protocol is used for vehicle diagnosis and is a *very*
common CAN bus use case.

More common than j1939? (Google uses words like 'widely used' and
'common' :)) To give you some perspective we don't enable Ethernet
vlan support by default, vlans are pretty common, too.

quoted

The config item only shows up when CONFIG_CAN is selected and then ISO
15765-2 should be enabled too. I have implemented and maintained the
out-of-tree driver for ~12 years now and the people have real problems
using e.g. Ubuntu with signed kernel modules when they need this protocol.

Therefore the option should default to 'y' to make sure the common
distros (that enable CONFIG_CAN) enable ISO-TP too.

I understand the motivation, but Linus had pushed back on defaulting to
'y' many times over the years, please read this:

https://lkml.org/lkml/2012/1/6/354

This really must not pop up on his screen as default 'y' when he does
an oldconfig after pulling the networking tree..

Ok. Understood.

I'll remove the default=y then. Would it be ok to add some text like

If you want to perfrom automotive vehicle diagnostic services (UDS), say 'y'.

?

Best regards,
Oliver