[Linux-stm32] [PATCH v5] net: stmmac: fix 'ethtool -P' return -EBUSY

[Hao Chen]

在 2021/7/19 下午9:57, Heiner Kallweit 写道:
> On 19.07.2021 15:08, Hao Chen wrote:
>> The permanent mac address should be available for query when the device
>> is not up.
>> NetworkManager, the system network daemon, uses 'ethtool -P' to obtain
>> the permanent address after the kernel start. When the network device
>> is not up, it will return the device busy error with 'ethtool -P'. At
>> that time, it is unable to access the Internet through the permanent
>> address by NetworkManager.
> At first a few formal aspects:
> - You don't get a lot of new friends when posting a new version every
>    few hours. Consolidate feedback and then post a new version,
>    typically not more than one version per day. Mileage of maintainers
>    may vary here.
> - When posting a new version include a change log.
> - Properly annotate the patch as net vs. net-next.
> - If you declare something to be a fix, add a Fixes tag.
Thanks for your guidance.
>> I think that the '.begin' is not used to check if the device is up, it's
>> just a pre hook for ethtool. We shouldn't check if the device is up
>> there.
>>
> Supposedly the check is there for a reason. Your statement leaves the
> impression that you're not aware of why the check exists.
> Therefore: First understand this, and then explain why it's safe to
> remove the check. This drivers uses runtime pm, so device
> might be in PCI D3 if interface is down (don't know this driver in
> detail). Therefore registers may not be accessible what may impact
> certain ethtool ops.

The network manager obtains and stores the permanent MAC address before 
the network

card is up, and then the user can specify the MAC address of the device 
in the desktop network

manager for networking.


If the driver needs the network card up to obtain the permanent MAC 
address, the user will

not be able to specify the device MAC address for networking after power on.


I don't think we should detect that the network card is up in '. Begin', 
which will cause that

all the ethtool commands can't be used when the network card is down. If 
some ethtool

commands can only be used in the up state, they should be detected in 
the corresponding

ethool OPS function instead of in the pre hook. This is too rude and 
unreasonable.


As for safe, remove the '. Begin' implementation. After modification, it 
should be safer to

operate Ethernet in the down state than in the up state.


I have checked the '. Begin' implementation of other drivers, most of 
which support the

submission of NIC driver for the first time. They are too long to know 
why '. Begin' is

implemented. I suspect that they have not noticed the usage of '. Begin'.

In cpsw NIC driver, commit ID 7898b1daf0555, a bug was found to be fixed:

"The CPSW might be suspended by RPM if all ethX interfaces are down,

but it still could be accesible through ethtool interfce. In this case
ethtool operations, requiring registers access, will cause L3 errors and
CPSW crash."

My machine uses platform GMAC. I don't have PCI network card. I can't 
test the use

of PCI network card. Through cat /sys/devices/... /power/ 
runtime_status, return

unsupported. Therefore, runtime PM is not supported.


To sum up, I think it's better to remove the '. Begin' implementation here.

Or use the following implementation:

   static int stmmac_ethtool_begin(struct net_device *dev)
{
           struct stmmac_priv *priv = netdev_priv(dev);
           int ret;

           if (strcmp(priv->device->bus_type->name, "pci"))
                   return 0;

           return pm_runtime_resume_and_get(dev);
}

   static void stmmac_ethtool_complete(struct net_device *dev)
{
           struct stmmac_priv *priv = netdev_priv(dev);

           if (strcmp(priv->device->bus_type->name, "pci"))
return;

pm_runtime_put(priv->device);
   }

  .begin = stmmac_ethtool_begin,
  .complete = stmmac_ethtool_complete,

>> Signed-off-by: Hao Chen <chenhaoa at uniontech.com>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
>> index d0ce608b81c3..8901dc9f758e 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
>> @@ -410,13 +410,6 @@ static void stmmac_ethtool_setmsglevel(struct net_device *dev, u32 level)
>>   
>>   }
>>   
>> -static int stmmac_check_if_running(struct net_device *dev)
>> -{
>> -	if (!netif_running(dev))
>> -		return -EBUSY;
>> -	return 0;
>> -}
>> -
>>   static int stmmac_ethtool_get_regs_len(struct net_device *dev)
>>   {
>>   	struct stmmac_priv *priv = netdev_priv(dev);
>> @@ -1073,7 +1066,6 @@ static int stmmac_set_tunable(struct net_device *dev,
>>   static const struct ethtool_ops stmmac_ethtool_ops = {
>>   	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>>   				     ETHTOOL_COALESCE_MAX_FRAMES,
>> -	.begin = stmmac_check_if_running,
>>   	.get_drvinfo = stmmac_ethtool_getdrvinfo,
>>   	.get_msglevel = stmmac_ethtool_getmsglevel,
>>   	.set_msglevel = stmmac_ethtool_setmsglevel,
>>
>