Hi Meghana, On 10/02/2025 12:33, Meghana Malladi wrote: > From: Roger Quadros <rogerq@xxxxxxxxxx> > > We have different cases for SWDATA (skb, page, cmd, etc) > so it is better to have a dedicated data structure for that. > We can embed the type field inside the struct and use it > to interpret the data in completion handlers. > > Increase SWDATA size to 48 so we have some room to add > more data if required. > > Signed-off-by: Roger Quadros <rogerq@xxxxxxxxxx> > Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx> > Signed-off-by: Meghana Malladi <m-malladi@xxxxxx> > --- > v1: https://lore.kernel.org/all/20250122124951.3072410-1-m-malladi@xxxxxx/ > > Changes since v1 (v2-v1): None > > drivers/net/ethernet/ti/icssg/icssg_common.c | 47 ++++++++++++------- > drivers/net/ethernet/ti/icssg/icssg_config.h | 2 +- > drivers/net/ethernet/ti/icssg/icssg_prueth.c | 6 +++ > drivers/net/ethernet/ti/icssg/icssg_prueth.h | 18 +++++++ > .../net/ethernet/ti/icssg/icssg_prueth_sr1.c | 4 +- > 5 files changed, 58 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c > index c3c1e2bf461e..a124c5773551 100644 > --- a/drivers/net/ethernet/ti/icssg/icssg_common.c > +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c > @@ -136,12 +136,12 @@ int emac_tx_complete_packets(struct prueth_emac *emac, int chn, > struct net_device *ndev = emac->ndev; > struct cppi5_host_desc_t *desc_tx; > struct netdev_queue *netif_txq; > + struct prueth_swdata *swdata; > struct prueth_tx_chn *tx_chn; > unsigned int total_bytes = 0; > struct sk_buff *skb; > dma_addr_t desc_dma; > int res, num_tx = 0; > - void **swdata; > > tx_chn = &emac->tx_chns[chn]; > > @@ -163,12 +163,18 @@ int emac_tx_complete_packets(struct prueth_emac *emac, int chn, > swdata = cppi5_hdesc_get_swdata(desc_tx); > > /* was this command's TX complete? */ > - if (emac->is_sr1 && *(swdata) == emac->cmd_data) { > + if (emac->is_sr1 && (void *)(swdata) == emac->cmd_data) { > prueth_xmit_free(tx_chn, desc_tx); > continue; > } > > - skb = *(swdata); > + if (swdata->type != PRUETH_SWDATA_SKB) { > + netdev_err(ndev, "tx_complete: invalid swdata type %d\n", swdata->type); > + budget++; > + continue; We are leaking the tx descriptor here. need to call prueth_xmit_free(tx_chn, desc_tx)? > + } > + > + skb = swdata->data.skb; > prueth_xmit_free(tx_chn, desc_tx); > > ndev = skb->dev; > @@ -472,9 +478,9 @@ int prueth_dma_rx_push_mapped(struct prueth_emac *emac, > { > struct net_device *ndev = emac->ndev; > struct cppi5_host_desc_t *desc_rx; > + struct prueth_swdata *swdata; > dma_addr_t desc_dma; > dma_addr_t buf_dma; > - void **swdata; > > buf_dma = page_pool_get_dma_addr(page) + PRUETH_HEADROOM; > desc_rx = k3_cppi_desc_pool_alloc(rx_chn->desc_pool); > @@ -490,7 +496,8 @@ int prueth_dma_rx_push_mapped(struct prueth_emac *emac, > cppi5_hdesc_attach_buf(desc_rx, buf_dma, buf_len, buf_dma, buf_len); > > swdata = cppi5_hdesc_get_swdata(desc_rx); > - *swdata = page; > + swdata->type = PRUETH_SWDATA_PAGE; > + swdata->data.page = page; > > return k3_udma_glue_push_rx_chn(rx_chn->rx_chn, PRUETH_RX_FLOW_DATA, > desc_rx, desc_dma); > @@ -539,11 +546,11 @@ static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id) > u32 buf_dma_len, pkt_len, port_id = 0; > struct net_device *ndev = emac->ndev; > struct cppi5_host_desc_t *desc_rx; > + struct prueth_swdata *swdata; > dma_addr_t desc_dma, buf_dma; > struct page *page, *new_page; > struct page_pool *pool; > struct sk_buff *skb; > - void **swdata; > u32 *psdata; > void *pa; > int ret; > @@ -561,7 +568,11 @@ static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id) > > desc_rx = k3_cppi_desc_pool_dma2virt(rx_chn->desc_pool, desc_dma); > swdata = cppi5_hdesc_get_swdata(desc_rx); > - page = *swdata; > + if (swdata->type != PRUETH_SWDATA_PAGE) { > + netdev_err(ndev, "rx_pkt: invalid swdata->type %d\n", swdata->type); what about freeing the rx descriptor? > + return 0; > + } need new line. > + page = swdata->data.page; > drop new line. > page_pool_dma_sync_for_cpu(pool, page, 0, PAGE_SIZE); > cppi5_hdesc_get_obuf(desc_rx, &buf_dma, &buf_dma_len); > @@ -627,16 +638,18 @@ static void prueth_rx_cleanup(void *data, dma_addr_t desc_dma) > { > struct prueth_rx_chn *rx_chn = data; > struct cppi5_host_desc_t *desc_rx; > + struct prueth_swdata *swdata; > struct page_pool *pool; > struct page *page; > - void **swdata; > > pool = rx_chn->pg_pool; > > desc_rx = k3_cppi_desc_pool_dma2virt(rx_chn->desc_pool, desc_dma); > swdata = cppi5_hdesc_get_swdata(desc_rx); > - page = *swdata; > - page_pool_recycle_direct(pool, page); > + if (swdata->type == PRUETH_SWDATA_PAGE) { > + page = swdata->data.page; > + page_pool_recycle_direct(pool, page); > + } need new line. > k3_cppi_desc_pool_free(rx_chn->desc_pool, desc_rx); > } > > @@ -673,13 +686,13 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev > struct prueth_emac *emac = netdev_priv(ndev); > struct prueth *prueth = emac->prueth; > struct netdev_queue *netif_txq; > + struct prueth_swdata *swdata; > struct prueth_tx_chn *tx_chn; > dma_addr_t desc_dma, buf_dma; > u32 pkt_len, dst_tag_id; > int i, ret = 0, q_idx; > bool in_tx_ts = 0; > int tx_ts_cookie; > - void **swdata; > u32 *epib; > > pkt_len = skb_headlen(skb); > @@ -741,7 +754,8 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev > k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma); > cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len); > swdata = cppi5_hdesc_get_swdata(first_desc); > - *swdata = skb; > + swdata->type = PRUETH_SWDATA_SKB; > + swdata->data.skb = skb; > > /* Handle the case where skb is fragmented in pages */ > cur_desc = first_desc; > @@ -844,15 +858,16 @@ static void prueth_tx_cleanup(void *data, dma_addr_t desc_dma) > { > struct prueth_tx_chn *tx_chn = data; > struct cppi5_host_desc_t *desc_tx; > + struct prueth_swdata *swdata; > struct sk_buff *skb; > - void **swdata; > > desc_tx = k3_cppi_desc_pool_dma2virt(tx_chn->desc_pool, desc_dma); > swdata = cppi5_hdesc_get_swdata(desc_tx); > - skb = *(swdata); > + if (swdata->type == PRUETH_SWDATA_SKB) { > + skb = swdata->data.skb; > + dev_kfree_skb_any(skb); > + } need new line. > prueth_xmit_free(tx_chn, desc_tx); > - > - dev_kfree_skb_any(skb); > } > > irqreturn_t prueth_rx_irq(int irq, void *dev_id) > diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h > index c884e9fa099e..eab84e11d80e 100644 > --- a/drivers/net/ethernet/ti/icssg/icssg_config.h > +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h > @@ -20,7 +20,7 @@ struct icssg_flow_cfg { > > #define PRUETH_PKT_TYPE_CMD 0x10 > #define PRUETH_NAV_PS_DATA_SIZE 16 /* Protocol specific data size */ > -#define PRUETH_NAV_SW_DATA_SIZE 16 /* SW related data size */ > +#define PRUETH_NAV_SW_DATA_SIZE 48 /* SW related data size */ Why do you need 48? I think it should fit in 16. > #define PRUETH_MAX_TX_DESC 512 > #define PRUETH_MAX_RX_DESC 512 > #define PRUETH_MAX_RX_FLOWS 1 /* excluding default flow */ > diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c > index 00ed97860547..e5e4efe485f6 100644 > --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c > +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c > @@ -1522,6 +1522,12 @@ static int prueth_probe(struct platform_device *pdev) > > np = dev->of_node; > > + if (sizeof(struct prueth_swdata) > PRUETH_NAV_SW_DATA_SIZE) { > + dev_err(dev, "insufficient SW_DATA size: %d vs %ld\n", > + PRUETH_NAV_SW_DATA_SIZE, sizeof(struct prueth_swdata)); > + return -ENOMEM; > + } > + Can this be made a build time check instead of runtime? > prueth = devm_kzalloc(dev, sizeof(*prueth), GFP_KERNEL); > if (!prueth) > return -ENOMEM; > diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h > index c7b906de18af..2c8585255b7c 100644 > --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h > +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h > @@ -136,6 +136,24 @@ struct prueth_rx_chn { > struct page_pool *pg_pool; > }; > > +enum prueth_swdata_type { > + PRUETH_SWDATA_INVALID = 0, > + PRUETH_SWDATA_SKB, > + PRUETH_SWDATA_PAGE, > + PRUETH_SWDATA_CMD, > +}; > + > +union prueth_data { > + struct sk_buff *skb; > + struct page *page; > + u32 cmd; > +}; > + > +struct prueth_swdata { > + union prueth_data data; > + enum prueth_swdata_type type; > +}; Can we re-write this like so with type first and union embedded inside? struct prueth_swdata { enum prueth_swdata_type type; union prueth_data { struct sk_buff *skb; struct page *page; u32 cmd; }; }; > + > /* There are 4 Tx DMA channels, but the highest priority is CH3 (thread 3) > * and lower three are lower priority channels or threads. > */ > diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c > index aeeb8a50376b..7bbe0808b3ec 100644 > --- a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c > +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c > @@ -275,10 +275,10 @@ static struct sk_buff *prueth_process_rx_mgm(struct prueth_emac *emac, > struct net_device *ndev = emac->ndev; > struct cppi5_host_desc_t *desc_rx; > struct page *page, *new_page; > + struct prueth_swdata *swdata; > dma_addr_t desc_dma, buf_dma; > u32 buf_dma_len, pkt_len; > struct sk_buff *skb; > - void **swdata; > void *pa; > int ret; > > @@ -301,7 +301,7 @@ static struct sk_buff *prueth_process_rx_mgm(struct prueth_emac *emac, > } > > swdata = cppi5_hdesc_get_swdata(desc_rx); > - page = *swdata; > + page = swdata->data.page; > cppi5_hdesc_get_obuf(desc_rx, &buf_dma, &buf_dma_len); > pkt_len = cppi5_hdesc_get_pktlen(desc_rx); > -- cheers, -roger
