Re: [net-next] net: phy: fix issue with loading PHY driver w/o initramfs

[Geert Uytterhoeven <geert@linux-m68k.org>]

	Hi Heiner,

> It was reported that on a system with nfsboot and w/o initramfs network
> fails because trying to load the PHY driver returns -ENOENT. Reason was
> that due to missing initramfs the modprobe binary isn't available.
> So we have to ignore error code -ENOENT.
> 
> Fixes: 13d0ab6750b2 ("net: phy: check return code when requesting PHY driver module")
> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Thanks for your patch!

>
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -552,10 +552,12 @@ static int phy_request_driver_module(struct phy_device *dev, int phy_id)
>  
>  	ret = request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
>  			     MDIO_ID_ARGS(phy_id));
> -	/* we only check for failures in executing the usermode binary,
> -	 * not whether a PHY driver module exists for the PHY ID
> +	/* We only check for failures in executing the usermode binary,
> +	 * not whether a PHY driver module exists for the PHY ID.
> +	 * Accept -ENOENT because this may occur in case no initramfs exists,
> +	 * then modprobe isn't available.
>  	 */
> -	if (IS_ENABLED(CONFIG_MODULES) && ret < 0) {
> +	if (IS_ENABLED(CONFIG_MODULES) && ret < 0 && ret != -ENOENT) {
>  		phydev_err(dev, "error %d loading PHY driver module for ID 0x%08x\n",
>  			   ret, phy_id);
>  		return ret;

While I believe this patch fixes the particular issue I was seeing, I'm
not convinced it fixes other possible issues.  There are several
different error cases in both the kernel (e.g. -ETIME) and userland that
can cause request_module() to fail, without actually having any impact,
if the particular PHY driver is builtin or already loaded.

IMHO, if the wanted PHY is available, all errors returned by
request_module() should be ignored.

In other subsystems, this is handled like:

    if (driver_is_missing)
        request_module(...);
    if (driver_is_missing)
        return -E...;

Note that most callers of request_module() don't even check the error
code it returns: the only thing that matters is if the (possibly loaded)
functionality is available afterwards or not.

Thanks!

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds