Thread (10 messages) 10 messages, 5 authors, 2025-07-14

Re: [PATCH net-next v22 2/2] mctp pcc: Implement MCTP over PCC Transport

From: Adam Young <hidden>
Date: 2025-07-11 20:03:59
Also in: lkml

All Changes are accepted.  I have attempted to answer your questions here.

quoted
+static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
+{
+       struct mctp_pcc_ndev *mctp_pcc_ndev;
+       struct pcc_header pcc_header;
+       struct mctp_skb_cb *cb;
+       struct sk_buff *skb;
+
+       mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
+       if (!buffer) {
+               dev_dstats_rx_dropped(mctp_pcc_ndev->mdev.dev);
+               return;
+       }
Mainly out of curiosity: how does this happen? How do we get a
completion where there is no original buffer?
If the sk_buff allocation fails, the logic falls back to the old code, 
which passes on a null buffer. There is logic there with notifying the 
sender that I don't want to skip or modify.

See the other patch, in the change to the irq handler.
I figure we're restricted to what the mailbox API provides, but is there
any way we can access the skb through a pointer, rather than having to
dig through these lists?

I think the issue is that the mbox API is using the void * buffer as
both the data to transfer, and the callback context, so we can't stash
useful context across the completion?
Correct, the SK_buff is a structure  that points to a buffer, and what 
gets to the send_data function is the buffer itself. That buffer has no 
pointer back to the sk_buff.

quoted
+
+       rc = mbox_send_message(mpnd->outbox.chan->mchan, skb->data);
+
+       if (rc < 0) {
+               pr_info("%s fail, rc = %d", __func__, rc);
+               return NETDEV_TX_BUSY;
+       }
What happens on mbox_send_message failure? The skb will still be present
in the outbox.packets queue - I assume we don't see a completion
callback in that case, and so the skb will be in the outbox.packets
queue forever?
Are you sure you want to return NETDEV_TX_BUSY here?

Is there any situation where the mbox_send_message will continually
fail? Should we ratelimit the pr_info() message there (and regardless,
better to use one of netdev_info / netdev_warn / etc functions, since we
are dealing with netdevs here).
The fail will happen if the ring buffer is full, so, yes, it makes sense 
not to queue the packet. I can move that to after the mbox_send_message.

The NETDEV_TX_BUSY is correct, as it means resend the packet, and we 
don't have any reference to it.

The only other failure path on the mbox_send_message code is due to 
timeouts, which we do not use from this driver. That is a recent change, 
and that was the case I was handling before.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help