Hi Brian,
? 2017/5/24 2:00, Brian Norris ??:
> On Fri, May 19, 2017 at 02:58:27PM +0800, Shawn Lin wrote:
>> This patch checks the link status before reading and
>> writing configure space of devices attached to the RC.
>> If the link status is down, we shouldn't try to access
>> the devices.
>
> I'm curious, in what situations are you seeing the link down? In all the
> cases where I can manage to screw up my endpoint and see system aborts
> due to config accesses, this check still says the link is up. Presumably
> you have some test cases that benefit from this though.
Of course. This patch doesn't prevent all these cases, for instance,
you do a memory read/write in the EP function driver, since it doesn't
call these two APIs at all.
The reason for me to added this check is that I saw a external abort
down to rockchip_pcie_rd_own_conf, of which I highly suspected was that
the link was re-init or total broken at that time.
>
>> Signed-off-by: Shawn Lin <shawn.lin at rock-chips.com>
>> ---
>>
>> drivers/pci/host/pcie-rockchip.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
>> index 0e020b6..1e64227 100644
>> --- a/drivers/pci/host/pcie-rockchip.c
>> +++ b/drivers/pci/host/pcie-rockchip.c
>> @@ -275,9 +275,21 @@ static void rockchip_pcie_update_txcredit_mui(struct rockchip_pcie *rockchip)
>> rockchip_pcie_write(rockchip, val, PCIE_CORE_TXCREDIT_CFG1);
>> }
>>
>> +static inline bool rockchip_pcie_link_up(struct rockchip_pcie *rockchip)
>> +{
>> + return PCIE_LINK_UP(rockchip_pcie_read(rockchip,
>> + PCIE_CLIENT_BASIC_STATUS1));
>> +}
>> +
>> static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip,
>> struct pci_bus *bus, int dev)
>> {
>> + /* do not access the devices if the link isn't completed */
>> + if (bus->number != rockchip->root_bus_nr) {
>> + if (!rockchip_pcie_link_up(rockchip))
>> + return 0;
>> + }
>
> Seems like this should be the last check in the function, after you
> check that the bus and dev numbers are reasonable?
>
I am fine with either one.
> Brian
>
>> +
>> /* access only one slot on each root port */
>> if (bus->number == rockchip->root_bus_nr && dev > 0)
>> return 0;
>> --
>> 1.9.1
>>
>>
>
>
>
