linux-kernel - Re: [PATCH net-next v4] net: ti: icssg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7456cbc2-7ea1-b68c-ce75-fa4b5392ec8c@ti.com>
Date:   Wed, 11 Oct 2023 12:40:17 +0530
From:   MD Danish Anwar <danishanwar@...com>
To:     Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Andrew Lunn <andrew@...n.ch>,
        Roger Quadros <rogerq@...nel.org>,
        Vignesh Raghavendra <vigneshr@...com>,
        Richard Cochran <richardcochran@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>, <vladimir.oltean@....com>,
        Simon Horman <horms@...nel.org>
CC:     <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <srk@...com>, <r-gunasekaran@...com>,
        <linux-arm-kernel@...ts.infradead.org>,
        Roger Quadros <rogerq@...com>
Subject: Re: [PATCH net-next v4] net: ti: icssg_prueth: add TAPRIO offload
 support

On 07/10/23 04:01, Vinicius Costa Gomes wrote:
> MD Danish Anwar <danishanwar@...com> writes:
> 
>> From: Roger Quadros <rogerq@...com>
>>
>> ICSSG dual-emac f/w supports Enhanced Scheduled Traffic (EST – defined
>> in P802.1Qbv/D2.2 that later got included in IEEE 802.1Q-2018)
>> configuration. EST allows express queue traffic to be scheduled
>> (placed) on the wire at specific repeatable time intervals. In
>> Linux kernel, EST configuration is done through tc command and
>> the taprio scheduler in the net core implements a software only
>> scheduler (SCH_TAPRIO). If the NIC is capable of EST configuration,
>> user indicate "flag 2" in the command which is then parsed by
>> taprio scheduler in net core and indicate that the command is to
>> be offloaded to h/w. taprio then offloads the command to the
>> driver by calling ndo_setup_tc() ndo ops. This patch implements
>> ndo_setup_tc() to offload EST configuration to ICSSG.
>>
>> Signed-off-by: Roger Quadros <rogerq@...com>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@...com>
>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>> ---
>> Cc: Roger Quadros <rogerq@...com>
>> Cc: Andrew Lunn <andrew@...n.ch>
>> Cc: Vinicius Costa Gomes <vinicius.gomes@...el.com>
>> Cc: Jakub Kicinski <kuba@...nel.org>
>>

[ ... ]

>> +
>> +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, idx;
>> +
>> +	if (taprio->cycle_time_extension) {
>> +		netdev_err(ndev, "Failed to set cycle time extension");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	if (taprio->num_entries == 0 ||
>> +	    taprio->num_entries > TAS_MAX_CMD_LISTS) {
>> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "unsupported num_entries %ld in taprio config\n",
>> +				       taprio->num_entries);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* If any time_interval is 0 in between the list, then exit */
>> +	for (idx = 0; idx < taprio->num_entries; idx++) {
>> +		if (taprio->entries[idx].interval == 0) {
>> +			NL_SET_ERR_MSG_MOD(taprio->extack, "0 interval in taprio config not supported\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	if (emac->qos.tas.taprio_admin)
>> +		devm_kfree(&ndev->dev, emac->qos.tas.taprio_admin);
>> +
>> +	est_new = devm_kzalloc(&ndev->dev,
>> +			       struct_size(est_new, entries, taprio->num_entries),
>> +			       GFP_KERNEL);
>> +	if (!est_new)
>> +		return -ENOMEM;
>> +
>> +	emac_cp_taprio(taprio, est_new);
>> +	emac->qos.tas.taprio_admin = est_new;
>> +	ret = tas_update_oper_list(emac);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret =  tas_set_state(emac, TAS_STATE_ENABLE);
> 
> The double space is still here...
> 

Sorry my bad. I'll fix this in next revision.

>> +	if (ret)
>> +		devm_kfree(&ndev->dev, est_new);
> 
> ... as is the free'ing of 'est_new' while 'taprio_admin' still points to it.
> 

I will add below line here to assign 'taprio_admin' to NULL once
'est_new' is freed.

		emac->qos.tas.taprio_admin = NULL;

I will send the next revision with these two fixes. Please let me know
if this is OK or if something else is also needed.

>> +
>> +	return ret;
>> +}
>> +

[ ... ]

>> +
>> +void icssg_qos_tas_init(struct net_device *ndev);
>> +int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>> +			   void *type_data);
>> +#endif /* __NET_TI_ICSSG_QOS_H */
>> -- 
>> 2.34.1
>>
> 

-- 
Thanks and Regards,
Danish