[PATCH v7 01/21] net/cpfl: support device initialization

[Ferruh Yigit]

On 2/27/2023 3:45 PM, Thomas Monjalon wrote:
> 27/02/2023 14:46, Ferruh Yigit:
>> On 2/16/2023 12:29 AM, Mingxia Liu wrote:
>>> +static int
>>> +cpfl_dev_configure(struct rte_eth_dev *dev)
>>> +{
>>> +	struct rte_eth_conf *conf = &dev->data->dev_conf;
>>> +
>>> +	if (conf->link_speeds & RTE_ETH_LINK_SPEED_FIXED) {
>>> +		PMD_INIT_LOG(ERR, "Setting link speed is not supported");
>>> +		return -ENOTSUP;
>>> +	}
>>> +
>>> +	if (conf->txmode.mq_mode != RTE_ETH_MQ_TX_NONE) {
>>> +		PMD_INIT_LOG(ERR, "Multi-queue TX mode %d is not supported",
>>> +			     conf->txmode.mq_mode);
>>> +		return -ENOTSUP;
>>> +	}
>>> +
>>> +	if (conf->lpbk_mode != 0) {
>>> +		PMD_INIT_LOG(ERR, "Loopback operation mode %d is not supported",
>>> +			     conf->lpbk_mode);
>>> +		return -ENOTSUP;
>>> +	}
>>> +
>>> +	if (conf->dcb_capability_en != 0) {
>>> +		PMD_INIT_LOG(ERR, "Priority Flow Control(PFC) if not supported");
>>> +		return -ENOTSUP;
>>> +	}
>>> +
>>> +	if (conf->intr_conf.lsc != 0) {
>>> +		PMD_INIT_LOG(ERR, "LSC interrupt is not supported");
>>> +		return -ENOTSUP;
>>> +	}
>>> +
>>> +	if (conf->intr_conf.rxq != 0) {
>>> +		PMD_INIT_LOG(ERR, "RXQ interrupt is not supported");
>>> +		return -ENOTSUP;
>>> +	}
>>> +
>>> +	if (conf->intr_conf.rmv != 0) {
>>> +		PMD_INIT_LOG(ERR, "RMV interrupt is not supported");
>>> +		return -ENOTSUP;
>>> +	}
>>> +
>>> +	return 0;
>>
>> This is '.dev_configure()' dev ops of a driver, there is nothing wrong
>> with the function but it is a good example to highlight a point.
>>
>>
>> 'rte_eth_dev_configure()' can fail from various reasons, what can an
>> application do in this case?
>> It is not clear why configuration failed, there is no way to figure out
>> failed config option dynamically.
> 
> There are some capabilities to read before calling "configure".
> 

Yes, but there are some PMD specific cases as well, like above
SPEED_FIXED is not supported. How an app can manage this?

Mainly "struct rte_eth_dev_info" is used for capabilities (although it
is a mixed bag), that is not symmetric with config/setup functions, I
mean for a config/setup function there is no clear matching capability
struct/function.

>> Application developer can read the log and find out what caused the
>> failure, but what can do next? Put a conditional check for the
>> particular device, assuming application supports multiple devices,
>> before configuration?
> 
> Which failures cannot be guessed with capability flags?
> 

At least for above sample as far as I can see some capabilities are missing:
- txmode.mq_mode
- rxmode.mq_mode
- lpbk_mode
- intr_conf.rxq

We can go through all list to detect gaps if we plan to have an action.

>> I think we need better error value, to help application detect what went
>> wrong and adapt dynamically, perhaps a bitmask of errors one per each
>> config option, what do you think?
> 
> I am not sure we can change such an old API.
> 

Yes that is hard, but if we keep the return value negative, that can
still be backward compatible.

Or API can keep the interface same but set a global 'reason' variable,
similar to 'errno', so optionally new application code can get it with a
new API and investigate it.

>> And I think this is another reason why we should not make a single API
>> too overloaded and complex.
> 
> Right, and I would support a work to have some of those "configure" features
> available as small functions.
> 

If there is enough appetite we can put something to deprecation notice
for next ABI release.