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

[Heiner Kallweit <hkallweit1@gmail.com>]

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
>