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

On Mon. 21 Aug. 2023 at 04:21, Oliver Hartkopp [off-list ref] wrote:

On 19.08.23 15:29, Vincent Mailhol wrote:

quoted

On Sat. 19 Aug. 2023 at 22:10, Vincent Mailhol
[off-list ref] wrote:

quoted

On Sat. 19 Aug. 2023, 01:19, Jakub Kicinski [off-list ref] wrote:

quoted

On Thu, 17 Aug 2023 12:10:13 +0200 Martin Hundebøll wrote:

quoted

+             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

quoted

quoted

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