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 Thanks! -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Phy and mdiobus fixes
On Fri, Sep 18, 2015 at 08:01:28AM -0700, Sören Brinkmann wrote: > Hi Russell, > > On Fri, 2015-09-18 at 10:56AM +0100, Russell King - ARM Linux wrote: > > Sorry guys, some of you will get the patches twice, as Sören's name > > in the header caused vger to reject all the patches. > > That is the first time I hear about an issue like that. I've been > receiving patches fine thus far and nobody reported any rejections (by > vger) to me. Is it some bounce on Xilinx/my side or is vger suddenly > rejecting non-ascii chars or is something in the mail processing chain > not properly encoding those chars? > Please, let me know if I can help with the problem. It's to do with how I generate the patch set and submit it to my MTA. The result is that the header fields contain an ö. Almost all MTAs accept this despite it being questionable, but zmailer is extra fussy and rejects this - and rejected the messages to lkml, netdev, etc. Normally, the MUA would encode "Sören Brinkmann" in a header into a 7-bit ascii representation as "=?utf-8?B?U8O2cmVu?= Brinkmann". It's one of the many issues of not using a MUA to send out patches. It's also worth noting that git-send-email misses out mime headers (or used to - which is why I have my own scripts for doing this) which technically makes the bodies it mails out in violation of the RFCs. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Phy and mdiobus fixes
Hi Russell, On Fri, 2015-09-18 at 10:56AM +0100, Russell King - ARM Linux wrote: > Sorry guys, some of you will get the patches twice, as Sören's name > in the header caused vger to reject all the patches. That is the first time I hear about an issue like that. I've been receiving patches fine thus far and nobody reported any rejections (by vger) to me. Is it some bounce on Xilinx/my side or is vger suddenly rejecting non-ascii chars or is something in the mail processing chain not properly encoding those chars? Please, let me know if I can help with the problem. Thanks, Sören -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Phy and mdiobus fixes
Sorry guys, some of you will get the patches twice, as Sören's name in the header caused vger to reject all the patches. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] Phy and mdiobus fixes
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. drivers/net/ethernet/apm/xgene/xgene_enet_hw.c| 23 ++--- drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 19 +++ drivers/net/ethernet/freescale/gianfar.c | 6 ++- drivers/net/ethernet/freescale/ucc_geth.c | 8 ++- drivers/net/ethernet/marvell/mvneta.c | 2 + drivers/net/ethernet/xilinx/xilinx_emaclite.c | 2 + drivers/net/phy/fixed_phy.c | 2 +- drivers/net/phy/mdio-mux.c| 19 --- drivers/net/phy/mdio_bus.c| 24 ++--- drivers/net/phy/phy_device.c | 62 ++- drivers/of/of_mdio.c | 27 -- include/linux/phy.h | 6 ++- 12 files changed, 156 insertions(+), 44 deletions(-) -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
