RE: [PATCH] PCI: pciehp: Fix the problem that the present bit is not cleared though slot is empty

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@xxxxxxx]
> Sent: Thursday, February 13, 2014 8:37 AM
> To: Rajat Jain
> Cc: Izumi, Taku; rajatxjain@xxxxxxxxx; Bjorn Helgaas; linux-
> pci@xxxxxxxxxxxxxxx; yinghai@xxxxxxxxxx; Guenter Roeck
> Subject: Re: [PATCH] PCI: pciehp: Fix the problem that the present bit
> is not cleared though slot is empty
> 
> At Thu, 13 Feb 2014 16:03:30 +0000,
> Rajat Jain wrote:
> >
> > [+Takashi]
> >
> > Hello,
> > >
> > > > > Huh, that doesn't sound good.  Does your slot have an attention
> > > > > button?  Can you collect the "lspci -vvv" output for the
> > > > > Downstream Port leading to this slot?
> > >
> > >    The slots have no Attention button and are not for ExpressCard.
> > >    lspci -vvv output is:
> > >
> > >         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
> > > ParErr+ Stepping- SERR+ FastB2B- DisINTx+
> > >         Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast
> > > >TAbort-
> > > <TAbort- <MAbort- >SERR- <PERR- INTx-
> > >         Latency: 0
> > >         Bus: primary=2c, secondary=2d, subordinate=2f, sec-latency=0
> > >         I/O behind bridge: 0000f000-00000fff
> > >         Memory behind bridge: b1800000-b1ffffff
> > >         Prefetchable memory behind bridge: 00000000ae800000-
> > > 00000000aeffffff
> > >         Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast
> > > >TAbort-
> > > <TAbort- <MAbort- <SERR- <PERR-
> > >         BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset-
> FastB2B-
> > >                 PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
> > >         Capabilities: [40] Power Management version 3
> > >                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA
> > > PME(D0+,D1-
> > > ,D2-,D3hot+,D3cold+)
> > >                 Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0
> PME-
> > >         Capabilities: [48] MSI: Enable+ Count=1/8 Maskable+ 64bit+
> > >                 Address: 00000000fee00518  Data: 0000
> > >                 Masking: 000000fe  Pending: 00000000
> > >         Capabilities: [68] Express (v2) Downstream Port (Slot+), MSI
> 00
> > >                 DevCap: MaxPayload 2048 bytes, PhantFunc 0, Latency
> > > L0s <64ns, L1 <1us
> > >                         ExtTag- RBE+ FLReset-
> > >                 DevCtl: Report errors: Correctable+ Non-Fatal+
> > > Fatal+
> > > Unsupported-
> > >                         RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
> > >                         MaxPayload 128 bytes, MaxReadReq 128 bytes
> > >                 DevSta: CorrErr+ UncorrErr- FatalErr- UnsuppReq+
> > > AuxPwr-
> > > TransPend-
> > >                 LnkCap: Port #1, Speed 8GT/s, Width x8, ASPM L1,
> > > Latency
> > > L0 <4us, L1 <4us
> > >                         ClockPM- Surprise+ LLActRep+ BwNot+
> > >                 LnkCtl: ASPM Disabled; Disabled- Retrain- CommClk-
> > >                         ExtSynch- ClockPM- AutWidDis- BWInt-
> AutBWInt-
> > >                 LnkSta: Speed 2.5GT/s, Width x0, TrErr- Train-
> > > SlotClk+
> > > DLActive- BWMgmt- ABWMgmt-
> > >                 SltCap: AttnBtn- PwrCtrl+ MRL+ AttnInd+ PwrInd+
> > > HotPlug+
> > > Surprise-
> > >                         Slot #67, PowerLimit 25.000W; Interlock-
> > > NoCompl-
> >
> > Hmm... I see that the Slot has a power controller. Which means that
> the power to the slot shall not be turned on automatically (by HW) when
> the card is plugged in. Also meaning that the link will not come up
> automatically - so this does not seem like the Link state based hotplug
> kicking in.
> >
> > What I suspect is this one:
> >
> > f02d1843d83b "PCI: pciehp: Remove surprise bit checks"
> >
> > Since this patch removes *all* the surprise bit checks causing all the
> presence detect changes (including 0->1) to be initiate hotplug. I think
> it may be worthwhile to try it out while removing this particular hunk:
> >
> > @@ -535,8 +535,6 @@ static void interrupt_event_handler(struct
> work_struct *work)
> >  		break;
> >  	case INT_PRESENCE_ON:
> >  	case INT_PRESENCE_OFF:
> > -		if (!HP_SUPR_RM(ctrl))
> > -			break;
> >  		ctrl_dbg(ctrl, "Surprise Removal\n");
> >  		handle_surprise_event(p_slot);
> >  		break;
> >
> > May be we should remove the surprise check for INT_PRESENCE_OFF only
> (and let it stay for INT_PRESEWNCE_ON)?
> 
> I checked my test devices, and they seem to have no power controller bit
> involved.  So I double-checked the recent commits, and indeed Rajat's
> recent work already fixed the detection (presumed that the commit
> [e48f1b67: PCI: pciehp: Use link change notifications for hot-plug and
> removal] does it) without fiddling with the surprise bit.

Thanks a lot for testing!

> 
> That said, my patch can be reverted completely.  Can anyone check
> whether it cures?

I still think that part of your patch is needed. At least the one that removes the surprise bit check if the card is suddenly yanked out.

Thanks,

Rajat

> 
> 
> thanks,
> 
> Takashi
> 

��.n��������+%������w��{.n�����{���"�)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥





[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux