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 linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
