netdev - Re: [PATCH v2 1/1] net: macb: Shorten max_tx_len to 4KiB

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: <20230420-absinthe-broiler-b992997c6cc5@wendy>
Date:   Thu, 20 Apr 2023 08:18:35 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Simon Horman <simon.horman@...igine.com>,
        <daire.mcnamara@...rochip.com>, <nicolas.ferre@...rochip.com>,
        <claudiu.beznea@...rochip.com>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <pabeni@...hat.com>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 1/1] net: macb: Shorten max_tx_len to 4KiB - 56 on mpfs

Jaukb, Simon,

On Wed, Apr 19, 2023 at 06:02:22PM -0700, Jakub Kicinski wrote:
> On Tue, 18 Apr 2023 16:28:25 +0200 Simon Horman wrote:

[readding the context]

> > > static const struct macb_config sama7g5_gem_config = {
> > > @@ -4986,8 +4985,17 @@ static int macb_probe(struct platform_device *pdev)
> > >       bp->tx_clk = tx_clk;
> > >       bp->rx_clk = rx_clk;
> > >       bp->tsu_clk = tsu_clk;
> > > -     if (macb_config)
> > > +     if (macb_config) {
> > > +             if (hw_is_gem(bp->regs, bp->native_io)) {
> > > +                     if (macb_config->max_tx_length)
> > > +                             bp->max_tx_length = macb_config->max_tx_length;
> > > +                     else
> > > +                             bp->max_tx_length = GEM_MAX_TX_LEN;
> > > +             } else {
> > > +                     bp->max_tx_length = MACB_MAX_TX_LEN;
> > > +             }

> > no need to refresh the patch on my account.
> > But can the above be simplified as:
> > 
> >                if (macb_is_gem(bp) && hw_is_gem(bp->regs, bp->native_io))
> >                        bp->max_tx_length = macb_config->max_tx_length;
> >                else
> >                        bp->max_tx_length = MACB_MAX_TX_LEN;
> 
> I suspect that DaveM agreed, because patch is set to Changes Requested
> in patchwork :) 
> 
> Daire, please respin with Simon's suggestion.

I'm feeling a bit stupid reading this suggestion as I am not sure how it
is supposed to work :(

Firstly, why macb_is_gem() and hw_is_gem()? They both do the same thing,
except last time around we established that macb_is_gem() cannot return
anything other than false at this point.
What have I missed here?

Secondly, is it guaranteed that macb_config::max_tx_length is even
set?

Also, another question...
Is it even possible for `if (macb_config)` to be false?
Isn't it either going to be set to &default_gem_config or to
match->data, no? The driver is pretty inconsistent about if it checks
whether macb_config is non-NULL before accessing it, but from reading
.probe, it seems to be like it is always set to something valid at this
point.

(btw Daire, Nicolas' email has no h in it)

Cheers,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)