netdev - Re: [PATCH net-next v2 11/12] bnxt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Feb 2017 13:30:02 -0800
From:   Michael Chan <michael.chan@...adcom.com>
To:     "Mintz, Yuval" <Yuval.Mintz@...ium.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 11/12] bnxt_en: Add basic XDP support.

On Thu, Feb 2, 2017 at 9:18 AM, Mintz, Yuval <Yuval.Mintz@...ium.com> wrote:
>> +config BNXT_XDP
>> +     bool "Xpress Data Path (XDP) driver support"
>> +     default n
>> +     depends on BNXT && BPF
>> +     ---help---
>> +       Say Y here if you want to enable XDP in the driver to support
>> +       eBPF programs in the fast path.
>> +
>
> Wasn't it recently discussed that per-feature option is preferable
> to a per-feature per-device option?
> Assuming BPF > XDP and thus shouldn't directly imply that XDP
> should be supported, perhaps the right thing is to add a global
> XDP config option?

Since the driver has other per-device options, I'm adding another
per-device option right now.  It can easily be converted to a more
tree-wide option if that's what we want to do.

>
>> +     if (prog && bp->dev->mtu > BNXT_MAX_PAGE_MODE_MTU) {
>> +             netdev_err(dev, "MTU %d larger than largest XDP supported
>> MTU %d.\n",
>> +                        bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
>> +             return -EOPNOTSUPP;
>> +     }
>
> Is it O.k. to print with netdev_err() for a user-provided unsupported
> configuration? Shouldn't that be limited?

You are suggesting to use lower level warning, such as netdev_warn(), right?

>
>> +             bool sh = (bp->flags & BNXT_FLAG_SHARED_RINGS) ? true :
>> false;
>
> Didn't you already check this flag is set?
>

You are right.  I checked already.