Re: [PATCH] PCI: designware: fix dw_pcie_cfg_write

[Bjorn Helgaas <bhelgaas@google.com>]

On Thu, Aug 20, 2015 at 09:58:33PM +0800, Gabriele Paoloni wrote:
> From: gabriele paoloni <gabriele.paoloni@huawei.com>
> 
> Currently in dw_pcie_cfg_write() if the input size is 2 bytes
> the address offset is wrongly calculated as we mask also bit0
> of "where" input parameter. Instead we should mask all bits of
> "where" except bit0 and bit1.

I think there are two problems in this area:

1) Most calls of dw_pcie_cfg_write() are of the form:

    dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, ...)
    dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3), where, ...)

   This is needlessly repetitive because the caller has to mention
   "where" twice and do the masking itself.  We could simply pass
   "pp->dbi_base" and "where" and let dw_pcie_cfg_write() do all the
   required masking internally.

2) Pratyush, this one's for you :)  spear13xx_pcie_establish_link()
   calls dw_pcie_cfg_read() and dw_pcie_cfg_write() several times like
   this:

    dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, ...)
    dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, ...)

   I think these are incorrect because dw_pcie_cfg_read() and
   dw_pcie_cfg_write() don't add anything to the "addr" argument
   ("pp->dbi_base" in this case) except the 0-3 byte select offset, so
   they use the correct alignment, but access the wrong registers.

As far as the original patch below, I don't think I have a strong
opinion either way.  You could consider doing something like:

  if (size == 4) {
    if (addr & 3) return PCIBIOS_BAD_REGISTER_NUMBER;
    writel(val, addr);
  } else if (size == 2) {
    if (addr & 1) return PCIBIOS_BAD_REGISTER_NUMBER;
    writew(val, addr);
  } ...

This is similar to what we do in pci_bus_write_config_word() (see
drivers/pci/access.c).

> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  drivers/pci/host/pcie-designware.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 69486be..a27f536 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -99,7 +99,7 @@ int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val)
>  	if (size == 4)
>  		writel(val, addr);
>  	else if (size == 2)
> -		writew(val, addr + (where & 2));
> +		writew(val, addr + (where & 3));
>  	else if (size == 1)
>  		writeb(val, addr + (where & 3));
>  	else
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html