RE: [PATCH v3 4/8] pciehp: Don't disable the link permanently,during removal

[Rajat Jain]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx]
> 
> [+cc yinghai@xxxxxxxxxx (seems to be Yinghai's preferred email]
> 
> On Tue, Dec 17, 2013 at 12:06:05PM -0800, Rajat Jain wrote:
> > We need future link up events for hot-add, thus don't disable the link
> > permanently during device removal. Also, remove the static functions
> > that are now left unused.
> 
> The changelog should mention that this reverts part of 2debd9289997
> ("PCI:
> pciehp: Disable/enable link during slot power off/on").

Sure, I'll add that. 

> 
> Yinghai, can you tell us whether this is an issue on your systems?
>

Thanks in advance Yinghai. 

Actually I did not understand the original problem and the solution in the first
place (so I also do not understand how might disabling of presence detect notification
help). If you can give more details on the original problem that shall be great. Here
is what I understood from the commit log:

The believe the HW looks like this:

PCIe port <----> Repeater <----> Device.

An in addition there is the presence detect pin that is connected directly from
The port to the device. Now, when the device is plugged out, the pin indicates
No presence. But are you saying the PCIe link from port to repeater is still up?

Part of log:
====================================
    ...
    It turns out the root complex is continually trying to train the link to
    the repeater because the repeater has not been reset.

    This patch will disable the link at removal time to allow the repeater
    to be reset properly.
    ...
====================================

1) I did not understand why would port try to retrain to the link in either cases -
Whether the link to the repeater is up or not?

2) When no link is seen, is THAT what causes repeater to go down and hence solve the
Problem?

3) I'd expect you'd continuously get "adapter not present" messages if some how
The driver was trying to add the device. But I did not understand where did
"adapter present" messages came from?

Sorry, but I guess I am totally confused now. I'll probably go to sleep :-) 

Just trying to understand.

Thanks,

Rajat

> > Signed-off-by: Rajat Jain <rajatjain@xxxxxxxxxxx>
> > Signed-off-by: Guenter Roeck <groeck@xxxxxxxxxxx>
> > ---
> > v3: no change, created by splitting the patch v2 [2/4]
> > v2: (non existent)
> > v1: (non existent)
> >
> >  drivers/pci/hotplug/pciehp_hpc.c |   18 ------------------
> >  1 file changed, 18 deletions(-)
> >
> > diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> > b/drivers/pci/hotplug/pciehp_hpc.c
> > index b45b568..ab12555 100644
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -278,11 +278,6 @@ static void pcie_wait_link_active(struct
> controller *ctrl)
> >  	__pcie_wait_link_active(ctrl, true);  }
> >
> > -static void pcie_wait_link_not_active(struct controller *ctrl) -{
> > -	__pcie_wait_link_active(ctrl, false);
> > -}
> > -
> >  static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)  {
> >  	u32 l;
> > @@ -383,11 +378,6 @@ static int pciehp_link_enable(struct controller
> *ctrl)
> >  	return __pciehp_link_set(ctrl, true);  }
> >
> > -static int pciehp_link_disable(struct controller *ctrl) -{
> > -	return __pciehp_link_set(ctrl, false);
> > -}
> > -
> >  int pciehp_get_attention_status(struct slot *slot, u8 *status)  {
> >  	struct controller *ctrl = slot->ctrl; @@ -620,14 +610,6 @@ int
> > pciehp_power_off_slot(struct slot * slot)
> >  	u16 cmd_mask;
> >  	int retval;
> >
> > -	/* Disable the link at first */
> > -	pciehp_link_disable(ctrl);
> > -	/* wait the link is down */
> > -	if (ctrl->link_active_reporting)
> > -		pcie_wait_link_not_active(ctrl);
> > -	else
> > -		msleep(1000);
> > -
> >  	slot_cmd = POWER_OFF;
> >  	cmd_mask = PCI_EXP_SLTCTL_PCC;
> >  	retval = pcie_write_cmd(ctrl, slot_cmd, cmd_mask);
> > --
> > 1.7.9.5
> >
> 

¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_