Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <94308180-5eb6-5327-4188-151c57944741@inbox.ru>
Date:   Thu, 16 Jun 2022 10:05:04 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v1 net-next] net: dsa: felix: update base time of
 time-aware shaper when adjusting PTP time
Content-Language: en-US
To:     Xiaoliang Yang <xiaoliang.yang_1@nxp.com>, davem@davemloft.net,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     allan.nielsen@microchip.com, joergen.andreasen@microchip.com,
        horatiu.vultur@microchip.com, UNGLinuxDriver@microchip.com,
        alexandre.belloni@bootlin.com, f.fainelli@gmail.com,
        vivien.didelot@gmail.com, pabeni@redhat.com, kuba@kernel.org,
        edumazet@google.com, andrew@lunn.ch, alexandru.marginean@nxp.com,
        claudiu.manoil@nxp.com, vladimir.oltean@nxp.com,
        leoyang.li@nxp.com, yangbo.lu@nxp.com
References: <20220616064641.11905-1-xiaoliang.yang_1@nxp.com>
From:   Maxim Kochetkov <fido_max@inbox.ru>
In-Reply-To: <20220616064641.11905-1-xiaoliang.yang_1@nxp.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 16.06.2022 09:46, Xiaoliang Yang wrote:
> When adjusting the PTP clock, the base time of the TAS configuration
> will become unreliable. We need reset the TAS configuration by using a
> new base time.
> 
> For example, if the driver gets a base time 0 of Qbv configuration from
> user, and current time is 20000. The driver will set the TAS base time
> to be 20000. After the PTP clock adjustment, the current time becomes
> 10000. If the TAS base time is still 20000, it will be a future time,
> and TAS entry list will stop running. Another example, if the current
> time becomes to be 10000000 after PTP clock adjust, a large time offset
> can cause the hardware to hang.
> 
> This patch introduces a tas_clock_adjust() function to reset the TAS
> module by using a new base time after the PTP clock adjustment. This can
> avoid issues above.
> 
> Due to PTP clock adjustment can occur at any time, it may conflict with
> the TAS configuration. We introduce a new TAS lock to serialize the
> access to the TAS registers.
> 
> Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
> ---
>   drivers/net/dsa/ocelot/felix_vsc9959.c | 83 ++++++++++++++++++++++++--
>   drivers/net/ethernet/mscc/ocelot.c     |  1 +
>   drivers/net/ethernet/mscc/ocelot_ptp.c | 12 +++-
>   include/soc/mscc/ocelot.h              |  7 +++
>   4 files changed, 95 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 570d0204b7be..dd9085ae0922 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -1196,10 +1196,13 @@ static void vsc9959_tas_gcl_set(struct ocelot *ocelot, const u32 gcl_ix,
>   static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
>   				    struct tc_taprio_qopt_offload *taprio)
>   {
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
>   	struct timespec64 base_ts;
>   	int ret, i;
>   	u32 val;
>   
> +	mutex_lock(&ocelot->tas_lock);
> +
>   	if (!taprio->enable) {
>   		ocelot_rmw_rix(ocelot,
>   			       QSYS_TAG_CONFIG_INIT_GATE_STATE(0xFF),
> @@ -1207,15 +1210,20 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
>   			       QSYS_TAG_CONFIG_INIT_GATE_STATE_M,
>   			       QSYS_TAG_CONFIG, port);
>   
> +		mutex_unlock(&ocelot->tas_lock);
>   		return 0;
>   	}
>   
>   	if (taprio->cycle_time > NSEC_PER_SEC ||
> -	    taprio->cycle_time_extension >= NSEC_PER_SEC)
> -		return -EINVAL;
> +	    taprio->cycle_time_extension >= NSEC_PER_SEC) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
>   
> -	if (taprio->num_entries > VSC9959_TAS_GCL_ENTRY_MAX)
> -		return -ERANGE;
> +	if (taprio->num_entries > VSC9959_TAS_GCL_ENTRY_MAX) {
> +		ret = -ERANGE;
> +		goto err;
> +	}
>   
>   	/* Enable guard band. The switch will schedule frames without taking
>   	 * their length into account. Thus we'll always need to enable the
> @@ -1236,8 +1244,10 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
>   	 * config is pending, need reset the TAS module
>   	 */
>   	val = ocelot_read(ocelot, QSYS_PARAM_STATUS_REG_8);
> -	if (val & QSYS_PARAM_STATUS_REG_8_CONFIG_PENDING)
> -		return  -EBUSY;
> +	if (val & QSYS_PARAM_STATUS_REG_8_CONFIG_PENDING) {
> +		ret = -EBUSY;
> +		goto err;
> +	}
>   
>   	ocelot_rmw_rix(ocelot,
>   		       QSYS_TAG_CONFIG_ENABLE |
> @@ -1248,6 +1258,8 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
>   		       QSYS_TAG_CONFIG_SCH_TRAFFIC_QUEUES_M,
>   		       QSYS_TAG_CONFIG, port);
>   
> +	ocelot_port->base_time = taprio->base_time;
> +
>   	vsc9959_new_base_time(ocelot, taprio->base_time,
>   			      taprio->cycle_time, &base_ts);
>   	ocelot_write(ocelot, base_ts.tv_nsec, QSYS_PARAM_CFG_REG_1);
> @@ -1271,9 +1283,67 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
>   				 !(val & QSYS_TAS_PARAM_CFG_CTRL_CONFIG_CHANGE),
>   				 10, 100000);
>   
> +err:
> +	mutex_unlock(&ocelot->tas_lock);
> +
>   	return ret;
>   }
>   
> +static void vsc9959_tas_clock_adjust(struct ocelot *ocelot)
> +{
> +	struct ocelot_port *ocelot_port;
> +	struct timespec64 base_ts;
> +	u64 cycletime;
> +	int port;
> +	u32 val;
> +
> +	mutex_lock(&ocelot->tas_lock);
> +
> +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> +		val = ocelot_read_rix(ocelot, QSYS_TAG_CONFIG, port);
> +		if (!(val & QSYS_TAG_CONFIG_ENABLE))
> +			continue;
> +
> +		ocelot_rmw(ocelot,
> +			   QSYS_TAS_PARAM_CFG_CTRL_PORT_NUM(port),
> +			   QSYS_TAS_PARAM_CFG_CTRL_PORT_NUM_M,
> +			   QSYS_TAS_PARAM_CFG_CTRL);
> +
> +		ocelot_rmw_rix(ocelot,
> +			       QSYS_TAG_CONFIG_INIT_GATE_STATE(0xFF),
> +			       QSYS_TAG_CONFIG_ENABLE |
> +			       QSYS_TAG_CONFIG_INIT_GATE_STATE_M,
> +			       QSYS_TAG_CONFIG, port);
> +
> +		cycletime = ocelot_read(ocelot, QSYS_PARAM_CFG_REG_4);
> +		ocelot_port = ocelot->ports[port];
> +
> +		vsc9959_new_base_time(ocelot, ocelot_port->base_time,
> +				      cycletime, &base_ts);
> +
> +		ocelot_write(ocelot, base_ts.tv_nsec, QSYS_PARAM_CFG_REG_1);
> +		ocelot_write(ocelot, lower_32_bits(base_ts.tv_sec),
> +			     QSYS_PARAM_CFG_REG_2);
> +		val = upper_32_bits(base_ts.tv_sec);
> +		ocelot_rmw(ocelot,
> +			   QSYS_PARAM_CFG_REG_3_BASE_TIME_SEC_MSB(val),
> +			   QSYS_PARAM_CFG_REG_3_BASE_TIME_SEC_MSB_M,
> +			   QSYS_PARAM_CFG_REG_3);
> +
> +		ocelot_rmw(ocelot, QSYS_TAS_PARAM_CFG_CTRL_CONFIG_CHANGE,
> +			   QSYS_TAS_PARAM_CFG_CTRL_CONFIG_CHANGE,
> +			   QSYS_TAS_PARAM_CFG_CTRL);
> +
> +		ocelot_rmw_rix(ocelot,
> +			       QSYS_TAG_CONFIG_INIT_GATE_STATE(0xFF) |
> +			       QSYS_TAG_CONFIG_ENABLE,
> +			       QSYS_TAG_CONFIG_ENABLE |
> +			       QSYS_TAG_CONFIG_INIT_GATE_STATE_M,
> +			       QSYS_TAG_CONFIG, port);
> +	}
> +	mutex_unlock(&ocelot->tas_lock);
> +}
> +
>   static int vsc9959_qos_port_cbs_set(struct dsa_switch *ds, int port,
>   				    struct tc_cbs_qopt_offload *cbs_qopt)
>   {
> @@ -2210,6 +2280,7 @@ static const struct ocelot_ops vsc9959_ops = {
>   	.psfp_filter_del	= vsc9959_psfp_filter_del,
>   	.psfp_stats_get		= vsc9959_psfp_stats_get,
>   	.cut_through_fwd	= vsc9959_cut_through_fwd,
> +	.tas_clock_adjust	= vsc9959_tas_clock_adjust,
>   };
>   
>   static const struct felix_info felix_info_vsc9959 = {
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 8da7e25a47c9..d4649e4ee0e7 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -3367,6 +3367,7 @@ int ocelot_init(struct ocelot *ocelot)
>   	mutex_init(&ocelot->ptp_lock);
>   	mutex_init(&ocelot->mact_lock);
>   	mutex_init(&ocelot->fwd_domain_lock);
> +	mutex_init(&ocelot->tas_lock);
>   	spin_lock_init(&ocelot->ptp_clock_lock);
>   	spin_lock_init(&ocelot->ts_id_lock);
>   	snprintf(queue_name, sizeof(queue_name), "%s-stats",
> diff --git a/drivers/net/ethernet/mscc/ocelot_ptp.c b/drivers/net/ethernet/mscc/ocelot_ptp.c
> index 87ad2137ba06..522fdc38d4d0 100644
> --- a/drivers/net/ethernet/mscc/ocelot_ptp.c
> +++ b/drivers/net/ethernet/mscc/ocelot_ptp.c
> @@ -72,15 +72,19 @@ int ocelot_ptp_settime64(struct ptp_clock_info *ptp,
>   	ocelot_write_rix(ocelot, val, PTP_PIN_CFG, TOD_ACC_PIN);
>   
>   	spin_unlock_irqrestore(&ocelot->ptp_clock_lock, flags);
> +
> +	if (ocelot->ops->tas_clock_adjust)
> +		ocelot->ops->tas_clock_adjust(ocelot);
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL(ocelot_ptp_settime64);
>   
>   int ocelot_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>   {
> +	struct ocelot *ocelot = container_of(ptp, struct ocelot, ptp_info);
> +
>   	if (delta > -(NSEC_PER_SEC / 2) && delta < (NSEC_PER_SEC / 2)) {
> -		struct ocelot *ocelot = container_of(ptp, struct ocelot,
> -						     ptp_info);
>   		unsigned long flags;
>   		u32 val;
>   
> @@ -117,6 +121,10 @@ int ocelot_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>   
>   		ocelot_ptp_settime64(ptp, &ts);
>   	}
> +
> +	if (ocelot->ops->tas_clock_adjust)
> +		ocelot->ops->tas_clock_adjust(ocelot);
> +

Here tas_clock_adjust() may be called twice (first from 
ocelot_ptp_settime64() )

>   	return 0;
>   }
>   EXPORT_SYMBOL(ocelot_ptp_adjtime);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 5f88385a7748..3737570116c3 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -575,6 +575,7 @@ struct ocelot_ops {
>   	int (*psfp_stats_get)(struct ocelot *ocelot, struct flow_cls_offload *f,
>   			      struct flow_stats *stats);
>   	void (*cut_through_fwd)(struct ocelot *ocelot);
> +	void (*tas_clock_adjust)(struct ocelot *ocelot);
>   };
>   
>   struct ocelot_vcap_policer {
> @@ -691,6 +692,9 @@ struct ocelot_port {
>   	int				bridge_num;
>   
>   	int				speed;
> +
> +	/* Store the AdminBaseTime of EST fetched from userspace. */
> +	s64				base_time;
>   };
>   
>   struct ocelot {
> @@ -757,6 +761,9 @@ struct ocelot {
>   	/* Lock for serializing forwarding domain changes */
>   	struct mutex			fwd_domain_lock;
>   
> +	/* Lock for serializing Time-Aware Shaper changes */
> +	struct mutex			tas_lock;
> +
Why do we need extra mutex? ocelot_ptp_settime64() and 
ocelot_ptp_adjtime() already uses 
spin_lock_irqsave(&ocelot->ptp_clock_lock, flags)

>   	struct workqueue_struct		*owq;
>   
>   	u8				ptp:1;