* Re: [PATCH net-next] net: phy: fix issue with loading PHY driver w/o initramfs
2019-01-19 9:30 [PATCH net-next] net: phy: fix issue with loading PHY driver w/o initramfs Heiner Kallweit
@ 2019-01-21 14:46 ` Krzysztof Kozlowski
2019-01-22 15:52 ` [net-next] " Geert Uytterhoeven
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2019-01-21 14:46 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [net-next] net: phy: fix issue with loading PHY driver w/o initramfs
2019-01-19 9:30 [PATCH net-next] net: phy: fix issue with loading PHY driver w/o initramfs Heiner Kallweit
2019-01-21 14:46 ` Krzysztof Kozlowski
@ 2019-01-22 15:52 ` Geert Uytterhoeven
2019-01-22 22:45 ` [PATCH net-next] " David Miller
2019-01-23 10:06 ` [net-next] " Geert Uytterhoeven
3 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2019-01-22 15:52 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Krzysztof Kozlowski, Andrew Lunn, Florian Fainelli, David Miller,
netdev, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: phy: fix issue with loading PHY driver w/o initramfs
2019-01-19 9:30 [PATCH net-next] net: phy: fix issue with loading PHY driver w/o initramfs Heiner Kallweit
2019-01-21 14:46 ` Krzysztof Kozlowski
2019-01-22 15:52 ` [net-next] " Geert Uytterhoeven
@ 2019-01-22 22:45 ` David Miller
2019-01-23 6:08 ` Heiner Kallweit
2019-01-23 10:06 ` [net-next] " Geert Uytterhoeven
3 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2019-01-22 22:45 UTC (permalink / raw)
To: hkallweit1; +Cc: krzk, andrew, f.fainelli, netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: phy: fix issue with loading PHY driver w/o initramfs
2019-01-22 22:45 ` [PATCH net-next] " David Miller
@ 2019-01-23 6:08 ` Heiner Kallweit
0 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2019-01-23 6:08 UTC (permalink / raw)
To: David Miller; +Cc: krzk, andrew, f.fainelli, netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next] net: phy: fix issue with loading PHY driver w/o initramfs
2019-01-19 9:30 [PATCH net-next] net: phy: fix issue with loading PHY driver w/o initramfs Heiner Kallweit
` (2 preceding siblings ...)
2019-01-22 22:45 ` [PATCH net-next] " David Miller
@ 2019-01-23 10:06 ` Geert Uytterhoeven
3 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2019-01-23 10:06 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Krzysztof Kozlowski, Andrew Lunn, Florian Fainelli, David Miller,
netdev, 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
^ permalink raw reply [flat|nested] 6+ messages in thread