You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/06/26 09:26:31 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6524: wireless/bcm43xxx: handle sdpcm data only if rx sframe is available
xiaoxiang781216 commented on code in PR #6524:
URL: https://github.com/apache/incubator-nuttx/pull/6524#discussion_r906782823
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdpcm.c:
##########
@@ -376,8 +374,10 @@ int bcmf_sdpcm_sendframe(FAR struct bcmf_dev_s *priv)
DEBUGPANIC();
}
- entry = sbus->tx_queue.tail;
- sframe = container_of(entry, struct bcmf_sdio_frame, list_entry);
+ sframe = list_remove_tail_type(&sbus->tx_queue, struct bcmf_sdio_frame,
Review Comment:
should we remove the head?
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdpcm.c:
##########
@@ -529,15 +510,16 @@ struct bcmf_frame_s *bcmf_sdpcm_get_rx_frame(FAR struct bcmf_dev_s *priv)
DEBUGPANIC();
}
- entry = bcmf_dqueue_pop_tail(&sbus->rx_queue);
+ sframe = list_remove_tail_type(&sbus->rx_queue,
Review Comment:
remove from head
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdpcm.c:
##########
@@ -317,7 +316,7 @@ int bcmf_sdpcm_readframe(FAR struct bcmf_dev_s *priv)
DEBUGPANIC();
}
- bcmf_dqueue_push(&sbus->rx_queue, &sframe->list_entry);
+ list_add_head(&sbus->rx_queue, &sframe->list_entry);
Review Comment:
should we add tail?
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdpcm.c:
##########
@@ -465,7 +447,7 @@ int bcmf_sdpcm_queue_frame(FAR struct bcmf_dev_s *priv,
DEBUGPANIC();
}
- bcmf_dqueue_push(&sbus->tx_queue, &sframe->list_entry);
+ list_add_head(&sbus->tx_queue, &sframe->list_entry);
Review Comment:
add to tail
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.c:
##########
@@ -1065,14 +1063,20 @@ struct bcmf_sdio_frame *bcmf_sdio_allocate_frame(FAR struct bcmf_dev_s *priv,
DEBUGPANIC();
}
- if ((entry = bcmf_dqueue_pop_tail(&sbus->free_queue)) != NULL)
+ list_for_every_entry(&sbus->free_queue, sframe,
Review Comment:
why not remove directly?
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.c:
##########
@@ -1023,17 +1024,17 @@ int bcmf_sdio_thread(int argc, char **argv)
if (sbus->intstatus & I_HMB_FRAME_IND)
{
- do
+ while ((sframe =
+ bcmf_sdio_allocate_frame(priv, false, false)) != NULL)
{
- ret = bcmf_sdpcm_readframe(priv);
- }
- while (ret == OK);
-
- if (ret == -ENODATA)
- {
- /* All frames processed */
-
- sbus->intstatus &= ~I_HMB_FRAME_IND;
+ ret = bcmf_sdpcm_readframe(priv, sframe);
+ if (ret == -ENODATA)
+ {
+ /* All frames processed */
Review Comment:
should we return frame in case of fail
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.c:
##########
@@ -951,6 +951,7 @@ int bcmf_sdio_thread(int argc, char **argv)
((uintptr_t)strtoul(argv[1], NULL, 16));
FAR struct bcmf_sdio_dev_s *sbus = (FAR struct bcmf_sdio_dev_s *)priv->bus;
uint32_t timeout = BCMF_LOWPOWER_TIMEOUT_TICK;
+ struct bcmf_sdio_frame *sframe;
Review Comment:
add FAR
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdpcm.c:
##########
@@ -135,108 +135,74 @@ int bcmf_sdpcm_process_header(FAR struct bcmf_sdio_dev_s *sbus,
* Public Functions
****************************************************************************/
-int bcmf_sdpcm_readframe(FAR struct bcmf_dev_s *priv)
+int bcmf_sdpcm_readframe(FAR struct bcmf_dev_s *priv,
+ FAR struct bcmf_sdio_frame *sframe)
Review Comment:
why not keep the allocation in bcmf_sdpcm_readframe
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.c:
##########
@@ -1077,17 +1078,12 @@ struct bcmf_sdio_frame *bcmf_sdio_allocate_frame(FAR struct bcmf_dev_s *priv,
break;
}
- if (block)
- {
- /* TODO use signaling semaphore */
+ nxsig_usleep(10 * 1000);
- wlinfo("alloc failed %d\n", tx);
- nxsig_usleep(100 * 1000);
- continue;
+ if (!block)
Review Comment:
why move block check after sleep
##########
drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.c:
##########
@@ -1023,17 +1024,17 @@ int bcmf_sdio_thread(int argc, char **argv)
if (sbus->intstatus & I_HMB_FRAME_IND)
{
- do
+ while ((sframe =
+ bcmf_sdio_allocate_frame(priv, false, false)) != NULL)
Review Comment:
why not block here if no buffer avail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org