Re: [PATCH v6 01/13] ethdev: allow start/stop from secondary process

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 23 Jul 2025 05:08:04 +0400 (+04)
Ivan Malov <ivan.malov@arknetworks.am> wrote:

>  +
> > +int
> > +ethdev_request(uint16_t port_id, enum ethdev_mp_operation operation,
> > +	       const void *buf, size_t buf_len)
> > +{
> > +	struct rte_mp_msg mp_req = { };
> > +	struct rte_mp_reply mp_reply;
> > +	struct ethdev_mp_request *req;
> > +	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
> > +	int ret;
> > +
> > +	if (sizeof(*req) + buf_len > RTE_MP_MAX_PARAM_LEN) {
> > +		RTE_ETHDEV_LOG_LINE(ERR,
> > +				    "request %u port %u invalid len %zu",
> > +				    operation, port_id, buf_len);
> > +		return -EINVAL;  
> 
> A couple of lines below 'resp->err_value' is used to set 'rte_errno'. Why not
> also set it here then? May be I'm wrong. Or may be set it in (ret < 0) check?
> 
> > +	}
> > +
> > +	strlcpy(mp_req.name, ETHDEV_MP, RTE_MP_MAX_NAME_LEN);
> > +	mp_req.len_param = sizeof(*req) + buf_len;
> > +
> > +	req = (struct ethdev_mp_request *)mp_req.param;
> > +	req->operation = operation;
> > +	req->port_id = port_id;
> > +
> > +	if (buf_len > 0)
> > +		memcpy(req->config, buf, buf_len);
> > +
> > +	ret = rte_mp_request_sync(&mp_req, &mp_reply, &ts);
> > +	if (ret == 0) {
> > +		const struct rte_mp_msg *mp_rep = &mp_reply.msgs[0];
> > +		const struct ethdev_mp_response *resp
> > +			= (const struct ethdev_mp_response *)mp_rep->param;
> > +
> > +		if (resp->err_value == 0)
> > +			ret = 0;
> > +		else
> > +			rte_errno = -resp->err_value;  
> 
> By the looks of it, this check sets 'ret = 0' when it is already 0 and, at the
> same time, when 'resp->err_value' is non-zero, does not override 'ret' with it.
> I apologise in case I've got this wrong.
> 
> > +		free(mp_reply.msgs);
> > +	}
> > +
> > +	if (ret < 0)
> > +		RTE_ETHDEV_LOG_LINE(ERR,
> > +		       "port %up ethdev op %u failed", port_id, operation);
> > +	return ret;
> > +}

Thanks, this code was copy/past from pdump so thats why it was not
perfect in error checking.

> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -1800,54 +1800,61 @@ rte_eth_dev_start(uint16_t port_id)
> >
> > 	if (dev->data->dev_configured == 0) {
> > 		RTE_ETHDEV_LOG_LINE(INFO,
> > -			"Device with port_id=%"PRIu16" is not configured.",
> > -			port_id);
> > +				    "Device with port_id=%"PRIu16" is not configured.",
> > +				    port_id);  
> 
> On the one hand, it's great that an improvement is being offered. On the other
> hand, one may argue this is unrelated to the stated purpose of the patch. And,
> finally, may be placing it 'RTE_ETHDEV_LOG_LINE(INFO, "Device with...' would
> also work and not trigger a checkpatch warning, but I may be wrong.
> 

Thanks this was from editor doing indent, not needed will remove.