Re: [PATCH v5 2/3] pci, pci-thunder-pem: Add PCIe host driver for ThunderX processors.

Hi David,

I don't have time today to look at this thoroughly, but I think I did
see one actual bug and a possible way to make this more readable.  I
think reading difficulty increases as at least the square of the
maximum indent level :)

Bjorn

On Fri, Feb 05, 2016 at 03:41:14PM -0800, David Daney wrote:

...
+static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 val)
+{
+	struct gen_pci *pci = bus->sysdata;
+	struct thunder_pem_pci *pem_pci;
+
+	pem_pci = container_of(pci, struct thunder_pem_pci, gen_pci);
+
+	/*
+	 * The first device on the bus in the PEM PCIe bridge.
+	 * Special case its config access.
+	 */
+	if (bus->number == pci->cfg.bus_range->start) {
+		u64 write_val, read_val;
+		u32 mask = 0;
+
+		if (devfn != 0 || where >= 2048)
+			return PCIBIOS_DEVICE_NOT_FOUND;
+
+		/*
+		 * 32-bit accesses only.  If the write is for a size
+		 * smaller than 32-bits, we must first read the 32-bit
+		 * value and merge in the desired bits and then write
+		 * the whole 32-bits back out.
+		 */
+		switch (size) {
+		case 1:
+			read_val = where & ~3ull;
+			writeq(read_val, pem_pci->pem_reg_base + PEM_CFG_RD);
+			read_val = readq(pem_pci->pem_reg_base + PEM_CFG_RD);
+			read_val >>= 32;
+			mask = ~(0xff << (8 * (where & 3)));
+			read_val &= mask;
+			val = (val & 0xff) << (8 * (where & 3));
+			val |= (u32)read_val;
+			break;
+		case 2:
+			read_val = where & ~3ull;
+			writeq(read_val, pem_pci->pem_reg_base + PEM_CFG_RD);
+			read_val = readq(pem_pci->pem_reg_base + PEM_CFG_RD);
+			read_val >>= 32;
+			mask = ~(0xffff << (8 * (where & 3)));
+			read_val &= mask;
+			val = (val & 0xffff) << (8 * (where & 3));
+			val |= (u32)read_val;
+			break;
+		default:
+			break;
+
+		}

I think it would make this easier to read if the root bus case were
split to a different function, e.g.,

  static u32 thunder_pem_root_w1c_bits(...)
  {
    ...
  }

  static int thunder_pem_root_config_write(...)
  {
    u32 mask[2] = {0xff, 0xffff};

    if (devfn != 0 || where >= 2048)
      return PCIBIOS_DEVICE_NOT_FOUND;

    if (size != 1 && size != 2 && size != 4)
      return PCIBIOS_BAD_REGISTER_NUMBER;

    /* 32-bit accesses supported natively */
    if (size == 4) {
      write_val = where & ~3ull;
      write_val |= (((u64)val) << 32);
      writeq(write_val, pem_pci->pem_reg_base + PEM_CFG_WR);
      return PCIBIOS_SUCCESSFUL;
    }

    /* smaller accesses require read/modify/write */
    read_val = where & ~3ull;
    writeq(read_val, pem_pci->pem_reg_base + PEM_CFG_RD);
    read_val = readq(pem_pci->pem_reg_base + PEM_CFG_RD);
    read_val >>= 32;

    /* mask W1C bits so we don't clear them inadvertently */
    write_val = read_val & ~thunder_pem_root_w1c_bits(...);

    /* deposit new write data (which may intentionally write W1C bits) */
    val &= mask[size];
    shift = 8 * (where & 3);

    write_val &= ~(mask[size] << shift);
    write_val |= val << shift;

    write_val = where & ~3ull;
    write_val |= (((u64)val) << 32);
    writeq(write_val, pem_pci->pem_reg_base + PEM_CFG_WR);
    return PCIBIOS_SUCCESSFUL;
  }

  static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
				    int where, int size, u32 val)
  {
    ...
    if (bus->number < pci->cfg.bus_range->start ||
        bus->number > pci->cfg.bus_range->end)
      return PCIBIOS_DEVICE_NOT_FOUND;

    if (bus->number == pci->cfg.bus_range->start)
      return thunder_pem_root_config_write(...);

    return pci_generic_config_write(bus, devfn, where, size, val);
  }

+
+		if (mask) {
+			/*
+			 * By expanding the write width to 32 bits, we
+			 * may inadvertently hit some W1C bits that
+			 * were not intended to be written.  Mask
+			 * these out.
+			 *
+			 * Some of the w1c_bits below also include
+			 * read-only or non-writable reserved bits,
+			 * this makes the code simpler and is OK as
+			 * the bits are not affected by writing zeros
+			 * to them.
+			 */
+			u32 w1c_bits = 0;
+
+			switch (where & 3) {
+			case 0x04: /* Command/Status */
+			case 0x1c: /* Base and I/O Limit/Secondary Status */
+				w1c_bits = 0xff000000;
+				break;
+			case 0x44: /* Power Management Control and Status */
+				w1c_bits = 0xfffffe00;
+				break;
+			case 0x78: /* Device Control/Device Status */
+			case 0x80: /* Link Control/Link Status */
+			case 0x88: /* Slot Control/Slot Status */
+			case 0x90: /* Root Status */
+			case 0xa0: /* Link Control 2 Registers/Link Status 2 */
+				w1c_bits = 0xffff0000;
+				break;
+			case 0x104: /* Uncorrectable Error Status */
+			case 0x110: /* Correctable Error Status */
+			case 0x130: /* Error Status */
+			case 0x160: /* Link Control 4 */

"where & 3" can never match any of these cases.  I suppose you meant
"where & ~3"?

+				w1c_bits = 0xffffffff;
+				break;
+			default:
+				break;
+
+			}
+			if (w1c_bits) {
+				mask &= w1c_bits;
+				val &= ~mask;
+			}
+		}
+
+		/*
+		 * Low order bits are the config address, the high
+		 * order 32 bits are the data to be written.
+		 */
+		write_val = where & ~3ull;
+		write_val |= (((u64)val) << 32);
+		writeq(write_val, pem_pci->pem_reg_base + PEM_CFG_WR);
+		return PCIBIOS_SUCCESSFUL;
+	}
+	if (bus->number < pci->cfg.bus_range->start ||
+	    bus->number > pci->cfg.bus_range->end)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	return pci_generic_config_write(bus, devfn, where, size, val);
+}

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html