On 03/10/2017 01:56 PM, Kishon Vijay Abraham I wrote:

Hi Niklas,

On Friday 10 March 2017 06:01 PM, Niklas Cassel wrote:

quoted

On 03/10/2017 12:36 PM, Kishon Vijay Abraham I wrote:

quoted

Hi,

On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:

quoted

On 03/09/2017 03:48 PM, Niklas Cassel wrote:

quoted

On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:

quoted

dwc has 2 dbi address space labeled dbics and dbics2. The existing
helper to access dbi address space can access only dbics. However
dbics2 has to be accessed for programming the BAR registers in the
case of EP mode. This is in preparation for adding EP mode support
to dwc driver.

Hello Kishon

I don't really like the idea of adding an extra argument to every existing read/write.
Will not a read/write using dbi2 be quite uncommon compared to a read/write
using dbi?

How about something like this:

void __dw_pcie_writel(struct dw_pcie *pci, void __iomem *base, u32 reg, u32 val)
{
    if (pci->ops->writel_dbi)
        pci->ops->writel_dbi(pci, base, reg, val);
    else
        writel(val, base + reg);
}

#define dw_pcie_writel_dbi(pci, reg, val) __dw_pcie_writel(pci, pci->dbi_base, reg, val)
#define dw_pcie_writel_dbi2(pci, reg, val) __dw_pcie_writel(pci, pci->dbi_base2, reg, val)

Perhaps make dw_pcie_writel_dbi2 a function rather than a define,
so we can return an error if pci->dbi_base2 == NULL.

Should we return an error? We don't return error for dbi_base either. I think
it should be sufficient to return errors while populating dbi_base or
dbi_base2. Otherwise it's a bug and should result in abort. Joao?

Sorry for previous empty email.


What I meant to write:

Right now we do error checking for dbi_base in platform specific code
and in pcie-designware-host.c:dw_pcie_host_init.

it's been done in dw_pcie_host_init not as an error checking but since it's
*optional* for certain platforms to populate dbi_base (i.e where dbi_base is
mapped to configuration space), host_init takes care of assigning dbi_base to
configuration space address.

What I'm afraid of is that we might get a NULL ptr dereference
when using dw_pcie_writel_dbi2, if platform specific code has
not populated dbi_base2.

Having a NULL check in generic code is just a fail safe if some
platform specific code failed to NULL check.

The code in dw_pcie_host_init might have been written just
to populate dbi_base when dbi is mapped to config space,
but the end result is that if platform specific code did not
populate dbi_base (and did not populate pp->cfg),
we will return -ENOMEM.
Which means that we can never get a NULL ptr dereference
when using dw_pcie_writel_dbi.

It might be a good idea to have a NULL check in generic code,
just as a fail safe, also for dw_pcie_ep_init.
That way we know that we will not get a NULL ptr dereference
when using dw_pcie_writel_dbi2.