netdev - Re: [PATCH net-next] net: phy: check return code when requesting PHY driver module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190115214327.GD26788@lunn.ch>
Date:   Tue, 15 Jan 2019 22:43:27 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: phy: check return code when requesting PHY
 driver module

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 <hkallweit1@...il.com>
> ---
>  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.

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.

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.

   Andrew