Re: [dpdk-dev] [PATCH 1/2] eal: expose device states in rte device

[Jeff Guo <jia.guo@intel.com>]

On 11/8/2018 3:32 AM, Stephen Hemminger wrote:
> On Wed,  7 Nov 2018 17:41:30 +0800
> Jeff Guo <jia.guo@intel.com> wrote:
>
>> Since the hotplug API and device event API have make public, so in order
>> to let applications or driver deal with device directly, it would be
>> helpful if the states of device could be exposed, especially for hotplug
>> process.
>>
>> This patch will add some devices states in rte device structure
>> to recode the device's current status, such as “RTE_DEV_UNUSED”,
>> “RTE_DEV_ATTACHED” and “RTE_DEV_REMOVED”.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> Looks good.
>
> I did spot a couple of minor nits you might want to address if
> resending this.
>
>
>> ---
>>   lib/librte_eal/common/include/rte_dev.h | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
>> index cd6c187..1bab0dd 100644
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -151,6 +151,18 @@ struct rte_driver {
>>   #define RTE_DEV_NAME_MAX_LEN 64
>>   
>>   /**
>> + * Possible states of an rte devcie.
>> + */
> s/devcie/device/


got it.


>> +enum rte_dev_state {
>> +	/** Device is unused before being hotplug add. */
>> +	RTE_DEV_UNUSED = 0,
>> +	/** Device is attached when allocated in probing. */
>> +	RTE_DEV_ATTACHED,
>> +	/** Device is in removed state when plug-out is detected. */
>> +	RTE_DEV_REMOVED,
>> +};
>> +
>> +/**
>>    * A structure describing a generic device.
>>    */
>>   struct rte_device {
>> @@ -160,6 +172,7 @@ struct rte_device {
>>   	const struct rte_bus *bus;    /**< Bus handle assigned on scan */
>>   	int numa_node;                /**< NUMA node connection */
>>   	struct rte_devargs *devargs;  /**< Arguments for latest probing */
>> +	enum rte_dev_state state; /**< Flag indicating the device state */
> Why not align comment with other fields here?


ok, maybe i could split the line if considerate with the line number limit.


>>   };
>>   
>>   /**