RE: [RFC Patch net-next v1 9/9] r8169: add support for ethtool

[Javen]

>> +static int rtl8169_set_channels(struct net_device *dev,
>> +                             struct ethtool_channels *ch) {
>> +     struct rtl8169_private *tp = netdev_priv(dev);
>> +     bool if_running = netif_running(dev);
>> +     int i;
>> +
>> +     if (!tp->rss_support && (ch->rx_count > 1 || ch->tx_count > 1)) {
>> +             netdev_warn(dev, "This chip does not support multiple
>channels/RSS.\n");
>> +             return -EOPNOTSUPP;
>> +     }
>> +
>> +     if (ch->rx_count == 0 || ch->tx_count == 0)
>> +             return -EINVAL;
>> +     if (ch->rx_count > tp->HwSuppNumRxQueues ||
>> +         ch->tx_count > tp->HwSuppNumTxQueues)
>> +             return -EINVAL;
>> +     if (ch->other_count || ch->combined_count)
>> +             return -EINVAL;
>> +
>> +     if (ch->rx_count == tp->num_rx_rings &&
>> +         ch->tx_count == tp->num_tx_rings)
>> +             return 0;
>> +
>> +     if (if_running)
>> +             rtl8169_close(dev);
>
>I assume this releases all the memory from the rings?
>
>> +
>> +     tp->num_rx_rings = ch->rx_count;
>> +     tp->num_tx_rings = ch->tx_count;
>> +
>> +     tp->rss_enable = (tp->num_rx_rings > 1 && tp->rss_support);
>> +
>> +     for (i = 0; i < tp->HwSuppIndirTblEntries; i++) {
>> +             if (tp->rss_enable)
>> +                     tp->rss_indir_tbl[i] = ethtool_rxfh_indir_default(i, tp-
>>num_rx_rings);
>> +             else
>> +                     tp->rss_indir_tbl[i] = 0;
>> +     }
>> +
>> +     if (tp->rss_enable)
>> +             tp->InitRxDescType = RX_DESC_RING_TYPE_RSS;
>> +     else
>> +             tp->InitRxDescType = RX_DESC_RING_TYPE_DEAFULT;
>> +
>> +     if (if_running)
>> +             return rtl_open(dev);
>
>And this tries to allocate the memory needed for the rings? And if the system
>is under memory pressure, it fails and your network is dead?
>
>Please modify the code so that is first allocated the new rings and then frees
>the old rings, so you can fail gracefully.
>
>     Andrew

Thank you for your advice. I have updated the code accordingly. This patch is 
based on RFC Patch net-next 9/9.

This patch fixes issues when using ethtool -L to change the number of RX rings. 
Specifically, if allocating memory for the new RX rings fails, it now gracefully rolls 
back to the original state.
Additionally, it fixes a bug where ping or iperf would fail after manually setting 
8 RX rings (when the default suggested RSS queue number is only 1 or 2). This 
was caused by a missing call to rtl_set_irq_mask, which is now added.

---
 drivers/net/ethernet/realtek/r8169_main.c | 101 +++++++++++++++++++---
 1 file changed, 90 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 57087abe7d88..4b2abd3deee0 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -6116,6 +6116,8 @@ static void rtl8169_double_check_rss_support(struct rtl8169_private *tp)
 	if (tp->num_rx_rings >= 2) {
 		tp->rss_enable = 1;
 		tp->InitRxDescType = RX_DESC_RING_TYPE_RSS;
+	} else if (tp->num_rx_rings == 1 && tp->irq_nvecs > 1) {
+		tp->rss_enable = 0;
 	} else {
 		tp->rss_enable = 0;
 		if (tp->irq_nvecs > 1) {
@@ -6534,18 +6536,70 @@ static void rtl8169_get_channels(struct net_device *dev,
 	ch->combined_count = 0;
 }
 
+static int rtl8169_realloc_rx(struct rtl8169_private *tp,
+			      struct rtl8169_rx_ring *new_rx,
+			      int new_count)
+{
+	int i, ret;
+
+	new_rx[0].rdsar_reg = RxDescAddrLow;
+	for (i = 1; i < new_count; i++)
+		new_rx[i].rdsar_reg = (u16)(RDSAR_Q1_LOW + (i - 1) * 8);
+
+	for (i = 0; i < new_count; i++)
+		new_rx[i].num_rx_desc = NUM_RX_DESC;
+
+	for (i = 0; i < new_count; i++) {
+		struct rtl8169_rx_ring *ring = &new_rx[i];
+
+		ring->RxDescAllocSize = (NUM_RX_DESC + 1) * sizeof(struct RxDesc);
+		ring->RxDescArray = dma_alloc_coherent(&tp->pci_dev->dev,
+						       ring->RxDescAllocSize,
+						       &ring->RxPhyAddr,
+						       GFP_KERNEL);
+		if (!ring->RxDescArray) {
+			ret = -ENOMEM;
+			goto err_free;
+		}
+
+		memset(ring->Rx_databuff, 0, sizeof(ring->Rx_databuff));
+		ret = rtl8169_rx_fill(tp, ring);
+		if (ret) {
+			dma_free_coherent(&tp->pci_dev->dev, ring->RxDescAllocSize,
+					  ring->RxDescArray, ring->RxPhyAddr);
+			goto err_free;
+		}
+	}
+	return 0;
+
+err_free:
+	while (--i >= 0) {
+		rtl8169_rx_clear(tp, &new_rx[i]);
+		dma_free_coherent(&tp->pci_dev->dev, new_rx[i].RxDescAllocSize,
+				  new_rx[i].RxDescArray, new_rx[i].RxPhyAddr);
+	}
+	return ret;
+}
+
 static int rtl8169_set_channels(struct net_device *dev,
 				struct ethtool_channels *ch)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	bool if_running = netif_running(dev);
-	int i;
+	struct rtl8169_rx_ring *new_rx;
+	u8 old_tx_desc_type = tp->InitRxDescType;
+	u8 new_desc_type;
+	bool new_rss_enable;
+	int i, ret;
 
 	if (!tp->rss_support && (ch->rx_count > 1 || ch->tx_count > 1)) {
 		netdev_warn(dev, "This chip does not support multiple channels/RSS.\n");
 		return -EOPNOTSUPP;
 	}
 
+	if (tp->features & RTL_FEATURE_MSI)
+		return -EINVAL;
+
 	if (ch->rx_count == 0 || ch->tx_count == 0)
 		return -EINVAL;
 	if (ch->rx_count > tp->HwSuppNumRxQueues ||
@@ -6558,13 +6612,39 @@ static int rtl8169_set_channels(struct net_device *dev,
 	    ch->tx_count == tp->num_tx_rings)
 		return 0;
 
-	if (if_running)
-		rtl8169_close(dev);
+	new_rss_enable = (ch->rx_count > 1 && tp->rss_support);
+	new_desc_type = new_rss_enable ? RX_DESC_RING_TYPE_RSS : RX_DESC_RING_TYPE_DEAFULT;
+	tp->InitRxDescType = new_desc_type;
+
+	if (!if_running) {
+		tp->num_rx_rings = ch->rx_count;
+		tp->rss_enable = new_rss_enable;
+		return 0;
+	}
+
+	new_rx = kcalloc(R8169_MAX_RX_QUEUES, sizeof(*new_rx), GFP_KERNEL);
+	if (!new_rx)
+		return -ENOMEM;
+
+	ret = rtl8169_realloc_rx(tp, new_rx, ch->rx_count);
+	if (ret) {
+		kfree(new_rx);
+		tp->InitRxDescType = old_tx_desc_type;
+		return ret;
+	}
+
+	netif_stop_queue(dev);
+	rtl8169_down(tp);
+
+	for (i = 0; i < tp->num_rx_rings; i++)
+		rtl8169_rx_clear(tp, &tp->rx_ring[i]);
+	rtl8169_free_rx_desc(tp);
 
 	tp->num_rx_rings = ch->rx_count;
-	tp->num_tx_rings = ch->tx_count;
+	tp->rss_enable = new_rss_enable;
 
-	tp->rss_enable = (tp->num_rx_rings > 1 && tp->rss_support);
+	memset(tp->rx_ring, 0, sizeof(tp->rx_ring));
+	memcpy(tp->rx_ring, new_rx, sizeof(*new_rx) * ch->rx_count);
 
 	for (i = 0; i < tp->HwSuppIndirTblEntries; i++) {
 		if (tp->rss_enable)
@@ -6573,13 +6653,12 @@ static int rtl8169_set_channels(struct net_device *dev,
 			tp->rss_indir_tbl[i] = 0;
 	}
 
-	if (tp->rss_enable)
-		tp->InitRxDescType = RX_DESC_RING_TYPE_RSS;
-	else
-		tp->InitRxDescType = RX_DESC_RING_TYPE_DEAFULT;
+	rtl_set_irq_mask(tp);
+
+	rtl8169_up(tp);
+	netif_start_queue(dev);
 
-	if (if_running)
-		return rtl_open(dev);
+	kfree(new_rx);
 
 	return 0;
 }
-- 
2.43.0