Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 9 May 2022 14:32:37 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Puranjay Mohan <p-mohan@ti.com>
Cc:     linux-kernel@vger.kernel.org, davem@davemloft.net,
        edumazet@google.com, krzysztof.kozlowski+dt@linaro.org,
        netdev@vger.kernel.org, devicetree@vger.kernel.org, nm@ti.com,
        ssantosh@kernel.org, s-anna@ti.com,
        linux-arm-kernel@lists.infradead.org, rogerq@kernel.org,
        grygorii.strashko@ti.com, vigneshr@ti.com, kishon@ti.com,
        robh+dt@kernel.org, afd@ti.com
Subject: Re: [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver
Message-ID: <YnkJ5bFd72d0FagD@lunn.ch>
References: <20220506052433.28087-1-p-mohan@ti.com>
 <20220506052433.28087-3-p-mohan@ti.com>
 <YnVQW7xpSWEE2/HP@lunn.ch>
 <f674c56c-0621-f471-9517-5c349940d362@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f674c56c-0621-f471-9517-5c349940d362@ti.com>
Precedence: bulk

> >> +static void icssg_init_emac_mode(struct prueth *prueth)
> >> +{
> >> +	u8 mac[ETH_ALEN] = { 0 };
> >> +
> >> +	if (prueth->emacs_initialized)
> >> +		return;
> >> +
> >> +	regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, SMEM_VLAN_OFFSET_MASK, 0);
> >> +	regmap_write(prueth->miig_rt, FDB_GEN_CFG2, 0);
> >> +	/* Clear host MAC address */
> >> +	icssg_class_set_host_mac_addr(prueth->miig_rt, mac);
> > 
> > Seems an odd thing to do, set it to 00:00:00:00:00:00. You probably
> > want to add a comment why you do this odd thing.
> 
> Actually, this is when the device is configured as a bridge, the host
> mac address has to be set to zero to while bringing it back to emac
> mode. I will add a comment to explain this.

I don't see any switchdev interface. How does it get into bridge mode?

> >> +	} else if (emac->link) {
> >> +		new_state = true;
> >> +		emac->link = 0;
> >> +		/* defaults for no link */
> >> +
> >> +		/* f/w should support 100 & 1000 */
> >> +		emac->speed = SPEED_1000;
> >> +
> >> +		/* half duplex may not supported by f/w */
> >> +		emac->duplex = DUPLEX_FULL;
> > 
> > Why set speed and duplex when you have just lost the link? They are
> > meaningless until the link comes back.
> 
> These were just the default values that we added.
> What do you suggest I put here?

Nothing. If the link is down, they are meaningless. If something is
accessing them when the link is down, that code is broken. So i
suppose you could give them poison values to help find your broken
code.

> >> +	for_each_child_of_node(eth_ports_node, eth_node) {
> >> +		u32 reg;
> >> +
> >> +		if (strcmp(eth_node->name, "port"))
> >> +			continue;
> >> +		ret = of_property_read_u32(eth_node, "reg", &reg);
> >> +		if (ret < 0) {
> >> +			dev_err(dev, "%pOF error reading port_id %d\n",
> >> +				eth_node, ret);
> >> +		}
> >> +
> >> +		if (reg == 0)
> >> +			eth0_node = eth_node;
> >> +		else if (reg == 1)
> >> +			eth1_node = eth_node;
> > 
> > and if reg == 4
> > 
> > Or reg 0 appears twice?
> 
> In both of the cases that you mentioned, the device tree schema check
> will fail, hence, we can safely assume that this will be 0 and 1 only.

Nothing forces you to run the scheme checker. It is not run by the
kernel before it starts accessing the DT blob. You should assume it is
invalid until you have proven it to be valid.

	Andrew