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: <20230421-gentleman-contrite-bad775caf1c9@spud>
Date:   Fri, 21 Apr 2023 17:39:52 +0100
From:   Conor Dooley <conor@...nel.org>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     Conor Dooley <conor.dooley@...rochip.com>,
        Jakub Kicinski <kuba@...nel.org>, 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

Hey Simon,

On Fri, Apr 21, 2023 at 04:05:19PM +0200, Simon Horman wrote:
> On Thu, Apr 20, 2023 at 08:18:35AM +0100, Conor Dooley wrote:
> > 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 :(

> just to clarify, my suggestion was at a slightly higher level regarding
> the arrangement of logic statements:
> 
> 	if (a)
> 		if (b)
> 
> 	vs
> 
> 	if (a && b)

Ah, I do at least feel less stupid now!
There are 3 possible conditions though, you'd be left with something
like:
	if !hw_is_gem()
	else if macb_config->max_tx_length
	else
> 
> I think your concerns are deeper and, in my reading of them, ought
> to be addressed.
> 
> > 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?

These two were concerns about your suggestion, so they can now be
disregarded as you'd not been seriously suggesting that particular
if (false && hw_is_gem()) test ;)

> > 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.

This one though is more of a question for the drivers's maintainers -
Daire's only gone and copied what's done about 4 lines above the top of
the diff. Removing useless NULL checks, assuming they are useless, is
surely out of scope for sorting out this erratum though, no?

Cheers,
Conor.

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