On Fri, Sep 03, 2021 at 02:26:07AM +0300, Vladimir Oltean wrote: > On Fri, Sep 03, 2021 at 01:02:06AM +0200, Andrew Lunn wrote: > > We should try to keep phylink_create and phylink_destroy symmetrical: > > > > /** > > * phylink_create() - create a phylink instance > > * @config: a pointer to the target &struct phylink_config > > * @fwnode: a pointer to a &struct fwnode_handle describing the network > > * interface > > * @iface: the desired link mode defined by &typedef phy_interface_t > > * @mac_ops: a pointer to a &struct phylink_mac_ops for the MAC. > > * > > * Create a new phylink instance, and parse the link parameters found in @np. > > * This will parse in-band modes, fixed-link or SFP configuration. > > * > > * Note: the rtnl lock must not be held when calling this function. > > > > Having different locking requirements will catch people out. > > > > Interestingly, there is no ASSERT_NO_RTNL(). Maybe we should add such > > a macro. > > In this case, the easiest might be to just take a different mutex in > dpaa2 which serializes all places that access the priv->mac references. > I don't know exactly why the SFP bus needs the rtnl_mutex, I've removed > those locks and will see what fails tomorrow, but I don't think dpaa2 > has a good enough justification to take the rtnl_mutex just so that it > can connect and disconnect to the MAC freely at runtime. It needs it to ensure that the sfp-bus code is safe. sfp-bus code sits between phylink and the sfp stuff, and will be called from either side. It can't have its own lock, because that gives lockdep splats. Removing a lock and then running the kernel is a down right stupid way to test to see if a lock is necessary. That approach is like having built a iron bridge, covered it in paint, then you remove most the bolts, and then test to see whether it's safe for vehicles to travel over it by riding your bicycle across it and declaring it safe. Sorry, but if you think "remove lock, run kernel, if it works fine the lock is unnecessary" is a valid approach, then you've just disqualified yourself from discussing this topic any further. Locking is done by knowing the code and code analysis, not by playing "does the code fail if I remove it" games. I am utterly shocked that you think that this is a valid approach. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
