Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Sep 26, 2024 at 12:16:13AM +0300, Vladimir Oltean wrote:
> On Wed, Sep 25, 2024 at 08:38:23PM +0100, Russell King (Oracle) wrote:
> > > There are 2 more changes which I believe should be made in sja1105_set_port_speed():
> > > - since it isn't called from mac_config() anymore but from mac_link_up()
> > >   (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN
> > > - we can trust that phylink will not call mac_link_up() with a speed
> > >   outside what we provided in mac_capabilities, so we can remove the
> > >   -EINVAL "default" speed_mbps case, and make this method return void,
> > >   as it can never truly cause an error
> > > 
> > > But I believe these are incremental changes which should be done after
> > > this patch. I've made a note of them and will create 2 patches on top
> > > when I have the spare time.
> > 
> > ... if we were to make those changes prior to this patch, then the
> > dev_err() will no longer be there and thus this becomes a non-issue.
> > So I'd suggest a patch prior to this one to make the changes you state
> > here, thus eliminating the need for this hunk in this patch.
> 
> That sounds good. Are you suggesting you will write up such a patch for v2?

Actually, the three patches become interdependent.

Let's say we want to eliminate SPEED_UNKNOWN. Prior to my patch in this
sub-thread, we have this:

                speed_mbps[i] = sja1105_port_speed_to_ethtool(priv,
                                                              mac[i].speed);
...
                rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);

sja1105_port_speed_to_ethtool() can return SPEED_UNKNOWN if
mac[i].speed is not one of the four encodings. If we can't guarantee
that it is one of the four encodings, then SPEED_UNKNOWN may be
passed into sja1105_adjust_port_config().

Similarly, as for the default case, we can't simply delete that,
because that'll leave "speed" uninitialised and we'll get a build
warning without my changes. We could change the default case to
simply:

	default:
		return 0;

but that just looks perverse.

So, I think rather than trying to do your suggestion before my patch,
my patch needs to stand as it currently is, and then your suggestion
must happen after it - otherwise we end up introducing more complexity
or weirdness.

Hmm?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!