lkml.org 
[lkml]   [2015]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 0/7] Phy and mdiobus fixes
From
Date
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@gmail.com>

Thanks!
--
Florian


\
 
 \ /
  Last update: 2015-09-19 23:01    [W:0.088 / U:1.540 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site