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

[Marc Kleine-Budde <mkl@pengutronix.de>]

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