| Message ID | 20211009131304.19729-2-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | report the controller capabilities through the netlink interface | expand |
On 09.10.2021 22:13:02, Vincent Mailhol wrote: > The statically enabled features of a CAN controller can be retrieved > using below formula: > > | u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported; > > As such, there is no need to store this information. This patch remove > the field ctrlmode_static of struct can_priv and provides, in > replacement, the inline function can_get_static_ctrlmode() which > returns the same value. > > A condition sine qua non for this to work is that the controller > static modes should never be set in can_priv::ctrlmode_supported. This > is already the case for existing drivers, however, we added a warning > message in can_set_static_ctrlmode() to check that. Please make the can_set_static_ctrlmode to return an error in case of a problem. Adjust the drivers using the function is this patch, too. Marc
Hi Marc, Welcome back on the mailing list, hope you had some nice holidays! And also thanks a lot for your support over the last few months on my other series to introduce the TDC netlink interface :) Le lun. 25 oct. 2021 à 03:30, Marc Kleine-Budde <mkl@pengutronix.de> a écrit : > > On 09.10.2021 22:13:02, Vincent Mailhol wrote: > > The statically enabled features of a CAN controller can be retrieved > > using below formula: > > > > | u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported; > > > > As such, there is no need to store this information. This patch remove > > the field ctrlmode_static of struct can_priv and provides, in > > replacement, the inline function can_get_static_ctrlmode() which > > returns the same value. > > > > A condition sine qua non for this to work is that the controller > > static modes should never be set in can_priv::ctrlmode_supported. This > > is already the case for existing drivers, however, we added a warning > > message in can_set_static_ctrlmode() to check that. > > Please make the can_set_static_ctrlmode to return an error in case of a > problem. Adjust the drivers using the function is this patch, too. I didn't do so initially because this is more a static configuration issue that should only occur during development. Nonetheless, what you suggest is really simple. I will just split the patch in two: one of the setter and one for the getter and address your comments. Yours sincerely, Vincent Mailhol
On 26.10.2021 02:22:36, Vincent MAILHOL wrote: > Welcome back on the mailing list, hope you had some nice > holidays! Thanks was really nice, good weather, 1000km of cycling and hanging around in Vienna. :D > And also thanks a lot for your support over the last > few months on my other series to introduce the TDC netlink > interface :) The pleasure is on my side, working with you! > Le lun. 25 oct. 2021 à 03:30, Marc Kleine-Budde <mkl@pengutronix.de> a écrit : > > > > On 09.10.2021 22:13:02, Vincent Mailhol wrote: > > > The statically enabled features of a CAN controller can be retrieved > > > using below formula: > > > > > > | u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported; > > > > > > As such, there is no need to store this information. This patch remove > > > the field ctrlmode_static of struct can_priv and provides, in > > > replacement, the inline function can_get_static_ctrlmode() which > > > returns the same value. > > > > > > A condition sine qua non for this to work is that the controller > > > static modes should never be set in can_priv::ctrlmode_supported. This > > > is already the case for existing drivers, however, we added a warning > > > message in can_set_static_ctrlmode() to check that. > > > > Please make the can_set_static_ctrlmode to return an error in case of a > > problem. Adjust the drivers using the function is this patch, too. > > I didn't do so initially because this is more a static > configuration issue that should only occur during > development. Nonetheless, what you suggest is really simple. > > I will just split the patch in two: one of the setter and one for > the getter and address your comments. Fine with me. Most important thing is, that the kernel compiles after each patch. regards, Marc
On Tue. 26 Oct 2021 at 04:06, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > On 26.10.2021 02:22:36, Vincent MAILHOL wrote: > > Welcome back on the mailing list, hope you had some nice > > holidays! > > Thanks was really nice, good weather, 1000km of cycling and hanging > around in Vienna. :D > > > And also thanks a lot for your support over the last > > few months on my other series to introduce the TDC netlink > > interface :) > > The pleasure is on my side, working with you! > > > Le lun. 25 oct. 2021 à 03:30, Marc Kleine-Budde <mkl@pengutronix.de> a écrit : > > > > > > On 09.10.2021 22:13:02, Vincent Mailhol wrote: > > > > The statically enabled features of a CAN controller can be retrieved > > > > using below formula: > > > > > > > > | u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported; > > > > > > > > As such, there is no need to store this information. This patch remove > > > > the field ctrlmode_static of struct can_priv and provides, in > > > > replacement, the inline function can_get_static_ctrlmode() which > > > > returns the same value. > > > > > > > > A condition sine qua non for this to work is that the controller > > > > static modes should never be set in can_priv::ctrlmode_supported. This > > > > is already the case for existing drivers, however, we added a warning > > > > message in can_set_static_ctrlmode() to check that. > > > > > > Please make the can_set_static_ctrlmode to return an error in case of a > > > problem. Adjust the drivers using the function is this patch, too. > > > > I didn't do so initially because this is more a static > > configuration issue that should only occur during > > development. Nonetheless, what you suggest is really simple. > > > > I will just split the patch in two: one of the setter and one for > > the getter and address your comments. > > Fine with me. Most important thing is, that the kernel compiles after > each patch. Yep, the v3 [1] compiles fine after each patch. The only limitation is that I could not do a runtime test for rcar_fd and m_can (second patch of the v3 [2]) because I do not own the hardware. [1] https://lore.kernel.org/linux-can/20211025172247.1774451-1-mailhol.vincent@wanadoo.fr/T/#t [2] https://lore.kernel.org/linux-can/20211025172247.1774451-3-mailhol.vincent@wanadoo.fr/ Yours sincerely, Vincent Mailhol
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c index e3d840b81357..59c79f92fccc 100644 --- a/drivers/net/can/dev/dev.c +++ b/drivers/net/can/dev/dev.c @@ -300,6 +300,7 @@ EXPORT_SYMBOL_GPL(free_candev); int can_change_mtu(struct net_device *dev, int new_mtu) { struct can_priv *priv = netdev_priv(dev); + u32 ctrlmode_static = can_get_static_ctrlmode(priv); /* Do not allow changing the MTU while running */ if (dev->flags & IFF_UP) @@ -309,7 +310,7 @@ int can_change_mtu(struct net_device *dev, int new_mtu) switch (new_mtu) { case CAN_MTU: /* 'CANFD-only' controllers can not switch to CAN_MTU */ - if (priv->ctrlmode_static & CAN_CTRLMODE_FD) + if (ctrlmode_static & CAN_CTRLMODE_FD) return -EINVAL; priv->ctrlmode &= ~CAN_CTRLMODE_FD; @@ -318,7 +319,7 @@ int can_change_mtu(struct net_device *dev, int new_mtu) case CANFD_MTU: /* check for potential CANFD ability */ if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) && - !(priv->ctrlmode_static & CAN_CTRLMODE_FD)) + !(ctrlmode_static & CAN_CTRLMODE_FD)) return -EINVAL; priv->ctrlmode |= CAN_CTRLMODE_FD; diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c index 80425636049d..6c9906e8040c 100644 --- a/drivers/net/can/dev/netlink.c +++ b/drivers/net/can/dev/netlink.c @@ -112,7 +112,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[], if (dev->flags & IFF_UP) return -EBUSY; cm = nla_data(data[IFLA_CAN_CTRLMODE]); - ctrlstatic = priv->ctrlmode_static; + ctrlstatic = can_get_static_ctrlmode(priv); maskedflags = cm->flags & cm->mask; /* check whether provided bits are allowed to be passed */ diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 2413253e54c7..2381ad9e0814 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -69,7 +69,6 @@ struct can_priv { /* CAN controller features - see include/uapi/linux/can/netlink.h */ u32 ctrlmode; /* current options setting */ u32 ctrlmode_supported; /* options that can be modified by netlink */ - u32 ctrlmode_static; /* static enabled options for driver/hardware */ int restart_ms; struct delayed_work restart_work; @@ -104,14 +103,23 @@ static inline void can_set_static_ctrlmode(struct net_device *dev, struct can_priv *priv = netdev_priv(dev); /* alloc_candev() succeeded => netdev_priv() is valid at this point */ + if (priv->ctrlmode_supported & static_mode) { + netdev_warn(dev, + "Controller features can not be supported and static at the same time\n"); + return; + } priv->ctrlmode = static_mode; - priv->ctrlmode_static = static_mode; /* override MTU which was set by default in can_setup()? */ if (static_mode & CAN_CTRLMODE_FD) dev->mtu = CANFD_MTU; } +static inline u32 can_get_static_ctrlmode(struct can_priv *priv) +{ + return priv->ctrlmode & ~priv->ctrlmode_supported; +} + void can_setup(struct net_device *dev); struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
The statically enabled features of a CAN controller can be retrieved using below formula: | u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported; As such, there is no need to store this information. This patch remove the field ctrlmode_static of struct can_priv and provides, in replacement, the inline function can_get_static_ctrlmode() which returns the same value. A condition sine qua non for this to work is that the controller static modes should never be set in can_priv::ctrlmode_supported. This is already the case for existing drivers, however, we added a warning message in can_set_static_ctrlmode() to check that. Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- drivers/net/can/dev/dev.c | 5 +++-- drivers/net/can/dev/netlink.c | 2 +- include/linux/can/dev.h | 12 ++++++++++-- 3 files changed, 14 insertions(+), 5 deletions(-)