netdev - Re: [PATCH net-next 04/11] eth: bnxt: move from .set_rxfh to .create_rxfh

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]

Message-ID: <CALs4sv33AdVBNomJ-tnZCmn8BeoPvVsSx9s0VUhocHmbp-AE=w@mail.gmail.com>
Date: Wed, 3 Jul 2024 18:19:18 +0530
From: Pavan Chebbi <pavan.chebbi@...adcom.com>
To: Edward Cree <ecree.xilinx@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net, netdev@...r.kernel.org, 
	edumazet@...gle.com, pabeni@...hat.com, michael.chan@...adcom.com
Subject: Re: [PATCH net-next 04/11] eth: bnxt: move from .set_rxfh to
 .create_rxfh_context and friends

On Wed, Jul 3, 2024 at 5:36 PM Edward Cree <ecree.xilinx@...il.com> wrote:
>
> On 03/07/2024 00:47, Jakub Kicinski wrote:
> > Use the new ethtool ops for RSS context management. The conversion
> > is pretty straightforward cut / paste of the right chunks of the
> > combined handler. Main change is that we let the core pick the IDs
> > (bitmap will be removed separately for ease of review), so we need
> > to tell the core when we lose a context.
> > Since the new API passes rxfh as const, change bnxt_modify_rss()
> > to also take const.
> >
> > Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ...
> > @@ -5271,6 +5296,7 @@ void bnxt_ethtool_free(struct bnxt *bp)
> >  const struct ethtool_ops bnxt_ethtool_ops = {
> >       .cap_link_lanes_supported       = 1,
> >       .cap_rss_ctx_supported          = 1,
> > +     .rxfh_max_context_id            = BNXT_MAX_ETH_RSS_CTX,
>
> According to Pavan [1], this limit only existed for the sake of the
>  SW side (presumably the rss_ctx_bmap), so probably it can be removed
>  in patch #5.
> The higher FW limit Pavan mentions appears to be on number rather
>  than index; at least I can't see anything in the driver feeding the
>  user-facing context ID to the device.  But I don't know whether FW

Hi Ed, you are right. It's a SW side number and the device has no
connection with the ID.

>  has any opportunity to say ENOMEM, or whether the driver needs to
>  validate against the hardware limit itself.  Hopefully Pavan (CCed)
>  can elaborate.

Because the driver is not aware of the hardware limit, and the limit
is dynamic, we can rely on FW to know if the resource request we made
was honored (there is no direct ENOMEM mechanism)
The driver already does this when we make a runtime check for
resources using bnxt_rfs_capable() when an RSS ctx is being created.
But for this version of the driver, I would prefer to keep a limit
because we have some FW improvements coming in, in the area of
resource management.
Though removing the limit may not break anything, I'd prefer to have
it removed once a FW with improvements (indicated by a query
flag/caps) is available.
Michael may also add his thoughts on this.

>
> -ed
>
> [1] https://lore.kernel.org/netdev/CALs4sv2dyy3uy+Xznm41M3uOkv1TSoGMwVBL5Cwzv=_E=+L_4A@mail.gmail.com/

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)