[Linux-stm32] [PATCH v5] net: stmmac: fix 'ethtool -P' return -EBUSY
Hao Chen
chenhaoa at uniontech.com
Wed Jul 21 06:07:28 UTC 2021
在 2021/7/21 上午11:02, Joakim Zhang 写道:
>> -----Original Message-----
>> From: Hao Chen <chenhaoa at uniontech.com>
>> Sent: 2021年7月21日 9:35
>> To: Heiner Kallweit <hkallweit1 at gmail.com>
>> Cc: alexandre.torgue at foss.st.com; joabreu at synopsys.com;
>> davem at davemloft.net; kuba at kernel.org; mcoquelin.stm32 at gmail.com;
>> linux at armlinux.org.uk; netdev at vger.kernel.org;
>> linux-stm32 at st-md-mailman.stormreply.com; Joakim Zhang
>> <qiangqing.zhang at nxp.com>; peppe.cavallaro at st.com
>> Subject: Re: [PATCH v5] net: stmmac: fix 'ethtool -P' return -EBUSY
>>
>>
>> 在 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'm not sure quite understand what's your requirements, AFAIK, NIC driver has
> .ndo_set_mac_address to set mac address, and it can be done whether ifup or ifdown.
I want to get permanent MAC address when the card is down.And I think it
is more convenient to get statistics
in the down state by ethtool -S. But current all of the ethool command
return -EBUSY.
>
>> 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.
> As Heiner mentioned, ethtool ops may access NIC hardware registers which need
> clocks enabled. Now we have .begin that means clocks enabled when use ethtool,
> if you remove .begin, you need ensure clocks enabled whenever ethtool used. So,
> remove .begin directly wound not be acceptable.
Yeah.
>> 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.
> I have a GMAC5.10 at my hand, and check the runtime PM status on net-next tree:
> ~# cat /sys/class/net/eth0/device/power/runtime_status
> Active
>
> Another main you said:
> "By browse the code and test it, I find that stmmac not support runtime_status,
>
> pm_runtime_get_sync will return -13 errno. So shouldn't modify it in this way.
>
> I think we should just delete the implementation of begin, it's a pre hook for ethtool."
>
> I don't know why " pm_runtime_get_sync will return -13 errno ". If so, STMMAC driver still
> can work at you side? Clocks always enabled after probed?
>
> Best Regards,
> Joakim Zhang
I find -13 error is because there:
@@drivers/base/power/runtime.c
733 static int rpm_resume(struct device *dev, int rpmflags)
734 __releases(&dev->power.lock) __acquires(&dev->power.lock)
735 {
......
743 if (dev->power.runtime_error)
744 retval = -EINVAL;
745 else if (dev->power.disable_depth == 1 &&
dev->power.is_suspended
746 && dev->power.runtime_status == RPM_ACTIVE)
747 retval = 1;
748 else if (dev->power.disable_depth > 0)
749 retval = -EACCES; //-13 errno.
750 if (retval)
751 goto out;
......
}
I used the kernel changed base on 4.19.90,maybe there is some problem
that I don't know.
The pm_runtime_get_sync is called at stmmac_open at latest kernel .
Maybe only I have problem when call
pm_runtime_get_sync at '.begin' :-(
@drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
3629 int stmmac_open(struct net_device *dev)
3630 {
3631 struct stmmac_priv *priv = netdev_priv(dev);
3632 int mode = priv->plat->phy_interface;
3633 int bfsize = 0;
3634 u32 chan;
3635 int ret;
3636
3637 ret = pm_runtime_get_sync(priv->device);
3638 if (ret < 0) {
3639 pm_runtime_put_noidle(priv->device);
3640 return ret;
3641 }
>> 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,
This go to be:
static int stmmac_ethtool_begin(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
return pm_runtime_resume_and_get(dev);
}
static void stmmac_ethtool_complete(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
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,
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://st-md-mailman.stormreply.com/pipermail/linux-stm32/attachments/20210721/3e052793/attachment.html>
More information about the Linux-stm32
mailing list