On Wed, 23 Jul 2025 05:08:04 +0400 (+04)
Ivan Malov <ivan.ma...@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.
