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] [day] [month] [year] [list]

Date:   Fri, 21 Apr 2023 19:43:10 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Conor Dooley <conor@...nel.org>
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

On Fri, Apr 21, 2023 at 05:39:52PM +0100, Conor Dooley wrote:
> 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 ;)

Yes, that's right. I would not have made the suggestion had I known that :)

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

FWIIW, I would say that a patch to address an erratum should only
address the erratum.