Re: [PATCH net-next v4 08/15] net: phy: mscc: macsec initialization

[Antoine Tenart]

Hello David,

On Thu, Dec 19, 2019 at 12:11:17PM -0800, David Miller wrote:
> From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> Date: Thu, 19 Dec 2019 11:55:08 +0100
> 
> > +static u32 __vsc8584_macsec_phy_read(struct phy_device *phydev,
> > +				     enum macsec_bank bank, u32 reg, bool init)
> > +{
> > +	u32 val, val_l = 0, val_h = 0;
> > +	unsigned long deadline;
> > +	int rc;
> > +
> > +	if (!init) {
> > +		rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
> > +		if (rc < 0)
> > +			goto failed;
> > +	} else {
> > +		__phy_write_page(phydev, MSCC_PHY_PAGE_MACSEC);
> > +	}
> 
> Having to export __phy_write_page() in the previous patch looked like
> a huge red flag to me, and indeed on top of it you're using it to do
> conditional locking here.
> 
> I'm going to unfortunately have to push back on this, please sanitize
> the locking here so that you can use the existing exports properly.

I do agree this conditional locking is not very good. We had discussions
with Andrew about how bad this is, but there are no easy fix for this.
At least the condition is consistent depending on if we're in the init
step or not, which is better than having different values in the same
context. The idea was not to duplicate hundreds of lines.

Having said that, the reason we had to do this is we have multiple PHYs
inside the same package and some steps are to be done for all PHYs at a
time. I had another look at this and, for MACsec only, we might be able
not to have a single common part. I'll test the changes and if that's
successful I'll be able to fix this in a clean way.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com