From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753441Ab1GETJY (ORCPT <rfc822;w@1wt.eu>);
	Tue, 5 Jul 2011 15:09:24 -0400
Received: from violet.fr.zoreil.com ([92.243.8.30]:55282 "EHLO
	violet.fr.zoreil.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752004Ab1GETJX (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 5 Jul 2011 15:09:23 -0400
Date: Tue, 5 Jul 2011 20:55:15 +0200
From: Francois Romieu <romieu@fr.zoreil.com>
To: Hayes Wang <hayeswang@realtek.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 2/6] r8169: modify the flow hw reset
Message-ID: <20110705185515.GB5105@electric-eye.fr.zoreil.com>
References: <1309859095-32031-1-git-send-email-hayeswang@realtek.com> <1309859095-32031-2-git-send-email-hayeswang@realtek.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1309859095-32031-2-git-send-email-hayeswang@realtek.com>
User-Agent: Mutt/1.4.2.2i
X-Organisation: Land of Sunshine Inc.
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hayes Wang <hayeswang@realtek.com> :
> Replace rtl8169_asic_down with rtl8169_hw_reset. Clear RxConfig
> bit 0 ~ 3 and do some checking before reset. Remove hw reset
> which is before hw_start because reset would be done in close or
> down.

The whole description of the changes ought to explain why things are
changed.

> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>  drivers/net/r8169.c |   43 ++++++++++++++++++++++++-------------------
>  1 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index 701ab6b..cdbbe47 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -731,6 +731,8 @@ static int rtl8169_poll(struct napi_struct *napi, int budget);
>  static const unsigned int rtl8169_rx_config =
>  	(RX_FIFO_THRESH << RxCfgFIFOShift) | (RX_DMA_BURST << RxCfgDMAShift);
>  
> +static void rtl8169_init_ring_indexes(struct rtl8169_private *tp);
> +

rtl8169_init_ring_indexes is really short. Please move it before its first use
and avoid the forward declaration.

>  static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
>  {
>  	void __iomem *ioaddr = tp->mmio_addr;
> @@ -1076,13 +1078,6 @@ static void rtl8169_irq_mask_and_ack(void __iomem *ioaddr)
>  	RTL_W16(IntrStatus, 0xffff);
>  }
>  
> -static void rtl8169_asic_down(void __iomem *ioaddr)
> -{
> -	RTL_W8(ChipCmd, 0x00);
> -	rtl8169_irq_mask_and_ack(ioaddr);
> -	RTL_R16(CPlusCmd);
> -}
> -
>  static unsigned int rtl8169_tbi_reset_pending(struct rtl8169_private *tp)
>  {
>  	void __iomem *ioaddr = tp->mmio_addr;
> @@ -3352,10 +3347,12 @@ static void rtl_hw_reset(struct rtl8169_private *tp)
>  
>  	/* Check that the chip has finished the reset. */
>  	for (i = 0; i < 100; i++) {
> +		udelay(100);
>  		if ((RTL_R8(ChipCmd) & CmdReset) == 0)
>  			break;

Nit: is it forbidden to perform the read - and thus the implicit PCI flush -
before the first 100 us delay ?

> -		msleep_interruptible(1);
>  	}
> +
> +	rtl8169_init_ring_indexes(tp);
>  }
>  
>  static int __devinit
> @@ -3737,6 +3734,16 @@ err_pm_runtime_put:
>  	goto out;
>  }
>  
> +static void rtl_rx_close(struct rtl8169_private *tp)
> +{
> +	void __iomem *ioaddr = tp->mmio_addr;
> +	u32 rxcfg = RTL_R32(RxConfig);
> +
> +	rxcfg &= ~(AcceptBroadcast | AcceptMulticast |
> +		   AcceptMyPhys | AcceptAllPhys);
> +	RTL_W32(RxConfig, rxcfg);
> +}
> +

Should not error and runt packets be considered too ?

<shot in the dark>
Is there any relationship with commit ca52efd5490f97f396d3c5863ba714624f272033 ?
</shot in the dark>

>  static void rtl8169_hw_reset(struct rtl8169_private *tp)
>  {
>  	void __iomem *ioaddr = tp->mmio_addr;
> @@ -3744,19 +3751,20 @@ static void rtl8169_hw_reset(struct rtl8169_private *tp)
>  	/* Disable interrupts */
>  	rtl8169_irq_mask_and_ack(ioaddr);
>  
> +	rtl_rx_close(tp);
> +
>  	if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
>  	    tp->mac_version == RTL_GIGA_MAC_VER_28 ||
>  	    tp->mac_version == RTL_GIGA_MAC_VER_31) {
>  		while (RTL_R8(TxPoll) & NPQ)
>  			udelay(20);
>  
> +	} else {
> +		RTL_W8(ChipCmd, RTL_R8(ChipCmd) | StopReq);
> +		udelay(100);

No posted PCI write flush ?

Please remove the empty line after the udelay(20). It should not
have been there in the first place.

>  	}
>  
> -	/* Reset the chipset */
> -	RTL_W8(ChipCmd, CmdReset);
> -
> -	/* PCI commit */
> -	RTL_R8(ChipCmd);
> +	rtl_hw_reset(tp);
>  }
>  
>  static void rtl_set_rx_tx_config_registers(struct rtl8169_private *tp)
> @@ -3776,8 +3784,6 @@ static void rtl_hw_start(struct net_device *dev)
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
>  
> -	rtl_hw_reset(tp);
> -
>  	tp->hw_start(dev);
>  
>  	netif_start_queue(dev);
> @@ -4718,7 +4724,6 @@ static void rtl8169_reset_task(struct work_struct *work)
>  
>  	rtl8169_tx_clear(tp);
>  
> -	rtl8169_init_ring_indexes(tp);
>  	rtl_hw_start(dev);
>  	netif_wake_queue(dev);
>  	rtl8169_check_link_status(dev, tp, tp->mmio_addr);

I do not see where the ring indexes will be set when __rtl8169_resume()
schedules rtl8169_reset_task. It could hurt.

-- 
Ueimor