diff mbox

[1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

Message ID 1484997072-19276-1-git-send-email-bharatku@xilinx.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Bharat Kumar Gogada Jan. 21, 2017, 11:11 a.m. UTC
- The legacy status register value for particular INTx becomes low
only after DEASSERT_INTx is received.
- Few End Points take time for sending DEASSERT_INTx, checking
legacy status register in while loop causes invoking of EP
handler continuosly until DEASSERT_INTx is received.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Comments

Marc Zyngier Jan. 23, 2017, 6:23 p.m. UTC | #1
On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> - The legacy status register value for particular INTx becomes low
> only after DEASSERT_INTx is received.
> - Few End Points take time for sending DEASSERT_INTx, checking
> legacy status register in while loop causes invoking of EP
> handler continuosly until DEASSERT_INTx is received.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index 43eaa4a..c8b5a33 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc *desc)
>  
>  	chained_irq_enter(chip, desc);
>  	pcie = irq_desc_get_handler_data(desc);
> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> +				  MSGF_LEG_SR_MASKALL;
>  
> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> -				MSGF_LEG_SR_MASKALL) != 0) {
> +	if (status != 0) {
>  		for_each_set_bit(bit, &status, INTX_NUM) {
>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
>  						bit + 1);
> 

But even if you only handle the interrupt once, it is still asserted,
right? You exit the low-level exception handler, only to take the
interrupt immediately again. So what is the gain here?

As an aside, please add a cover letter to your patch series. It is
immensely useful as a summary of what is being done, as well as an
anchor for the patches themselves.

Thanks,

	M.
Bharat Kumar Gogada Jan. 24, 2017, 10:15 a.m. UTC | #2
> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > - The legacy status register value for particular INTx becomes low
> > only after DEASSERT_INTx is received.
> > - Few End Points take time for sending DEASSERT_INTx, checking legacy
> > status register in while loop causes invoking of EP handler
> > continuosly until DEASSERT_INTx is received.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > index 43eaa4a..c8b5a33 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc
> > *desc)
> >
> >  	chained_irq_enter(chip, desc);
> >  	pcie = irq_desc_get_handler_data(desc);
> > +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> > +				  MSGF_LEG_SR_MASKALL;
> >
> > -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> > -				MSGF_LEG_SR_MASKALL) != 0) {
> > +	if (status != 0) {
> >  		for_each_set_bit(bit, &status, INTX_NUM) {
> >  			virq = irq_find_mapping(pcie->legacy_irq_domain,
> >  						bit + 1);
> >
> 
> But even if you only handle the interrupt once, it is still asserted, right? You exit
> the low-level exception handler, only to take the interrupt immediately again. So
> what is the gain here?
> 
Whenever masking of INTx happens (like as in my next patch[PATCH 2/4]), the irq line goes low,
but  status bit will be set until DEASSERT_INTx comes.
In this scenario if it is always in while loop, even though line went low it will not be seen 
until DEASSERT_INTx, unnecessarily invoking EP driver. so instead of while loop  
using if condition so that this change in line is noticed.
> As an aside, please add a cover letter to your patch series. It is immensely useful
> as a summary of what is being done, as well as an anchor for the patches
> themselves.
> 
Agreed, will do next time. 

Thanks & Regards,
Bharat

--
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
Marc Zyngier Jan. 24, 2017, 11:07 a.m. UTC | #3
On 24/01/17 10:15, Bharat Kumar Gogada wrote:
>> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
>>> - The legacy status register value for particular INTx becomes low
>>> only after DEASSERT_INTx is received.
>>> - Few End Points take time for sending DEASSERT_INTx, checking legacy
>>> status register in while loop causes invoking of EP handler
>>> continuosly until DEASSERT_INTx is received.
>>>
>>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
>>> ---
>>>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
>>> b/drivers/pci/host/pcie-xilinx-nwl.c
>>> index 43eaa4a..c8b5a33 100644
>>> --- a/drivers/pci/host/pcie-xilinx-nwl.c
>>> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
>>> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc
>>> *desc)
>>>
>>>  	chained_irq_enter(chip, desc);
>>>  	pcie = irq_desc_get_handler_data(desc);
>>> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
>>> +				  MSGF_LEG_SR_MASKALL;
>>>
>>> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
>>> -				MSGF_LEG_SR_MASKALL) != 0) {
>>> +	if (status != 0) {
>>>  		for_each_set_bit(bit, &status, INTX_NUM) {
>>>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
>>>  						bit + 1);
>>>
>>
>> But even if you only handle the interrupt once, it is still asserted, right? You exit
>> the low-level exception handler, only to take the interrupt immediately again. So
>> what is the gain here?
>>
> Whenever masking of INTx happens (like as in my next patch[PATCH 2/4]), the irq line goes low,
> but  status bit will be set until DEASSERT_INTx comes.
> In this scenario if it is always in while loop, even though line went low it will not be seen 
> until DEASSERT_INTx, unnecessarily invoking EP driver. so instead of while loop  
> using if condition so that this change in line is noticed.

But what guarantees that you will observe this DEASSERT if you return to
the interrupted context early? From what I understand, your interrupt
flow is the following:


	read status
	mask
	handler
	unmask

If the EP takes time to send a deassert after the handler has poked it
and the interrupt is still active, then the only thing that this patch
buys you is that by returning to the interrupted context, you waste a
lot of cycles, giving the device a chance to send its deassert. But
that's just luck. Plug this on a fast CPU, and the same issue will
resurface.

It could also just hide a driver bug where the write acknowledging the
interrupt has been posted, but has not taken effect yet.

Thanks,

	M.
Bharat Kumar Gogada Jan. 24, 2017, 2 p.m. UTC | #4
> On 24/01/17 10:15, Bharat Kumar Gogada wrote:
> >> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> >>> - The legacy status register value for particular INTx becomes low
> >>> only after DEASSERT_INTx is received.
> >>> - Few End Points take time for sending DEASSERT_INTx, checking
> >>> legacy status register in while loop causes invoking of EP handler
> >>> continuosly until DEASSERT_INTx is received.
> >>>
> >>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> >>> ---
> >>>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
> >>>  1 files changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> >>> b/drivers/pci/host/pcie-xilinx-nwl.c
> >>> index 43eaa4a..c8b5a33 100644
> >>> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> >>> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> >>> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct
> >>> irq_desc
> >>> *desc)
> >>>
> >>>  	chained_irq_enter(chip, desc);
> >>>  	pcie = irq_desc_get_handler_data(desc);
> >>> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> >>> +				  MSGF_LEG_SR_MASKALL;
> >>>
> >>> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> >>> -				MSGF_LEG_SR_MASKALL) != 0) {
> >>> +	if (status != 0) {
> >>>  		for_each_set_bit(bit, &status, INTX_NUM) {
> >>>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
> >>>  						bit + 1);
> >>>
> >>
> >> But even if you only handle the interrupt once, it is still asserted,
> >> right? You exit the low-level exception handler, only to take the
> >> interrupt immediately again. So what is the gain here?
> >>
> > Whenever masking of INTx happens (like as in my next patch[PATCH
> > 2/4]), the irq line goes low, but  status bit will be set until DEASSERT_INTx
> comes.
> > In this scenario if it is always in while loop, even though line went
> > low it will not be seen until DEASSERT_INTx, unnecessarily invoking EP
> > driver. so instead of while loop using if condition so that this change in line is
> noticed.
> 
> But what guarantees that you will observe this DEASSERT if you return to the
> interrupted context early? From what I understand, your interrupt flow is the
> following:
> 
> 
> 	read status
> 	mask
> 	handler
> 	unmask
> 
> If the EP takes time to send a deassert after the handler has poked it and the
> interrupt is still active, then the only thing that this patch buys you is that by
> returning to the interrupted context, you waste a lot of cycles, giving the device
> a chance to send its deassert. But that's just luck. Plug this on a fast CPU, and
> the same issue will resurface.
> 

Agreed, will leave it as it is.

Thanks & Regards,
Bharat


 
--
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
diff mbox

Patch

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 43eaa4a..c8b5a33 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -342,9 +342,10 @@  static void nwl_pcie_leg_handler(struct irq_desc *desc)
 
 	chained_irq_enter(chip, desc);
 	pcie = irq_desc_get_handler_data(desc);
+	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
+				  MSGF_LEG_SR_MASKALL;
 
-	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
-				MSGF_LEG_SR_MASKALL) != 0) {
+	if (status != 0) {
 		for_each_set_bit(bit, &status, INTX_NUM) {
 			virq = irq_find_mapping(pcie->legacy_irq_domain,
 						bit + 1);