[net-next,v3,1/2] dsa: lan9303: Add port_max_mtu API

  Adding in support for reporting the max mtu for a given
port.

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
 drivers/net/dsa/lan9303-core.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
  

On Tue, Dec 06, 2022 at 12:34:59PM -0600, Jerry Ray wrote:
> +/* For non-cpu ports, the max frame size is 1518.
> + * The CPU port supports a max frame size of 1522.
> + * There is a JUMBO flag to make the max size 2048, but this driver
> + * presently does not support using it.
> + */
> +static int lan9303_port_max_mtu(struct dsa_switch *ds, int port)
> +{
> +	struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port));

You can put debugging prints in the code, but please, in the code that
you submit, do remove gratuitous poking in the master net_device.

> +	struct lan9303 *chip = ds->priv;
> +
> +	dev_dbg(chip->dev, "%s(%d) entered. NET max_mtu is %d",
> +		__func__, port, p->max_mtu);
> +
> +	if (dsa_port_is_cpu(dsa_to_port(ds, port)))

The ds->ops->port_max_mtu() function is never called for the CPU port.
You must know this, you put a debugging print right above. If this would
have been called for anything other than user ports, dsa_port_to_master()
would have triggered a NULL pointer dereference (dp->cpu_dp is set to
NULL for CPU ports).

So please remove dead code.

> +		return 1522 - ETH_HLEN - ETH_FCS_LEN;
> +	else
> +		return 1518 - ETH_HLEN - ETH_FCS_LEN;

Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN".

Which brings me to a more serious question. If you say that the max_mtu
is equal to the default interface MTU (1500), and you provide no means
for the user to change the MTU to a different value, then why write the
patch? What behaves differently with and without it?

> +}
> +
>  static const struct dsa_switch_ops lan9303_switch_ops = {
>  	.get_tag_protocol = lan9303_get_tag_protocol,
>  	.setup = lan9303_setup,
> @@ -1299,6 +1318,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
>  	.port_fdb_dump          = lan9303_port_fdb_dump,
>  	.port_mdb_add           = lan9303_port_mdb_add,
>  	.port_mdb_del           = lan9303_port_mdb_del,
> +	.port_max_mtu		= lan9303_port_max_mtu,
>  };
  

> > +/* For non-cpu ports, the max frame size is 1518.
> > + * The CPU port supports a max frame size of 1522.
> > + * There is a JUMBO flag to make the max size 2048, but this driver
> > + * presently does not support using it.
> > + */
> > +static int lan9303_port_max_mtu(struct dsa_switch *ds, int port)
> > +{
> > +     struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port));
> 
> You can put debugging prints in the code, but please, in the code that
> you submit, do remove gratuitous poking in the master net_device.
> 
> > +     struct lan9303 *chip = ds->priv;
> > +
> > +     dev_dbg(chip->dev, "%s(%d) entered. NET max_mtu is %d",
> > +             __func__, port, p->max_mtu);
> > +
> > +     if (dsa_port_is_cpu(dsa_to_port(ds, port)))
> 
> The ds->ops->port_max_mtu() function is never called for the CPU port.
> You must know this, you put a debugging print right above. If this would
> have been called for anything other than user ports, dsa_port_to_master()
> would have triggered a NULL pointer dereference (dp->cpu_dp is set to
> NULL for CPU ports).
> 
> So please remove dead code.
> 

I've written the function to handle being called with any port.  While I
couldn't directly exercise calling the port_max_mtu with the cpu port, I did
simulate it to verify it would work.

I'm using the dsa_to_port() rather than the dsa_port_to_master() function.

I'd rather include support for calling the api with the cpu port. I didn't
want to assume otherwise.  That's why I don't consider this dead code.

> > +             return 1522 - ETH_HLEN - ETH_FCS_LEN;
> > +     else
> > +             return 1518 - ETH_HLEN - ETH_FCS_LEN;
> 
> Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN".
> 
> Which brings me to a more serious question. If you say that the max_mtu
> is equal to the default interface MTU (1500), and you provide no means
> for the user to change the MTU to a different value, then why write the
> patch? What behaves differently with and without it?
> 

I began adding the port_max_mtu api to attempt to get rid of the following
error message:
"macb f802c000.ethernet eth0: error -22 setting MTU to 1504 to include DSA overhead"

If someone were to check the max_mtu supported on the CPU port of the LAN9303,
they would see that 1504 is okay.

> > +}
> > +
> >  static const struct dsa_switch_ops lan9303_switch_ops = {
> >       .get_tag_protocol = lan9303_get_tag_protocol,
> >       .setup = lan9303_setup,
> > @@ -1299,6 +1318,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
> >       .port_fdb_dump          = lan9303_port_fdb_dump,
> >       .port_mdb_add           = lan9303_port_mdb_add,
> >       .port_mdb_del           = lan9303_port_mdb_del,
> > +     .port_max_mtu           = lan9303_port_max_mtu,
> >  };
> 

Regards,
Jerry.
  

On Tue, Dec 06, 2022 at 11:44:58PM +0000, Jerry.Ray@microchip.com wrote:
> > > +/* For non-cpu ports, the max frame size is 1518.
> > > + * The CPU port supports a max frame size of 1522.
> > > + * There is a JUMBO flag to make the max size 2048, but this driver
> > > + * presently does not support using it.
> > > + */
> > > +static int lan9303_port_max_mtu(struct dsa_switch *ds, int port)
> > > +{
> > > +     struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port));
> > 
> > You can put debugging prints in the code, but please, in the code that
> > you submit, do remove gratuitous poking in the master net_device.
> > 
> > > +     struct lan9303 *chip = ds->priv;
> > > +
> > > +     dev_dbg(chip->dev, "%s(%d) entered. NET max_mtu is %d",
> > > +             __func__, port, p->max_mtu);
> > > +
> > > +     if (dsa_port_is_cpu(dsa_to_port(ds, port)))
> > 
> > The ds->ops->port_max_mtu() function is never called for the CPU port.
> > You must know this, you put a debugging print right above. If this would
> > have been called for anything other than user ports, dsa_port_to_master()
> > would have triggered a NULL pointer dereference (dp->cpu_dp is set to
> > NULL for CPU ports).
> > 
> > So please remove dead code.
> > 
> 
> I've written the function to handle being called with any port.  While I
> couldn't directly exercise calling the port_max_mtu with the cpu port, I did
> simulate it to verify it would work.
> 
> I'm using the dsa_to_port() rather than the dsa_port_to_master() function.

No, you're using the dsa_to_port() *and* the dsa_port_to_master() functions.
See? It's in the code you posted:

static int lan9303_port_max_mtu(struct dsa_switch *ds, int port)
{
	struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port));
			       ~~~~~~~~~~~~~~~~~~

> I'd rather include support for calling the api with the cpu port. I didn't
> want to assume otherwise.  That's why I don't consider this dead code.
> 
> > > +             return 1522 - ETH_HLEN - ETH_FCS_LEN;
> > > +     else
> > > +             return 1518 - ETH_HLEN - ETH_FCS_LEN;
> > 
> > Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN".
> > 
> > Which brings me to a more serious question. If you say that the max_mtu
> > is equal to the default interface MTU (1500), and you provide no means
> > for the user to change the MTU to a different value, then why write the
> > patch? What behaves differently with and without it?
> > 
> 
> I began adding the port_max_mtu api to attempt to get rid of the following
> error message:
> "macb f802c000.ethernet eth0: error -22 setting MTU to 1504 to include DSA overhead"

And how well did that go? That error message is saying that the macb driver
(drivers/net/ethernet/cadence/macb_main.c) does not accept the MTU of 1504.
Maybe because it doesn't have MACB_CAPS_JUMBO, I don't know. But this
patch is clearly unrelated to the problem you've observed.

> If someone were to check the max_mtu supported on the CPU port of the LAN9303,
> they would see that 1504 is okay.

No, they would not see that 1504 is okay. They would get a NULL pointer
dereference in your function, if port_max_mtu() was ever called for a
CPU port.

Don't believe me? You don't even have to. Please look at this patch,
study it, run it, and see what happens with your port_max_mtu()
implementation.

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index e5f156940c67..636e4b4df79a 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -473,6 +473,12 @@ static int dsa_port_setup(struct dsa_port *dp)
 			break;
 		dsa_port_enabled = true;
 
+		if (ds->ops->port_max_mtu) {
+			dev_info(ds->dev, "max MTU of CPU port %d is %d\n",
+				 dp->index,
+				 ds->ops->port_max_mtu(ds, dp->index));
+		}
+
 		break;
 	case DSA_PORT_TYPE_DSA:
 		if (dp->dn) {

The max_mtu of the CPU port is simply a question that the DSA core does
not ask, so there's no reason to report it. How things are supposed to
work is that the max_mtu of the user ports is propagated to their
net_devices, and when the MTU of any user port is changed, the
port_change_mtu() of that user port is called, and the maximum MTU of
all user ports is recalculated and all CPU and DSA ports also get a
port_change_mtu() call with that maximum value. If those ports need to
program their hardware with something that also includes their tagging
protocol overhead, they do so privately.