21/04/2017 04:27, Yuanhan Liu: > On Thu, Apr 20, 2017 at 09:17:24AM +0000, Zhao1, Wei wrote: > > > > > Please explain exactly the responsibility of this function, and how > > > > > it is different from calling stop/configure/start. > > > > > > > > In this reset feature, reset function can do the calling > > > > stop/configure/start process, but also It can also do some restore > > > > work for the port, for example, it can restore the added parameters > > > > of > > > > > > vlan, mac_addrs, promisc_unicast_enabled falg and > > > promisc_multicast_enabled flag. > > > > > > > Maybe , I should add this explanation in the patch comments or > > > > function > > > > > > comments? > > > > > > I'm curious why we have to do save & restore for a reset operation. > > > Why some configures have to be saved and restored? Doesn't "reset" > > > literally means reset everything? > > > > Users maybe do not want to do a second configuration operation to waste > > time after reset which lost all previous configuration. But he still want > > these configuration valid after reset. > > So, save & restore can help them to save this process time and effort. > > > > > Even though, how do you tell what kind of configures need be restored > > > and > > > what should not? Again, even though, will all PMDs supports restoring > > > the > > > same set of configurations? > > > > Yes, this is hard to say what may be need and what may be not for user. > > Now, the kinds of supported is list in patch set comment. And only i40e > > NIC support this feature. > Why it's the configurations listed in patch 2? Because they are requested > by customers? > > Is that all could be saved? If not, are you going to save & restore all > possible configurations? > > Assuming the configurations saved & restored may differ from different > PMD drivers, how could you keep the consistency then? And judging that the > application has no idea about the difference (yet it knows nothing about > what kind of configurations will be saved), how could the application know > that some configurations are not saved & restored by the driver that it > has to do re-configuration by itself? > > > > While looking at your reset implementation for i40e, it looks more > > > complex > > > than necessary: just thinking we have to call "xxx_queue_setup" > > > for all PMDs. > > > > > > I'm thinking a simple hardware reset might be enough? > > > > > > /* literally reset the hardware: reset everything */ > > > rte_eth_reset(port) > > > { > > > > > > eth_dev->ops->reset(); > > > > > > } > > > > You mean just do a reset and do not restore any configuration? > > That may not meet the need for this feature from customer? > > Right, I'm just aware of the configuration might be done by PF (but not > only by the application), that the VF port may be not aware of those > configurations. So the save & restore is needed. I don't quite like > how it is done in this patch set though. I also don't think the API is > well named: as said, reset should literally reset everything. > > We may need think how to do it properly. > > Thomas, Konstantin, what do you guys think of it?
I have the same concerns. I think we should better document the current status of start/stop and which configuration parameters are lost or saved.