| Message ID | 20240902061454.85744-1-takamitz@amazon.co.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1,net-next] e1000e: Remove duplicated writel() in e1000_configure_tx/rx() | expand |
On Mon, Sep 02, 2024 at 03:14:54PM +0900, Takamitsu Iwai wrote: > Duplicated register initialization codes exist in e1000_configure_tx() > and e1000_configure_rx(). What does the datasheet say about these registers? Since we are talking about hardware here, before you remove anything you need to be sure these writes are not actually required by the hardware. > commit 0845d45e900c ("e1000e: Modify Tx/Rx configurations to avoid > null pointer dereferences in e1000_open") has introduced these > writel(). This commit moved register writing to > e1000_configure_tx/rx(), and as result, it caused duplication in > e1000_configure_tx/rx(). Did the same sequence of read/writes happen before 0845d45e900c? Or did 0845d45e900c add additional writes, not just move them around? Andrew
> Did the same sequence of read/writes happen before 0845d45e900c? Or > did 0845d45e900c add additional writes, not just move them around? The sequence of read/writes happened before 0845d45e900c because the similar writel() exists in ew32() above the writel() moved by 0845d45e900c. The commit 0845d45e900c moved writel() in e1000_clean_tx/rx_ring() to e1000_configure_tx/rx() to avoid null pointer dereference. But since the same writel() exists in e1000_configure_tx/rx(), we just needed to remove writel() from e1000_clean_rx/tx_ring().
On Tue, Sep 03, 2024 at 07:46:42PM +0900, Takamitsu Iwai wrote: > > Did the same sequence of read/writes happen before 0845d45e900c? Or > > did 0845d45e900c add additional writes, not just move them around? > > The sequence of read/writes happened before 0845d45e900c because the similar > writel() exists in ew32() above the writel() moved by 0845d45e900c. > > The commit 0845d45e900c moved writel() in e1000_clean_tx/rx_ring() to > e1000_configure_tx/rx() to avoid null pointer dereference. But since the same > writel() exists in e1000_configure_tx/rx(), we just needed to remove writel() > from e1000_clean_rx/tx_ring(). So you have confirmed with the datsheet that the write is not needed? As i said, this is a hardware register, not memory. Writes are not always idempotent. It might be necessary to write it twice. Andrew
> So you have confirmed with the datsheet that the write is not needed? > > As i said, this is a hardware register, not memory. Writes are not > always idempotent. It might be necessary to write it twice. I have checked following datasheets and I can not find that we need to write RDH, RDT, TDH, TDT registers twice at initialization. https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/82577-gbe-phy-datasheet.pdf https://www.intel.com/content/www/us/en/content-details/613460/intel-82583v-gbe-controller-datasheet.html Write happened once before commit 0845d45e900c, so just out of curiosity, have you seen such a device? My colleague, Kohei, tested the patch with a real hardware and will provide his Tested-by shortly.
> My colleague, Kohei, tested the patch with a real hardware and will provide his > Tested-by shortly. I have tested the patch using my physical hardware, an Intel Ethernet controller I219-V. The device was properly attached by the e1000e driver and functioned correctly. The test was performed on a custom kernel based on kernel-core-6.10.6-200.fc40.x86_64. The PCI device is identified as an Intel Corporation Ethernet Connection (17) I219-V (rev 11), with vendor ID 0x8086 and device ID 0x1a1d. This device ID matches the E1000_DEV_ID_PCH_ADP_I219_V17 definition in the e1000e driver code. ``` $ lspci | grep -i ethernet 00:1f.6 Ethernet controller: Intel Corporation Ethernet Connection (17) I219-V (rev 11) $ cat /sys/bus/pci/devices/0000:00:1f.6/{vendor,device} 0x8086 0x1a1d $ grep -ri 0x1a1d ~/ghq/github.com/torvalds/linux/drivers/net/ethernet/intel/e1000e /home/kohei/ghq/github.com/torvalds/linux/drivers/net/ethernet/intel/e1000e/hw.h:#define E1000_DEV_ID_PCH_ADP_I219_V17 0x1A1D ``` So this testing confirms that the patch does not introduce any regressions for this specific hardware configuration. Tested-by: Kohei Enju <enjuk@amazon.com> Thanks!
On Wed, Sep 04, 2024 at 02:56:46PM +0900, Takamitsu Iwai wrote: > > So you have confirmed with the datsheet that the write is not needed? > > > > As i said, this is a hardware register, not memory. Writes are not > > always idempotent. It might be necessary to write it twice. > > I have checked following datasheets and I can not find that we need to write > RDH, RDT, TDH, TDT registers twice at initialization. > > https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/82577-gbe-phy-datasheet.pdf > https://www.intel.com/content/www/us/en/content-details/613460/intel-82583v-gbe-controller-datasheet.html > > Write happened once before commit 0845d45e900c, so just out of curiosity, > have you seen such a device? This is just risk minimisation. I don't want e1000e to be broken because you removed a write. I'm trying to ensure you fully understand what you are changing, and have verified it is a safe change. I don't have this hardware, so i cannot test it. > My colleague, Kohei, tested the patch with a real hardware and will provide his > Tested-by shortly. Please resend the patch, adding his Tested-by: and update the commit message to summarise this discussion. Explain how you determined this is safe. Thanks Andrew
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index 360ee26557f7..cf352befaeb9 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -2928,11 +2928,8 @@ static void e1000_configure_tx(struct e1000_adapter *adapter) tx_ring->head = adapter->hw.hw_addr + E1000_TDH(0); tx_ring->tail = adapter->hw.hw_addr + E1000_TDT(0); - writel(0, tx_ring->head); if (adapter->flags2 & FLAG2_PCIM2PCI_ARBITER_WA) e1000e_update_tdt_wa(tx_ring, 0); - else - writel(0, tx_ring->tail); /* Set the Tx Interrupt Delay register */ ew32(TIDV, adapter->tx_int_delay); @@ -3253,11 +3250,8 @@ static void e1000_configure_rx(struct e1000_adapter *adapter) rx_ring->head = adapter->hw.hw_addr + E1000_RDH(0); rx_ring->tail = adapter->hw.hw_addr + E1000_RDT(0); - writel(0, rx_ring->head); if (adapter->flags2 & FLAG2_PCIM2PCI_ARBITER_WA) e1000e_update_rdt_wa(rx_ring, 0); - else - writel(0, rx_ring->tail); /* Enable Receive Checksum Offload for TCP and UDP */ rxcsum = er32(RXCSUM);
Duplicated register initialization codes exist in e1000_configure_tx() and e1000_configure_rx(). For example, writel(0, tx_ring->head) writes 0 to tx_ring->head, which is adapter->hw.hw_addr + E1000_TDH(0). This initialization is already done in ew32(TDH(0), 0). ew32(TDH(0), 0) is equivalent to __ew32(hw, E1000_TDH(0), 0). It executes writel(0, hw->hw_addr + E1000_TDH(0)). Since variable hw is set to &adapter->hw, it is equal to writel(0, tx_ring->head). We can remove similar four writel() in e1000_configure_tx() and e1000_configure_rx(). commit 0845d45e900c ("e1000e: Modify Tx/Rx configurations to avoid null pointer dereferences in e1000_open") has introduced these writel(). This commit moved register writing to e1000_configure_tx/rx(), and as result, it caused duplication in e1000_configure_tx/rx(). Signed-off-by: Takamitsu Iwai <takamitz@amazon.co.jp> --- drivers/net/ethernet/intel/e1000e/netdev.c | 6 ------ 1 file changed, 6 deletions(-)