On Thu, Nov 6, 2025 at 5:01 AM Aditya Garg <[email protected]> wrote: > > On 06-11-2025 05:47, Jakub Kicinski wrote: > > On Wed, 5 Nov 2025 22:10:23 +0530 Aditya Garg wrote: > >>>> if (err) { > >>>> (void)skb_dequeue_tail(&txq->pending_skbs); > >>>> + mana_unmap_skb(skb, apc); > >>>> netdev_warn(ndev, "Failed to post TX OOB: %d\n", err); > >>> > >>> You have a print right here and in the callee. This condition must > >>> (almost) never happen in practice. It's likely fine to just drop > >>> the packet. > >> > >> The logs placed in callee doesn't covers all the failure scenarios, > >> hence I feel to have this log here with proper status. Maybe I can > >> remove the log in the callee? > > > > I think my point was that since there are logs (per packet!) when the > > condition is hit -- if it did in fact hit with any noticeable frequency > > your users would have complained. So handling the condition gracefully > > and returning BUSY is likely just unnecessary complexity in practice. > > > > In this, we are returning tx_busy when the error reason is -ENOSPC, for > all other errors, skb is dropped. > Is it okay requeue only for -ENOSPC cases or should we drop the skb?
I would avoid NETDEV_TX_BUSY like the plague. Most drivers get it wrong (including mana) Documentation/networking/driver.rst Please drop the packet. > > > The logs themselves I don't care all that much about. Sure, having two > > lines for one error is a bit unclean. > > > >>> Either way -- this should be a separate patch. > >>> > >> Are you suggesting a separate patch altogether or two patch in the same > >> series? > > > > The changes feel related enough to make them a series, but either way > > is fine. > > Regards, > Aditya >
