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

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

[Heiner Kallweit]

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>
---
 drivers/net/phy/phy_device.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 264312137..1a12acfd9 100644
--- 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;
-- 
2.20.1

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

[Krzysztof Kozlowski]

On Sat, 19 Jan 2019 at 10:30, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> 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>
> ---

Fixes the issue for me, thanks!
Tested-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

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

[Geert Uytterhoeven]

	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

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

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sat, 19 Jan 2019 10:30:21 +0100

> 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>

Applied.

However, I agree with Geert that we should adopt the:

	if (module_not_present)
		request_module();
	if (module_not_present)
		goto failed_to_load;

pattern.

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

[Heiner Kallweit]

On 22.01.2019 23:45, David Miller wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Sat, 19 Jan 2019 10:30:21 +0100
> 
>> 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>
> 
> Applied.
> 
> However, I agree with Geert that we should adopt the:
> 
> 	if (module_not_present)
> 		request_module();
> 	if (module_not_present)
> 		goto failed_to_load;
> 
> pattern.
> 
I know this is the standard pattern for request_module().
Unfortunately the situation is a little bit tricky with PHY drivers.
We don't know whether there's a module matching the PHY ID and
it's a valid use case that there's no such module.
In such a case we bind the genphy driver later, and a lot of PHY's
are totally happy with the genphy driver and therefore no dedicated
PHY drivers exist.

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

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert@linux-m68k.org>

	Hi Heiner,

> On 22.01.2019 23:45, David Miller wrote:
> > From: Heiner Kallweit <hkallweit1@gmail.com>
> > Date: Sat, 19 Jan 2019 10:30:21 +0100
> >
> >> 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>
> >
> > Applied.
> >
> > However, I agree with Geert that we should adopt the:
> >
> > 	if (module_not_present)
> > 		request_module();
> > 	if (module_not_present)
> > 		goto failed_to_load;
> >
> > pattern.
>
> I know this is the standard pattern for request_module().
> Unfortunately the situation is a little bit tricky with PHY drivers.
> We don't know whether there's a module matching the PHY ID and
> it's a valid use case that there's no such module.
> In such a case we bind the genphy driver later, and a lot of PHY's
> are totally happy with the genphy driver and therefore no dedicated
> PHY drivers exist.

Currently, if request_module() fails (for whatever reason, except
-ENOENT), you don't bind to the genphy driver, but propagate an
error[*], leaving the user without network interface.

Is that better than ignoring the error, and binding to the genphy
driver?
When do you expect the phy-specific driver to become available, if
ever?

[*] The actual error code returned by request_module(), and not
    -EPROBE_DEFER.  The latter may sound attractive, as it is meant to
    cause a retry later, but has its own set of problems with optional
    drivers that may never become available (e.g. missing drivers for
    DMA controllers).

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