usb: device_next: remove udc_buf_get_all()

[tmon-nordic]

Function udc_buf_get_all() was intended to be a helper to remove all requests from endpoint FIFO. While for just freeing all queue the it may be argued that there may be some doubtful simplicity argument, merging multiple submitted transfers into one is just enforcing unnecessary complexity on class implementations.

At general level, every submitted (enqueued) request should get corresponding completion (request callback) call. UDC drivers were violating this sensible behavior when dequeuing (cancelling) requests by merging all submitted requests into one.

Remove udc_buf_get_all() and replace all uses with simple loops. For most classes (that submit just one request for an endpoint at a time) this has no functional difference. For classes that implement double buffering this simplifies completion handling.

---

Related discussion: #99439

[mathieuchopstm]

Seems fair. +1 for STM32 driver.

As there appears to be two main patterns repeating, it may be sensible to provide helper functions for them though, e.g.:

static void udc_buf_unref_all(struct udc_ep_config *cfg)
{
  struct net_buf *buf;

  while ((buf = udc_buf_get(cfg))) {
    net_buf_unref(buf);
  }
}

/* this name is pretty bad but i'm out of ideas */
static void udc_drain_queue_with_ep_event(const struct device *dev, struct udc_ep_config *cfg, int status) {
  struct net_buf *buf;

  while ((buf = udc_buf_get(cfg))) {
    udc_submit_ep_event(dev, buf, status);
  }
}

[mathieuchopstm]

STM32 changes LGTM

[jfischer-no]

IIRC it should be reworked after 3d306c1, but yes maybe it is better to remove it.

It looks like there should be helpers for the two common patterns to flush the queue and cancel all transfers.

-	buf = udc_buf_get_all(cfg);
-	if (buf) {
+	while ((buf = udc_buf_get(cfg))) {
		udc_submit_ep_event(dev, buf, -ECONNABORTED);
	}

-	buf = udc_buf_get_all(cfg);
-	if (buf) {
+	while ((buf = udc_buf_get(cfg_out))) {
		net_buf_unref(buf);
	}

while ((buf = udc_buf_get(cfg_out))) is a bit problematic as it is foreseeable that people will get confused. Also, I do not want someone to get triggered, for IMO valid reasons here, by the rule "The result of an assignment operator should not be used". Please add helpers like:

/**
 * @brief Unref all queued UDC request
 *
 * Remove all the request from the endpoint FIFO and unref them.
 *
 * @param[in] cfg Pointer to endpoint configuration
 */
void udc_ep_unref_queued(struct udc_ep_config *const cfg);

void udc_ep_unref_queued(struct udc_ep_config *const cfg)
{
	struct net_buf *buf;

	buf = udc_buf_get(cfg)
	while (buf != NULL) {
		net_buf_unref(buf);
		buf = udc_buf_get(cfg)
	}
}

/**
 * @brief Cancel all queued UDC request
 *
 * Remove all the request from the endpoint FIFO and submit
 * them to higher level.
 *
 * @param[in] cfg Pointer to endpoint configuration
 */
void udc_ep_cancel_queued(struct udc_ep_config *const cfg);

void udc_ep_cancel_queued(struct udc_ep_config *const cfg)
{
	struct net_buf *buf;

	buf = udc_buf_get(cfg)
	while (buf != NULL) {
		udc_submit_ep_event(dev, buf, -ECONNABORTED);
		buf = udc_buf_get(cfg)
	}
}

[tmon-nordic]

while ((buf = udc_buf_get(cfg_out))) is a bit problematic as it is foreseeable that people will get confused

The while (( is rather common in Zephyr codebase, for example:

zephyr/subsys/bluetooth/host/iso.c

Line 469 in 37eb5ad

while ((buf = k_fifo_get(&chan->iso->iso.txq, K_NO_WAIT))) {

zephyr/subsys/bluetooth/host/l2cap.c

Lines 2399 to 2406 in 37eb5ad

	while ((buf = k_fifo_get(&le_chan->tx_queue, K_NO_WAIT))) {
	net_buf_unref(buf);
	}
	/* Remove buffers on the RX queue */
	while ((buf = k_fifo_get(&le_chan->rx_queue, K_NO_WAIT))) {
	net_buf_unref(buf);
	}

zephyr/subsys/bluetooth/host/l2cap.c

Line 281 in 37eb5ad

while ((buf = k_fifo_get(&le_chan->tx_queue, K_NO_WAIT))) {

void udc_ep_unref_queued(struct udc_ep_config *const cfg)

This sounds like a bad advice to me. UDC should not be freeing queued buffers on its own, but rather it should be completing them. The only reason why the requests are currently freed by UDC driver is the broken design for how control transfers are handled. Hopefully this is resolved by #103493

void udc_ep_cancel_queued(struct udc_ep_config *const cfg)

I don't see the point. Maybe if the reason was provided as parameter (instead of hardcoding -ECONNABORTED) there would be some merit to it.

Would you revise your -1 given above argumentation?

[jfischer-no]

The while (( is rather common in Zephyr codebase, for example:

It is up to Bluetooth to fix "The result of an assignment operator should not be used".

Would you revise your -1 given above argumentation?

No, after #103493 is merged, there is still at common pattern in your changes

-	buf = udc_buf_get_all(cfg);
-	if (buf) {
+	while ((buf = udc_buf_get(cfg))) {
		udc_submit_ep_event(dev, buf, -ECONNABORTED);
	}

and that should be moved to something like

/**
 * @brief Cancel all queued UDC request
 *
 * Remove all the request from the endpoint FIFO and submit
 * them to higher level.
 *
 * @param[in] cfg Pointer to endpoint configuration
 */
void udc_ep_cancel_queued(struct udc_ep_config *const cfg);

void udc_ep_cancel_queued(struct udc_ep_config *const cfg)
{
	struct net_buf *buf;

	buf = udc_buf_get(cfg)
	while (buf != NULL) {
		udc_submit_ep_event(dev, buf, -ECONNABORTED);
		buf = udc_buf_get(cfg)
	}
}

This sounds like a bad advice to me. UDC should not be freeing queued buffers on its own, but rather it should be completing them. The only reason why the requests are currently freed by UDC driver is the broken design for how control transfers are handled. Hopefully this is resolved by #103493

One still can find this pattern here:
3a619d9#diff-9636542b03fbd14ba20898a4f5562fc753535a02a6547cb9ac3e346b9a2b6e2dR2182-R2184
and here

zephyr/drivers/usb/udc/udc_common.c

Lines 626 to 628 in ee0cc5a

	for (buf = udc_buf_get(cfg); buf; buf = udc_buf_get(cfg)) {
	net_buf_unref(buf);
	}

zephyr/drivers/usb/udc/udc_common.c

Lines 633 to 635 in ee0cc5a

	for (buf = udc_buf_get(cfg); buf; buf = udc_buf_get(cfg)) {
	net_buf_unref(buf);
	}

Looks like it still makes sense to have helper function void udc_ep_unref_queued().

[tmon-nordic]

It is up to Bluetooth to fix "The result of an assignment operator should not be used".

Please open bug report so they are aware that this is an issue. That having said, I am fairly certain it is not issue and you are completely unreasonably forcing people to use different constructs.

I added void udc_ep_cancel_queued(const struct device *dev, struct udc_ep_config *const cfg);

Looks like it still makes sense to have helper function void udc_ep_unref_queued().

I prefer not to. These should eventually get removed, but it should be considered out-of-scope here.

[josuah]

usb: device_next: Allocate control buffers in stack (244b015) already removed all other uses of udc_buf_get_all() as a side effect.

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.5% Duplication on New Code

See analysis details on SonarQube Cloud