Re: [PATCH] r8169: Fix rtl8169_rx_interrupt()

[Sergey Senozhatsky <sergey.senozhatsky@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5145 bytes --]

Hello,

This hunk is rejected. I suppose I should apply patch against unpatched version (Eric's patch).
Correct? /* Eric's patch does make sense to my mind. */


@@ -4604,22 +4601,19 @@
        void __iomem *ioaddr = tp->mmio_addr;
        int work_done;
 
+
+       RTL_W16(IntrStatus, tp->napi_event);
+
        work_done = rtl8169_rx_interrupt(dev, tp, ioaddr, (u32) budget);
        rtl8169_tx_interrupt(dev, tp, ioaddr);
 
        if (work_done < budget) {
                napi_complete(napi);
 
-               /* We need for force the visibility of tp->intr_mask
-                * for other CPUs, as we can loose an MSI interrupt
-                * and potentially wait for a retransmit timeout if we don't.
-                * The posted write to IntrMask is safe, as it will
-                * eventually make it to the chip and we won't loose anything
-                * until it does.
-                */
-               tp->intr_mask = 0xffff;
-               smp_wmb();
                RTL_W16(IntrMask, tp->intr_event);
+               mmiowb();
+
+               rtl_napi_cond_schedule(tp, RTL_R16(IntrStatus));
        }



	Sergey



On (03/18/10 00:55), Francois Romieu wrote:
> This one should help too if Sergey owns a (MSI) 8168.
> 
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index dfc3573..721e7f3 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -4532,21 +4532,39 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
>  	return count;
>  }
>  
> +static void rtl_napi_cond_schedule(struct rtl8169_private *tp, u16 status)
> +{
> +	if (status & tp->napi_event) {
> +		void __iomem *ioaddr = tp->mmio_addr;
> +
> +		RTL_W16(IntrMask, tp->intr_event & ~tp->napi_event);
> +		mmiowb();
> +		napi_schedule(&tp->napi);
> +	}
> +}
> +
>  static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>  {
>  	struct net_device *dev = dev_instance;
>  	struct rtl8169_private *tp = netdev_priv(dev);
>  	void __iomem *ioaddr = tp->mmio_addr;
>  	int handled = 0;
> -	int status;
> +	u16 status;
>  
>  	/* loop handling interrupts until we have no new ones or
>  	 * we hit a invalid/hotplug case.
>  	 */
>  	status = RTL_R16(IntrStatus);
>  	while (status && status != 0xffff) {
> +		u16 acked;
> +
>  		handled = 1;
>  
> +		acked = (status & RxFIFOOver) ? (status | RxOverflow) : status;
> +		acked &= ~tp->napi_event;
> +
> +		RTL_W16(IntrStatus, acked);
> +
>  		/* Handle all of the error cases first. These will reset
>  		 * the chip, so just exit the loop.
>  		 */
> @@ -4557,7 +4575,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>  
>  		/* Work around for rx fifo overflow */
>  		if (unlikely(status & RxFIFOOver) &&
> -		(tp->mac_version == RTL_GIGA_MAC_VER_11)) {
> +		    (tp->mac_version == RTL_GIGA_MAC_VER_11)) {
>  			netif_stop_queue(dev);
>  			rtl8169_tx_timeout(dev);
>  			break;
> @@ -4571,30 +4589,9 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>  		if (status & LinkChg)
>  			rtl8169_check_link_status(dev, tp, ioaddr);
>  
> -		/* We need to see the lastest version of tp->intr_mask to
> -		 * avoid ignoring an MSI interrupt and having to wait for
> -		 * another event which may never come.
> -		 */
> -		smp_rmb();
> -		if (status & tp->intr_mask & tp->napi_event) {
> -			RTL_W16(IntrMask, tp->intr_event & ~tp->napi_event);
> -			tp->intr_mask = ~tp->napi_event;
> -
> -			if (likely(napi_schedule_prep(&tp->napi)))
> -				__napi_schedule(&tp->napi);
> -			else
> -				netif_info(tp, intr, dev,
> -					   "interrupt %04x in poll\n", status);
> -		}
> +		rtl_napi_cond_schedule(tp, status);
>  
> -		/* We only get a new MSI interrupt when all active irq
> -		 * sources on the chip have been acknowledged. So, ack
> -		 * everything we've seen and check if new sources have become
> -		 * active to avoid blocking all interrupts from the chip.
> -		 */
> -		RTL_W16(IntrStatus,
> -			(status & RxFIFOOver) ? (status | RxOverflow) : status);
> -		status = RTL_R16(IntrStatus);
> +		break;
>  	}
>  
>  	return IRQ_RETVAL(handled);
> @@ -4607,22 +4604,19 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
>  	void __iomem *ioaddr = tp->mmio_addr;
>  	int work_done;
>  
> +
> +	RTL_W16(IntrStatus, tp->napi_event);
> +
>  	work_done = rtl8169_rx_interrupt(dev, tp, ioaddr, (u32) budget);
>  	rtl8169_tx_interrupt(dev, tp, ioaddr);
>  
>  	if (work_done < budget) {
>  		napi_complete(napi);
>  
> -		/* We need for force the visibility of tp->intr_mask
> -		 * for other CPUs, as we can loose an MSI interrupt
> -		 * and potentially wait for a retransmit timeout if we don't.
> -		 * The posted write to IntrMask is safe, as it will
> -		 * eventually make it to the chip and we won't loose anything
> -		 * until it does.
> -		 */
> -		tp->intr_mask = 0xffff;
> -		smp_wmb();
>  		RTL_W16(IntrMask, tp->intr_event);
> +		mmiowb();
> +
> +		rtl_napi_cond_schedule(tp, RTL_R16(IntrStatus));
>  	}
>  
>  	return work_done;
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]