[PATCH 0/6] can: gs_usb: new features: GS_CAN_FEATURE_GET_STATE, GS_CAN_FEATURE_BERR_REPORTING

[PATCH 0/6] can: gs_usb: new features: GS_CAN_FEATURE_GET_STATE, GS_CAN_FEATURE_BERR_REPORTING

[Marc Kleine-Budde]

Hello,

this series consists of a few clean up patches by me and Jeroen
Hofstee patches that adds 2 new features (GS_CAN_FEATURE_GET_STATE,
GS_CAN_FEATURE_BERR_REPORTING). These patches are based on the
venus-5.19.4-gsusb branch of his Linux tree.

| https://github.com/jhofstee/linux/tree/venus-5.19.4-gsusb

Changes since venus-5.19.4-gsusb:
- fixed 2nd parameter of usb_control_msg_recv()
- rebased to current net-next/main
- renumbered the FEATURE, MODE and BREQ bits, as termination support
  landed mainline first
- split GS_CAN_FEATURE_GET_STATE patch into documentation and
  implementation part
- added Not-Signed-off-by: tag, as these patches are not Signed-off yet

regards,
Marc

Link: https://github.com/candle-usb/candleLight_fw/pull/107
Link: https://github.com/jhofstee/linux/tree/venus-5.19.4-gsusb

[PATCH 1/6] can: gs_usb: gs_can_open(): allow loopback and listen only at the same time

[Marc Kleine-Budde]

There's no reason why loopback and listen only should not be allowed
at the same time. Replace the "else if" by "if" to reflect this in the
code.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index f0065d40eb24..53293086faf0 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -911,7 +911,8 @@ static int gs_can_open(struct net_device *netdev)
 	/* flags */
 	if (ctrlmode & CAN_CTRLMODE_LOOPBACK)
 		flags |= GS_CAN_MODE_LOOP_BACK;
-	else if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
+
+	if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
 		flags |= GS_CAN_MODE_LISTEN_ONLY;
 
 	/* Controller is not allowed to retry TX
-- 
2.35.1

[PATCH 2/6] can: gs_usb: gs_can_open(): sort checks for ctrlmode

[Marc Kleine-Budde]

Sort the checks for dev->can.ctrlmode by values of CAN_CTRLMODE_*, so
that it's clear where to add new checks.

While there, remove the comment that the Atmel UC3C hardware doesn't
support One Shot Mode. One Shot Mode is only available to be activated
by the user, if the device specifies the GS_CAN_FEATURE_ONE_SHOT.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 53293086faf0..57daf46b2aec 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -843,8 +843,6 @@ static int gs_can_open(struct net_device *netdev)
 
 	ctrlmode = dev->can.ctrlmode;
 	if (ctrlmode & CAN_CTRLMODE_FD) {
-		flags |= GS_CAN_MODE_FD;
-
 		if (dev->feature & GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX)
 			dev->hf_size_tx = struct_size(hf, canfd_quirk, 1);
 		else
@@ -915,14 +913,14 @@ static int gs_can_open(struct net_device *netdev)
 	if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
 		flags |= GS_CAN_MODE_LISTEN_ONLY;
 
-	/* Controller is not allowed to retry TX
-	 * this mode is unavailable on atmels uc3c hardware
-	 */
+	if (ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+		flags |= GS_CAN_MODE_TRIPLE_SAMPLE;
+
 	if (ctrlmode & CAN_CTRLMODE_ONE_SHOT)
 		flags |= GS_CAN_MODE_ONE_SHOT;
 
-	if (ctrlmode & CAN_CTRLMODE_3_SAMPLES)
-		flags |= GS_CAN_MODE_TRIPLE_SAMPLE;
+	if (ctrlmode & CAN_CTRLMODE_FD)
+		flags |= GS_CAN_MODE_FD;
 
 	/* if hardware supports timestamps, enable it */
 	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
-- 
2.35.1

[PATCH 3/6] can: gs_usb: gs_can_open(): merge setting of timestamp flags and init

[Marc Kleine-Budde]

Merge the bodies of 2 consecutive "if (dev->feature &
GS_CAN_FEATURE_HW_TIMESTAMP)" statements.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 57daf46b2aec..7c1f13a90419 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -923,12 +923,12 @@ static int gs_can_open(struct net_device *netdev)
 		flags |= GS_CAN_MODE_FD;
 
 	/* if hardware supports timestamps, enable it */
-	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
 		flags |= GS_CAN_MODE_HW_TIMESTAMP;
 
-	/* start polling timestamp */
-	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		/* start polling timestamp */
 		gs_usb_timestamp_init(dev);
+	}
 
 	/* finally start device */
 	dev->can.state = CAN_STATE_ERROR_ACTIVE;
-- 
2.35.1

[PATCH 4/6] can: gs_usb: add ability to enable / disable berr rerporting

[Marc Kleine-Budde]

From: Jeroen Hofstee <jhofstee@victronenergy.com>

The open source firmware candleLight report bus errors
unconditionally. This adds support to enable / disable bus error
reporting with the standard netlink property.

Not-Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 7c1f13a90419..ea85140fe3df 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -134,6 +134,7 @@ struct gs_device_config {
 /* GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX BIT(9) */
 /* GS_CAN_FEATURE_BT_CONST_EXT BIT(10) */
 /* GS_CAN_FEATURE_TERMINATION BIT(11) */
+#define GS_CAN_MODE_BERR_REPORTING BIT(12)
 
 struct gs_device_mode {
 	__le32 mode;
@@ -174,7 +175,8 @@ struct gs_device_termination_state {
 #define GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX BIT(9)
 #define GS_CAN_FEATURE_BT_CONST_EXT BIT(10)
 #define GS_CAN_FEATURE_TERMINATION BIT(11)
-#define GS_CAN_FEATURE_MASK GENMASK(11, 0)
+#define GS_CAN_FEATURE_BERR_REPORTING BIT(12)
+#define GS_CAN_FEATURE_MASK GENMASK(12, 0)
 
 /* internal quirks - keep in GS_CAN_FEATURE space for now */
 
@@ -919,6 +921,9 @@ static int gs_can_open(struct net_device *netdev)
 	if (ctrlmode & CAN_CTRLMODE_ONE_SHOT)
 		flags |= GS_CAN_MODE_ONE_SHOT;
 
+	if (ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
+		flags |= GS_CAN_MODE_BERR_REPORTING;
+
 	if (ctrlmode & CAN_CTRLMODE_FD)
 		flags |= GS_CAN_MODE_FD;
 
@@ -1223,6 +1228,9 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		}
 	}
 
+	if (feature & GS_CAN_FEATURE_BERR_REPORTING)
+		dev->can.ctrlmode_supported |= CAN_CTRLMODE_BERR_REPORTING;
+
 	/* The CANtact Pro from LinkLayer Labs is based on the
 	 * LPC54616 µC, which is affected by the NXP LPC USB transfer
 	 * erratum. However, the current firmware (version 2) doesn't
-- 
2.35.1

[PATCH 5/6] can: gs_usb: document GS_CAN_FEATURE_GET_STATE

[Marc Kleine-Budde]

From: Jeroen Hofstee <jhofstee@victronenergy.com>

Document the new feature ("GS_CAN_FEATURE_GET_STATE") that indicates
that the state of the CAN controller can be queried with the new
GS_USB_BREQ_GET_STATE control message.

Not-Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index ea85140fe3df..fc5e8d08eb4d 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -66,6 +66,7 @@ enum gs_usb_breq {
 	GS_USB_BREQ_BT_CONST_EXT,
 	GS_USB_BREQ_SET_TERMINATION,
 	GS_USB_BREQ_GET_TERMINATION,
+	GS_USB_BREQ_GET_STATE,
 };
 
 enum gs_can_mode {
@@ -135,6 +136,7 @@ struct gs_device_config {
 /* GS_CAN_FEATURE_BT_CONST_EXT BIT(10) */
 /* GS_CAN_FEATURE_TERMINATION BIT(11) */
 #define GS_CAN_MODE_BERR_REPORTING BIT(12)
+/* GS_CAN_FEATURE_GET_STATE BIT(13) */
 
 struct gs_device_mode {
 	__le32 mode;
@@ -176,7 +178,8 @@ struct gs_device_termination_state {
 #define GS_CAN_FEATURE_BT_CONST_EXT BIT(10)
 #define GS_CAN_FEATURE_TERMINATION BIT(11)
 #define GS_CAN_FEATURE_BERR_REPORTING BIT(12)
-#define GS_CAN_FEATURE_MASK GENMASK(12, 0)
+#define GS_CAN_FEATURE_GET_STATE BIT(13)
+#define GS_CAN_FEATURE_MASK GENMASK(13, 0)
 
 /* internal quirks - keep in GS_CAN_FEATURE space for now */
 
-- 
2.35.1

[PATCH 6/6] can: gs_usb: support reading error counters

[Marc Kleine-Budde]

From: Jeroen Hofstee <jhofstee@victronenergy.com>

the format of this message is a bit weird, single bytes would be
sufficient. At the moment it uses the existing struct gs_device_state.

Not-Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 40 ++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index fc5e8d08eb4d..dfabd9cec19b 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -961,6 +961,43 @@ static int gs_can_open(struct net_device *netdev)
 	return 0;
 }
 
+static int gs_usb_get_state(const struct net_device *netdev,
+			    struct can_berr_counter *bec,
+			    enum can_state *state)
+{
+	struct gs_can *dev = netdev_priv(netdev);
+	struct gs_device_state ds;
+	int rc;
+
+	rc = usb_control_msg_recv(interface_to_usbdev(dev->iface), 0,
+				  GS_USB_BREQ_GET_STATE,
+				  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+				  dev->channel, 0,
+				  &ds, sizeof(ds),
+				  USB_CTRL_GET_TIMEOUT,
+				  GFP_KERNEL);
+
+	if (rc)
+		return rc;
+
+	if (ds.state >= CAN_STATE_MAX)
+		return -EOPNOTSUPP;
+
+	*state = ds.state;
+	bec->txerr = ds.txerr;
+	bec->rxerr = ds.rxerr;
+
+	return 0;
+}
+
+static int gs_usb_can_get_berr_counter(const struct net_device *netdev,
+				       struct can_berr_counter *bec)
+{
+	enum can_state state;
+
+	return gs_usb_get_state(netdev, bec, &state);
+}
+
 static int gs_can_close(struct net_device *netdev)
 {
 	int rc;
@@ -1234,6 +1271,9 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	if (feature & GS_CAN_FEATURE_BERR_REPORTING)
 		dev->can.ctrlmode_supported |= CAN_CTRLMODE_BERR_REPORTING;
 
+	if (feature & GS_CAN_FEATURE_GET_STATE)
+		dev->can.do_get_berr_counter = gs_can_get_berr_counter;
+
 	/* The CANtact Pro from LinkLayer Labs is based on the
 	 * LPC54616 µC, which is affected by the NXP LPC USB transfer
 	 * erratum. However, the current firmware (version 2) doesn't
-- 
2.35.1

Re: [PATCH 0/6] can: gs_usb: new features: GS_CAN_FEATURE_GET_STATE, GS_CAN_FEATURE_BERR_REPORTING

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1216 bytes --]

On 06.10.2022 18:24:46, Marc Kleine-Budde wrote:
> Hello,
> 
> this series consists of a few clean up patches by me and Jeroen
> Hofstee patches that adds 2 new features (GS_CAN_FEATURE_GET_STATE,
> GS_CAN_FEATURE_BERR_REPORTING). These patches are based on the
> venus-5.19.4-gsusb branch of his Linux tree.
> 
> | https://github.com/jhofstee/linux/tree/venus-5.19.4-gsusb
> 
> Changes since venus-5.19.4-gsusb:
> - fixed 2nd parameter of usb_control_msg_recv()

See:
https://github.com/jhofstee/linux/commit/6757d45f74b7e408cf3054cd4acf66883051c03d

> - rebased to current net-next/main
> - renumbered the FEATURE, MODE and BREQ bits, as termination support
>   landed mainline first
> - split GS_CAN_FEATURE_GET_STATE patch into documentation and
>   implementation part
> - added Not-Signed-off-by: tag, as these patches are not Signed-off yet

Jeroen, can I add your Signed-off-by for the patches?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/6] can: gs_usb: add ability to enable / disable berr rerporting

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

On 06.10.2022 18:24:50, Marc Kleine-Budde wrote:
> From: Jeroen Hofstee <jhofstee@victronenergy.com>
> 
> The open source firmware candleLight report bus errors
> unconditionally. This adds support to enable / disable bus error
> reporting with the standard netlink property.

I haven't checked the candleLight firmware, yet.

If the unmodified firmware sends bus errors per default and we introduce
BERR_REPORTING as suggested in this patch, we have to modify the default
behavior for bus errors: By default the firmware will not send bus
errors, but only if GS_CAN_MODE_BERR_REPORTING is requested during
open().

I'm not sure if we want to change the default behavior of the
firmware....To work around this backwards compatibility issue we can
explicitly turn BERR reporting on or off during open via
GS_CAN_MODE_BERR_REPORTING_ON or GS_CAN_MODE_BERR_REPORTING_OFF.

What do you think?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 6/6] can: gs_usb: support reading error counters

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

On 06.10.2022 18:24:52, Marc Kleine-Budde wrote:
> From: Jeroen Hofstee <jhofstee@victronenergy.com>
> 
> the format of this message is a bit weird, single bytes would be
> sufficient. At the moment it uses the existing struct gs_device_state.

From my point of view this looks good. The struct gs_device_state wasn't
used at all, yet! This is a good use of it.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/6] can: gs_usb: add ability to enable / disable berr rerporting

[Jeroen Hofstee]

Hello Marc,

Thanks for looking into this.

On 10/6/22 18:36, Marc Kleine-Budde wrote:
> On 06.10.2022 18:24:50, Marc Kleine-Budde wrote:
>> From: Jeroen Hofstee <jhofstee@victronenergy.com>
>>
>> The open source firmware candleLight report bus errors
>> unconditionally. This adds support to enable / disable bus error
>> reporting with the standard netlink property.
> I haven't checked the candleLight firmware, yet.
>
> If the unmodified firmware sends bus errors per default and we introduce
> BERR_REPORTING as suggested in this patch, we have to modify the default
> behavior for bus errors: By default the firmware will not send bus
> errors, but only if GS_CAN_MODE_BERR_REPORTING is requested during
> open().
>
> I'm not sure if we want to change the default behavior of the
> firmware....To work around this backwards compatibility issue we can
> explicitly turn BERR reporting on or off during open via
> GS_CAN_MODE_BERR_REPORTING_ON or GS_CAN_MODE_BERR_REPORTING_OFF.
>
> What do you think?
>
Personally, I wouldn't care about backward compatibility, it was added
later on despite they know it should be a socketcan option, see [1] and
for most if not all users, having error frames on a bit level / crc mismatch
level is only annoying, unless you are really looking deep into problems
and then enabling a simple flag isn't your biggest problem, especially not
if it is a documented/common flag which is supposed to do exactly that.

Obviously state changes are still send after my changes to candleLight
are accepted without bit error reporting being set.

So yes afaiac berr-reporting is simply turned off by default. Unless there
really are users which need them and can't simply set a single bit; my
guess there isn't any of them.

With kind regards,

Jeroen

[1] https://github.com/candle-usb/candleLight_fw/issues/59

Re: [PATCH 0/6] can: gs_usb: new features: GS_CAN_FEATURE_GET_STATE, GS_CAN_FEATURE_BERR_REPORTING

[Jeroen Hofstee]

Hello Marc,

On 10/6/22 18:24, Marc Kleine-Budde wrote:
> Hello,
>
> this series consists of a few clean up patches by me and Jeroen
> Hofstee patches that adds 2 new features (GS_CAN_FEATURE_GET_STATE,
> GS_CAN_FEATURE_BERR_REPORTING). These patches are based on the
> venus-5.19.4-gsusb branch of his Linux tree.
>
> | https://github.com/jhofstee/linux/tree/venus-5.19.4-gsusb
>
> Changes since venus-5.19.4-gsusb:
> - fixed 2nd parameter of usb_control_msg_recv()
> - rebased to current net-next/main
> - renumbered the FEATURE, MODE and BREQ bits, as termination support
>    landed mainline first
> - split GS_CAN_FEATURE_GET_STATE patch into documentation and
>    implementation part
> - added Not-Signed-off-by: tag, as these patches are not Signed-off yet
>
Thanks, I delayed sending them myself, since there were several
patches here floating around for the gs-usb driver, so it would
lead to more conflicts / claims for the same feature bits.

And yes I wrote this, intended for linux, strictly speaking
Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
or
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>

Regards,
Jeroen

Re: [PATCH 0/6] can: gs_usb: new features: GS_CAN_FEATURE_GET_STATE, GS_CAN_FEATURE_BERR_REPORTING

[Jeroen Hofstee]

Sorry for the spam, I already replied to the cover-letter,

On 10/6/22 18:28, Marc Kleine-Budde wrote:
> On 06.10.2022 18:24:46, Marc Kleine-Budde wrote:
>> Hello,
>>
>> this series consists of a few clean up patches by me and Jeroen
>> Hofstee patches that adds 2 new features (GS_CAN_FEATURE_GET_STATE,
>> GS_CAN_FEATURE_BERR_REPORTING). These patches are based on the
>> venus-5.19.4-gsusb branch of his Linux tree.
>>
>> | https://github.com/jhofstee/linux/tree/venus-5.19.4-gsusb
>>
>> Changes since venus-5.19.4-gsusb:
>> - fixed 2nd parameter of usb_control_msg_recv()
> See:
> https://github.com/jhofstee/linux/commit/6757d45f74b7e408cf3054cd4acf66883051c03d
>
>> - rebased to current net-next/main
>> - renumbered the FEATURE, MODE and BREQ bits, as termination support
>>    landed mainline first
>> - split GS_CAN_FEATURE_GET_STATE patch into documentation and
>>    implementation part
>> - added Not-Signed-off-by: tag, as these patches are not Signed-off yet
> Jeroen, can I add your Signed-off-by for the patches?
>
> regards,
> Marc
>
Yes I wrote this, intended for linux, strictly speaking
Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
or
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>

Regards,

Jeroen

Re: [PATCH 4/6] can: gs_usb: add ability to enable / disable berr rerporting

[Jeroen Hofstee]

Hi,

On 10/7/22 20:30, Jeroen Hofstee wrote:
> Hello Marc,
>
> Thanks for looking into this.
>
> On 10/6/22 18:36, Marc Kleine-Budde wrote:
>> On 06.10.2022 18:24:50, Marc Kleine-Budde wrote:
>>> From: Jeroen Hofstee <jhofstee@victronenergy.com>
>>>
>>> The open source firmware candleLight report bus errors
>>> unconditionally. This adds support to enable / disable bus error
>>> reporting with the standard netlink property.
>> I haven't checked the candleLight firmware, yet.
>>
>> If the unmodified firmware sends bus errors per default and we introduce
>> BERR_REPORTING as suggested in this patch, we have to modify the default
>> behavior for bus errors: By default the firmware will not send bus
>> errors, but only if GS_CAN_MODE_BERR_REPORTING is requested during
>> open().
>>
>> I'm not sure if we want to change the default behavior of the
>> firmware....To work around this backwards compatibility issue we can
>> explicitly turn BERR reporting on or off during open via
>> GS_CAN_MODE_BERR_REPORTING_ON or GS_CAN_MODE_BERR_REPORTING_OFF.
>>
>> What do you think?
>>
> Personally, I wouldn't care about backward compatibility, it was added
> later on despite they know it should be a socketcan option, see [1] and
> for most if not all users, having error frames on a bit level / crc 
> mismatch
> level is only annoying, unless you are really looking deep into problems
> and then enabling a simple flag isn't your biggest problem, especially 
> not
> if it is a documented/common flag which is supposed to do exactly that.
>
> Obviously state changes are still send after my changes to candleLight
> are accepted without bit error reporting being set.
>
> So yes afaiac berr-reporting is simply turned off by default. Unless 
> there
> really are users which need them and can't simply set a single bit; my
> guess there isn't any of them.
>
>
mmm, that is with the patch included. For distro's that will take quite some
time. I will have a look if a backwards compatible version can be created
which doesn't look too odd.

Regards,

Jeroen

Re: [PATCH 4/6] can: gs_usb: add ability to enable / disable berr rerporting

[Jeroen Hofstee]

Hi,

On 10/6/22 18:36, Marc Kleine-Budde wrote:
> On 06.10.2022 18:24:50, Marc Kleine-Budde wrote:
>> From: Jeroen Hofstee <jhofstee@victronenergy.com>
>>
>> The open source firmware candleLight report bus errors
>> unconditionally. This adds support to enable / disable bus error
>> reporting with the standard netlink property.
> I haven't checked the candleLight firmware, yet.
>
> If the unmodified firmware sends bus errors per default and we introduce
> BERR_REPORTING as suggested in this patch, we have to modify the default
> behavior for bus errors: By default the firmware will not send bus
> errors, but only if GS_CAN_MODE_BERR_REPORTING is requested during
> open().
>
> I'm not sure if we want to change the default behavior of the
> firmware....To work around this backwards compatibility issue we can
> explicitly turn BERR reporting on or off during open via
> GS_CAN_MODE_BERR_REPORTING_ON or GS_CAN_MODE_BERR_REPORTING_OFF.

It would look like this, untested:

subject: can: gs_usb: add ability to enable / disable berr rerporting

The open source firmware candleLight report bus errors
unconditionally. This adds support to enable / disable bus error
reporting with the standard netlink property.

Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
  drivers/net/can/usb/gs_usb.c | 18 +++++++++++++++++-
  1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 7c1f13a90419..039e8d91ad88 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -135,6 +135,13 @@ struct gs_device_config {
  /* GS_CAN_FEATURE_BT_CONST_EXT BIT(10) */
  /* GS_CAN_FEATURE_TERMINATION BIT(11) */

+/* Previous versions of this driver didn't support the berr-reporting flag,
+ * this flag tells if berr-reporting is supported. When not set, the device
+ * can default to its own preference to be backwards compatible.
+ */
+#define GS_CAN_MODE_BERR_REPORTING_SET BIT(12)
+#define GS_CAN_MODE_BERR_REPORTING BIT(13)
+
  struct gs_device_mode {
      __le32 mode;
      __le32 flags;
@@ -174,7 +181,9 @@ struct gs_device_termination_state {
  #define GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX BIT(9)
  #define GS_CAN_FEATURE_BT_CONST_EXT BIT(10)
  #define GS_CAN_FEATURE_TERMINATION BIT(11)
-#define GS_CAN_FEATURE_MASK GENMASK(11, 0)
+/* #define GS_CAN_FEATURE_BERR_REPORTING BIT_SET(12) */
+#define GS_CAN_FEATURE_BERR_REPORTING BIT(13)
+#define GS_CAN_FEATURE_MASK GENMASK(13, 0)

  /* internal quirks - keep in GS_CAN_FEATURE space for now */

@@ -919,6 +928,10 @@ static int gs_can_open(struct net_device *netdev)
      if (ctrlmode & CAN_CTRLMODE_ONE_SHOT)
          flags |= GS_CAN_MODE_ONE_SHOT;

+    flags |= GS_CAN_MODE_BERR_REPORTING_SET;
+    if (ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
+        flags |= GS_CAN_MODE_BERR_REPORTING;
+
      if (ctrlmode & CAN_CTRLMODE_FD)
          flags |= GS_CAN_MODE_FD;

@@ -1223,6 +1236,9 @@ static struct gs_can *gs_make_candev(unsigned int 
channel,
          }
      }

+    if (feature & GS_CAN_FEATURE_BERR_REPORTING)
+        dev->can.ctrlmode_supported |= CAN_CTRLMODE_BERR_REPORTING;
+
      /* The CANtact Pro from LinkLayer Labs is based on the
       * LPC54616 µC, which is affected by the NXP LPC USB transfer
       * erratum. However, the current firmware (version 2) doesn't
-- 
2.25.1


That claims another bit, but at least prevents the !NOT_BIT_ERRORS road.

Well, as far as I am concerned it is up to you guys. As long as there is 
a way
to suppress them I would be happy. With stats + counters you generally know
more then enough, no reason to flood the bus with useless error messages.

Regards,
Jeroen

Re: [PATCH 0/6] can: gs_usb: new features: GS_CAN_FEATURE_GET_STATE, GS_CAN_FEATURE_BERR_REPORTING

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

On 07.10.2022 20:59:43, Jeroen Hofstee wrote:
> > Jeroen, can I add your Signed-off-by for the patches?

> Yes I wrote this, intended for linux, strictly speaking
> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
> or
> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>

Thanks, I've used the victronenergy.com address, it matches the author.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/6] can: gs_usb: add ability to enable / disable berr rerporting

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2300 bytes --]

On 07.10.2022 20:30:27, Jeroen Hofstee wrote:
> > > The open source firmware candleLight report bus errors
> > > unconditionally. This adds support to enable / disable bus error
> > > reporting with the standard netlink property.
> > I haven't checked the candleLight firmware, yet.
> > 
> > If the unmodified firmware sends bus errors per default and we introduce
> > BERR_REPORTING as suggested in this patch, we have to modify the default
> > behavior for bus errors: By default the firmware will not send bus
> > errors, but only if GS_CAN_MODE_BERR_REPORTING is requested during
> > open().
> > 
> > I'm not sure if we want to change the default behavior of the
> > firmware....To work around this backwards compatibility issue we can
> > explicitly turn BERR reporting on or off during open via
> > GS_CAN_MODE_BERR_REPORTING_ON or GS_CAN_MODE_BERR_REPORTING_OFF.
> > 
> > What do you think?
> > 
> Personally, I wouldn't care about backward compatibility, it was added
> later on despite they know it should be a socketcan option, see [1] and
> for most if not all users, having error frames on a bit level / crc mismatch
> level is only annoying, unless you are really looking deep into problems
> and then enabling a simple flag isn't your biggest problem, especially not
> if it is a documented/common flag which is supposed to do exactly that.
> 
> Obviously state changes are still send after my changes to candleLight
> are accepted without bit error reporting being set.
> 
> So yes afaiac berr-reporting is simply turned off by default. Unless there
> really are users which need them and can't simply set a single bit; my
> guess there isn't any of them.

Makes sense. If a user is interested in debugging such low level
problems a firmware update should be no problem and is suggested
anyways.

Let's keep your original logic, where bus error reporting
is disabled by default.

I'll try to find time to look into your firmware changes later this
week.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/6] can: gs_usb: add ability to enable / disable berr rerporting

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2674 bytes --]

On 09.10.2022 00:11:49, Jeroen Hofstee wrote:
> Hi,
> 
> On 10/6/22 18:36, Marc Kleine-Budde wrote:
> > On 06.10.2022 18:24:50, Marc Kleine-Budde wrote:
> > > From: Jeroen Hofstee <jhofstee@victronenergy.com>
> > > 
> > > The open source firmware candleLight report bus errors
> > > unconditionally. This adds support to enable / disable bus error
> > > reporting with the standard netlink property.
> > I haven't checked the candleLight firmware, yet.
> > 
> > If the unmodified firmware sends bus errors per default and we introduce
> > BERR_REPORTING as suggested in this patch, we have to modify the default
> > behavior for bus errors: By default the firmware will not send bus
> > errors, but only if GS_CAN_MODE_BERR_REPORTING is requested during
> > open().
> > 
> > I'm not sure if we want to change the default behavior of the
> > firmware....To work around this backwards compatibility issue we can
> > explicitly turn BERR reporting on or off during open via
> > GS_CAN_MODE_BERR_REPORTING_ON or GS_CAN_MODE_BERR_REPORTING_OFF.
> 
> It would look like this, untested:
> 
> subject: can: gs_usb: add ability to enable / disable berr rerporting
> 
> The open source firmware candleLight report bus errors
> unconditionally. This adds support to enable / disable bus error
> reporting with the standard netlink property.
> 
> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/usb/gs_usb.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 7c1f13a90419..039e8d91ad88 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -135,6 +135,13 @@ struct gs_device_config {
>  /* GS_CAN_FEATURE_BT_CONST_EXT BIT(10) */
>  /* GS_CAN_FEATURE_TERMINATION BIT(11) */
> 
> +/* Previous versions of this driver didn't support the berr-reporting flag,
> + * this flag tells if berr-reporting is supported. When not set, the device
> + * can default to its own preference to be backwards compatible.
> + */
> +#define GS_CAN_MODE_BERR_REPORTING_SET BIT(12)
> +#define GS_CAN_MODE_BERR_REPORTING BIT(13)

You've convinced me :) This is overengineered, and it "breaks" the nice
1-to-1 mapping of feature and mode bits :)

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]