From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH net-next] net: phy: check return code when requesting PHY driver module Date: Tue, 15 Jan 2019 22:58:15 +0100 Message-ID: References: <7943c933-c353-1c0e-14c0-cdbb4d874d0a@gmail.com> <20190115214327.GD26788@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Florian Fainelli , David Miller , "netdev@vger.kernel.org" To: Andrew Lunn Return-path: Received: from mail-wm1-f68.google.com ([209.85.128.68]:39839 "EHLO mail-wm1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730622AbfAOV6X (ORCPT ); Tue, 15 Jan 2019 16:58:23 -0500 Received: by mail-wm1-f68.google.com with SMTP id y8so4814139wmi.4 for ; Tue, 15 Jan 2019 13:58:22 -0800 (PST) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 15.01.2019 22:52, Heiner Kallweit wrote: > On 15.01.2019 22:43, Andrew Lunn wrote: >> On Tue, Jan 15, 2019 at 09:33:52PM +0100, Heiner Kallweit wrote: >>> When requesting the PHY driver module fails we'll bind the genphy >>> driver later. This isn't obvious to the user and may cause, depending >>> on the PHY, different types of issues. Therefore check the return code >>> of request_module() and inform the user in case of failure. >>> >>> Signed-off-by: Heiner Kallweit >>> --- >>> drivers/net/phy/phy_device.c | 17 +++++++++++++---- >>> 1 file changed, 13 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >>> index a2423cbb2..1527ed0f2 100644 >>> --- a/drivers/net/phy/phy_device.c >>> +++ b/drivers/net/phy/phy_device.c >>> @@ -548,6 +548,17 @@ static const struct device_type mdio_bus_phy_type = { >>> .pm = MDIO_BUS_PHY_PM_OPS, >>> }; >>> >>> +static void phy_request_driver_module(struct phy_device *dev, int phy_id) >>> +{ >>> + int ret; >>> + >>> + ret = request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT, >>> + MDIO_ID_ARGS(phy_id)); >>> + if (IS_ENABLED(CONFIG_MODULES) && ret < 0) >>> + phydev_err(dev, "error %d loading PHY driver module for ID 0x%08x\n", >>> + ret, phy_id); >>> +} >>> + >>> struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id, >>> bool is_c45, >>> struct phy_c45_device_ids *c45_ids) >>> @@ -610,12 +621,10 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id, >>> if (!(c45_ids->devices_in_package & (1 << i))) >>> continue; >>> >>> - request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT, >>> - MDIO_ID_ARGS(c45_ids->device_ids[i])); >>> + phy_request_driver_module(dev, c45_ids->device_ids[i]); >> >> Hi Heiner >> >> I'm not sure this is a good idea for a c45 device. It can have >> multiple devices ids. All we really need is that a driver is loaded >> for one of them. That driver should then be able to control all the >> devices in the package. So i would probably only warn when all >> request_module() calls of failed. >> > I explicitly check for ret < 0. If there's no PHY driver module for > a specific PHY ID then the return code would be > 0. > My understanding of request_module() is that a return code < 0 > is returned if something bad happens in modprobe() call as such. > >> Maybe it is actually better to warning when we bind the generic phy >> driver to the PHY? That is the real problem we are trying to warn >> about. >> Ah, I see. We have a misunderstanding about what is actually checked. My intention is to check whether something failed in the usermode helper or modprobe internally. I don't want to check whether a PHY driver module was found for the PHY ID. > There are several PHY's which don't need a dedicated driver because > they work perfectly fine with the generic PHY driver. Therefore I > think it's hard to tell between regular genphy usage and error when > binding the genphy driver. > >> I also think reporting this as an error is too strong. Some PHYs are >> happy with the generic PHY driver. So i think a warning is better than >> an error. >> > As stated above, the error message isn't triggered if there's no > PHY driver module for a PHY ID. > >> Andrew >> > Heiner >