[PATCH 0/2] can: per-device hardware filter support

[PATCH 0/2] can: per-device hardware filter support

[Martin Hundebøll]

Hi all,

Based on the prior discussions on hardware filtering in CAN devices[0],
I've implemented such support in the m_can driver.

The first patch is almost entirely identical to Oliver Hartkopp's patch
from 2018[1] - I've just rebased it to v6.6 and fixed a checkpatch
warning. Not sure what to do about the "Not-Signed-off-by" tag though?

The second patch is new. I've tested it with a tcan4550 device together
with Oliver's proof-of-concept change in iproute2[2].

Has anyone tried this approach with other devices, e.g. sja1000 ?

Thanks,
Martin

[0] https://lore.kernel.org/linux-can/6B05F8DE-7FF3-4065-9828-530BB9C91D1B@vanille.de/T/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?h=can-hw-filter&id=87128f7a953ef2eef5f2d2a02ce354350e2c4f7f
[2] https://marc.info/?l=linux-can&m=151949929522529

Martin Hundebøll (2):
  can: netlink: support setting hardware filters
  can: m_can: support setting hw filters

 drivers/net/can/dev/dev.c        |   3 +
 drivers/net/can/dev/netlink.c    |  33 ++++++++
 drivers/net/can/m_can/m_can.c    | 137 ++++++++++++++++++++++++++++++-
 include/linux/can/dev.h          |   5 ++
 include/uapi/linux/can/netlink.h |   1 +
 5 files changed, 175 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH 1/2] can: netlink: support setting hardware filters

[Martin Hundebøll]

Add a netlink flag to pass per-device hardware filter configurations
similar to the per-socket software filters. Since different devices
supports different numbers and forms of filters (e.g. standard and
extended message IDs), a driver callback is added to validate the passed
filters.

Each filter consist of an ID value and a mask. The latter controls which
bit(s) in the message ID to filter against, and the former controls what
values the matched bits must have to accept the message.

For example, setting id=3f0 and mask=7f0 will accept messages with IDs
ranging from 0x3f0 to 0x3ff.

Each driver needs to implement first the validate_hw_filter() function,
and then configure the filters found in can->hw_filter when upping the
interface.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/dev.c        |  3 +++
 drivers/net/can/dev/netlink.c    | 33 ++++++++++++++++++++++++++++++++
 include/linux/can/dev.h          |  5 +++++
 include/uapi/linux/can/netlink.h |  1 +
 4 files changed, 42 insertions(+)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 7f9334a8af50..c62d2af49e74 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -280,6 +280,9 @@ EXPORT_SYMBOL_GPL(alloc_candev_mqs);
 /* Free space of the CAN network device */
 void free_candev(struct net_device *dev)
 {
+	struct can_priv *priv = netdev_priv(dev);
+
+	kfree(priv->hw_filter);
 	free_netdev(dev);
 }
 EXPORT_SYMBOL_GPL(free_candev);
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 036d85ef07f5..72cec9212bb8 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -22,6 +22,7 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
 	[IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
 	[IFLA_CAN_TDC] = { .type = NLA_NESTED },
 	[IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
+	[IFLA_CAN_HW_FILTER]    = { .type = NLA_UNSPEC },
 };
 
 static const struct nla_policy can_tdc_policy[IFLA_CAN_TDC_MAX + 1] = {
@@ -386,6 +387,38 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		priv->termination = termval;
 	}
 
+	if (data[IFLA_CAN_HW_FILTER]) {
+		int len = nla_len(data[IFLA_CAN_HW_FILTER]);
+		int num_filter = len / sizeof(struct can_filter);
+		struct can_filter *filter = nla_data(data[IFLA_CAN_HW_FILTER]);
+
+		if (!priv->validate_hw_filter)
+			return -EOPNOTSUPP;
+
+		/* Do not allow changing HW filters while running */
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+
+		if (len % sizeof(struct can_filter))
+			return -EINVAL;
+
+		/* let the CAN driver validate the given hw filters */
+		err = priv->validate_hw_filter(dev, filter, num_filter);
+		if (err)
+			return err;
+
+		kfree(priv->hw_filter);
+		priv->hw_filter = NULL;
+		priv->hw_filter_cnt = 0;
+
+		if (len) {
+			priv->hw_filter = kmemdup(filter, len, GFP_KERNEL);
+			if (!priv->hw_filter)
+				return -ENOMEM;
+			priv->hw_filter_cnt = num_filter;
+		}
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 982ba245eb41..a6b27c75c4ac 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -56,6 +56,8 @@ struct can_priv {
 	const u32 *data_bitrate_const;
 	unsigned int data_bitrate_const_cnt;
 	u32 bitrate_max;
+	struct can_filter *hw_filter;
+	unsigned int hw_filter_cnt;
 	struct can_clock clock;
 
 	unsigned int termination_const_cnt;
@@ -80,6 +82,9 @@ struct can_priv {
 	int (*do_set_data_bittiming)(struct net_device *dev);
 	int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
 	int (*do_set_termination)(struct net_device *dev, u16 term);
+	int (*validate_hw_filter)(struct net_device *dev,
+				  struct can_filter *hwf,
+				  unsigned int hwf_cnt);
 	int (*do_get_state)(const struct net_device *dev,
 			    enum can_state *state);
 	int (*do_get_berr_counter)(const struct net_device *dev,
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 02ec32d69474..2dfa09153bc4 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -138,6 +138,7 @@ enum {
 	IFLA_CAN_BITRATE_MAX,
 	IFLA_CAN_TDC,
 	IFLA_CAN_CTRLMODE_EXT,
+	IFLA_CAN_HW_FILTER,
 
 	/* add new constants above here */
 	__IFLA_CAN_MAX,
-- 
2.41.0

[PATCH 2/2] can: m_can: support setting hw filters

[Martin Hundebøll]

Implement the validate_hw_filter() callback to allow setting hardware
filter for m_can-based devices, and configure the filters when starting
the device.

The m_can chip requires separate configuration for standard and extended
ID filters, so the implementation considers filters with an ID or a mask
larger than 0x7ff to be an extended ID. Users needing to filter on the
lower 11 bits on extended message IDs can do so by passing an ID greater
than 0x7ff, while masking in the lower 11 bits only.

The number of allowed filters depends on the MRAM configuration. See
`sidf_elems` and `xidf_elems` elements in the bosch,mram-cfg device tree
binding for further information.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 drivers/net/can/m_can/m_can.c | 137 +++++++++++++++++++++++++++++++++-
 1 file changed, 133 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 16ecc11c7f62..7c8110076256 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -223,6 +223,52 @@ enum m_can_reg {
 #define ILE_EINT1	BIT(1)
 #define ILE_EINT0	BIT(0)
 
+/* Standard ID message filters (SIDF) */
+#define SIDF_SFT_MASK		GENMASK(31, 30)
+#define SIDF_SFEC_MASK		GENMASK(29, 27)
+#define SIDF_SFID1_MASK		GENMASK(26, 16)
+#define SIDF_SFID2_MASK		GENMASK(10, 0)
+
+/* Extended ID message filters (XIDF) */
+#define XIDF_EFT_MASK		GENMASK_ULL(63, 62)
+#define XIDF_EFID2_MASK		GENMASK_ULL(60, 32)
+#define XIDF_EFEC_MASK		GENMASK_ULL(31, 29)
+#define XIDF_EFID1_MASK		GENMASK_ULL(28, 0)
+
+/* Standard Filter Type */
+#define SFT_RANGE	0x0
+#define SFT_DUAL	0x1
+#define SFT_CLASSIC	0x2
+#define SFT_DISABLED	0x3
+
+/* Standard Filter Element Configuration */
+#define SFEC_DISABLE	0x0
+#define SFEC_RXF0	0x1
+#define SFEC_RXF1	0x2
+#define SFEC_REJECT	0x3
+#define SFEC_PRIO	0x4
+#define SFEC_PRIO_RXF0	0x5
+#define SFEC_PRIO_RXF1	0x6
+#define SFEC_DEBUG	0x7
+
+/* Global Filter Configuration Field */
+#define GFC_ANFS	GENMASK(5, 4)
+#define GFC_ANFE	GENMASK(3, 2)
+#define GFC_RRFS	BIT(1)
+#define GFC_RRFE	BIT(0)
+
+#define ANFS_ACCEPT_RXF0	0x0
+#define ANFS_ACCEPT_RXF1	0x1
+#define ANFS_REJECT		0x2
+
+/* Standard ID Filter Configuration */
+#define SIDFC_LSS	GENMASK(23, 16)
+#define SIDFC_FLSSA	GENMASK(15, 0)
+
+/* Extended ID Filter Configuration */
+#define XIDFC_LSE	GENMASK(22, 16)
+#define XIDFC_FLSAE	GENMASK(15, 0)
+
 /* Rx FIFO 0/1 Configuration (RXF0C/RXF1C) */
 #define RXFC_FWM_MASK	GENMASK(30, 24)
 #define RXFC_FS_MASK	GENMASK(22, 16)
@@ -382,6 +428,56 @@ static inline bool m_can_tx_fifo_full(struct m_can_classdev *cdev)
 	return _m_can_tx_fifo_full(m_can_read(cdev, M_CAN_TXFQS));
 }
 
+static int m_can_validate_hw_filter(struct net_device *dev,
+				    struct can_filter *hwf,
+				    unsigned int hwf_cnt)
+{
+	struct m_can_classdev *cdev = netdev_priv(dev);
+	size_t sff_filter_cnt = 0;
+	size_t eff_filter_cnt = 0;
+	int i;
+
+	for (i = 0; i < hwf_cnt; i++)
+		if (hwf[i].can_id <= CAN_SFF_MASK || hwf[i].can_mask <= CAN_SFF_MASK)
+			sff_filter_cnt++;
+		else
+			eff_filter_cnt++;
+
+	if (sff_filter_cnt > cdev->mcfg[MRAM_SIDF].num)
+		return -EINVAL;
+
+	if (eff_filter_cnt > cdev->mcfg[MRAM_XIDF].num)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int m_can_sid_filter_write(struct m_can_classdev *cdev,
+				  struct can_filter *filter,
+				  size_t index)
+{
+	u32 addr_offset = cdev->mcfg[MRAM_SIDF].off + SIDF_ELEMENT_SIZE * index;
+	u32 sidf = FIELD_PREP(SIDF_SFT_MASK, SFT_CLASSIC) |
+		   FIELD_PREP(SIDF_SFEC_MASK, SFEC_RXF0) |
+		   FIELD_PREP(SIDF_SFID1_MASK, filter->can_id) |
+		   FIELD_PREP(SIDF_SFID2_MASK, filter->can_mask);
+
+	return cdev->ops->write_fifo(cdev, addr_offset, &sidf, sizeof(sidf));
+}
+
+static int m_can_xid_filter_write(struct m_can_classdev *cdev,
+				  struct can_filter *filter,
+				  size_t index)
+{
+	u32 addr_offset = cdev->mcfg[MRAM_XIDF].off + XIDF_ELEMENT_SIZE * index;
+	u64 xidf = FIELD_PREP(XIDF_EFT_MASK, SFT_CLASSIC) |
+		   FIELD_PREP(XIDF_EFEC_MASK, SFEC_RXF0) |
+		   FIELD_PREP(XIDF_EFID1_MASK, filter->can_id) |
+		   FIELD_PREP(XIDF_EFID2_MASK, filter->can_mask);
+
+	return cdev->ops->write_fifo(cdev, addr_offset, &xidf, sizeof(xidf));
+}
+
 static void m_can_config_endisable(struct m_can_classdev *cdev, bool enable)
 {
 	u32 cccr = m_can_read(cdev, M_CAN_CCCR);
@@ -1266,8 +1362,10 @@ static int m_can_chip_config(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	u32 interrupts = IR_ALL_INT;
-	u32 cccr, test;
-	int err;
+	size_t sff_filter_cnt = 0;
+	size_t eff_filter_cnt = 0;
+	u32 cccr, test, anfs;
+	int err, i;
 
 	err = m_can_init_ram(cdev);
 	if (err) {
@@ -1288,8 +1386,38 @@ static int m_can_chip_config(struct net_device *dev)
 		    FIELD_PREP(RXESC_F1DS_MASK, RXESC_64B) |
 		    FIELD_PREP(RXESC_F0DS_MASK, RXESC_64B));
 
-	/* Accept Non-matching Frames Into FIFO 0 */
-	m_can_write(cdev, M_CAN_GFC, 0x0);
+	/* Configure HW filters and count standard vs extended id filters */
+	for (i = 0; i < cdev->can.hw_filter_cnt; i++) {
+		struct can_filter *filter = &cdev->can.hw_filter[i];
+
+		if (filter->can_id <= CAN_SFF_MASK ||
+		    filter->can_mask <= CAN_SFF_MASK)
+			err = m_can_sid_filter_write(cdev, filter,
+						     sff_filter_cnt++);
+		else
+			err = m_can_xid_filter_write(cdev, filter,
+						     eff_filter_cnt++);
+
+		if (err)
+			return err;
+	}
+
+	/* Configure offset to and number of standard id filters in MRAM */
+	m_can_write(cdev, M_CAN_SIDFC,
+		    FIELD_PREP(SIDFC_FLSSA, cdev->mcfg[MRAM_SIDF].off) |
+		    FIELD_PREP(SIDFC_LSS, sff_filter_cnt));
+	/* Configure offset to and number of extended id filters in MRAM */
+	m_can_write(cdev, M_CAN_XIDFC,
+		    FIELD_PREP(XIDFC_FLSAE, cdev->mcfg[MRAM_XIDF].off) |
+		    FIELD_PREP(XIDFC_LSE, sff_filter_cnt));
+
+	/* If any filter is configured, the Global Filter Configuration is set
+	 * to reject non-matching frames.
+	 */
+	anfs = cdev->can.hw_filter_cnt ? ANFS_REJECT : ANFS_ACCEPT_RXF0;
+	m_can_write(cdev, M_CAN_GFC,
+		    FIELD_PREP(GFC_ANFS, anfs) | FIELD_PREP(GFC_ANFE, anfs));
+
 
 	if (cdev->version == 30) {
 		/* only support one Tx Buffer currently */
@@ -1524,6 +1652,7 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
 	/* Shared properties of all M_CAN versions */
 	cdev->version = m_can_version;
 	cdev->can.do_set_mode = m_can_set_mode;
+	cdev->can.validate_hw_filter = m_can_validate_hw_filter;
 	cdev->can.do_get_berr_counter = m_can_get_berr_counter;
 
 	/* Set M_CAN supported operations */
-- 
2.41.0

Re: [PATCH 1/2] can: netlink: support setting hardware filters

[Jakub Kicinski]

On Thu, 17 Aug 2023 12:10:13 +0200 Martin Hundebøll wrote:
> +		int len = nla_len(data[IFLA_CAN_HW_FILTER]);
> +		int num_filter = len / sizeof(struct can_filter);
> +		struct can_filter *filter = nla_data(data[IFLA_CAN_HW_FILTER]);

This will prevent you from ever extending struct can_filter in 
a backward-compatible fashion, right? I obviously know very little
about CAN but are you confident a more bespoke API to manipulate
filters individually and allow extensibility is not warranted?

Re: [PATCH 1/2] can: netlink: support setting hardware filters

[Vincent Mailhol]

On Sat. 19 Aug. 2023, 01:19, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 17 Aug 2023 12:10:13 +0200 Martin Hundebøll wrote:
> > +             int len = nla_len(data[IFLA_CAN_HW_FILTER]);
> > +             int num_filter = len / sizeof(struct can_filter);
> > +             struct can_filter *filter = nla_data(data[IFLA_CAN_HW_FILTER]);
>
> This will prevent you from ever extending struct can_filter in
> a backward-compatible fashion, right? I obviously know very little
> about CAN but are you confident a more bespoke API to manipulate
> filters individually and allow extensibility is not warranted?

I follow Jakub's point of view.

The current struct can_filter is not sound. Some devices such as the
ES582.1 supports filtering of the CAN frame based on the flags (i.e.
SFF/EFF, RTR, FDF).

I think that each of the fields of the filter should have its own NLA
declaration with the whole thing wrapped within a NLA_NESTED_ARRAY.

I also think that there should then be a method to report the precise
filtering capabilities of the hardware.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 1/2] can: netlink: support setting hardware filters

[Vincent Mailhol]

On Sat. 19 Aug. 2023 at 22:10, Vincent Mailhol
<vincent.mailhol@gmail.com> wrote:
> On Sat. 19 Aug. 2023, 01:19, Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 17 Aug 2023 12:10:13 +0200 Martin Hundebøll wrote:
> > > +             int len = nla_len(data[IFLA_CAN_HW_FILTER]);
> > > +             int num_filter = len / sizeof(struct can_filter);
> > > +             struct can_filter *filter = nla_data(data[IFLA_CAN_HW_FILTER]);
> >
> > This will prevent you from ever extending struct can_filter in
> > a backward-compatible fashion, right? I obviously know very little
> > about CAN but are you confident a more bespoke API to manipulate
> > filters individually and allow extensibility is not warranted?
>
> I follow Jakub's point of view.
>
> The current struct can_filter is not sound. Some devices such as the
> ES582.1 supports filtering of the CAN frame based on the flags (i.e.
> SFF/EFF, RTR, FDF).

I wrote too fast. The EFF and RTR flags are contained in the canid_t,
so the current struct can_filter is able to handle these two flags.
But it remains true that the CAN-FD flags (FDF and BRS) are currently
not handled. Not to mention that more flags will come with the
upcoming CAN XL.

> I think that each of the fields of the filter should have its own NLA
> declaration with the whole thing wrapped within a NLA_NESTED_ARRAY.
>
> I also think that there should then be a method to report the precise
> filtering capabilities of the hardware.
>
>
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH 1/2] can: netlink: support setting hardware filters

[Oliver Hartkopp]

On 19.08.23 15:29, Vincent Mailhol wrote:
> On Sat. 19 Aug. 2023 at 22:10, Vincent Mailhol
> <vincent.mailhol@gmail.com> wrote:
>> On Sat. 19 Aug. 2023, 01:19, Jakub Kicinski <kuba@kernel.org> wrote:
>>>
>>> On Thu, 17 Aug 2023 12:10:13 +0200 Martin Hundebøll wrote:
>>>> +             int len = nla_len(data[IFLA_CAN_HW_FILTER]);
>>>> +             int num_filter = len / sizeof(struct can_filter);
>>>> +             struct can_filter *filter = nla_data(data[IFLA_CAN_HW_FILTER]);
>>>
>>> This will prevent you from ever extending struct can_filter in
>>> a backward-compatible fashion, right? I obviously know very little
>>> about CAN but are you confident a more bespoke API to manipulate
>>> filters individually and allow extensibility is not warranted?
>>
>> I follow Jakub's point of view.
>>
>> The current struct can_filter is not sound. Some devices such as the
>> ES582.1 supports filtering of the CAN frame based on the flags (i.e.
>> SFF/EFF, RTR, FDF).
> 
> I wrote too fast. The EFF and RTR flags are contained in the canid_t,
> so the current struct can_filter is able to handle these two flags.
> But it remains true that the CAN-FD flags (FDF and BRS) are currently
> not handled. Not to mention that more flags will come with the
> upcoming CAN XL.

You are right with FDF where we could use the former CAN_ERR_FLAG value 
which is not needed for hw filter API.

But regarding CAN XL we could use the Standard 11 bit ID handling with 
another flag inside the remaining 18 bits.

The general concept of re-using the struct can_filter makes sense to me 
as this follows the widely used pattern in the af_can.c core and CAN_RAW 
sockets.

Best regards,
Oliver

> 
>> I think that each of the fields of the filter should have its own NLA
>> declaration with the whole thing wrapped within a NLA_NESTED_ARRAY.
>>
>> I also think that there should then be a method to report the precise
>> filtering capabilities of the hardware.
>>
>>
>> Yours sincerely,
>> Vincent Mailhol
> 

Re: [PATCH 1/2] can: netlink: support setting hardware filters

[Vincent Mailhol]

On Mon. 21 Aug. 2023 at 04:21, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 19.08.23 15:29, Vincent Mailhol wrote:
> > On Sat. 19 Aug. 2023 at 22:10, Vincent Mailhol
> > <vincent.mailhol@gmail.com> wrote:
> >> On Sat. 19 Aug. 2023, 01:19, Jakub Kicinski <kuba@kernel.org> wrote:
> >>>
> >>> On Thu, 17 Aug 2023 12:10:13 +0200 Martin Hundebøll wrote:
> >>>> +             int len = nla_len(data[IFLA_CAN_HW_FILTER]);
> >>>> +             int num_filter = len / sizeof(struct can_filter);
> >>>> +             struct can_filter *filter = nla_data(data[IFLA_CAN_HW_FILTER]);
> >>>
> >>> This will prevent you from ever extending struct can_filter in
> >>> a backward-compatible fashion, right? I obviously know very little
> >>> about CAN but are you confident a more bespoke API to manipulate
> >>> filters individually and allow extensibility is not warranted?
> >>
> >> I follow Jakub's point of view.
> >>
> >> The current struct can_filter is not sound. Some devices such as the
> >> ES582.1 supports filtering of the CAN frame based on the flags (i.e.
> >> SFF/EFF, RTR, FDF).
> >
> > I wrote too fast. The EFF and RTR flags are contained in the canid_t,
> > so the current struct can_filter is able to handle these two flags.
> > But it remains true that the CAN-FD flags (FDF and BRS) are currently
> > not handled. Not to mention that more flags will come with the
> > upcoming CAN XL.
>
> You are right with FDF where we could use the former CAN_ERR_FLAG value
> which is not needed for hw filter API.

And what about the BRS flag?

> But regarding CAN XL we could use the Standard 11 bit ID handling with
> another flag inside the remaining 18 bits.

Then, wouldn't you still need one more flag to indicate that this is a
CAN XL filter?

> The general concept of re-using the struct can_filter makes sense to me
> as this follows the widely used pattern in the af_can.c core and CAN_RAW
> sockets.
>
> Best regards,
> Oliver
>
> >
> >> I think that each of the fields of the filter should have its own NLA
> >> declaration with the whole thing wrapped within a NLA_NESTED_ARRAY.
> >>
> >> I also think that there should then be a method to report the precise
> >> filtering capabilities of the hardware.
> >>
> >>
> >> Yours sincerely,
> >> Vincent Mailhol