netdev - Re: [RFC net-next 1/5] net: stmmac: introduce IEEE 802.1Qbv configuration functionalities

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: <20190619030729.GA26784@lunn.ch>
Date:   Wed, 19 Jun 2019 05:07:29 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Voon Weifeng <weifeng.voon@...el.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jose Abreu <joabreu@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Ong Boon Leong <boon.leong.ong@...el.com>
Subject: Re: [RFC net-next 1/5] net: stmmac: introduce IEEE 802.1Qbv
 configuration functionalities

On Wed, Jun 19, 2019 at 05:36:14AM +0800, Voon Weifeng wrote:

Hi Voon

> +static int est_poll_srwo(void *ioaddr)
> +{
> +	/* Poll until the EST GCL Control[SRWO] bit clears.
> +	 * Total wait = 12 x 50ms ~= 0.6s.
> +	 */
> +	unsigned int retries = 12;
> +	unsigned int value;
> +
> +	do {
> +		value = TSN_RD32(ioaddr + MTL_EST_GCL_CTRL);
> +		if (!(value & MTL_EST_GCL_CTRL_SRWO))
> +			return 0;
> +		msleep(50);
> +	} while (--retries);
> +
> +	return -ETIMEDOUT;

Maybe use one of the readx_poll_timeout() macros?

> +static int est_read_gce(void *ioaddr, unsigned int row,
> +			unsigned int *gates, unsigned int *ti_nsec,
> +			unsigned int dbgb, unsigned int dbgm)
> +{
> +	struct tsn_hw_cap *cap = &dw_tsn_hwcap;
> +	unsigned int ti_wid = cap->ti_wid;
> +	unsigned int gates_mask;
> +	unsigned int ti_mask;
> +	unsigned int value;
> +	int ret;
> +
> +	gates_mask = (1 << cap->txqcnt) - 1;
> +	ti_mask = (1 << ti_wid) - 1;
> +
> +	ret = est_read_gcl_config(ioaddr, &value, row, 0, dbgb, dbgm);
> +	if (ret) {
> +		TSN_ERR("Read GCE failed! row=%u\n", row);

It is generally not a good idea to put wrappers around the kernel
print functions. It would be better if all these functions took struct
stmmac_priv *priv rather than ioaddr, so you could then do

	netdev_err(priv->dev, "Read GCE failed! row=%u\n", row);

> +	/* Ensure that HW is not in the midst of GCL transition */
> +	value = TSN_RD32(ioaddr + MTL_EST_CTRL);

Also, don't put wrapper around readl()/writel().

> +	value &= ~MTL_EST_CTRL_SSWL;
> +
> +	/* MTL_EST_CTRL value has been read earlier, if TILS value
> +	 * differs, we update here.
> +	 */
> +	if (tils != dw_tsn_hwtunable[TSN_HWTUNA_TX_EST_TILS]) {
> +		value &= ~MTL_EST_CTRL_TILS;
> +		value |= (tils << MTL_EST_CTRL_TILS_SHIFT);
> +
> +		TSN_WR32(value, ioaddr + MTL_EST_CTRL);
> +		dw_tsn_hwtunable[TSN_HWTUNA_TX_EST_TILS] = tils;
> +	}
> +
> +	return 0;
> +}
> +
> +static int est_set_ov(void *ioaddr,
> +		      const unsigned int *ptov,
> +		      const unsigned int *ctov)
> +{
> +	unsigned int value;
> +
> +	if (!dw_tsn_feat_en[TSN_FEAT_ID_EST])
> +		return -ENOTSUPP;
> +
> +	value = TSN_RD32(ioaddr + MTL_EST_CTRL);
> +	value &= ~MTL_EST_CTRL_SSWL;
> +
> +	if (ptov) {
> +		if (*ptov > EST_PTOV_MAX) {
> +			TSN_WARN("EST: invalid PTOV(%u), max=%u\n",
> +				 *ptov, EST_PTOV_MAX);

It looks like most o the TSN_WARN should actually be netdev_dbg().

   Andrew