Messages in this thread Patch in this message | | | Date | Mon, 13 May 2024 11:56:17 +0530 | Subject | Re: [PATCH net-next v5] net: ti: icssg_prueth: add TAPRIO offload support | From | MD Danish Anwar <> |
| |
Hi Paolo,
On 02/05/24 5:29 pm, Paolo Abeni wrote: > On Mon, 2024-04-29 at 16:00 +0530, MD Danish Anwar wrote: >> +static int emac_taprio_replace(struct net_device *ndev, >> + struct tc_taprio_qopt_offload *taprio) >> +{ >> + struct prueth_emac *emac = netdev_priv(ndev); >> + struct tc_taprio_qopt_offload *est_new; >> + int ret; >> + >> + if (taprio->cycle_time_extension) { >> + NL_SET_ERR_MSG_MOD(taprio->extack, "Cycle time extension not supported"); >> + return -EOPNOTSUPP; >> + } >> + >> + if (taprio->cycle_time < TAS_MIN_CYCLE_TIME) { >> + NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "cycle_time %llu is less than min supported cycle_time %d", >> + taprio->cycle_time, TAS_MIN_CYCLE_TIME); >> + return -EINVAL; >> + } >> + >> + if (taprio->num_entries > TAS_MAX_CMD_LISTS) { >> + NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "num_entries %lu is more than max supported entries %d", >> + taprio->num_entries, TAS_MAX_CMD_LISTS); >> + return -EINVAL; >> + } >> + >> + if (emac->qos.tas.taprio_admin) >> + devm_kfree(&ndev->dev, emac->qos.tas.taprio_admin); > > it looks like 'qos.tas.taprio_admin' is initialized from > taprio_offload_get(), so it should be free with taprio_offload_free(), > right? >
'qos.tas.taprio_admin' is assigned by "emac->qos.tas.taprio_admin = taprio_offload_get(taprio);". Here I will free it with taprio_offload_free()
>> + >> + est_new = devm_kzalloc(&ndev->dev, >> + struct_size(est_new, entries, taprio->num_entries), >> + GFP_KERNEL); >> + if (!est_new) >> + return -ENOMEM; > > Why are you allocating 'est_new'? it looks like it's not used > anywhere?!? >
Sorry my bad. Forgot to remove est_new. I will remove it.
>> + >> + emac->qos.tas.taprio_admin = taprio_offload_get(taprio); >> + ret = tas_update_oper_list(emac); >> + if (ret) >> + return ret; > > Should the above clear 'taprio_admin' on error, as well? >
Yes here also we should clear taprio_admin and taprio on error. I will add a goto label and clear taprio on both errors.
Below is the diff to address handling of taprio and taprio_admin.
diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.c b/drivers/net/ethernet/ti/icssg/icssg_qos.c index 459463ea6c20..c7cadab0edec 100644 --- a/drivers/net/ethernet/ti/icssg/icssg_qos.c +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.c @@ -210,7 +210,7 @@ static int emac_taprio_replace(struct net_device *ndev, }
if (emac->qos.tas.taprio_admin) - devm_kfree(&ndev->dev, emac->qos.tas.taprio_admin); + taprio_offload_free(emac->qos.tas.taprio_admin);
est_new = devm_kzalloc(&ndev->dev, struct_size(est_new, entries, taprio->num_entries), @@ -221,13 +221,15 @@ static int emac_taprio_replace(struct net_device *ndev, emac->qos.tas.taprio_admin = taprio_offload_get(taprio); ret = tas_update_oper_list(emac); if (ret) - return ret; + goto clear_taprio;
ret = tas_set_state(emac, TAS_STATE_ENABLE); - if (ret) { - emac->qos.tas.taprio_admin = NULL; - taprio_offload_free(taprio); - } + if (ret) + goto clear_taprio; + +clear_taprio: + emac->qos.tas.taprio_admin = NULL; + taprio_offload_free(taprio);
return ret; } Please have a look and let me know if this looks ok to you.
>> > Thanks, > > Paolo >
-- Thanks and Regards, Danish
| |