Re: [PATCH net-next 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

On Fri, Nov 20, 2020 at 1:33 PM Andrew Lunn [off-list ref] wrote:

quoted

+static const struct xrs700x_mib xrs700x_mibs[] = {
+     {XRS_RX_GOOD_OCTETS_L(0), "rx_good_octets"},
+     {XRS_RX_BAD_OCTETS_L(0), "rx_bad_octets"},
+     {XRS_RX_UNICAST_L(0), "rx_unicast"},
+     {XRS_RX_BROADCAST_L(0), "rx_broadcast"},
+     {XRS_RX_MULTICAST_L(0), "rx_multicast"},
+     {XRS_RX_UNDERSIZE_L(0), "rx_undersize"},
+     {XRS_RX_FRAGMENTS_L(0), "rx_fragments"},
+     {XRS_RX_OVERSIZE_L(0), "rx_oversize"},
+     {XRS_RX_JABBER_L(0), "rx_jabber"},
+     {XRS_RX_ERR_L(0), "rx_err"},
+     {XRS_RX_CRC_L(0), "rx_crc"},
+     {XRS_RX_64_L(0), "rx_64"},
+     {XRS_RX_65_127_L(0), "rx_65_127"},
+     {XRS_RX_128_255_L(0), "rx_128_255"},
+     {XRS_RX_256_511_L(0), "rx_256_511"},
+     {XRS_RX_512_1023_L(0), "rx_512_1023"},
+     {XRS_RX_1024_1536_L(0), "rx_1024_1536"},
+     {XRS_RX_HSR_PRP_L(0), "rx_hsr_prp"},
+     {XRS_RX_WRONGLAN_L(0), "rx_wronglan"},
+     {XRS_RX_DUPLICATE_L(0), "rx_duplicate"},
+     {XRS_TX_OCTETS_L(0), "tx_octets"},
+     {XRS_TX_UNICAST_L(0), "tx_unicast"},
+     {XRS_TX_BROADCAST_L(0), "tx_broadcast"},
+     {XRS_TX_MULTICAST_L(0), "tx_multicast"},
+     {XRS_TX_HSR_PRP_L(0), "tx_hsr_prp"},
+     {XRS_PRIQ_DROP_L(0), "priq_drop"},
+     {XRS_EARLY_DROP_L(0), "early_drop"},

Can we drop the (0). It does not seem to have any purpose, always
being 0.

It hurts my OCD when the register macros don't match the same pattern but okay.

quoted

+};
+

quoted

+static void xrs700x_read_port_counters(struct xrs700x *priv, int port)
+{
+     int i;
+     struct xrs700x_port *p = &priv->ports[port];

Reverse christmas tree. Please check and fix everywhere.

done. I left the order in xrs700x_setup_regmap_range as is because
they're almost the same length and I want them in the order they're in
in the register. Let me know if that's an issue and I'll change it.

quoted

+static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
+                                    u8 state)
+{
+     struct xrs700x *priv = ds->priv;
+     unsigned int val;
+
+     switch (state) {
+     case BR_STATE_DISABLED:
+             val = XRS_PORT_DISABLED;
+             break;
+     case BR_STATE_LISTENING:
+             val = XRS_PORT_DISABLED;
+             break;

No listening state?

No, just forwarding, learning and disabled. See:
https://www.flexibilis.com/downloads/xrs/SpeedChip_XRS7000_3000_User_Manual.pdf
page 82.

quoted

+     case BR_STATE_LEARNING:
+             val = XRS_PORT_LEARNING;
+             break;
+     case BR_STATE_FORWARDING:
+             val = XRS_PORT_FORWARDING;
+             break;
+     case BR_STATE_BLOCKING:
+             val = XRS_PORT_DISABLED;
+             break;

Hum. What exactly does XRS_PORT_DISABLED mean? When blocking, it is
expected you can still send/receive BPDUs.

Datasheet says: "Disabled. Port neither learns MAC addresses nor forwards data."

quoted

+struct xrs700x *xrs700x_switch_alloc(struct device *base, void *priv)
+{
+     struct dsa_switch *ds;
+     struct xrs700x *dev;
+
+     ds = devm_kzalloc(base, sizeof(*ds), GFP_KERNEL);
+     if (!ds)
+             return NULL;
+
+     ds->dev = base;
+     ds->num_ports = DSA_MAX_PORTS;

Is this needed? detect should fill it in.

Removed. I added this before I added detect and forgot to take it out.

quoted

+int xrs700x_switch_register(struct xrs700x *dev)
+{
+     int ret;
+     int i;
+
+     ret = xrs700x_detect(dev);
+     if (ret)
+             return ret;
+
+     ret = xrs700x_setup_regmap_range(dev);
+     if (ret)
+             return ret;
+
+     dev->ports = devm_kzalloc(dev->dev,
+                               sizeof(*dev->ports) * dev->ds->num_ports,
+                               GFP_KERNEL);
+     if (!dev->ports)
+             return -ENOMEM;
+
+     for (i = 0; i < dev->ds->num_ports; i++) {
+             ret = xrs700x_alloc_port_mib(dev, i);
+             if (ret)
+                     return ret;
+     }
+
+     ret = dsa_register_switch(dev->ds);
+
+     if (ret)
+             cancel_delayed_work_sync(&dev->mib_work);

It would be nice to have to symmetry here. It is not obvious what is
starting this? It happens in the setup op? So can this be moved
into the teardown op?

Agreed. Moved to teardown.

quoted

+static int xrs700x_i2c_reg_read(void *context, unsigned int reg,
+                             unsigned int *val)
+{
+     int ret;
+     unsigned char buf[4];
+     struct device *dev = context;
+     struct i2c_client *i2c = to_i2c_client(dev);
+
+     buf[0] = reg >> 23 & 0xff;
+     buf[1] = reg >> 15 & 0xff;
+     buf[2] = reg >> 7 & 0xff;
+     buf[3] = (reg & 0x7f) << 1;
+
+     ret = i2c_master_send(i2c, buf, sizeof(buf));

Are you allowed to perform transfers on stack buffers? I think any I2C
bus driver using DMA is going to be unhappy.

It should be fine. See the following file, there is a good write up about this:
See Documentation/i2c/dma-considerations.rst

quoted

+static const struct of_device_id xrs700x_i2c_dt_ids[] = {
+     { .compatible = "arrow,xrs7003" },
+     { .compatible = "arrow,xrs7004" },
+     {},

Please validate that the compatible string actually matches the switch
found. Otherwise we can get into all sorts of horrible backward
compatibility issues.

Okay. What kind of compatibility issues? Do you have a hypothetical
example? I guess I will just use of_device_is_compatible() to check.

quoted

+static const struct of_device_id xrs700x_mdio_dt_ids[] = {
+     { .compatible = "arrow,xrs7003" },
+     { .compatible = "arrow,xrs7004" },
+     {},

Same here.

     Andrew

Thanks