by Andrew Lunn

[permalink] [raw]

On Sat, Feb 04, 2023 at 03:02:00PM +0000, Daniel Golle wrote:
> Hi Vladimir,
>
> thank you for the review!
>
> On Sat, Feb 04, 2023 at 12:19:15AM +0200, Vladimir Oltean wrote:
> > On Fri, Feb 03, 2023 at 07:06:53AM +0000, Daniel Golle wrote:
> > > @@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
> > >
> > > switch (interface) {
> > > case PHY_INTERFACE_MODE_TRGMII:
> > > + return &priv->pcs[port].pcs;
> > > case PHY_INTERFACE_MODE_SGMII:
> > > case PHY_INTERFACE_MODE_1000BASEX:
> > > case PHY_INTERFACE_MODE_2500BASEX:
> > > - return &priv->pcs[port].pcs;
> > > + if (!mt753x_is_mac_port(port))
> > > + return ERR_PTR(-EINVAL);
> >
> > What is the reason for returning ERR_PTR(-EINVAL) to mac_select_pcs()?
>
> The SerDes PCS units are only available for port 5 and 6. The code
> should make sure that the corresponding interface modes are only used
> on these two ports, so a BUG_ON(!mt753x_is_mac_port(port)) would also
> do the trick, I guess. However, as dsa_port_phylink_mac_select_pcs may
> also return ERR_PTR(-EOPNOTSUPP), returning ERR_PTR(-EINVAL) felt like
> the right thing to do in that case.
> Are you suggesting to use BUG_ON() instead or rather return
> ERR_PTR(-EOPNOTSUPP)?

I was not suggesting anything, I was just asking why the inconsistency
exists between the handling of SERDES protocols on ports != 5 or 6
(return ERR_PTR(-EINVAL)), and the handling of unknown PHY modes
(return NULL). If you don't want to convey any special message, then
returning NULL to phylink should be sufficient to say there is no PCS.
The driver already ensures that phylink validation fails with wrong PHY
mode on wrong port due to the way in which it populates supported_interfaces
in mt7531_mac_port_get_caps().

Also, no BUG_ON() for the reasons Andrew pointed out.

> > > + return priv->sgmii_pcs[port - 5];
> >
> > How about putting the pcs pointer in struct mt7530_port?
>
> There are only two SerDes units available, only for port 5 and port 6.
> Hence I wouldn't want to allocate additional unused sizeof(void*) for
> ports 0 to 4.

I asked the question fully knowing that there will be no more than 2
ports with a non-NULL PCS pointer. There's a time and place for
(premature) optimizations, but I don't think that the control path of a
switch is one particular place that comes at the top. Between
priv->ports[port].sgmii_pcs and priv->sgmii_pcs[port - 5], my personal
sensibilities for simple and maintainable code would always choose the
former. However I'm not going to cling onto this. Whichever way you prefer.

> > > + for (i = 0; i < 2; ++i)
> >
> > There is no ++i in this driver and there are 11 i++, so please be
> > consistent with what exists.
>
> As most likely in all cases a pre-increment is sufficient and less
> resource consuming than a post-increment operation we should consider
> switching them all to ++i instead of i++.
> I will anyway use i++ in v2 for now.

And what would you suggest is the difference in the compiled code between
"for (i = 0; i < n; i++)" and "for (i = 0; i < n; ++i)"?