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@xxxxxxxxx> wrote:
> > On Sat. 19 Aug. 2023, 01:19, Jakub Kicinski <kuba@xxxxxxxxxx> 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