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

[Simon Horman <simon.horman@corigine.com>]

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 :(

Hi Conor, all,

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)

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