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

[Jeroen Hofstee <jhofstee@victronenergy.com>]

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