[dpdk-dev] [PATCH] ethdev: fix device state on close

[Shahaf Shuler]

Wednesday, August 16, 2017 6:26 PM, Gaëtan Rivet:
> > Even though it is reasonable for driver to call the
> rte_eth_dev_port_release, I still think the ethdev layer should protect from
> such bad behavior from the application side.
> > It is more robust than counting on the different PMD to implement such
> release.
> >
> 
> The ethdev layer does so in the rte_eth_dev_detach function, which is
> currently the only way to detach a device from the ethdev level.
> 
> `rte_eth_dev_detach` setting the device state seems to me to be a crutch
> against badly implemented drivers. If I was nitpicky I would actually remove it
> and ideally everyone should enforce the call of rte_eth_dev_release_port
> from device removal functions when reviewing PMD implementations.
> 
> The hotplug API is available to applications and the only way to have
> consistent devices states after calling rte_eal_hotplug_remove is to have
> drivers using a hook in the ethdev layer allowing to clean-up resources upon
> release. While it is trivial in its current state, such entry-point in the ethdev
> layer will be useful and should be kept and enforced IMO.
> 
> I'm thinking about the 16-bit portid scheduled for v17.11, which implies an
> arbitrary number of port available. This would imply a dynamic allocation of
> rte_eth_devices, which would *need* such release hook to be available.
> Well the API should not be designed from conjectures or speculations of
> course, but I think it should be useful and there is no reason to remove it.
> 
> Going further, I find it dangerous to have two points where the port is
> semantically released (device state set to UNUSED). If the API of the port
> release changes, we now have two points where we need to enforce the
> changes. While trivial concerning an enum, it could become more complex /
> dangerous if this veered toward memory management.

Those are valid concerns, and you convinced me the RTE_ETH_DEV_UNUSED cannot be set after device close.

I still think the ethdev layer missing protection against driver calls (other than detach) following a device close. The API not allows, but the ethdev should enforce it.
Considering some driver memset to 0 their control structs after device close, with no protection such bad calls might lead to segfault, which is not what we want even under bad behavior. 

One could suggest to protect it inside the driver by replacing the vtable of the driver ops, however it will introduce a lot of duplicated code which can easily be avoided if ethdev would not pass such
Calls after device close.

So, how about adding another state which will indicate the device is closed, cannot be used, yet still attached?