diff mbox series

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

Message ID 20230817101014.3484715-2-martin@geanix.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series can: per-device hardware filter support | expand

Checks

Context Check Description
netdev/tree_selection success Series ignored based on subject

Commit Message

Martin Hundebøll Aug. 17, 2023, 10:10 a.m. UTC 
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(+)

Comments

Jakub Kicinski Aug. 17, 2023, 4:45 p.m. UTC | #1 
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?
Vincent Mailhol Aug. 19, 2023, 1:10 p.m. UTC | #2 
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
Vincent Mailhol Aug. 19, 2023, 1:29 p.m. UTC | #3 
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
Oliver Hartkopp Aug. 20, 2023, 7:20 p.m. UTC | #4 
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
>
Vincent Mailhol Aug. 21, 2023, 4:50 p.m. UTC | #5 
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
diff mbox series

Patch

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,