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

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

[Heiner Kallweit]

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@gmail.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]);
 		}
 	} else {
-		request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
-			       MDIO_ID_ARGS(phy_id));
+		phy_request_driver_module(dev, phy_id);
 	}
 
 	device_initialize(&mdiodev->dev);
-- 
2.20.1

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

[Andrew Lunn]

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@gmail.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

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

[Heiner Kallweit]

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 <hkallweit1@gmail.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.
> 
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.
> 
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

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

[Heiner Kallweit]

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 <hkallweit1@gmail.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.
>>
> 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
> 

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

[Florian Fainelli]

On 1/15/19 12:33 PM, 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.

Would you see any value in checking but not propagating the
request_module() return value back to the caller? Surely, something will
fail later on which may, or may not be properly handled.

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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]);
>  		}
>  	} else {
> -		request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
> -			       MDIO_ID_ARGS(phy_id));
> +		phy_request_driver_module(dev, phy_id);
>  	}
>  
>  	device_initialize(&mdiodev->dev);
> 


-- 
Florian

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

[Andrew Lunn]

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

Ah, O.K. I was expecting an ENODEV or similar if there was no module
to load.

I wonder if this depends on the user space implementation? busybox
might do something different to udev?

Maybe add a comment about the expectation that no error is given when
the module does not exist.

    Andrew

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

[Heiner Kallweit]

On 15.01.2019 23:32, Florian Fainelli wrote:
> On 1/15/19 12:33 PM, 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.
> 
> Would you see any value in checking but not propagating the
> request_module() return value back to the caller? Surely, something will
> fail later on which may, or may not be properly handled.
> 
Right, if we have a hard fail in request_module() and load the genphy
driver later then the PHY may work properly or it may not. So indeed
it may be better to return an error from phy_device_create().

Based on the discussion with Andrew I'll also add a comment explaining
what is checked (and what not).

>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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]);
>>  		}
>>  	} else {
>> -		request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
>> -			       MDIO_ID_ARGS(phy_id));
>> +		phy_request_driver_module(dev, phy_id);
>>  	}
>>  
>>  	device_initialize(&mdiodev->dev);
>>
> 
>