回复: [EXT] Re: [PATCH v1 net-next] net: dsa: felix: update base time of time-aware shaper when adjusting PTP time

From: Xiaoliang Yang
Date: Thu Jun 16 2022 - 04:16:41 EST

Next message: Vlastimil Babka: "Re: [PATCH v4 1/2] [PATCH 1/2] mm: refactor of vma_merge()"
Previous message: Jean-Philippe Brucker: "Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove"
In reply to: Maxim Kochetkov: "Re: [PATCH v1 net-next] net: dsa: felix: update base time of time-aware shaper when adjusting PTP time"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 16 Jun 2022 15:05 +0800 Maxim Kochetkov wrote:
> 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@xxxxxxx>
> > ---
> > 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() )
>
Sure, it may be called twice here, I would correct that, thanks.

> > 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)
>
This mutex is to serialize TAS configuration. The TAS is configured not
only through ptp adjust, but also via tc-taprio. Therefore I add a new
tas_lock.

Next message: Vlastimil Babka: "Re: [PATCH v4 1/2] [PATCH 1/2] mm: refactor of vma_merge()"
Previous message: Jean-Philippe Brucker: "Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove"
In reply to: Maxim Kochetkov: "Re: [PATCH v1 net-next] net: dsa: felix: update base time of time-aware shaper when adjusting PTP time"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]