Message-ID: <55FDCA5F.9090604@gmail.com>
Date:	Sat, 19 Sep 2015 13:49:35 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	devicetree@...r.kernel.org, Frank Rowand <frowand.list@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	Iyappan Subramanian <isubramanian@....com>,
	Keyur Chudgar <kchudgar@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linuxppc-dev@...ts.ozlabs.org, Li Yang <leoli@...escale.com>,
	Michal Simek <michal.simek@...inx.com>, netdev@...r.kernel.org,
	Robert Richter <rric@...nel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Sören Brinkmann <soren.brinkmann@...inx.com>,
	Sunil Goutham <sgoutham@...ium.com>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
Subject: Re: [PATCH 0/7] Phy and mdiobus fixes

Le 09/18/15 02:46, Russell King - ARM Linux a écrit :
> Hi,
> 
> While looking at the phy code, I identified a number of weaknesses
> where refcounting on device structures was being leaked, where
> modules could be removed while in-use, and where the fixed-phy could
> end up having unintended consequences caused by incorrect calls to
> fixed_phy_update_state().
> 
> This patch series resolves those issues, some of which were discovered
> with testing on an Armada 388 board.  Not all patches are fully tested,
> particularly the one which touches several network drivers.
> 
> When resolving the struct device refcounting problems, several different
> solutions were considered before settling on the implementation here -
> one of the considerations was to avoid touching many network drivers.
> The solution here is:
> 
> 	phy_attach*() - takes a refcount
> 	phy_detach*() - drops the phy_attach refcount
> 
> Provided drivers always attach and detach their phys, which they should
> already be doing, this should change nothing, even if they leak a refcount.
> 
> 	of_phy_find_device() and of_* functions which use that take
> 	a refcount.  Arrange for this refcount to be dropped once
> 	the phy is attached.
> 
> This is the reason why the previous change is important - we can't drop
> this refcount taken by of_phy_find_device() until something else holds
> a reference on the device.  This resolves the leaked refcount caused by
> using of_phy_connect() or of_phy_attach().
> 
> Even without the above changes, these drivers are leaking by calling
> of_phy_find_device().  These drivers are addressed by adding the
> appropriate release of that refcount.
> 
> The mdiobus code also suffered from the same kind of leak, but thankfully
> this only happened in one place - the mdio-mux code.
> 
> I also found that the try_module_get() in the phy layer code was utterly
> useless: phydev->dev.driver was guaranteed to always be NULL, so
> try_module_get() was always being called with a NULL argument.  I proved
> this with my SFP code, which declares its own MDIO bus - the module use
> count was never incremented irrespective of how I set the MDIO bus up.
> This allowed the MDIO bus code to be removed from the kernel while there
> were still PHYs attached to it.
> 
> One other bug was discovered: while using in-band-status with mvneta, it
> was found that if a real phy is attached with in-band-status enabled,
> and another ethernet interface is using the fixed-phy infrastructure, the
> interface using the fixed-phy infrastructure is configured according to
> the other interface using the in-band-status - which is caused by the
> fixed-phy code not verifying that the phy_device passed in is actually
> a fixed-phy device, rather than a real MDIO phy.
> 
> Lastly, having mdio_bus reversing phy_device_register() internals seems
> like a layering violation - it's trivial to move that code to the phy
> device layer.

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>

Thanks!
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists