On 03.09.2021 16:17:04, Vincent Mailhol wrote: > struct can_priv has a set of flags (can_priv::ctrlmode) which are > correlated with the other fields of the structure. In > can_changelink(), those flags are set first and copied to can_priv. If > the function has to return early, for example due to an out of range > value provided by the user, then the global configuration might become > incoherent. > > Example: the user provides an out of range dbitrate (e.g. 20 > Mbps). The command fails (-EINVAL), however the FD flag was already > set resulting in a configuration where FD is on but the databittiming > parameters are empty. > > * Illustration of above example * > > | $ ip link set can0 type can bitrate 500000 dbitrate 20000000 fd on > | RTNETLINK answers: Invalid argument > | $ ip --details link show can0 > | 1: can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group default qlen 10 > | link/can promiscuity 0 minmtu 0 maxmtu 0 > | can <FD> state STOPPED restart-ms 0 > ^^ FD flag is set without any of the databittiming parameters... > | bitrate 500000 sample-point 0.875 > | tq 12 prop-seg 69 phase-seg1 70 phase-seg2 20 sjw 1 > | ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp-inc 1 > | ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 dbrp-inc 1 > | clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 > > To prevent this from happening, we do a local copy of can_priv, work > on it, an copy it at the very end of the function (i.e. only if all > previous checks succeeded). I don't like the optimization of using a static priv. If it's too big to be allocated on the stack, allocate it on the heap, i.e. using kmemdup()/kfree(). > Once this done, there is no more need to have a temporary variable for > a specific parameter. As such, the bittiming and data bittiming (bt > and dbt) are directly written to the temporary priv variable. > > Finally, function can_calc_tdco() was retrieving can_priv from the > net_device and directly modifying it. We changed the prototype so that > it instead writes its changes into our temporary priv variable. Is it possible to split this into a separate patch, so that the part without the tdco can be backported more easily to older kernels not having tdco? The patch fixing the tdco would be the 2nd patch... > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > --- > Resending because I got no answers on: > https://lore.kernel.org/linux-can/20210823024750.702542-1-mailhol.vincent@xxxxxxxxxx/T/#u > (I guess everyone bas busy with the upcoming merge window) Busy yes, but not with the merge window :) > I am not sure whether or not this needs a "Fixes" tag. Just in case, > there it is: > > Fixes: 9859ccd2c8be ("can: introduce the data bitrate configuration for CAN FD") ...if it's possible to split this patch into 2 parts, add individual fixes tags to them. 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:
signature.asc
Description: PGP signature
