2019-11-11 04:43:09

by Po Liu

Subject: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

ENETC supports in hardware for time-based egress shaping according
to IEEE 802.1Qbv. This patch implement the Qbv enablement by the
hardware offload method qdisc tc-taprio method.
Also update cbdr writeback to up level since control bd ring may
writeback data to control bd ring.

Signed-off-by: Po Liu <[email protected]>
Singed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Claudiu Manoil <[email protected]>
---
drivers/net/ethernet/freescale/enetc/Makefile | 1 +
drivers/net/ethernet/freescale/enetc/enetc.c | 19 ++-
drivers/net/ethernet/freescale/enetc/enetc.h | 2 +
.../net/ethernet/freescale/enetc/enetc_cbdr.c | 5 +-
.../net/ethernet/freescale/enetc/enetc_hw.h | 150 ++++++++++++++++--
.../net/ethernet/freescale/enetc/enetc_qos.c | 130 +++++++++++++++
6 files changed, 285 insertions(+), 22 deletions(-)
create mode 100644 drivers/net/ethernet/freescale/enetc/enetc_qos.c

diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile
index d200c27c3bf6..389f722efc43 100644
--- a/drivers/net/ethernet/freescale/enetc/Makefile
+++ b/drivers/net/ethernet/freescale/enetc/Makefile
@@ -5,6 +5,7 @@ common-objs := enetc.o enetc_cbdr.o enetc_ethtool.o
obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o
fsl-enetc-y := enetc_pf.o enetc_mdio.o $(common-objs)
fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o
+fsl-enetc-$(CONFIG_NET_SCH_TAPRIO) += enetc_qos.o

obj-$(CONFIG_FSL_ENETC_VF) += fsl-enetc-vf.o
fsl-enetc-vf-y := enetc_vf.o $(common-objs)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 3e8f9819f08c..d58dbc2c4270 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1427,8 +1427,7 @@ int enetc_close(struct net_device *ndev)
return 0;
}

-int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
- void *type_data)
+int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
{
struct enetc_ndev_priv *priv = netdev_priv(ndev);
struct tc_mqprio_qopt *mqprio = type_data;
@@ -1436,9 +1435,6 @@ int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
u8 num_tc;
int i;

- if (type != TC_SETUP_QDISC_MQPRIO)
- return -EOPNOTSUPP;
-
mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
num_tc = mqprio->num_tc;

@@ -1483,6 +1479,19 @@ int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
return 0;
}

+int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
+ void *type_data)
+{
+ switch (type) {
+ case TC_SETUP_QDISC_MQPRIO:
+ return enetc_setup_tc_mqprio(ndev, type_data);
+ case TC_SETUP_QDISC_TAPRIO:
+ return enetc_setup_tc_taprio(ndev, type_data);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
struct net_device_stats *enetc_get_stats(struct net_device *ndev)
{
struct enetc_ndev_priv *priv = netdev_priv(ndev);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 541b4e2073fe..8676631041d5 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -244,3 +244,5 @@ int enetc_set_fs_entry(struct enetc_si *si, struct enetc_cmd_rfse *rfse,
void enetc_set_rss_key(struct enetc_hw *hw, const u8 *bytes);
int enetc_get_rss_table(struct enetc_si *si, u32 *table, int count);
int enetc_set_rss_table(struct enetc_si *si, const u32 *table, int count);
+int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd);
+int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
index de466b71bf8f..201cbc362e33 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
@@ -32,7 +32,7 @@ static int enetc_cbd_unused(struct enetc_cbdr *r)
r->bd_count;
}

-static int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
+int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
{
struct enetc_cbdr *ring = &si->cbd_ring;
int timeout = ENETC_CBDR_TIMEOUT;
@@ -66,6 +66,9 @@ static int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
if (!timeout)
return -EBUSY;

+ /* CBD may writeback data, feedback up level */
+ *cbd = *dest_cbd;
+
enetc_clean_cbdr(si);

return 0;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 88276299f447..75a7c0f1f8ce 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -18,6 +18,7 @@
#define ENETC_SICTR0 0x18
#define ENETC_SICTR1 0x1c
#define ENETC_SIPCAPR0 0x20
+#define ENETC_SIPCAPR0_QBV BIT(4)
#define ENETC_SIPCAPR0_RSS BIT(8)
#define ENETC_SIPCAPR1 0x24
#define ENETC_SITGTGR 0x30
@@ -148,6 +149,12 @@ enum enetc_bdr_type {TX, RX};
#define ENETC_PORT_BASE 0x10000
#define ENETC_PMR 0x0000
#define ENETC_PMR_EN GENMASK(18, 16)
+#define ENETC_PMR_PSPEED_MASK GENMASK(11, 8)
+#define ENETC_PMR_PSPEED_10M 0x000
+#define ENETC_PMR_PSPEED_100M 0x100
+#define ENETC_PMR_PSPEED_1000M 0x200
+#define ENETC_PMR_PSPEED_2500M 0x400
+
#define ENETC_PSR 0x0004 /* RO */
#define ENETC_PSIPMR 0x0018
#define ENETC_PSIPMR_SET_UP(n) BIT(n) /* n = SI index */
@@ -440,22 +447,6 @@ union enetc_rx_bd {
#define EMETC_MAC_ADDR_FILT_RES 3 /* # of reserved entries at the beginning */
#define ENETC_MAX_NUM_VFS 2

-struct enetc_cbd {
- union {
- struct {
- __le32 addr[2];
- __le32 opt[4];
- };
- __le32 data[6];
- };
- __le16 index;
- __le16 length;
- u8 cmd;
- u8 cls;
- u8 _res;
- u8 status_flags;
-};
-
#define ENETC_CBD_FLAGS_SF BIT(7) /* short format */
#define ENETC_CBD_STATUS_MASK 0xf

@@ -554,3 +545,130 @@ static inline void enetc_set_bdr_prio(struct enetc_hw *hw, int bdr_idx,
val |= ENETC_TBMR_SET_PRIO(prio);
enetc_txbdr_wr(hw, bdr_idx, ENETC_TBMR, val);
}
+
+enum bdcr_cmd_class {
+ BDCR_CMD_UNSPEC = 0,
+ BDCR_CMD_MAC_FILTER,
+ BDCR_CMD_VLAN_FILTER,
+ BDCR_CMD_RSS,
+ BDCR_CMD_RFS,
+ BDCR_CMD_PORT_GCL,
+ BDCR_CMD_RECV_CLASSIFIER,
+ __BDCR_CMD_MAX_LEN,
+ BDCR_CMD_MAX_LEN = __BDCR_CMD_MAX_LEN - 1,
+};
+
+/* class 5, command 0 */
+struct tgs_gcl_conf {
+ u8 atc; /* init gate value */
+ u8 res[7];
+ union {
+ struct {
+ u8 res1[4];
+ __le16 acl_len;
+ u8 res2[2];
+ };
+ struct {
+ u32 cctl;
+ u32 ccth;
+ };
+ };
+};
+
+#define ENETC_CBDR_SGL_IOMEN BIT(0)
+#define ENETC_CBDR_SGL_IPVEN BIT(3)
+#define ENETC_CBDR_SGL_GTST BIT(4)
+#define ENETC_CBDR_SGL_IPV_MASK 0xe
+
+/* gate control list entry */
+struct gce {
+ u32 period;
+ u8 gate;
+ u8 res[3];
+};
+
+/* tgs_gcl_conf address point to this data space */
+struct tgs_gcl_data {
+ u32 btl;
+ u32 bth;
+ u32 ct;
+ u32 cte;
+};
+
+/* class 5, command 1 */
+struct tgs_gcl_query {
+ u8 res[12];
+ union {
+ struct {
+ __le16 acl_len; /* admin list length */
+ __le16 ocl_len; /* operation list length */
+ };
+ struct {
+ u16 admin_list_len;
+ u16 oper_list_len;
+ };
+ };
+};
+
+/* tgs_gcl_query command response data format */
+struct tgs_gcl_resp {
+ u32 abtl; /* base time */
+ u32 abth;
+ u32 act; /* cycle time */
+ u32 acte; /* cycle time extend */
+ u32 cctl; /* config change time */
+ u32 ccth;
+ u32 obtl; /* operation base time */
+ u32 obth;
+ u32 oct; /* operation cycle time */
+ u32 octe; /* operation cycle time extend */
+ u32 ccel; /* config change error */
+ u32 cceh;
+};
+
+struct enetc_cbd {
+ union{
+ struct {
+ __le32 addr[2];
+ union {
+ __le32 opt[4];
+ struct tgs_gcl_conf gcl_conf;
+ struct tgs_gcl_query gcl_query;
+ };
+ }; /* Long format */
+ __le32 data[6];
+ };
+ __le16 index;
+ __le16 length;
+ u8 cmd;
+ u8 cls;
+ u8 _res;
+ u8 status_flags;
+};
+
+#define ENETC_PTCFPR(n) (0x1910 + (n) * 4) /* n = [0 ..7] */
+#define ENETC_FPE BIT(31)
+
+/* Port capability register 0 */
+#define ENETC_PCAPR0_PSFPM BIT(10)
+#define ENETC_PCAPR0_PSFP BIT(9)
+#define ENETC_PCAPR0_TSN BIT(4)
+#define ENETC_PCAPR0_QBU BIT(3)
+
+/* port time gating control register */
+#define ENETC_QBV_PTGCR_OFFSET 0x11a00
+#define ENETC_QBV_TGE 0x80000000
+#define ENETC_QBV_TGPE BIT(30)
+#define ENETC_QBV_TGDROP_DISABLE BIT(29)
+
+/* Port time gating capability register */
+#define ENETC_QBV_PTGCAPR_OFFSET 0x11a08
+#define ENETC_QBV_MAX_GCL_LEN_MASK 0xffff
+
+/* Port time gating admin gate list status register */
+#define ENETC_QBV_PTGAGLSR_OFFSET 0x11a10
+#define ENETC_QBV_CFG_PEND_MASK 0x00000002
+
+#define ENETC_TGLSTR 0xa200
+#define ENETC_TGS_MIN_DIS_MASK 0x80000000
+#define ENETC_MIN_LOOKAHEAD_MASK 0xffff
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
new file mode 100644
index 000000000000..036bb39c7a0b
--- /dev/null
+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/* Copyright 2019 NXP */
+
+#include "enetc.h"
+
+#include <net/pkt_sched.h>
+
+static u16 enetc_get_max_gcl_len(struct enetc_hw *hw)
+{
+ return enetc_rd(hw, ENETC_QBV_PTGCAPR_OFFSET)
+ & ENETC_QBV_MAX_GCL_LEN_MASK;
+}
+
+static int enetc_setup_taprio(struct net_device *ndev,
+ struct tc_taprio_qopt_offload *admin_conf)
+{
+ struct enetc_ndev_priv *priv = netdev_priv(ndev);
+ struct enetc_cbd cbd = {.cmd = 0};
+ struct tgs_gcl_conf *gcl_config;
+ struct tgs_gcl_data *gcl_data;
+ struct gce *gce;
+ dma_addr_t dma;
+ u16 data_size;
+ u16 gcl_len;
+ u32 temp;
+ int i;
+
+ gcl_len = admin_conf->num_entries;
+ if (gcl_len > enetc_get_max_gcl_len(&priv->si->hw))
+ return -EINVAL;
+
+ if (admin_conf->enable) {
+ enetc_wr(&priv->si->hw,
+ ENETC_QBV_PTGCR_OFFSET,
+ temp & (~ENETC_QBV_TGE));
+ usleep_range(10, 20);
+ enetc_wr(&priv->si->hw,
+ ENETC_QBV_PTGCR_OFFSET,
+ temp | ENETC_QBV_TGE);
+ } else {
+ enetc_wr(&priv->si->hw,
+ ENETC_QBV_PTGCR_OFFSET,
+ temp & (~ENETC_QBV_TGE));
+ return 0;
+ }
+
+ /* Configure the (administrative) gate control list using the
+ * control BD descriptor.
+ */
+ gcl_config = &cbd.gcl_conf;
+
+ data_size = sizeof(struct tgs_gcl_data) + gcl_len * sizeof(struct gce);
+
+ gcl_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL);
+ if (!gcl_data)
+ return -ENOMEM;
+
+ gce = (struct gce *)(gcl_data + 1);
+
+ /* Since no initial state config in taprio, set gates open as default.
+ */
+ gcl_config->atc = 0xff;
+ gcl_config->acl_len = cpu_to_le16(gcl_len);
+
+ if (!admin_conf->base_time) {
+ gcl_data->btl =
+ cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR0));
+ gcl_data->bth =
+ cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR1));
+ } else {
+ gcl_data->btl =
+ cpu_to_le32(lower_32_bits(admin_conf->base_time));
+ gcl_data->bth =
+ cpu_to_le32(upper_32_bits(admin_conf->base_time));
+ }
+
+ gcl_data->ct = cpu_to_le32(admin_conf->cycle_time);
+ gcl_data->cte = cpu_to_le32(admin_conf->cycle_time_extension);
+
+ for (i = 0; i < gcl_len; i++) {
+ struct tc_taprio_sched_entry *temp_entry;
+ struct gce *temp_gce = gce + i;
+
+ temp_entry = &admin_conf->entries[i];
+
+ temp_gce->gate = cpu_to_le32(temp_entry->gate_mask);
+ temp_gce->period = cpu_to_le32(temp_entry->interval);
+ }
+
+ cbd.length = cpu_to_le16(data_size);
+ cbd.status_flags = 0;
+
+ dma = dma_map_single(&priv->si->pdev->dev, gcl_data,
+ data_size, DMA_TO_DEVICE);
+ if (dma_mapping_error(&priv->si->pdev->dev, dma)) {
+ netdev_err(priv->si->ndev, "DMA mapping failed!\n");
+ kfree(gcl_data);
+ return -ENOMEM;
+ }
+
+ cbd.addr[0] = lower_32_bits(dma);
+ cbd.addr[1] = upper_32_bits(dma);
+ cbd.cls = BDCR_CMD_PORT_GCL;
+
+ /* Updated by ENETC on completion of the configuration
+ * command. A zero value indicates success.
+ */
+ cbd.status_flags = 0;
+
+ enetc_send_cmd(priv->si, &cbd);
+
+ dma_unmap_single(&priv->si->pdev->dev, dma, data_size, DMA_TO_DEVICE);
+ kfree(gcl_data);
+
+ return 0;
+}
+
+int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
+{
+ struct tc_taprio_qopt_offload *taprio = type_data;
+ struct enetc_ndev_priv *priv = netdev_priv(ndev);
+ int i;
+
+ for (i = 0; i < priv->num_tx_rings; i++)
+ enetc_set_bdr_prio(&priv->si->hw,
+ priv->tx_ring[i]->index,
+ taprio->enable ? i : 0);
+
+ return enetc_setup_taprio(ndev, taprio);
+}
--
2.17.1

2019-11-12 05:53:24

by David Miller

Subject: Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

From: Po Liu <[email protected]>
Date: Mon, 11 Nov 2019 04:41:26 +0000

> +fsl-enetc-$(CONFIG_NET_SCH_TAPRIO) += enetc_qos.o

Code is Kconfig guarded.
> + case TC_SETUP_QDISC_TAPRIO:
> + return enetc_setup_tc_taprio(ndev, type_data);

Yet invoked unconditionally.

I can see just by reading your code that various configurations will
result in link errors.

...
> +int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
> +{
> + struct tc_taprio_qopt_offload *taprio = type_data;
> + struct enetc_ndev_priv *priv = netdev_priv(ndev);
> + int i;
> +
> + for (i = 0; i < priv->num_tx_rings; i++)
> + enetc_set_bdr_prio(&priv->si->hw,
> + priv->tx_ring[i]->index,
> + taprio->enable ? i : 0);
> +
> + return enetc_setup_taprio(ndev, taprio);
> +}
> --
> 2.17.1
>

2019-11-12 06:49:59

by Po Liu

Subject: RE: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

Hi David,


Br,
Po Liu

> -----Original Message-----
> From: David Miller <[email protected]>
> Sent: 2019??11??12?? 13:51
> To: Po Liu <[email protected]>
> Cc: Claudiu Manoil <[email protected]>; [email protected];
> [email protected]; [email protected]; Vladimir Oltean
> <[email protected]>; Alexandru Marginean
> <[email protected]>; Xiaoliang Yang
> <[email protected]>; Roy Zang <[email protected]>; Mingkai Hu
> <[email protected]>; Jerry Huang <[email protected]>; Leo Li
> <[email protected]>
> Subject: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via
> tc-taprio offload
>
> Caution: EXT Email
>
> From: Po Liu <[email protected]>
> Date: Mon, 11 Nov 2019 04:41:26 +0000
>
> > +fsl-enetc-$(CONFIG_NET_SCH_TAPRIO) += enetc_qos.o
>
> Code is Kconfig guarded.
> > + case TC_SETUP_QDISC_TAPRIO:
> > + return enetc_setup_tc_taprio(ndev, type_data);
>
> Yet invoked unconditionally.
>
> I can see just by reading your code that various configurations will result in link
> errors.

I get it. Thanks!

>
> ...
> > +int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data) {
> > + struct tc_taprio_qopt_offload *taprio = type_data;
> > + struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > + int i;
> > +
> > + for (i = 0; i < priv->num_tx_rings; i++)
> > + enetc_set_bdr_prio(&priv->si->hw,
> > + priv->tx_ring[i]->index,
> > + taprio->enable ? i : 0);
> > +
> > + return enetc_setup_taprio(ndev, taprio); }
> > --
> > 2.17.1
> >

2019-11-12 09:45:21

by Simon Horman

Subject: Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

On Mon, Nov 11, 2019 at 04:41:26AM +0000, Po Liu wrote:
> ENETC supports in hardware for time-based egress shaping according
> to IEEE 802.1Qbv. This patch implement the Qbv enablement by the
> hardware offload method qdisc tc-taprio method.
> Also update cbdr writeback to up level since control bd ring may
> writeback data to control bd ring.
>
> Signed-off-by: Po Liu <[email protected]>
> Singed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Claudiu Manoil <[email protected]>
> ---
> drivers/net/ethernet/freescale/enetc/Makefile | 1 +
> drivers/net/ethernet/freescale/enetc/enetc.c | 19 ++-
> drivers/net/ethernet/freescale/enetc/enetc.h | 2 +
> .../net/ethernet/freescale/enetc/enetc_cbdr.c | 5 +-
> .../net/ethernet/freescale/enetc/enetc_hw.h | 150 ++++++++++++++++--
> .../net/ethernet/freescale/enetc/enetc_qos.c | 130 +++++++++++++++
> 6 files changed, 285 insertions(+), 22 deletions(-)
> create mode 100644 drivers/net/ethernet/freescale/enetc/enetc_qos.c
>
> diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile
> index d200c27c3bf6..389f722efc43 100644
> --- a/drivers/net/ethernet/freescale/enetc/Makefile
> +++ b/drivers/net/ethernet/freescale/enetc/Makefile
> @@ -5,6 +5,7 @@ common-objs := enetc.o enetc_cbdr.o enetc_ethtool.o
> obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o
> fsl-enetc-y := enetc_pf.o enetc_mdio.o $(common-objs)
> fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o
> +fsl-enetc-$(CONFIG_NET_SCH_TAPRIO) += enetc_qos.o
>
> obj-$(CONFIG_FSL_ENETC_VF) += fsl-enetc-vf.o
> fsl-enetc-vf-y := enetc_vf.o $(common-objs)
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 3e8f9819f08c..d58dbc2c4270 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1427,8 +1427,7 @@ int enetc_close(struct net_device *ndev)
> return 0;
> }
>
> -int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> - void *type_data)
> +int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
> {
> struct enetc_ndev_priv *priv = netdev_priv(ndev);
> struct tc_mqprio_qopt *mqprio = type_data;
> @@ -1436,9 +1435,6 @@ int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> u8 num_tc;
> int i;
>
> - if (type != TC_SETUP_QDISC_MQPRIO)
> - return -EOPNOTSUPP;
> -
> mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
> num_tc = mqprio->num_tc;
>
> @@ -1483,6 +1479,19 @@ int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> return 0;
> }
>
> +int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> + void *type_data)
> +{
> + switch (type) {
> + case TC_SETUP_QDISC_MQPRIO:
> + return enetc_setup_tc_mqprio(ndev, type_data);
> + case TC_SETUP_QDISC_TAPRIO:
> + return enetc_setup_tc_taprio(ndev, type_data);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> struct net_device_stats *enetc_get_stats(struct net_device *ndev)
> {
> struct enetc_ndev_priv *priv = netdev_priv(ndev);
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
> index 541b4e2073fe..8676631041d5 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> @@ -244,3 +244,5 @@ int enetc_set_fs_entry(struct enetc_si *si, struct enetc_cmd_rfse *rfse,
> void enetc_set_rss_key(struct enetc_hw *hw, const u8 *bytes);
> int enetc_get_rss_table(struct enetc_si *si, u32 *table, int count);
> int enetc_set_rss_table(struct enetc_si *si, const u32 *table, int count);
> +int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd);
> +int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data);
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> index de466b71bf8f..201cbc362e33 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> @@ -32,7 +32,7 @@ static int enetc_cbd_unused(struct enetc_cbdr *r)
> r->bd_count;
> }
>
> -static int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> +int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> {
> struct enetc_cbdr *ring = &si->cbd_ring;
> int timeout = ENETC_CBDR_TIMEOUT;
> @@ -66,6 +66,9 @@ static int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> if (!timeout)
> return -EBUSY;
>
> + /* CBD may writeback data, feedback up level */
> + *cbd = *dest_cbd;
> +
> enetc_clean_cbdr(si);
>
> return 0;
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> index 88276299f447..75a7c0f1f8ce 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> @@ -18,6 +18,7 @@
> #define ENETC_SICTR0 0x18
> #define ENETC_SICTR1 0x1c
> #define ENETC_SIPCAPR0 0x20
> +#define ENETC_SIPCAPR0_QBV BIT(4)
> #define ENETC_SIPCAPR0_RSS BIT(8)
> #define ENETC_SIPCAPR1 0x24
> #define ENETC_SITGTGR 0x30
> @@ -148,6 +149,12 @@ enum enetc_bdr_type {TX, RX};
> #define ENETC_PORT_BASE 0x10000
> #define ENETC_PMR 0x0000
> #define ENETC_PMR_EN GENMASK(18, 16)
> +#define ENETC_PMR_PSPEED_MASK GENMASK(11, 8)
> +#define ENETC_PMR_PSPEED_10M 0x000
> +#define ENETC_PMR_PSPEED_100M 0x100
> +#define ENETC_PMR_PSPEED_1000M 0x200
> +#define ENETC_PMR_PSPEED_2500M 0x400

Perhaps BIT() is appropriate here.

The changes above to enetc_hw.h are not used until the following patch,
perhaps it would be better if they were part of that patch.

> +
> #define ENETC_PSR 0x0004 /* RO */
> #define ENETC_PSIPMR 0x0018
> #define ENETC_PSIPMR_SET_UP(n) BIT(n) /* n = SI index */
> @@ -440,22 +447,6 @@ union enetc_rx_bd {
> #define EMETC_MAC_ADDR_FILT_RES 3 /* # of reserved entries at the beginning */
> #define ENETC_MAX_NUM_VFS 2
>
> -struct enetc_cbd {
> - union {
> - struct {
> - __le32 addr[2];
> - __le32 opt[4];
> - };
> - __le32 data[6];
> - };
> - __le16 index;
> - __le16 length;
> - u8 cmd;
> - u8 cls;
> - u8 _res;
> - u8 status_flags;
> -};
> -
> #define ENETC_CBD_FLAGS_SF BIT(7) /* short format */
> #define ENETC_CBD_STATUS_MASK 0xf
>
> @@ -554,3 +545,130 @@ static inline void enetc_set_bdr_prio(struct enetc_hw *hw, int bdr_idx,
> val |= ENETC_TBMR_SET_PRIO(prio);
> enetc_txbdr_wr(hw, bdr_idx, ENETC_TBMR, val);
> }
> +
> +enum bdcr_cmd_class {
> + BDCR_CMD_UNSPEC = 0,
> + BDCR_CMD_MAC_FILTER,
> + BDCR_CMD_VLAN_FILTER,
> + BDCR_CMD_RSS,
> + BDCR_CMD_RFS,
> + BDCR_CMD_PORT_GCL,
> + BDCR_CMD_RECV_CLASSIFIER,
> + __BDCR_CMD_MAX_LEN,
> + BDCR_CMD_MAX_LEN = __BDCR_CMD_MAX_LEN - 1,
> +};
> +
> +/* class 5, command 0 */
> +struct tgs_gcl_conf {
> + u8 atc; /* init gate value */
> + u8 res[7];
> + union {
> + struct {
> + u8 res1[4];
> + __le16 acl_len;

Given that u* types are used in this structure I think le16 would
be more appropriate than __le16.

> + u8 res2[2];
> + };
> + struct {
> + u32 cctl;
> + u32 ccth;
> + };

I'm a little surprised to see host endian values in a structure
that appears to be written to hardware. Is this intentional?

Also, these fields do not seem to be used in this patch-set.
Is that also intentional?

> + };
> +};
> +
> +#define ENETC_CBDR_SGL_IOMEN BIT(0)
> +#define ENETC_CBDR_SGL_IPVEN BIT(3)
> +#define ENETC_CBDR_SGL_GTST BIT(4)
> +#define ENETC_CBDR_SGL_IPV_MASK 0xe

Perhaps GENMASK() is appropriate here and elsewhere for generating masks.

> +
> +/* gate control list entry */
> +struct gce {
> + u32 period;

Likewise, I'm a little surprised to see host-byte order data to
be written to HW. And below too. Though as I've noted below,
some of these values are used to store le32 data, so it seems
that the types are incorrect.

> + u8 gate;
> + u8 res[3];
> +};
> +
> +/* tgs_gcl_conf address point to this data space */
> +struct tgs_gcl_data {
> + u32 btl;
> + u32 bth;
> + u32 ct;
> + u32 cte;
> +};
> +
> +/* class 5, command 1 */
> +struct tgs_gcl_query {
> + u8 res[12];
> + union {
> + struct {
> + __le16 acl_len; /* admin list length */
> + __le16 ocl_len; /* operation list length */
> + };
> + struct {
> + u16 admin_list_len;
> + u16 oper_list_len;

Again, is it intentional that these are a) host-byte order and
b) unused?

> + };
> + };
> +};
> +
> +/* tgs_gcl_query command response data format */
> +struct tgs_gcl_resp {
> + u32 abtl; /* base time */
> + u32 abth;
> + u32 act; /* cycle time */
> + u32 acte; /* cycle time extend */
> + u32 cctl; /* config change time */
> + u32 ccth;
> + u32 obtl; /* operation base time */
> + u32 obth;
> + u32 oct; /* operation cycle time */
> + u32 octe; /* operation cycle time extend */
> + u32 ccel; /* config change error */
> + u32 cceh;
> +};

This structure seems unused in this patchset.

> +
> +struct enetc_cbd {
> + union{
> + struct {
> + __le32 addr[2];
> + union {
> + __le32 opt[4];
> + struct tgs_gcl_conf gcl_conf;
> + struct tgs_gcl_query gcl_query;
> + };
> + }; /* Long format */
> + __le32 data[6];
> + };
> + __le16 index;
> + __le16 length;
> + u8 cmd;
> + u8 cls;
> + u8 _res;
> + u8 status_flags;
> +};
> +
> +#define ENETC_PTCFPR(n) (0x1910 + (n) * 4) /* n = [0 ..7] */
> +#define ENETC_FPE BIT(31)
> +
> +/* Port capability register 0 */
> +#define ENETC_PCAPR0_PSFPM BIT(10)
> +#define ENETC_PCAPR0_PSFP BIT(9)
> +#define ENETC_PCAPR0_TSN BIT(4)
> +#define ENETC_PCAPR0_QBU BIT(3)
> +
> +/* port time gating control register */
> +#define ENETC_QBV_PTGCR_OFFSET 0x11a00
> +#define ENETC_QBV_TGE 0x80000000
> +#define ENETC_QBV_TGPE BIT(30)
> +#define ENETC_QBV_TGDROP_DISABLE BIT(29)
> +
> +/* Port time gating capability register */
> +#define ENETC_QBV_PTGCAPR_OFFSET 0x11a08
> +#define ENETC_QBV_MAX_GCL_LEN_MASK 0xffff
> +
> +/* Port time gating admin gate list status register */
> +#define ENETC_QBV_PTGAGLSR_OFFSET 0x11a10
> +#define ENETC_QBV_CFG_PEND_MASK 0x00000002
> +
> +#define ENETC_TGLSTR 0xa200
> +#define ENETC_TGS_MIN_DIS_MASK 0x80000000
> +#define ENETC_MIN_LOOKAHEAD_MASK 0xffff
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> new file mode 100644
> index 000000000000..036bb39c7a0b
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> @@ -0,0 +1,130 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/* Copyright 2019 NXP */
> +
> +#include "enetc.h"
> +
> +#include <net/pkt_sched.h>
> +
> +static u16 enetc_get_max_gcl_len(struct enetc_hw *hw)
> +{
> + return enetc_rd(hw, ENETC_QBV_PTGCAPR_OFFSET)
> + & ENETC_QBV_MAX_GCL_LEN_MASK;
> +}
> +
> +static int enetc_setup_taprio(struct net_device *ndev,
> + struct tc_taprio_qopt_offload *admin_conf)
> +{
> + struct enetc_ndev_priv *priv = netdev_priv(ndev);
> + struct enetc_cbd cbd = {.cmd = 0};
> + struct tgs_gcl_conf *gcl_config;
> + struct tgs_gcl_data *gcl_data;
> + struct gce *gce;
> + dma_addr_t dma;
> + u16 data_size;
> + u16 gcl_len;
> + u32 temp;
> + int i;
> +
> + gcl_len = admin_conf->num_entries;
> + if (gcl_len > enetc_get_max_gcl_len(&priv->si->hw))
> + return -EINVAL;
> +
> + if (admin_conf->enable) {
> + enetc_wr(&priv->si->hw,
> + ENETC_QBV_PTGCR_OFFSET,
> + temp & (~ENETC_QBV_TGE));

The enetc_wr() call seems to be common to both arms of the condition.
If so perhaps it could be move outside of the condition.

> + usleep_range(10, 20);
> + enetc_wr(&priv->si->hw,
> + ENETC_QBV_PTGCR_OFFSET,
> + temp | ENETC_QBV_TGE);
> + } else {
> + enetc_wr(&priv->si->hw,
> + ENETC_QBV_PTGCR_OFFSET,
> + temp & (~ENETC_QBV_TGE));
> + return 0;
> + }
> +
> + /* Configure the (administrative) gate control list using the
> + * control BD descriptor.
> + */
> + gcl_config = &cbd.gcl_conf;
> +
> + data_size = sizeof(struct tgs_gcl_data) + gcl_len * sizeof(struct gce);

I think struct_size() can be used here.

> +
> + gcl_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL);
> + if (!gcl_data)
> + return -ENOMEM;
> +
> + gce = (struct gce *)(gcl_data + 1);
> +
> + /* Since no initial state config in taprio, set gates open as default.
> + */
> + gcl_config->atc = 0xff;
> + gcl_config->acl_len = cpu_to_le16(gcl_len);
> +
> + if (!admin_conf->base_time) {
> + gcl_data->btl =
> + cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR0));
> + gcl_data->bth =
> + cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR1));
> + } else {
> + gcl_data->btl =
> + cpu_to_le32(lower_32_bits(admin_conf->base_time));
> + gcl_data->bth =
> + cpu_to_le32(upper_32_bits(admin_conf->base_time));
> + }

It looks like the types of the btl and bth fields are u32.
Perhaps they should be le32?

Please consider running sparse over this code to check for any endian
problems.

> +
> + gcl_data->ct = cpu_to_le32(admin_conf->cycle_time);
> + gcl_data->cte = cpu_to_le32(admin_conf->cycle_time_extension);
> +
> + for (i = 0; i < gcl_len; i++) {
> + struct tc_taprio_sched_entry *temp_entry;
> + struct gce *temp_gce = gce + i;
> +
> + temp_entry = &admin_conf->entries[i];
> +
> + temp_gce->gate = cpu_to_le32(temp_entry->gate_mask);

Gate is a u8 followed by 3 reserved bytes.
Perhaps there needs to be some bounds checking on
the value stored there given that the source is 32bits wide.

Also, its not clear to me that the above logic, which I assume
takes the last significant byte of a 32bit value, works on
big endian systems as the 32bit value is always little endian.

> + temp_gce->period = cpu_to_le32(temp_entry->interval);

It looks like the types of the gate field is u32.
Perhaps it should be le32?

> + }
> +
> + cbd.length = cpu_to_le16(data_size);
> + cbd.status_flags = 0;
> +
> + dma = dma_map_single(&priv->si->pdev->dev, gcl_data,
> + data_size, DMA_TO_DEVICE);
> + if (dma_mapping_error(&priv->si->pdev->dev, dma)) {
> + netdev_err(priv->si->ndev, "DMA mapping failed!\n");
> + kfree(gcl_data);
> + return -ENOMEM;
> + }
> +
> + cbd.addr[0] = lower_32_bits(dma);
> + cbd.addr[1] = upper_32_bits(dma);
> + cbd.cls = BDCR_CMD_PORT_GCL;
> +
> + /* Updated by ENETC on completion of the configuration
> + * command. A zero value indicates success.
> + */
> + cbd.status_flags = 0;
> +
> + enetc_send_cmd(priv->si, &cbd);
> +
> + dma_unmap_single(&priv->si->pdev->dev, dma, data_size, DMA_TO_DEVICE);
> + kfree(gcl_data);
> +
> + return 0;
> +}
> +
> +int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
> +{
> + struct tc_taprio_qopt_offload *taprio = type_data;
> + struct enetc_ndev_priv *priv = netdev_priv(ndev);
> + int i;
> +
> + for (i = 0; i < priv->num_tx_rings; i++)
> + enetc_set_bdr_prio(&priv->si->hw,
> + priv->tx_ring[i]->index,
> + taprio->enable ? i : 0);
> +
> + return enetc_setup_taprio(ndev, taprio);
> +}
> --
> 2.17.1
>

2019-11-12 11:21:48

by Po Liu

Subject: RE: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

Br,
Po Liu

> -----Original Message-----
> From: Simon Horman <[email protected]>
> Sent: 2019??11??12?? 17:41
> To: Po Liu <[email protected]>
> Cc: Claudiu Manoil <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> Vladimir Oltean <[email protected]>; Alexandru Marginean
> <[email protected]>; Xiaoliang Yang
> <[email protected]>; Roy Zang <[email protected]>; Mingkai Hu
> <[email protected]>; Jerry Huang <[email protected]>; Leo Li
> <[email protected]>
> Subject: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via
> tc-taprio offload
>
> Caution: EXT Email
>
> On Mon, Nov 11, 2019 at 04:41:26AM +0000, Po Liu wrote:
> > ENETC supports in hardware for time-based egress shaping according to
> > IEEE 802.1Qbv. This patch implement the Qbv enablement by the hardware
> > offload method qdisc tc-taprio method.
> > Also update cbdr writeback to up level since control bd ring may
> > writeback data to control bd ring.
> >
> > Signed-off-by: Po Liu <[email protected]>
> > Singed-off-by: Vladimir Oltean <[email protected]>
> > Signed-off-by: Claudiu Manoil <[email protected]>
> > ---
> > drivers/net/ethernet/freescale/enetc/Makefile | 1 +
> > drivers/net/ethernet/freescale/enetc/enetc.c | 19 ++-
> > drivers/net/ethernet/freescale/enetc/enetc.h | 2 +
> > .../net/ethernet/freescale/enetc/enetc_cbdr.c | 5 +-
> > .../net/ethernet/freescale/enetc/enetc_hw.h | 150 ++++++++++++++++--
> > .../net/ethernet/freescale/enetc/enetc_qos.c | 130 +++++++++++++++
> > 6 files changed, 285 insertions(+), 22 deletions(-) create mode
> > 100644 drivers/net/ethernet/freescale/enetc/enetc_qos.c
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/Makefile
> > b/drivers/net/ethernet/freescale/enetc/Makefile
> > index d200c27c3bf6..389f722efc43 100644
> > --- a/drivers/net/ethernet/freescale/enetc/Makefile
> > +++ b/drivers/net/ethernet/freescale/enetc/Makefile
> > @@ -5,6 +5,7 @@ common-objs := enetc.o enetc_cbdr.o enetc_ethtool.o
> > obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o fsl-enetc-y := enetc_pf.o
> > enetc_mdio.o $(common-objs)
> > fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o
> > +fsl-enetc-$(CONFIG_NET_SCH_TAPRIO) += enetc_qos.o
> >
> > obj-$(CONFIG_FSL_ENETC_VF) += fsl-enetc-vf.o fsl-enetc-vf-y :=
> > enetc_vf.o $(common-objs) diff --git
> > a/drivers/net/ethernet/freescale/enetc/enetc.c
> > b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 3e8f9819f08c..d58dbc2c4270 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -1427,8 +1427,7 @@ int enetc_close(struct net_device *ndev)
> > return 0;
> > }
> >
> > -int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> > - void *type_data)
> > +int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
> > {
> > struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > struct tc_mqprio_qopt *mqprio = type_data; @@ -1436,9 +1435,6 @@
> > int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> > u8 num_tc;
> > int i;
> >
> > - if (type != TC_SETUP_QDISC_MQPRIO)
> > - return -EOPNOTSUPP;
> > -
> > mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
> > num_tc = mqprio->num_tc;
> >
> > @@ -1483,6 +1479,19 @@ int enetc_setup_tc(struct net_device *ndev,
> enum tc_setup_type type,
> > return 0;
> > }
> >
> > +int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> > + void *type_data)
> > +{
> > + switch (type) {
> > + case TC_SETUP_QDISC_MQPRIO:
> > + return enetc_setup_tc_mqprio(ndev, type_data);
> > + case TC_SETUP_QDISC_TAPRIO:
> > + return enetc_setup_tc_taprio(ndev, type_data);
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > struct net_device_stats *enetc_get_stats(struct net_device *ndev) {
> > struct enetc_ndev_priv *priv = netdev_priv(ndev); diff --git
> > a/drivers/net/ethernet/freescale/enetc/enetc.h
> > b/drivers/net/ethernet/freescale/enetc/enetc.h
> > index 541b4e2073fe..8676631041d5 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> > @@ -244,3 +244,5 @@ int enetc_set_fs_entry(struct enetc_si *si, struct
> > enetc_cmd_rfse *rfse, void enetc_set_rss_key(struct enetc_hw *hw,
> > const u8 *bytes); int enetc_get_rss_table(struct enetc_si *si, u32
> > *table, int count); int enetc_set_rss_table(struct enetc_si *si,
> > const u32 *table, int count);
> > +int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd); int
> > +enetc_setup_tc_taprio(struct net_device *ndev, void *type_data);
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> > b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> > index de466b71bf8f..201cbc362e33 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> > @@ -32,7 +32,7 @@ static int enetc_cbd_unused(struct enetc_cbdr *r)
> > r->bd_count;
> > }
> >
> > -static int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> > +int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> > {
> > struct enetc_cbdr *ring = &si->cbd_ring;
> > int timeout = ENETC_CBDR_TIMEOUT; @@ -66,6 +66,9 @@ static int
> > enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> > if (!timeout)
> > return -EBUSY;
> >
> > + /* CBD may writeback data, feedback up level */
> > + *cbd = *dest_cbd;
> > +
> > enetc_clean_cbdr(si);
> >
> > return 0;
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> > b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> > index 88276299f447..75a7c0f1f8ce 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> > @@ -18,6 +18,7 @@
> > #define ENETC_SICTR0 0x18
> > #define ENETC_SICTR1 0x1c
> > #define ENETC_SIPCAPR0 0x20
> > +#define ENETC_SIPCAPR0_QBV BIT(4)
> > #define ENETC_SIPCAPR0_RSS BIT(8)
> > #define ENETC_SIPCAPR1 0x24
> > #define ENETC_SITGTGR 0x30
> > @@ -148,6 +149,12 @@ enum enetc_bdr_type {TX, RX};
> > #define ENETC_PORT_BASE 0x10000
> > #define ENETC_PMR 0x0000
> > #define ENETC_PMR_EN GENMASK(18, 16)
> > +#define ENETC_PMR_PSPEED_MASK GENMASK(11, 8) #define
> > +ENETC_PMR_PSPEED_10M 0x000 #define ENETC_PMR_PSPEED_100M
> 0x100
> > +#define ENETC_PMR_PSPEED_1000M 0x200 #define
> ENETC_PMR_PSPEED_2500M
> > +0x400
>
> Perhaps BIT() is appropriate here.

Will change to BIT().

>
> The changes above to enetc_hw.h are not used until the following patch,
> perhaps it would be better if they were part of that patch.
>
Yes, better to move to patch 2/2.

> > +
> > #define ENETC_PSR 0x0004 /* RO */
> > #define ENETC_PSIPMR 0x0018
> > #define ENETC_PSIPMR_SET_UP(n) BIT(n) /* n = SI index */
> > @@ -440,22 +447,6 @@ union enetc_rx_bd {
> > #define EMETC_MAC_ADDR_FILT_RES 3 /* # of reserved entries at the
> beginning */
> > #define ENETC_MAX_NUM_VFS 2
> >
> > -struct enetc_cbd {
> > - union {
> > - struct {
> > - __le32 addr[2];
> > - __le32 opt[4];
> > - };
> > - __le32 data[6];
> > - };
> > - __le16 index;
> > - __le16 length;
> > - u8 cmd;
> > - u8 cls;
> > - u8 _res;
> > - u8 status_flags;
> > -};
> > -
> > #define ENETC_CBD_FLAGS_SF BIT(7) /* short format */
> > #define ENETC_CBD_STATUS_MASK 0xf
> >
> > @@ -554,3 +545,130 @@ static inline void enetc_set_bdr_prio(struct
> enetc_hw *hw, int bdr_idx,
> > val |= ENETC_TBMR_SET_PRIO(prio);
> > enetc_txbdr_wr(hw, bdr_idx, ENETC_TBMR, val); }
> > +
> > +enum bdcr_cmd_class {
> > + BDCR_CMD_UNSPEC = 0,
> > + BDCR_CMD_MAC_FILTER,
> > + BDCR_CMD_VLAN_FILTER,
> > + BDCR_CMD_RSS,
> > + BDCR_CMD_RFS,
> > + BDCR_CMD_PORT_GCL,
> > + BDCR_CMD_RECV_CLASSIFIER,
> > + __BDCR_CMD_MAX_LEN,
> > + BDCR_CMD_MAX_LEN = __BDCR_CMD_MAX_LEN - 1, };
> > +
> > +/* class 5, command 0 */
> > +struct tgs_gcl_conf {
> > + u8 atc; /* init gate value */
> > + u8 res[7];
> > + union {
> > + struct {
> > + u8 res1[4];
> > + __le16 acl_len;
>
> Given that u* types are used in this structure I think le16 would be more
> appropriate than __le16.

Here keep the same code style of this .h file. I think it is better to have another patch to fix them all. Do you agree?

>
> > + u8 res2[2];
> > + };
> > + struct {
> > + u32 cctl;
> > + u32 ccth;
> > + };
>
> I'm a little surprised to see host endian values in a structure that appears to be
> written to hardware. Is this intentional?

Will remove.

>
> Also, these fields do not seem to be used in this patch-set.
> Is that also intentional?

This is defined hardware writeback value, can be remove temporary.

>
> > + };
> > +};
> > +
> > +#define ENETC_CBDR_SGL_IOMEN BIT(0)
> > +#define ENETC_CBDR_SGL_IPVEN BIT(3)
> > +#define ENETC_CBDR_SGL_GTST BIT(4)
> > +#define ENETC_CBDR_SGL_IPV_MASK 0xe
>
> Perhaps GENMASK() is appropriate here and elsewhere for generating masks.
>

Will remove these define in this patch.

> > +
> > +/* gate control list entry */
> > +struct gce {
> > + u32 period;
>
> Likewise, I'm a little surprised to see host-byte order data to be written to HW.
> And below too. Though as I've noted below, some of these values are used to
> store le32 data, so it seems that the types are incorrect

Ok. change to le32.

>
> > + u8 gate;
> > + u8 res[3];
> > +};
> > +
> > +/* tgs_gcl_conf address point to this data space */ struct
> > +tgs_gcl_data {
> > + u32 btl;
> > + u32 bth;
> > + u32 ct;
> > + u32 cte;
> > +};
> > +

Ok. change to le32.

> > +/* class 5, command 1 */
> > +struct tgs_gcl_query {
> > + u8 res[12];
> > + union {
> > + struct {
> > + __le16 acl_len; /* admin list length */
> > + __le16 ocl_len; /* operation list length */
> > + };
> > + struct {
> > + u16 admin_list_len;
> > + u16 oper_list_len;
>
> Again, is it intentional that these are a) host-byte order and
> b) unused?

Ok. Can be remove temporary.

>
> > + };
> > + };
> > +};
> > +
> > +/* tgs_gcl_query command response data format */ struct tgs_gcl_resp
> > +{
> > + u32 abtl; /* base time */
> > + u32 abth;
> > + u32 act; /* cycle time */
> > + u32 acte; /* cycle time extend */
> > + u32 cctl; /* config change time */
> > + u32 ccth;
> > + u32 obtl; /* operation base time */
> > + u32 obth;
> > + u32 oct; /* operation cycle time */
> > + u32 octe; /* operation cycle time extend */
> > + u32 ccel; /* config change error */
> > + u32 cceh;
> > +};
>
> This structure seems unused in this patchset.

Will remove.

>
> > +
> > +struct enetc_cbd {
> > + union{
> > + struct {
> > + __le32 addr[2];
> > + union {
> > + __le32 opt[4];
> > + struct tgs_gcl_conf gcl_conf;
> > + struct tgs_gcl_query gcl_query;
> > + };
> > + }; /* Long format */
> > + __le32 data[6];
> > + };
> > + __le16 index;
> > + __le16 length;
> > + u8 cmd;
> > + u8 cls;
> > + u8 _res;
> > + u8 status_flags;
> > +};
> > +
> > +#define ENETC_PTCFPR(n) (0x1910 + (n) * 4) /* n = [0 ..7] */
> > +#define ENETC_FPE BIT(31)
> > +
> > +/* Port capability register 0 */
> > +#define ENETC_PCAPR0_PSFPM BIT(10)
> > +#define ENETC_PCAPR0_PSFP BIT(9)
> > +#define ENETC_PCAPR0_TSN BIT(4)
> > +#define ENETC_PCAPR0_QBU BIT(3)
> > +
> > +/* port time gating control register */
> > +#define ENETC_QBV_PTGCR_OFFSET 0x11a00
> > +#define ENETC_QBV_TGE 0x80000000
> > +#define ENETC_QBV_TGPE BIT(30)
> > +#define ENETC_QBV_TGDROP_DISABLE BIT(29)
> > +
> > +/* Port time gating capability register */
> > +#define ENETC_QBV_PTGCAPR_OFFSET 0x11a08
> > +#define ENETC_QBV_MAX_GCL_LEN_MASK 0xffff
> > +
> > +/* Port time gating admin gate list status register */
> > +#define ENETC_QBV_PTGAGLSR_OFFSET 0x11a10
> > +#define ENETC_QBV_CFG_PEND_MASK 0x00000002
> > +
> > +#define ENETC_TGLSTR 0xa200
> > +#define ENETC_TGS_MIN_DIS_MASK 0x80000000
> > +#define ENETC_MIN_LOOKAHEAD_MASK 0xffff
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> > b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> > new file mode 100644
> > index 000000000000..036bb39c7a0b
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > +/* Copyright 2019 NXP */
> > +
> > +#include "enetc.h"
> > +
> > +#include <net/pkt_sched.h>
> > +
> > +static u16 enetc_get_max_gcl_len(struct enetc_hw *hw) {
> > + return enetc_rd(hw, ENETC_QBV_PTGCAPR_OFFSET)
> > + & ENETC_QBV_MAX_GCL_LEN_MASK; }
> > +
> > +static int enetc_setup_taprio(struct net_device *ndev,
> > + struct tc_taprio_qopt_offload *admin_conf)
> > +{
> > + struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > + struct enetc_cbd cbd = {.cmd = 0};
> > + struct tgs_gcl_conf *gcl_config;
> > + struct tgs_gcl_data *gcl_data;
> > + struct gce *gce;
> > + dma_addr_t dma;
> > + u16 data_size;
> > + u16 gcl_len;
> > + u32 temp;
> > + int i;
> > +
> > + gcl_len = admin_conf->num_entries;
> > + if (gcl_len > enetc_get_max_gcl_len(&priv->si->hw))
> > + return -EINVAL;
> > +
> > + if (admin_conf->enable) {
> > + enetc_wr(&priv->si->hw,
> > + ENETC_QBV_PTGCR_OFFSET,
> > + temp & (~ENETC_QBV_TGE));
>
> The enetc_wr() call seems to be common to both arms of the condition.
> If so perhaps it could be move outside of the condition.

Ok. will move it down after the if condition.

>
> > + usleep_range(10, 20);
> > + enetc_wr(&priv->si->hw,
> > + ENETC_QBV_PTGCR_OFFSET,
> > + temp | ENETC_QBV_TGE);
> > + } else {
> > + enetc_wr(&priv->si->hw,
> > + ENETC_QBV_PTGCR_OFFSET,
> > + temp & (~ENETC_QBV_TGE));
> > + return 0;
> > + }
> > +
> > + /* Configure the (administrative) gate control list using the
> > + * control BD descriptor.
> > + */
> > + gcl_config = &cbd.gcl_conf;
> > +
> > + data_size = sizeof(struct tgs_gcl_data) + gcl_len *
> > + sizeof(struct gce);
>
> I think struct_size() can be used here.

Ok.

>
> > +
> > + gcl_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL);
> > + if (!gcl_data)
> > + return -ENOMEM;
> > +
> > + gce = (struct gce *)(gcl_data + 1);
> > +
> > + /* Since no initial state config in taprio, set gates open as default.
> > + */
> > + gcl_config->atc = 0xff;
> > + gcl_config->acl_len = cpu_to_le16(gcl_len);
> > +
> > + if (!admin_conf->base_time) {
> > + gcl_data->btl =
> > + cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR0));
> > + gcl_data->bth =
> > + cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR1));
> > + } else {
> > + gcl_data->btl =
> > + cpu_to_le32(lower_32_bits(admin_conf->base_time));
> > + gcl_data->bth =
> > + cpu_to_le32(upper_32_bits(admin_conf->base_time));
> > + }
>
> It looks like the types of the btl and bth fields are u32.
> Perhaps they should be le32?

Ok. should be le32.

>
> Please consider running sparse over this code to check for any endian problems.
>
> > +
> > + gcl_data->ct = cpu_to_le32(admin_conf->cycle_time);
> > + gcl_data->cte = cpu_to_le32(admin_conf->cycle_time_extension);
> > +
> > + for (i = 0; i < gcl_len; i++) {
> > + struct tc_taprio_sched_entry *temp_entry;
> > + struct gce *temp_gce = gce + i;
> > +
> > + temp_entry = &admin_conf->entries[i];
> > +
> > + temp_gce->gate = cpu_to_le32(temp_entry->gate_mask);
>
> Gate is a u8 followed by 3 reserved bytes.
> Perhaps there needs to be some bounds checking on
> the value stored there given that the source is 32bits wide.
>
> Also, its not clear to me that the above logic, which I assume
> takes the last significant byte of a 32bit value, works on
> big endian systems as the 32bit value is always little endian.

temp_entry->gate_mask is 32bit for wide possible input. Here change to hardware set 8bit wide.
Can it just be like:
temp_gce->gate = (u8) temp_entry->gate_mask;

>
> > + temp_gce->period = cpu_to_le32(temp_entry->interval);
>
> It looks like the types of the gate field is u32.
> Perhaps it should be le32?

Ok. should be le32.

>
> > + }
> > +
> > + cbd.length = cpu_to_le16(data_size);
> > + cbd.status_flags = 0;
> > +
> > + dma = dma_map_single(&priv->si->pdev->dev, gcl_data,
> > + data_size, DMA_TO_DEVICE);
> > + if (dma_mapping_error(&priv->si->pdev->dev, dma)) {
> > + netdev_err(priv->si->ndev, "DMA mapping failed!\n");
> > + kfree(gcl_data);
> > + return -ENOMEM;
> > + }
> > +
> > + cbd.addr[0] = lower_32_bits(dma);
> > + cbd.addr[1] = upper_32_bits(dma);
> > + cbd.cls = BDCR_CMD_PORT_GCL;
> > +
> > + /* Updated by ENETC on completion of the configuration
> > + * command. A zero value indicates success.
> > + */
> > + cbd.status_flags = 0;
> > +
> > + enetc_send_cmd(priv->si, &cbd);
> > +
> > + dma_unmap_single(&priv->si->pdev->dev, dma, data_size,
> DMA_TO_DEVICE);
> > + kfree(gcl_data);
> > +
> > + return 0;
> > +}
> > +
> > +int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data) {
> > + struct tc_taprio_qopt_offload *taprio = type_data;
> > + struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > + int i;
> > +
> > + for (i = 0; i < priv->num_tx_rings; i++)
> > + enetc_set_bdr_prio(&priv->si->hw,
> > + priv->tx_ring[i]->index,
> > + taprio->enable ? i : 0);
> > +
> > + return enetc_setup_taprio(ndev, taprio); }
> > --
> > 2.17.1
> >

2019-11-12 11:57:46

by Claudiu Manoil

Subject: RE: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

>-----Original Message-----
>From: Po Liu <[email protected]>
[...]
>> -----Original Message-----
>> From: Simon Horman <[email protected]>
[...]
>> > +/* class 5, command 0 */
>> > +struct tgs_gcl_conf {
>> > + u8 atc; /* init gate value */
>> > + u8 res[7];
>> > + union {
>> > + struct {
>> > + u8 res1[4];
>> > + __le16 acl_len;
>>
>> Given that u* types are used in this structure I think le16 would be more
>> appropriate than __le16.
>
>Here keep the same code style of this .h file. I think it is better to have
>another patch to fix them all. Do you agree?
>

I don't see why "le16" would be more appropriate than "__le16" in this context.
The "__leXX" types are widely used in kernel drivers and not only, to annotate the
endianess of the hardware. These are generic types defined din "include/uapi/linux/types.h".
Whereas "leXX" are defined in "fs/ntfs/types.h", and there's no usage of these types
in other h/w device drivers (I didn't find any). Am I missing anything?

-Claudiu

2019-11-12 12:32:30

by kernel test robot

Subject: Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

Hi Po,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on v5.4-rc7 next-20191111]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Po-Liu/enetc-Configure-the-Time-Aware-Scheduler-via-tc-taprio-offload/20191112-193235
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git ca22d6977b9b4ab0fd2e7909b57e32ba5b95046f
config: s390-allmodconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=s390

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

In file included from arch/s390/include/asm/io.h:76:0,
from include/linux/io.h:13,
from include/linux/pci.h:39,
from drivers/net/ethernet/freescale/enetc/enetc.h:5,
from drivers/net/ethernet/freescale/enetc/enetc_qos.c:4:
drivers/net/ethernet/freescale/enetc/enetc_qos.c: In function 'enetc_setup_tc_taprio':
>> include/asm-generic/io.h:735:19: warning: 'temp' may be used uninitialized in this function [-Wmaybe-uninitialized]
#define iowrite32 iowrite32
^~~~~~~~~
drivers/net/ethernet/freescale/enetc/enetc_qos.c:25:6: note: 'temp' was declared here
u32 temp;
^~~~
--
In file included from arch/s390/include/asm/io.h:76:0,
from include/linux/io.h:13,
from include/linux/pci.h:39,
from drivers/net//ethernet/freescale/enetc/enetc.h:5,
from drivers/net//ethernet/freescale/enetc/enetc_qos.c:4:
drivers/net//ethernet/freescale/enetc/enetc_qos.c: In function 'enetc_setup_tc_taprio':
>> include/asm-generic/io.h:735:19: warning: 'temp' may be used uninitialized in this function [-Wmaybe-uninitialized]
#define iowrite32 iowrite32
^~~~~~~~~
drivers/net//ethernet/freescale/enetc/enetc_qos.c:25:6: note: 'temp' was declared here
u32 temp;
^~~~

vim +/temp +735 include/asm-generic/io.h

9216efafc52ff9 Thierry Reding 2014-10-01 733
9216efafc52ff9 Thierry Reding 2014-10-01 734 #ifndef iowrite32
9216efafc52ff9 Thierry Reding 2014-10-01 @735 #define iowrite32 iowrite32
9216efafc52ff9 Thierry Reding 2014-10-01 736 static inline void iowrite32(u32 value, volatile void __iomem *addr)
9216efafc52ff9 Thierry Reding 2014-10-01 737 {
9216efafc52ff9 Thierry Reding 2014-10-01 738 writel(value, addr);
9216efafc52ff9 Thierry Reding 2014-10-01 739 }
9216efafc52ff9 Thierry Reding 2014-10-01 740 #endif
9216efafc52ff9 Thierry Reding 2014-10-01 741

:::::: The code at line 735 was first introduced by commit
:::::: 9216efafc52ff99e9351ef60de5fcafc2bc8adb6 asm-generic/io.h: Reconcile I/O accessor overrides

:::::: TO: Thierry Reding <[email protected]>
:::::: CC: Thierry Reding <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation

Attachments:

2019-11-12 12:47:41

by kernel test robot

Subject: Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

Hi Po,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on v5.4-rc7 next-20191112]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Po-Liu/enetc-Configure-the-Time-Aware-Scheduler-via-tc-taprio-offload/20191112-193235
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git ca22d6977b9b4ab0fd2e7909b57e32ba5b95046f
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=arm64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

drivers/net/ethernet/freescale/enetc/enetc_qos.c: In function 'enetc_setup_tc_taprio':
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:25:6: warning: 'temp' may be used uninitialized in this function [-Wmaybe-uninitialized]
u32 temp;
^~~~

vim +/temp +25 drivers/net/ethernet/freescale/enetc/enetc_qos.c

13
14 static int enetc_setup_taprio(struct net_device *ndev,
15 struct tc_taprio_qopt_offload *admin_conf)
16 {
17 struct enetc_ndev_priv *priv = netdev_priv(ndev);
18 struct enetc_cbd cbd = {.cmd = 0};
19 struct tgs_gcl_conf *gcl_config;
20 struct tgs_gcl_data *gcl_data;
21 struct gce *gce;
22 dma_addr_t dma;
23 u16 data_size;
24 u16 gcl_len;
> 25 u32 temp;
26 int i;
27
28 gcl_len = admin_conf->num_entries;
29 if (gcl_len > enetc_get_max_gcl_len(&priv->si->hw))
30 return -EINVAL;
31
32 if (admin_conf->enable) {
33 enetc_wr(&priv->si->hw,
34 ENETC_QBV_PTGCR_OFFSET,
35 temp & (~ENETC_QBV_TGE));
36 usleep_range(10, 20);
37 enetc_wr(&priv->si->hw,
38 ENETC_QBV_PTGCR_OFFSET,
39 temp | ENETC_QBV_TGE);
40 } else {
41 enetc_wr(&priv->si->hw,
42 ENETC_QBV_PTGCR_OFFSET,
43 temp & (~ENETC_QBV_TGE));
44 return 0;
45 }
46
47 /* Configure the (administrative) gate control list using the
48 * control BD descriptor.
49 */
50 gcl_config = &cbd.gcl_conf;
51
52 data_size = sizeof(struct tgs_gcl_data) + gcl_len * sizeof(struct gce);
53
54 gcl_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL);
55 if (!gcl_data)
56 return -ENOMEM;
57
58 gce = (struct gce *)(gcl_data + 1);
59
60 /* Since no initial state config in taprio, set gates open as default.
61 */
62 gcl_config->atc = 0xff;
63 gcl_config->acl_len = cpu_to_le16(gcl_len);
64
65 if (!admin_conf->base_time) {
66 gcl_data->btl =
67 cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR0));
68 gcl_data->bth =
69 cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR1));
70 } else {
71 gcl_data->btl =
72 cpu_to_le32(lower_32_bits(admin_conf->base_time));
73 gcl_data->bth =
74 cpu_to_le32(upper_32_bits(admin_conf->base_time));
75 }
76
77 gcl_data->ct = cpu_to_le32(admin_conf->cycle_time);
78 gcl_data->cte = cpu_to_le32(admin_conf->cycle_time_extension);
79
80 for (i = 0; i < gcl_len; i++) {
81 struct tc_taprio_sched_entry *temp_entry;
82 struct gce *temp_gce = gce + i;
83
84 temp_entry = &admin_conf->entries[i];
85
86 temp_gce->gate = cpu_to_le32(temp_entry->gate_mask);
87 temp_gce->period = cpu_to_le32(temp_entry->interval);
88 }
89
90 cbd.length = cpu_to_le16(data_size);
91 cbd.status_flags = 0;
92
93 dma = dma_map_single(&priv->si->pdev->dev, gcl_data,
94 data_size, DMA_TO_DEVICE);
95 if (dma_mapping_error(&priv->si->pdev->dev, dma)) {
96 netdev_err(priv->si->ndev, "DMA mapping failed!\n");
97 kfree(gcl_data);
98 return -ENOMEM;
99 }
100
101 cbd.addr[0] = lower_32_bits(dma);
102 cbd.addr[1] = upper_32_bits(dma);
103 cbd.cls = BDCR_CMD_PORT_GCL;
104
105 /* Updated by ENETC on completion of the configuration
106 * command. A zero value indicates success.
107 */
108 cbd.status_flags = 0;
109
110 enetc_send_cmd(priv->si, &cbd);
111
112 dma_unmap_single(&priv->si->pdev->dev, dma, data_size, DMA_TO_DEVICE);
113 kfree(gcl_data);
114
115 return 0;
116 }
117

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation

Attachments:

2019-11-12 13:47:52

by Simon Horman

Subject: Re: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

On Tue, Nov 12, 2019 at 11:54:29AM +0000, Claudiu Manoil wrote:
> >-----Original Message-----
> >From: Po Liu <[email protected]>
> [...]
> >> -----Original Message-----
> >> From: Simon Horman <[email protected]>
> [...]
> >> > +/* class 5, command 0 */
> >> > +struct tgs_gcl_conf {
> >> > + u8 atc; /* init gate value */
> >> > + u8 res[7];
> >> > + union {
> >> > + struct {
> >> > + u8 res1[4];
> >> > + __le16 acl_len;
> >>
> >> Given that u* types are used in this structure I think le16 would be more
> >> appropriate than __le16.
> >
> >Here keep the same code style of this .h file. I think it is better to have
> >another patch to fix them all. Do you agree?
> >
>
> I don't see why "le16" would be more appropriate than "__le16" in this context.
> The "__leXX" types are widely used in kernel drivers and not only, to annotate the
> endianess of the hardware. These are generic types defined din "include/uapi/linux/types.h".
> Whereas "leXX" are defined in "fs/ntfs/types.h", and there's no usage of these types
> in other h/w device drivers (I didn't find any). Am I missing anything?

My point is a cosmetic one:
I think that __u8 goes with __le16, while u8 goes with le16.

2019-11-12 13:54:25

by Simon Horman

Subject: Re: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

On Tue, Nov 12, 2019 at 11:19:43AM +0000, Po Liu wrote:

...

> > > +/* class 5, command 0 */
> > > +struct tgs_gcl_conf {
> > > + u8 atc; /* init gate value */
> > > + u8 res[7];
> > > + union {
> > > + struct {
> > > + u8 res1[4];
> > > + __le16 acl_len;
> >
> > Given that u* types are used in this structure I think le16 would be more
> > appropriate than __le16.
>
> Here keep the same code style of this .h file. I think it is better to have another patch to fix them all. Do you agree?
>
> >

> > > + u8 res2[2];
> > > + };
> > > + struct {
> > > + u32 cctl;
> > > + u32 ccth;
> > > + };
> >
> > I'm a little surprised to see host endian values in a structure that appears to be
> > written to hardware. Is this intentional?
>
> Will remove.

If the HW defines these fields then I think its fine to leave them,
though with the correct byte-order.

I was more asking if it is intentional that the value for these
fields, when sent to the HW, is always zero in the context of this
patch-set. Likewise elsewhere.

...

> > > +
> > > + gcl_data->ct = cpu_to_le32(admin_conf->cycle_time);
> > > + gcl_data->cte = cpu_to_le32(admin_conf->cycle_time_extension);
> > > +
> > > + for (i = 0; i < gcl_len; i++) {
> > > + struct tc_taprio_sched_entry *temp_entry;
> > > + struct gce *temp_gce = gce + i;
> > > +
> > > + temp_entry = &admin_conf->entries[i];
> > > +
> > > + temp_gce->gate = cpu_to_le32(temp_entry->gate_mask);
> >
> > Gate is a u8 followed by 3 reserved bytes.
> > Perhaps there needs to be some bounds checking on
> > the value stored there given that the source is 32bits wide.
> >
> > Also, its not clear to me that the above logic, which I assume
> > takes the last significant byte of a 32bit value, works on
> > big endian systems as the 32bit value is always little endian.
>
> temp_entry->gate_mask is 32bit for wide possible input. Here change to hardware set 8bit wide.
> Can it just be like:
> temp_gce->gate = (u8) temp_entry->gate_mask;

I think that would be better.
Perhaps its best to also mask out the unwanted bits.

...

2019-11-12 16:05:29

by Ivan Khoronzhuk

Subject: Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

On Mon, Nov 11, 2019 at 04:41:26AM +0000, Po Liu wrote:
>ENETC supports in hardware for time-based egress shaping according
>to IEEE 802.1Qbv. This patch implement the Qbv enablement by the
>hardware offload method qdisc tc-taprio method.
>Also update cbdr writeback to up level since control bd ring may
>writeback data to control bd ring.
>
>Signed-off-by: Po Liu <[email protected]>
>Singed-off-by: Vladimir Oltean <[email protected]>
>Signed-off-by: Claudiu Manoil <[email protected]>
>---
> drivers/net/ethernet/freescale/enetc/Makefile | 1 +
> drivers/net/ethernet/freescale/enetc/enetc.c | 19 ++-
> drivers/net/ethernet/freescale/enetc/enetc.h | 2 +
> .../net/ethernet/freescale/enetc/enetc_cbdr.c | 5 +-
> .../net/ethernet/freescale/enetc/enetc_hw.h | 150 ++++++++++++++++--
> .../net/ethernet/freescale/enetc/enetc_qos.c | 130 +++++++++++++++
> 6 files changed, 285 insertions(+), 22 deletions(-)
> create mode 100644 drivers/net/ethernet/freescale/enetc/enetc_qos.c
>
>diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile
>index d200c27c3bf6..389f722efc43 100644
>--- a/drivers/net/ethernet/freescale/enetc/Makefile
>+++ b/drivers/net/ethernet/freescale/enetc/Makefile
>@@ -5,6 +5,7 @@ common-objs := enetc.o enetc_cbdr.o enetc_ethtool.o
> obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o
> fsl-enetc-y := enetc_pf.o enetc_mdio.o $(common-objs)
> fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o
>+fsl-enetc-$(CONFIG_NET_SCH_TAPRIO) += enetc_qos.o
>
> obj-$(CONFIG_FSL_ENETC_VF) += fsl-enetc-vf.o
> fsl-enetc-vf-y := enetc_vf.o $(common-objs)
>diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
>index 3e8f9819f08c..d58dbc2c4270 100644
>--- a/drivers/net/ethernet/freescale/enetc/enetc.c
>+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
>@@ -1427,8 +1427,7 @@ int enetc_close(struct net_device *ndev)
> return 0;
> }
>
>-int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>- void *type_data)
>+int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
> {
> struct enetc_ndev_priv *priv = netdev_priv(ndev);
> struct tc_mqprio_qopt *mqprio = type_data;
>@@ -1436,9 +1435,6 @@ int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> u8 num_tc;
> int i;
>
>- if (type != TC_SETUP_QDISC_MQPRIO)
>- return -EOPNOTSUPP;
>-
> mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
> num_tc = mqprio->num_tc;
>
>@@ -1483,6 +1479,19 @@ int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> return 0;
> }
>
>+int enetc_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>+ void *type_data)
>+{
>+ switch (type) {
>+ case TC_SETUP_QDISC_MQPRIO:
>+ return enetc_setup_tc_mqprio(ndev, type_data);

This patch is for taprio offload, I see that mqprio is related and is part of
taprio offload configuration. But taprio offload has own mqprio settings.
The taprio mqprio part is not offloaded with TC_SETUP_QDISC_MQPRIO.

So, a combination of mqprio and tario qdiscs used.
Could you please share the commands were used for your setup?

And couple interesting questions about all of this:
- The taprio qdisc has to have mqprio settings, but if it's done with
mqprio then it just skipped (by reading tc class num).
- If no separate mqprio qdisc configuration then mqprio conf from taprio
is set, who should restore tc mappings when taprio qdisc is unloaded?

Maybe there is reason to implement TC_SETUP_QDISC_MQPRIO offload in taprio
since it's required feature?

Would be better to move changes for mqprio in separate patch with explanation.

>+ case TC_SETUP_QDISC_TAPRIO:
>+ return enetc_setup_tc_taprio(ndev, type_data);
>+ default:
>+ return -EOPNOTSUPP;
>+ }
>+}
>+
> struct net_device_stats *enetc_get_stats(struct net_device *ndev)
> {
> struct enetc_ndev_priv *priv = netdev_priv(ndev);
>diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
>index 541b4e2073fe..8676631041d5 100644
>--- a/drivers/net/ethernet/freescale/enetc/enetc.h
>+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
>@@ -244,3 +244,5 @@ int enetc_set_fs_entry(struct enetc_si *si, struct enetc_cmd_rfse *rfse,
> void enetc_set_rss_key(struct enetc_hw *hw, const u8 *bytes);
> int enetc_get_rss_table(struct enetc_si *si, u32 *table, int count);
> int enetc_set_rss_table(struct enetc_si *si, const u32 *table, int count);
>+int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd);
>+int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data);
>diff --git a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
>index de466b71bf8f..201cbc362e33 100644
>--- a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
>+++ b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
>@@ -32,7 +32,7 @@ static int enetc_cbd_unused(struct enetc_cbdr *r)
> r->bd_count;
> }
>
>-static int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
>+int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> {
> struct enetc_cbdr *ring = &si->cbd_ring;
> int timeout = ENETC_CBDR_TIMEOUT;
>@@ -66,6 +66,9 @@ static int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd)
> if (!timeout)
> return -EBUSY;
>
>+ /* CBD may writeback data, feedback up level */
>+ *cbd = *dest_cbd;
>+
> enetc_clean_cbdr(si);
>
> return 0;
>diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
>index 88276299f447..75a7c0f1f8ce 100644
>--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
>+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
>@@ -18,6 +18,7 @@
> #define ENETC_SICTR0 0x18
> #define ENETC_SICTR1 0x1c
> #define ENETC_SIPCAPR0 0x20
>+#define ENETC_SIPCAPR0_QBV BIT(4)
> #define ENETC_SIPCAPR0_RSS BIT(8)
> #define ENETC_SIPCAPR1 0x24
> #define ENETC_SITGTGR 0x30
>@@ -148,6 +149,12 @@ enum enetc_bdr_type {TX, RX};
> #define ENETC_PORT_BASE 0x10000
> #define ENETC_PMR 0x0000
> #define ENETC_PMR_EN GENMASK(18, 16)
>+#define ENETC_PMR_PSPEED_MASK GENMASK(11, 8)
>+#define ENETC_PMR_PSPEED_10M 0x000
>+#define ENETC_PMR_PSPEED_100M 0x100
>+#define ENETC_PMR_PSPEED_1000M 0x200
>+#define ENETC_PMR_PSPEED_2500M 0x400
>+
> #define ENETC_PSR 0x0004 /* RO */
> #define ENETC_PSIPMR 0x0018
> #define ENETC_PSIPMR_SET_UP(n) BIT(n) /* n = SI index */
>@@ -440,22 +447,6 @@ union enetc_rx_bd {
> #define EMETC_MAC_ADDR_FILT_RES 3 /* # of reserved entries at the beginning */
> #define ENETC_MAX_NUM_VFS 2
>
>-struct enetc_cbd {
>- union {
>- struct {
>- __le32 addr[2];
>- __le32 opt[4];
>- };
>- __le32 data[6];
>- };
>- __le16 index;
>- __le16 length;
>- u8 cmd;
>- u8 cls;
>- u8 _res;
>- u8 status_flags;
>-};
>-
> #define ENETC_CBD_FLAGS_SF BIT(7) /* short format */
> #define ENETC_CBD_STATUS_MASK 0xf
>
>@@ -554,3 +545,130 @@ static inline void enetc_set_bdr_prio(struct enetc_hw *hw, int bdr_idx,
> val |= ENETC_TBMR_SET_PRIO(prio);
> enetc_txbdr_wr(hw, bdr_idx, ENETC_TBMR, val);
> }
>+
>+enum bdcr_cmd_class {
>+ BDCR_CMD_UNSPEC = 0,
>+ BDCR_CMD_MAC_FILTER,
>+ BDCR_CMD_VLAN_FILTER,
>+ BDCR_CMD_RSS,
>+ BDCR_CMD_RFS,
>+ BDCR_CMD_PORT_GCL,
>+ BDCR_CMD_RECV_CLASSIFIER,
>+ __BDCR_CMD_MAX_LEN,
>+ BDCR_CMD_MAX_LEN = __BDCR_CMD_MAX_LEN - 1,
>+};
>+
>+/* class 5, command 0 */
>+struct tgs_gcl_conf {
>+ u8 atc; /* init gate value */
>+ u8 res[7];
>+ union {
>+ struct {
>+ u8 res1[4];
>+ __le16 acl_len;
>+ u8 res2[2];
>+ };
>+ struct {
>+ u32 cctl;
>+ u32 ccth;
>+ };
>+ };
>+};
>+
>+#define ENETC_CBDR_SGL_IOMEN BIT(0)
>+#define ENETC_CBDR_SGL_IPVEN BIT(3)
>+#define ENETC_CBDR_SGL_GTST BIT(4)
>+#define ENETC_CBDR_SGL_IPV_MASK 0xe
>+
>+/* gate control list entry */
>+struct gce {
>+ u32 period;
>+ u8 gate;
>+ u8 res[3];
>+};
>+
>+/* tgs_gcl_conf address point to this data space */
>+struct tgs_gcl_data {
>+ u32 btl;
>+ u32 bth;
>+ u32 ct;
>+ u32 cte;
>+};
>+
>+/* class 5, command 1 */
>+struct tgs_gcl_query {
>+ u8 res[12];
>+ union {
>+ struct {
>+ __le16 acl_len; /* admin list length */
>+ __le16 ocl_len; /* operation list length */
>+ };
>+ struct {
>+ u16 admin_list_len;
>+ u16 oper_list_len;
>+ };
>+ };
>+};
>+
>+/* tgs_gcl_query command response data format */
>+struct tgs_gcl_resp {
>+ u32 abtl; /* base time */
>+ u32 abth;
>+ u32 act; /* cycle time */
>+ u32 acte; /* cycle time extend */
>+ u32 cctl; /* config change time */
>+ u32 ccth;
>+ u32 obtl; /* operation base time */
>+ u32 obth;
>+ u32 oct; /* operation cycle time */
>+ u32 octe; /* operation cycle time extend */
>+ u32 ccel; /* config change error */
>+ u32 cceh;
>+};
>+
>+struct enetc_cbd {
>+ union{
>+ struct {
>+ __le32 addr[2];
>+ union {
>+ __le32 opt[4];
>+ struct tgs_gcl_conf gcl_conf;
>+ struct tgs_gcl_query gcl_query;
>+ };
>+ }; /* Long format */
>+ __le32 data[6];
>+ };
>+ __le16 index;
>+ __le16 length;
>+ u8 cmd;
>+ u8 cls;
>+ u8 _res;
>+ u8 status_flags;
>+};
>+
>+#define ENETC_PTCFPR(n) (0x1910 + (n) * 4) /* n = [0 ..7] */
>+#define ENETC_FPE BIT(31)
>+
>+/* Port capability register 0 */
>+#define ENETC_PCAPR0_PSFPM BIT(10)
>+#define ENETC_PCAPR0_PSFP BIT(9)
>+#define ENETC_PCAPR0_TSN BIT(4)
>+#define ENETC_PCAPR0_QBU BIT(3)
>+
>+/* port time gating control register */
>+#define ENETC_QBV_PTGCR_OFFSET 0x11a00
>+#define ENETC_QBV_TGE 0x80000000
>+#define ENETC_QBV_TGPE BIT(30)
>+#define ENETC_QBV_TGDROP_DISABLE BIT(29)
>+
>+/* Port time gating capability register */
>+#define ENETC_QBV_PTGCAPR_OFFSET 0x11a08
>+#define ENETC_QBV_MAX_GCL_LEN_MASK 0xffff
>+
>+/* Port time gating admin gate list status register */
>+#define ENETC_QBV_PTGAGLSR_OFFSET 0x11a10
>+#define ENETC_QBV_CFG_PEND_MASK 0x00000002
>+
>+#define ENETC_TGLSTR 0xa200
>+#define ENETC_TGS_MIN_DIS_MASK 0x80000000
>+#define ENETC_MIN_LOOKAHEAD_MASK 0xffff
>diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
>new file mode 100644
>index 000000000000..036bb39c7a0b
>--- /dev/null
>+++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
>@@ -0,0 +1,130 @@
>+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
>+/* Copyright 2019 NXP */
>+
>+#include "enetc.h"
>+
>+#include <net/pkt_sched.h>
>+
>+static u16 enetc_get_max_gcl_len(struct enetc_hw *hw)
>+{
>+ return enetc_rd(hw, ENETC_QBV_PTGCAPR_OFFSET)
>+ & ENETC_QBV_MAX_GCL_LEN_MASK;
>+}
>+
>+static int enetc_setup_taprio(struct net_device *ndev,
>+ struct tc_taprio_qopt_offload *admin_conf)
>+{
>+ struct enetc_ndev_priv *priv = netdev_priv(ndev);
>+ struct enetc_cbd cbd = {.cmd = 0};
>+ struct tgs_gcl_conf *gcl_config;
>+ struct tgs_gcl_data *gcl_data;
>+ struct gce *gce;
>+ dma_addr_t dma;
>+ u16 data_size;
>+ u16 gcl_len;
>+ u32 temp;
>+ int i;
>+
>+ gcl_len = admin_conf->num_entries;
>+ if (gcl_len > enetc_get_max_gcl_len(&priv->si->hw))
>+ return -EINVAL;
>+
>+ if (admin_conf->enable) {
>+ enetc_wr(&priv->si->hw,
>+ ENETC_QBV_PTGCR_OFFSET,
>+ temp & (~ENETC_QBV_TGE));
>+ usleep_range(10, 20);
>+ enetc_wr(&priv->si->hw,
>+ ENETC_QBV_PTGCR_OFFSET,
>+ temp | ENETC_QBV_TGE);
>+ } else {
>+ enetc_wr(&priv->si->hw,
>+ ENETC_QBV_PTGCR_OFFSET,
>+ temp & (~ENETC_QBV_TGE));
>+ return 0;
>+ }
>+
>+ /* Configure the (administrative) gate control list using the
>+ * control BD descriptor.
>+ */
>+ gcl_config = &cbd.gcl_conf;
>+
>+ data_size = sizeof(struct tgs_gcl_data) + gcl_len * sizeof(struct gce);
>+
>+ gcl_data = kzalloc(data_size, __GFP_DMA | GFP_KERNEL);
>+ if (!gcl_data)
>+ return -ENOMEM;
>+
>+ gce = (struct gce *)(gcl_data + 1);
>+
>+ /* Since no initial state config in taprio, set gates open as default.
>+ */
>+ gcl_config->atc = 0xff;
>+ gcl_config->acl_len = cpu_to_le16(gcl_len);
>+
>+ if (!admin_conf->base_time) {
>+ gcl_data->btl =
>+ cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR0));
>+ gcl_data->bth =
>+ cpu_to_le32(enetc_rd(&priv->si->hw, ENETC_SICTR1));
>+ } else {
>+ gcl_data->btl =
>+ cpu_to_le32(lower_32_bits(admin_conf->base_time));
>+ gcl_data->bth =
>+ cpu_to_le32(upper_32_bits(admin_conf->base_time));
>+ }
>+
>+ gcl_data->ct = cpu_to_le32(admin_conf->cycle_time);
>+ gcl_data->cte = cpu_to_le32(admin_conf->cycle_time_extension);
>+
>+ for (i = 0; i < gcl_len; i++) {
>+ struct tc_taprio_sched_entry *temp_entry;
>+ struct gce *temp_gce = gce + i;
>+
>+ temp_entry = &admin_conf->entries[i];
>+
>+ temp_gce->gate = cpu_to_le32(temp_entry->gate_mask);
>+ temp_gce->period = cpu_to_le32(temp_entry->interval);
>+ }
>+
>+ cbd.length = cpu_to_le16(data_size);
>+ cbd.status_flags = 0;
>+
>+ dma = dma_map_single(&priv->si->pdev->dev, gcl_data,
>+ data_size, DMA_TO_DEVICE);
>+ if (dma_mapping_error(&priv->si->pdev->dev, dma)) {
>+ netdev_err(priv->si->ndev, "DMA mapping failed!\n");
>+ kfree(gcl_data);
>+ return -ENOMEM;
>+ }
>+
>+ cbd.addr[0] = lower_32_bits(dma);
>+ cbd.addr[1] = upper_32_bits(dma);
>+ cbd.cls = BDCR_CMD_PORT_GCL;
>+
>+ /* Updated by ENETC on completion of the configuration
>+ * command. A zero value indicates success.
>+ */
>+ cbd.status_flags = 0;
>+
>+ enetc_send_cmd(priv->si, &cbd);
>+
>+ dma_unmap_single(&priv->si->pdev->dev, dma, data_size, DMA_TO_DEVICE);
>+ kfree(gcl_data);
>+
>+ return 0;
>+}
>+
>+int enetc_setup_tc_taprio(struct net_device *ndev, void *type_data)
>+{
>+ struct tc_taprio_qopt_offload *taprio = type_data;
>+ struct enetc_ndev_priv *priv = netdev_priv(ndev);
>+ int i;
>+
>+ for (i = 0; i < priv->num_tx_rings; i++)
>+ enetc_set_bdr_prio(&priv->si->hw,
>+ priv->tx_ring[i]->index,
>+ taprio->enable ? i : 0);
>+
>+ return enetc_setup_taprio(ndev, taprio);
>+}
>--
>2.17.1
>

--
Regards,
Ivan Khoronzhuk

2019-11-12 19:00:39

by David Miller

Subject: Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

Oops, I didn't see this feedback because v2 had been posted.

I'll revert that now.

Please address Simon's feedback on these two patches, and then post a v3,
thank you.

2019-11-12 19:03:35

by David Miller

Subject: Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

From: David Miller <[email protected]>
Date: Tue, 12 Nov 2019 10:58:59 -0800 (PST)

>
> Oops, I didn't see this feedback because v2 had been posted.
>
> I'll revert that now.
>
> Please address Simon's feedback on these two patches, and then post a v3,
> thank you.

Also, v2 doesn't even compile :-(

In file included from drivers/net/ethernet/freescale/enetc/enetc.h:14,
from drivers/net/ethernet/freescale/enetc/enetc_qos.c:4:
drivers/net/ethernet/freescale/enetc/enetc_qos.c: In function ?enetc_setup_tc_taprio?:
drivers/net/ethernet/freescale/enetc/enetc_hw.h:308:32: warning: ?temp? may be used uninitialized in this function [-Wmaybe-uninitialized]
#define enetc_wr_reg(reg, val) iowrite32((val), (reg))
^~~~~~~~~
drivers/net/ethernet/freescale/enetc/enetc_qos.c:59:6: note: ?temp? was declared here
u32 temp;
^~~~
ERROR: "enetc_sched_speed_set" [drivers/net/ethernet/freescale/enetc/fsl-enetc-vf.ko] undefined!
ERROR: "enetc_setup_tc_taprio" [drivers/net/ethernet/freescale/enetc/fsl-enetc-vf.ko] undefined!
make[1]: *** [scripts/Makefile.modpost:94: __modpost] Error 1
make: *** [Makefile:1282: modules] Error 2

2019-11-12 22:59:44

by kernel test robot

Subject: [RFC PATCH] enetc: enetc_setup_tc_mqprio() can be static

Fixes: 0ffc020dc8f3 ("enetc: Configure the Time-Aware Scheduler via tc-taprio offload")
Signed-off-by: kbuild test robot <[email protected]>
---
enetc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index d58dbc2c4270e..724dc90bf87a2 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1427,7 +1427,7 @@ int enetc_close(struct net_device *ndev)
return 0;
}

-int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
+static int enetc_setup_tc_mqprio(struct net_device *ndev, void *type_data)
{
struct enetc_ndev_priv *priv = netdev_priv(ndev);
struct tc_mqprio_qopt *mqprio = type_data;

2019-11-12 23:02:00

by kernel test robot

Subject: Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

Hi Po,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on v5.4-rc7 next-20191111]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Po-Liu/enetc-Configure-the-Time-Aware-Scheduler-via-tc-taprio-offload/20191112-193235
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git ca22d6977b9b4ab0fd2e7909b57e32ba5b95046f
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-29-g781bc5d-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>


sparse warnings: (new ones prefixed by >>)

>> drivers/net/ethernet/freescale/enetc/enetc.c:1430:5: sparse: sparse: symbol 'enetc_setup_tc_mqprio' was not declared. Should it be static?
--
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:66:31: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] btl @@ got restrunsigned int [usertype] btl @@
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:66:31: sparse: expected unsigned int [usertype] btl
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:66:31: sparse: got restricted __le32 [usertype]
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:68:31: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] bth @@ got restrunsigned int [usertype] bth @@
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:68:31: sparse: expected unsigned int [usertype] bth
drivers/net/ethernet/freescale/enetc/enetc_qos.c:68:31: sparse: got restricted __le32 [usertype]
drivers/net/ethernet/freescale/enetc/enetc_qos.c:71:31: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] btl @@ got restrunsigned int [usertype] btl @@
drivers/net/ethernet/freescale/enetc/enetc_qos.c:71:31: sparse: expected unsigned int [usertype] btl
drivers/net/ethernet/freescale/enetc/enetc_qos.c:71:31: sparse: got restricted __le32 [usertype]
drivers/net/ethernet/freescale/enetc/enetc_qos.c:73:31: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] bth @@ got restrunsigned int [usertype] bth @@
drivers/net/ethernet/freescale/enetc/enetc_qos.c:73:31: sparse: expected unsigned int [usertype] bth
drivers/net/ethernet/freescale/enetc/enetc_qos.c:73:31: sparse: got restricted __le32 [usertype]
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:77:22: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] ct @@ got restrunsigned int [usertype] ct @@
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:77:22: sparse: expected unsigned int [usertype] ct
drivers/net/ethernet/freescale/enetc/enetc_qos.c:77:22: sparse: got restricted __le32 [usertype]
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:78:23: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] cte @@ got restrunsigned int [usertype] cte @@
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:78:23: sparse: expected unsigned int [usertype] cte
drivers/net/ethernet/freescale/enetc/enetc_qos.c:78:23: sparse: got restricted __le32 [usertype]
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:86:32: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned char [usertype] gate @@ got restunsigned char [usertype] gate @@
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:86:32: sparse: expected unsigned char [usertype] gate
drivers/net/ethernet/freescale/enetc/enetc_qos.c:86:32: sparse: got restricted __le32 [usertype]
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:87:34: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] period @@ got restrunsigned int [usertype] period @@
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:87:34: sparse: expected unsigned int [usertype] period
drivers/net/ethernet/freescale/enetc/enetc_qos.c:87:34: sparse: got restricted __le32 [usertype]
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:101:21: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le32 @@ got unsignerestricted __le32 @@
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:101:21: sparse: expected restricted __le32
>> drivers/net/ethernet/freescale/enetc/enetc_qos.c:101:21: sparse: got unsigned int [usertype]
drivers/net/ethernet/freescale/enetc/enetc_qos.c:102:21: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le32 @@ got unsignerestricted __le32 @@
drivers/net/ethernet/freescale/enetc/enetc_qos.c:102:21: sparse: expected restricted __le32
drivers/net/ethernet/freescale/enetc/enetc_qos.c:102:21: sparse: got unsigned int [usertype]

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation

2019-11-13 04:58:04

by Po Liu

Subject: RE: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via tc-taprio offload

Hi David,

Thanks!
Br,
Po Liu

> -----Original Message-----
> From: David Miller <[email protected]>
> Sent: 2019年11月13日 3:00
> To: [email protected]
> Cc: Po Liu <[email protected]>; Claudiu Manoil <[email protected]>;
> [email protected]; [email protected];
> [email protected]; Vladimir Oltean <[email protected]>;
> Alexandru Marginean <[email protected]>; Xiaoliang Yang
> <[email protected]>; Roy Zang <[email protected]>; Mingkai Hu
> <[email protected]>; Jerry Huang <[email protected]>; Leo Li
> <[email protected]>
> Subject: [EXT] Re: [net-next, 1/2] enetc: Configure the Time-Aware Scheduler via
> tc-taprio offload
>
> Caution: EXT Email
>
> From: David Miller <[email protected]>
> Date: Tue, 12 Nov 2019 10:58:59 -0800 (PST)
>
> >
> > Oops, I didn't see this feedback because v2 had been posted.
> >
> > I'll revert that now.
> >
> > Please address Simon's feedback on these two patches, and then post a
> > v3, thank you.
>
> Also, v2 doesn't even compile :-(
>
> In file included from drivers/net/ethernet/freescale/enetc/enetc.h:14,
> from drivers/net/ethernet/freescale/enetc/enetc_qos.c:4:
> drivers/net/ethernet/freescale/enetc/enetc_qos.c: In function
> ʽenetc_setup_tc_taprioʼ:
> drivers/net/ethernet/freescale/enetc/enetc_hw.h:308:32: warning: ʽtempʼ may
> be used uninitialized in this function [-Wmaybe-uninitialized] #define
> enetc_wr_reg(reg, val) iowrite32((val), (reg))
> ^~~~~~~~~
> drivers/net/ethernet/freescale/enetc/enetc_qos.c:59:6: note: ʽtempʼ was
> declared here

More delete one line. Will fix in v3.

> u32 temp;
> ^~~~
> ERROR: "enetc_sched_speed_set" [drivers/net/ethernet/freescale/enetc/fsl-
> enetc-vf.ko] undefined!
> ERROR: "enetc_setup_tc_taprio" [drivers/net/ethernet/freescale/enetc/fsl-
> enetc-vf.ko] undefined!
> make[1]: *** [scripts/Makefile.modpost:94: __modpost] Error 1
> make: *** [Makefile:1282: modules] Error 2

I got this situation.
It happened when set module mode, since vf drvier should not own the taprio set.
Thanks!