From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
To: Marc Kleine-Budde <mkl@pengutronix.de>, linux-can@vger.kernel.org
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Subject: [PATCH v3 2/4] can: dev: add sanity check in can_set_static_ctrlmode()
Date: Tue, 26 Oct 2021 02:22:45 +0900 [thread overview]
Message-ID: <20211025172247.1774451-3-mailhol.vincent@wanadoo.fr> (raw)
In-Reply-To: <20211025172247.1774451-1-mailhol.vincent@wanadoo.fr>
Previous patch removed can_priv::ctrlmode_static to replace it with
can_get_static_ctrlmode().
A condition sine qua non for this to work is that the controller
static modes should never be set in can_priv::ctrlmode_supported
(c.f. the comment on can_priv::ctrlmode_supported which states that it
is for "options that can be *modified* by netlink"). Also, this
condition is already correctly fulfilled by all existing drivers
which rely on the ctrlmode_static feature.
Nonetheless, we added an extra safeguard in can_set_static_ctrlmode()
to return an error value and to warn the developer who would be
adventurous enough to set to static a given feature that is already
set to supported.
The drivers which rely on the static controller mode are then updated
to check the return value of can_set_static_ctrlmode().
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Some few comments on how the rcar_canfd and m_can drivers free their
allocated resources when an error occurs during probing.
The function rcar_canfd_channel_probe() is quite inconsistent with the
way it handles errors. After the call to alloc_candev, there are
several "goto fail" statements that would directly exit without
calling free_candev()!
Nonetheless, later on the driver will check the return value of
rcar_canfd_channel_probe() and call rcar_canfd_channel_remove() which
will correctly call free_candev(). Even if this is inconsistent, there
is no sign of a memory leak. So I just applied the change the
can_set_static_ctrlmode() without bothering more (N.B. I do not own
that device so I am not willing to take the risk of making bigger
changes because I can not test).
On the other hand, m_can_dev_setup() is fine: the return value is
checked by the caller and necessary actions are taken.
As such, for both driver, we did a minimal change.
---
drivers/net/can/m_can/m_can.c | 10 +++++++---
drivers/net/can/rcar/rcar_canfd.c | 4 +++-
include/linux/can/dev.h | 11 +++++++++--
3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 2470c47b2e31..ab4371aa4ff1 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1463,7 +1463,7 @@ static bool m_can_niso_supported(struct m_can_classdev *cdev)
static int m_can_dev_setup(struct m_can_classdev *cdev)
{
struct net_device *dev = cdev->net;
- int m_can_version;
+ int m_can_version, err;
m_can_version = m_can_check_core_release(cdev);
/* return if unsupported version */
@@ -1493,13 +1493,17 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
switch (cdev->version) {
case 30:
/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.0.x */
- can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
+ err = can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
+ if (err)
+ return err;
cdev->can.bittiming_const = &m_can_bittiming_const_30X;
cdev->can.data_bittiming_const = &m_can_data_bittiming_const_30X;
break;
case 31:
/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.1.x */
- can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
+ err = can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
+ if (err)
+ return err;
cdev->can.bittiming_const = &m_can_bittiming_const_31X;
cdev->can.data_bittiming_const = &m_can_data_bittiming_const_31X;
break;
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index ff9d0f5ae0dd..e225c1c03812 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1706,7 +1706,9 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
&rcar_canfd_data_bittiming_const;
/* Controller starts in CAN FD only mode */
- can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD);
+ err = can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD);
+ if (err)
+ goto fail;
priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
} else {
/* Controller starts in Classical CAN only mode */
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 92e2d69462f0..fff3f70df697 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -131,17 +131,24 @@ static inline s32 can_get_relative_tdco(const struct can_priv *priv)
}
/* helper to define static CAN controller features at device creation time */
-static inline void can_set_static_ctrlmode(struct net_device *dev,
- u32 static_mode)
+static inline int __must_check can_set_static_ctrlmode(struct net_device *dev,
+ u32 static_mode)
{
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 -EINVAL;
+ }
priv->ctrlmode = static_mode;
/* override MTU which was set by default in can_setup()? */
if (static_mode & CAN_CTRLMODE_FD)
dev->mtu = CANFD_MTU;
+
+ return 0;
}
static inline u32 can_get_static_ctrlmode(struct can_priv *priv)
--
2.32.0
next prev parent reply other threads:[~2021-10-25 17:23 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-25 17:22 [PATCH v3 0/4] report the controller capabilities through the netlink interface Vincent Mailhol
2021-10-25 17:22 ` [PATCH v3 1/4] can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode() Vincent Mailhol
2021-10-25 17:22 ` Vincent Mailhol [this message]
2021-10-25 17:22 ` [PATCH v3 3/4] can: dev: reorder struct can_priv members for better packing Vincent Mailhol
2021-10-25 17:22 ` [PATCH v3 4/4] can: netlink: report the CAN controller mode supported flags Vincent Mailhol
2021-10-26 3:29 ` Vincent MAILHOL
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211025172247.1774451-3-mailhol.vincent@wanadoo.fr \
--to=mailhol.vincent@wanadoo.fr \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).