Re: [PATCH 2/3] net: ethernet: actions: Add Actions Semi Owl Ethernet MAC driver

[Cristian Ciocaltea <cristian.ciocaltea@gmail.com>]

On Sun, Mar 14, 2021 at 05:36:32AM +0100, Andrew Lunn wrote:
> > > > +	if (phy->interface != PHY_INTERFACE_MODE_RMII) {
> > > > +		netdev_err(netdev, "unsupported phy mode: %s\n",
> > > > +			   phy_modes(phy->interface));
> > > > +		phy_disconnect(phy);
> > > > +		netdev->phydev = NULL;
> > > > +		return -EINVAL;
> > > > +	}
> > > 
> > > It looks like the MAC only supports symmetric pause. So you should
> > > call phy_set_sym_pause() to let the PHY know this.
> > 
> > I did not find any reference related to the supported pause types,
> > is this normally dependant on the PHY interface mode?
> 
> There is a MAC / PHY split there. The PHY is responsible for the
> negotiation for what each end can do. But it is the MAC which actually
> implements pause. The MAC needs to listen to pause frames and not send
> out data frames when the link peer indicates pause. And the MAC needs
> to send a pause frames when its receive buffers are full. The code you
> have in this MAC driver seems to indicate the MAC only supports
> symmetric pause. Hence you need to configure the PHY to only auto-neg
> symmetric pause.

Thanks for explaining this, I will implement the indicated PHY
configuration and, additionally, also enable the SMII interface.

> > > > +	ret = crypto_skcipher_encrypt(req);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "failed to encrypt S/N: %d\n", ret);
> > > > +		goto err_free_tfm;
> > > > +	}
> > > > +
> > > > +	netdev->dev_addr[0] = 0xF4;
> > > > +	netdev->dev_addr[1] = 0x4E;
> > > > +	netdev->dev_addr[2] = 0xFD;
> > > 
> > > 0xF4 has the locally administered bit 0. So this is a true OUI. Who
> > > does it belong to? Ah!
> > > 
> > > F4:4E:FD Actions Semiconductor Co.,Ltd.(Cayman Islands)
> > > 
> > > Which makes sense. But is there any sort of agreement this is allowed?
> > > It is going to cause problems if they are giving out these MAC
> > > addresses in a controlled way.
> > 
> > Unfortunately this is another undocumented logic taken from the vendor
> > code. I have already disabled it from being built by default, although,
> > personally, I prefer to have it enabled in order to get a stable MAC
> > address instead of using a randomly generated one or manually providing
> > it via DT.
> > 
> > Just for clarification, I did not have any agreement or preliminary
> > discussion with the vendor. This is just a personal initiative to
> > improve the Owl SoC support in the mainline kernel.
> > 
> > > Maybe it would be better to set bit 1 of byte 0? And then you can use
> > > 5 bytes from enc_sn, not just 4.
> > 
> > I included the MAC generation feature in the driver to be fully
> > compatible with the original implementation, but I'm open for changes
> > if it raises concerns and compatibility is less important.
> 
> This is not a simple question to answer. If the vendor driver does
> this, then the vendor can never assign MAC addresses in a controlled
> way, unless they have a good idea how the algorithm turns serial
> numbers into MAC addresses, and they can avoid MAC addresses for
> serial numbers already issued.
> 
> But should the Linux kernel do the same? If all you want is a stable
> MAC address, my personal preference would be to set the locally
> administered bit, and fill the other 5 bytes from the hash
> algorithm. You then have a stable MAC addresses, but you clearly
> indicate it is not guaranteed to by globally unique, and you do not
> need to worry about what the vendor is doing.

I fully agree, so I'm going to set byte 0 to value 0xF6 and replace
bytes 1 & 2 with entries from the computed hash. I will also document
this modification and the rationale behind.

> > > Otherwise, this look a new clean driver.
> > 
> > Well, I tried to do my best, given my limited experience as a self-taught
> > kernel developer. Hopefully reviewing my code will not cause too many
> > headaches! :)
> 
> This is actually above average for a self-taught kernel
> developer. Well done.

Thank you, Andrew!

> 	   Andrew