Messages in this thread | | | Date | Thu, 9 Jan 2020 14:18:42 +0100 | From | Antoine Tenart <> | Subject | Re: [PATCH net-next v4 08/15] net: phy: mscc: macsec initialization |
| |
Hello David,
On Thu, Dec 19, 2019 at 12:11:17PM -0800, David Miller wrote: > From: Antoine Tenart <antoine.tenart@bootlin.com> > 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
| |