On 2/26/2025 3:59 PM, Dan Carpenter wrote:
On Mon, Feb 24, 2025 at 04: 31: 01PM +0530, Meghana Malladi wrote: >
From: Roger Quadros <rogerq@ kernel. org> > > We have different cases
for SWDATA (skb, page, cmd, etc) > so it is better to have a dedicated
data structure for
ZjQcmQRYFpfptBannerStart
This message was sent from outside of Texas Instruments.
Do not click links or open attachments unless you recognize the source
of this email and know the content is safe.
Report Suspicious
<https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!
uldqnT1FPMdbygXOdhMC_1iqujTzdK2ZTrOjiy9hZrrhggm_lCduTFVqMq5QnhhjXmDeSX6KQhxE9U6zpHeghHYcqYQiiHpSSbljHpY$>
ZjQcmQRYFpfptBannerEnd
On Mon, Feb 24, 2025 at 04:31:01PM +0530, 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.
What is the "SWDATA size"? Where is that specified? Is
that a variable or a define or the size of a struct or
what?
Will be removing this line, since "increase SWDATA size" change is not
applicable anymore. It is a macro PRUETH_NAV_SW_DATA_SIZE used to define
the size held for swdata.
Signed-off-by: Roger Quadros <rogerq@xxxxxxxxxx>
Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx>
Signed-off-by: Meghana Malladi <m-malladi@xxxxxx>
---
Changes since v2 (v3-v2):
- Fix leaking tx descriptor in emac_tx_complete_packets()
- Free rx descriptor if swdata type is not page in emac_rx_packet()
- Revert back the size of PRUETH_NAV_SW_DATA_SIZE
- Use build time check for prueth_swdata size
- re-write prueth_swdata to have enum type as first member in the struct
and prueth_data union embedded in the struct
All the above changes have been suggested by Roger Quadros <rogerq@xxxxxxxxxx>
drivers/net/ethernet/ti/icssg/icssg_common.c | 52 +++++++++++++------
drivers/net/ethernet/ti/icssg/icssg_prueth.c | 3 ++
drivers/net/ethernet/ti/icssg/icssg_prueth.h | 16 ++++++
.../net/ethernet/ti/icssg/icssg_prueth_sr1.c | 4 +-
4 files changed, 57 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index acbb79ad8b0c..01eeabe83eff 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,19 @@ 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) {
I don't think this conversion is correct. You still need to say:
if (emac->is_sr1 && swdata->data.something == emac->cmd_data) {
Where something is probably "page".
Yes you are right. This needs to be changes to:
if (emac->is_sr1 && swdata->data.cmd== emac->cmd_data) {
This issue can be addressed more cleanly with the fix Roger mentioned
here:
https://lore.kernel.org/all/3d3d180a-12b7-4bee-8172-700f0dae2439@xxxxxxxxxx/
regards,
dan carpenter
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);
+ prueth_xmit_free(tx_chn, desc_tx);
+ budget++;
+ continue;
+ }
