* [PATCH v3 0/5] staging: et131x: patches for et131x driver
@ 2013-11-20 7:53 ZHAO Gang
2013-11-20 7:54 ` [PATCH v3 1/5] staging: et131x: clean up code ZHAO Gang
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: ZHAO Gang @ 2013-11-20 7:53 UTC (permalink / raw)
To: Mark Einon, Greg Kroah-Hartman; +Cc: linux-kernel
This patch set should apply to current staging-next tree
ZHAO Gang (5):
staging: et131x: clean up code
staging: et131x: drop packet when error occurs in et131x_tx
staging: et131x: stop read when hit max delay in et131x_phy_mii_read
staging: et131x: remove spinlock adapter->lock
staging: et131x: update TODO list
drivers/staging/et131x/README | 5 -
drivers/staging/et131x/et131x.c | 301 +++++++++++++++++-----------------------
2 files changed, 127 insertions(+), 179 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/5] staging: et131x: clean up code
2013-11-20 7:53 [PATCH v3 0/5] staging: et131x: patches for et131x driver ZHAO Gang
@ 2013-11-20 7:54 ` ZHAO Gang
2013-11-21 21:18 ` Mark Einon
2013-11-20 7:55 ` [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx ZHAO Gang
` (4 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: ZHAO Gang @ 2013-11-20 7:54 UTC (permalink / raw)
To: Mark Einon, Greg Kroah-Hartman; +Cc: linux-kernel
1. change function name: et1310_phy_power_down -> et1310_phy_power_switch
change function name to better describe its functionality.
2. as TODO file suggested, do this sort of things to reduce split lines
struct fbr_lookup *fbr;
fbr = rx_local->fbr[id];
Then replace all the instances of "rx_local->fbr[id]" with fbr.
3. delete unnecessary variable in function et131x_init
variable u32 numrfd is not necessary in this function.
4. some code style change
Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
---
drivers/staging/et131x/et131x.c | 209 ++++++++++++++++++++--------------------
1 file changed, 103 insertions(+), 106 deletions(-)
diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index d9446c4..836a4db 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -1679,7 +1679,7 @@ static int et131x_mdio_reset(struct mii_bus *bus)
return 0;
}
-/* et1310_phy_power_down - PHY power control
+/* et1310_phy_power_switch - PHY power control
* @adapter: device to control
* @down: true for off/false for back on
*
@@ -1688,7 +1688,7 @@ static int et131x_mdio_reset(struct mii_bus *bus)
* Can't you see that this code processed
* Phy power, phy power..
*/
-static void et1310_phy_power_down(struct et131x_adapter *adapter, bool down)
+static void et1310_phy_power_switch(struct et131x_adapter *adapter, bool down)
{
u16 data;
@@ -1822,6 +1822,9 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
u32 __iomem *base_hi;
u32 __iomem *base_lo;
+ struct fbr_lookup *fbr;
+ fbr = rx_local->fbr[id];
+
if (id == 0) {
num_des = &rx_dma->fbr0_num_des;
full_offset = &rx_dma->fbr0_full_offset;
@@ -1837,12 +1840,10 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
}
/* Now's the best time to initialize FBR contents */
- fbr_entry =
- (struct fbr_desc *) rx_local->fbr[id]->ring_virtaddr;
- for (entry = 0;
- entry < rx_local->fbr[id]->num_entries; entry++) {
- fbr_entry->addr_hi = rx_local->fbr[id]->bus_high[entry];
- fbr_entry->addr_lo = rx_local->fbr[id]->bus_low[entry];
+ fbr_entry = (struct fbr_desc *)fbr->ring_virtaddr;
+ for (entry = 0; entry < fbr->num_entries; entry++) {
+ fbr_entry->addr_hi = fbr->bus_high[entry];
+ fbr_entry->addr_lo = fbr->bus_low[entry];
fbr_entry->word2 = entry;
fbr_entry++;
}
@@ -1850,19 +1851,16 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
/* Set the address and parameters of Free buffer ring 1 and 0
* into the 1310's registers
*/
- writel(upper_32_bits(rx_local->fbr[id]->ring_physaddr),
- base_hi);
- writel(lower_32_bits(rx_local->fbr[id]->ring_physaddr),
- base_lo);
- writel(rx_local->fbr[id]->num_entries - 1, num_des);
+ writel(upper_32_bits(fbr->ring_physaddr), base_hi);
+ writel(lower_32_bits(fbr->ring_physaddr), base_lo);
+ writel(fbr->num_entries - 1, num_des);
writel(ET_DMA10_WRAP, full_offset);
/* This variable tracks the free buffer ring 1 full position,
* so it has to match the above.
*/
- rx_local->fbr[id]->local_full = ET_DMA10_WRAP;
- writel(((rx_local->fbr[id]->num_entries *
- LO_MARK_PERCENT_FOR_RX) / 100) - 1,
+ fbr->local_full = ET_DMA10_WRAP;
+ writel((fbr->num_entries * LO_MARK_PERCENT_FOR_RX / 100) - 1,
min_des);
}
@@ -1938,7 +1936,7 @@ static void et131x_adapter_setup(struct et131x_adapter *adapter)
et1310_config_macstat_regs(adapter);
- et1310_phy_power_down(adapter, 0);
+ et1310_phy_power_switch(adapter, 0);
et131x_xcvr_init(adapter);
}
@@ -2204,6 +2202,7 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
u32 pktstat_ringsize;
u32 fbr_chunksize;
struct rx_ring *rx_ring;
+ struct fbr_lookup *fbr;
/* Setup some convenience pointers */
rx_ring = &adapter->rx_ring;
@@ -2252,15 +2251,15 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
adapter->rx_ring.fbr[1]->num_entries;
for (id = 0; id < NUM_FBRS; id++) {
+ fbr = rx_ring->fbr[id];
+
/* Allocate an area of memory for Free Buffer Ring */
- bufsize =
- (sizeof(struct fbr_desc) * rx_ring->fbr[id]->num_entries);
- rx_ring->fbr[id]->ring_virtaddr =
- dma_alloc_coherent(&adapter->pdev->dev,
- bufsize,
- &rx_ring->fbr[id]->ring_physaddr,
- GFP_KERNEL);
- if (!rx_ring->fbr[id]->ring_virtaddr) {
+ bufsize = sizeof(struct fbr_desc) * fbr->num_entries;
+ fbr->ring_virtaddr = dma_alloc_coherent(&adapter->pdev->dev,
+ bufsize,
+ &fbr->ring_physaddr,
+ GFP_KERNEL);
+ if (!fbr->ring_virtaddr) {
dev_err(&adapter->pdev->dev,
"Cannot alloc memory for Free Buffer Ring %d\n", id);
return -ENOMEM;
@@ -2268,25 +2267,26 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
}
for (id = 0; id < NUM_FBRS; id++) {
- fbr_chunksize = (FBR_CHUNKS * rx_ring->fbr[id]->buffsize);
+ fbr = rx_ring->fbr[id];
+ fbr_chunksize = (FBR_CHUNKS * fbr->buffsize);
- for (i = 0;
- i < (rx_ring->fbr[id]->num_entries / FBR_CHUNKS); i++) {
+ for (i = 0; i < (fbr->num_entries / FBR_CHUNKS); i++) {
dma_addr_t fbr_tmp_physaddr;
- rx_ring->fbr[id]->mem_virtaddrs[i] = dma_alloc_coherent(
- &adapter->pdev->dev, fbr_chunksize,
- &rx_ring->fbr[id]->mem_physaddrs[i],
- GFP_KERNEL);
+ fbr->mem_virtaddrs[i] =
+ dma_alloc_coherent(&adapter->pdev->dev,
+ fbr_chunksize,
+ &fbr->mem_physaddrs[i],
+ GFP_KERNEL);
- if (!rx_ring->fbr[id]->mem_virtaddrs[i]) {
+ if (!fbr->mem_virtaddrs[i]) {
dev_err(&adapter->pdev->dev,
"Could not alloc memory\n");
return -ENOMEM;
}
/* See NOTE in "Save Physical Address" comment above */
- fbr_tmp_physaddr = rx_ring->fbr[id]->mem_physaddrs[i];
+ fbr_tmp_physaddr = fbr->mem_physaddrs[i];
for (j = 0; j < FBR_CHUNKS; j++) {
u32 index = (i * FBR_CHUNKS) + j;
@@ -2294,19 +2294,18 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
/* Save the Virtual address of this index for
* quick access later
*/
- rx_ring->fbr[id]->virt[index] =
- (u8 *) rx_ring->fbr[id]->mem_virtaddrs[i] +
- (j * rx_ring->fbr[id]->buffsize);
+ fbr->virt[index] = (u8 *)fbr->mem_virtaddrs[i] +
+ (j * fbr->buffsize);
/* now store the physical address in the
* descriptor so the device can access it
*/
- rx_ring->fbr[id]->bus_high[index] =
+ fbr->bus_high[index] =
upper_32_bits(fbr_tmp_physaddr);
- rx_ring->fbr[id]->bus_low[index] =
+ fbr->bus_low[index] =
lower_32_bits(fbr_tmp_physaddr);
- fbr_tmp_physaddr += rx_ring->fbr[id]->buffsize;
+ fbr_tmp_physaddr += fbr->buffsize;
}
}
}
@@ -2322,7 +2321,7 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
if (!rx_ring->ps_ring_virtaddr) {
dev_err(&adapter->pdev->dev,
- "Cannot alloc memory for Packet Status Ring\n");
+ "Cannot alloc memory for Packet Status Ring\n");
return -ENOMEM;
}
pr_info("Packet Status Ring %llx\n",
@@ -2341,7 +2340,7 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
GFP_KERNEL);
if (!rx_ring->rx_status_block) {
dev_err(&adapter->pdev->dev,
- "Cannot alloc memory for Status Block\n");
+ "Cannot alloc memory for Status Block\n");
return -ENOMEM;
}
rx_ring->num_rfd = NIC_DEFAULT_NUM_RFD;
@@ -2365,6 +2364,7 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
u32 pktstat_ringsize;
struct rfd *rfd;
struct rx_ring *rx_ring;
+ struct fbr_lookup *fbr;
/* Setup some convenience pointers */
rx_ring = &adapter->rx_ring;
@@ -2383,34 +2383,33 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
/* Free Free Buffer Rings */
for (id = 0; id < NUM_FBRS; id++) {
- if (!rx_ring->fbr[id]->ring_virtaddr)
+ fbr = rx_ring->fbr[id];
+
+ if (!fbr->ring_virtaddr)
continue;
/* First the packet memory */
- for (index = 0;
- index < (rx_ring->fbr[id]->num_entries / FBR_CHUNKS);
+ for (index = 0; index < (fbr->num_entries / FBR_CHUNKS);
index++) {
- if (rx_ring->fbr[id]->mem_virtaddrs[index]) {
- bufsize =
- rx_ring->fbr[id]->buffsize * FBR_CHUNKS;
+ if (fbr->mem_virtaddrs[index]) {
+ bufsize = fbr->buffsize * FBR_CHUNKS;
dma_free_coherent(&adapter->pdev->dev,
- bufsize,
- rx_ring->fbr[id]->mem_virtaddrs[index],
- rx_ring->fbr[id]->mem_physaddrs[index]);
+ bufsize,
+ fbr->mem_virtaddrs[index],
+ fbr->mem_physaddrs[index]);
- rx_ring->fbr[id]->mem_virtaddrs[index] = NULL;
+ fbr->mem_virtaddrs[index] = NULL;
}
}
- bufsize =
- sizeof(struct fbr_desc) * rx_ring->fbr[id]->num_entries;
+ bufsize = sizeof(struct fbr_desc) * fbr->num_entries;
dma_free_coherent(&adapter->pdev->dev, bufsize,
- rx_ring->fbr[id]->ring_virtaddr,
- rx_ring->fbr[id]->ring_physaddr);
+ fbr->ring_virtaddr,
+ fbr->ring_physaddr);
- rx_ring->fbr[id]->ring_virtaddr = NULL;
+ fbr->ring_virtaddr = NULL;
}
/* Free Packet Status Ring */
@@ -2419,8 +2418,8 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
adapter->rx_ring.psr_num_entries;
dma_free_coherent(&adapter->pdev->dev, pktstat_ringsize,
- rx_ring->ps_ring_virtaddr,
- rx_ring->ps_ring_physaddr);
+ rx_ring->ps_ring_virtaddr,
+ rx_ring->ps_ring_physaddr);
rx_ring->ps_ring_virtaddr = NULL;
}
@@ -2428,8 +2427,10 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
/* Free area of memory for the writeback of status information */
if (rx_ring->rx_status_block) {
dma_free_coherent(&adapter->pdev->dev,
- sizeof(struct rx_status_block),
- rx_ring->rx_status_block, rx_ring->rx_status_bus);
+ sizeof(struct rx_status_block),
+ rx_ring->rx_status_block,
+ rx_ring->rx_status_bus);
+
rx_ring->rx_status_block = NULL;
}
@@ -2450,7 +2451,6 @@ static int et131x_init_recv(struct et131x_adapter *adapter)
{
struct rfd *rfd;
u32 rfdct;
- u32 numrfd = 0;
struct rx_ring *rx_ring;
/* Setup some convenience pointers */
@@ -2467,9 +2467,8 @@ static int et131x_init_recv(struct et131x_adapter *adapter)
/* Add this RFD to the recv_list */
list_add_tail(&rfd->list_node, &rx_ring->recv_list);
- /* Increment both the available RFD's, and the total RFD's. */
+ /* Increment the available RFD's */
rx_ring->num_ready_recv++;
- numrfd++;
}
return 0;
@@ -2511,7 +2510,10 @@ static void nic_return_rfd(struct et131x_adapter *adapter, struct rfd *rfd)
*/
if (buff_index < rx_local->fbr[ring_index]->num_entries) {
u32 __iomem *offset;
+ u32 free_buff_ring;
struct fbr_desc *next;
+ struct fbr_lookup *fbr;
+ fbr = rx_local->fbr[ring_index];
spin_lock_irqsave(&adapter->fbr_lock, flags);
@@ -2520,27 +2522,25 @@ static void nic_return_rfd(struct et131x_adapter *adapter, struct rfd *rfd)
else
offset = &rx_dma->fbr1_full_offset;
- next = (struct fbr_desc *)
- (rx_local->fbr[ring_index]->ring_virtaddr) +
- INDEX10(rx_local->fbr[ring_index]->local_full);
+ next = (struct fbr_desc *)(fbr->ring_virtaddr) +
+ INDEX10(fbr->local_full);
/* Handle the Free Buffer Ring advancement here. Write
* the PA / Buffer Index for the returned buffer into
* the oldest (next to be freed)FBR entry
*/
- next->addr_hi = rx_local->fbr[ring_index]->bus_high[buff_index];
- next->addr_lo = rx_local->fbr[ring_index]->bus_low[buff_index];
+ next->addr_hi = fbr->bus_high[buff_index];
+ next->addr_lo = fbr->bus_low[buff_index];
next->word2 = buff_index;
- writel(bump_free_buff_ring(
- &rx_local->fbr[ring_index]->local_full,
- rx_local->fbr[ring_index]->num_entries - 1),
- offset);
+ free_buff_ring = bump_free_buff_ring(&fbr->local_full,
+ fbr->num_entries - 1);
+ writel(free_buff_ring, offset);
spin_unlock_irqrestore(&adapter->fbr_lock, flags);
} else {
dev_err(&adapter->pdev->dev,
- "%s illegal Buffer Index returned\n", __func__);
+ "%s illegal Buffer Index returned\n", __func__);
}
/* The processing on this RFD is done, so put it back on the tail of
@@ -2569,17 +2569,18 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
struct rx_ring *rx_local = &adapter->rx_ring;
struct rx_status_block *status;
struct pkt_stat_desc *psr;
+ struct list_head *element;
+ struct fbr_lookup *fbr;
+ struct sk_buff *skb;
struct rfd *rfd;
- u32 i;
- u8 *buf;
unsigned long flags;
- struct list_head *element;
- u8 ring_index;
- u16 buff_index;
u32 len;
u32 word0;
u32 word1;
- struct sk_buff *skb;
+ u32 i;
+ u16 buff_index;
+ u8 ring_index;
+ u8 *buf;
/* RX Status block is written by the DMA engine prior to every
* interrupt. It contains the next to be used entry in the Packet
@@ -2601,6 +2602,7 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
*/
len = psr->word1 & 0xFFFF;
ring_index = (psr->word1 >> 26) & 0x03;
+ fbr = rx_local->fbr[ring_index];
buff_index = (psr->word1 >> 16) & 0x3FF;
word0 = psr->word0;
@@ -2616,11 +2618,11 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
writel(rx_local->local_psr_full, &adapter->regs->rxdma.psr_full_offset);
- if (ring_index > 1 ||
- buff_index > rx_local->fbr[ring_index]->num_entries - 1) {
+ if (ring_index > 1 || buff_index > fbr->num_entries - 1) {
/* Illegal buffer or ring index cannot be used by S/W*/
dev_err(&adapter->pdev->dev,
- "NICRxPkts PSR Entry %d indicates length of %d and/or bad bi(%d)\n",
+ "NICRxPkts PSR Entry %d indicates"
+ " length of %d and/or bad bi(%d)\n",
rx_local->local_psr_full & 0xFFF, len, buff_index);
return NULL;
}
@@ -2670,7 +2672,7 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
&& !(adapter->packet_filter & ET131X_PACKET_TYPE_PROMISCUOUS)
&& !(adapter->packet_filter &
ET131X_PACKET_TYPE_ALL_MULTICAST)) {
- buf = rx_local->fbr[ring_index]->virt[buff_index];
+ buf = fbr->virt[buff_index];
/* Loop through our list to see if the destination
* address of this packet matches one in our list.
@@ -2723,9 +2725,7 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
adapter->net_stats.rx_bytes += rfd->len;
- memcpy(skb_put(skb, rfd->len),
- rx_local->fbr[ring_index]->virt[buff_index],
- rfd->len);
+ memcpy(skb_put(skb, rfd->len), fbr->virt[buff_index], rfd->len);
skb->protocol = eth_type_trans(skb, adapter->netdev);
skb->ip_summed = CHECKSUM_NONE;
@@ -2813,10 +2813,10 @@ static int et131x_tx_dma_memory_alloc(struct et131x_adapter *adapter)
desc_size = (sizeof(struct tx_desc) * NUM_DESC_PER_RING_TX);
tx_ring->tx_desc_ring =
- (struct tx_desc *) dma_alloc_coherent(&adapter->pdev->dev,
- desc_size,
- &tx_ring->tx_desc_ring_pa,
- GFP_KERNEL);
+ (struct tx_desc *)dma_alloc_coherent(&adapter->pdev->dev,
+ desc_size,
+ &tx_ring->tx_desc_ring_pa,
+ GFP_KERNEL);
if (!adapter->tx_ring.tx_desc_ring) {
dev_err(&adapter->pdev->dev,
"Cannot alloc memory for Tx Ring\n");
@@ -2832,9 +2832,9 @@ static int et131x_tx_dma_memory_alloc(struct et131x_adapter *adapter)
*/
/* Allocate memory for the Tx status block */
tx_ring->tx_status = dma_alloc_coherent(&adapter->pdev->dev,
- sizeof(u32),
- &tx_ring->tx_status_pa,
- GFP_KERNEL);
+ sizeof(u32),
+ &tx_ring->tx_status_pa,
+ GFP_KERNEL);
if (!adapter->tx_ring.tx_status_pa) {
dev_err(&adapter->pdev->dev,
"Cannot alloc memory for Tx status block\n");
@@ -2855,19 +2855,17 @@ static void et131x_tx_dma_memory_free(struct et131x_adapter *adapter)
if (adapter->tx_ring.tx_desc_ring) {
/* Free memory relating to Tx rings here */
desc_size = (sizeof(struct tx_desc) * NUM_DESC_PER_RING_TX);
- dma_free_coherent(&adapter->pdev->dev,
- desc_size,
- adapter->tx_ring.tx_desc_ring,
- adapter->tx_ring.tx_desc_ring_pa);
+ dma_free_coherent(&adapter->pdev->dev, desc_size,
+ adapter->tx_ring.tx_desc_ring,
+ adapter->tx_ring.tx_desc_ring_pa);
adapter->tx_ring.tx_desc_ring = NULL;
}
/* Free memory for the Tx status block */
if (adapter->tx_ring.tx_status) {
- dma_free_coherent(&adapter->pdev->dev,
- sizeof(u32),
- adapter->tx_ring.tx_status,
- adapter->tx_ring.tx_status_pa);
+ dma_free_coherent(&adapter->pdev->dev, sizeof(u32),
+ adapter->tx_ring.tx_status,
+ adapter->tx_ring.tx_status_pa);
adapter->tx_ring.tx_status = NULL;
}
@@ -3204,9 +3202,8 @@ static inline void free_send_packet(struct et131x_adapter *adapter,
* they point to
*/
do {
- desc = (struct tx_desc *)
- (adapter->tx_ring.tx_desc_ring +
- INDEX10(tcb->index_start));
+ desc = (adapter->tx_ring.tx_desc_ring +
+ INDEX10(tcb->index_start));
dma_addr = desc->addr_lo;
dma_addr |= (u64)desc->addr_hi << 32;
@@ -3222,7 +3219,7 @@ static inline void free_send_packet(struct et131x_adapter *adapter,
tcb->index_start ^= ET_DMA10_WRAP;
}
} while (desc != (adapter->tx_ring.tx_desc_ring +
- INDEX10(tcb->index)));
+ INDEX10(tcb->index)));
dev_kfree_skb_any(tcb->skb);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx
2013-11-20 7:53 [PATCH v3 0/5] staging: et131x: patches for et131x driver ZHAO Gang
2013-11-20 7:54 ` [PATCH v3 1/5] staging: et131x: clean up code ZHAO Gang
@ 2013-11-20 7:55 ` ZHAO Gang
2013-11-21 21:19 ` Mark Einon
2013-11-20 7:56 ` [PATCH v3 3/5] staging: et131x: stop read when hit max delay in et131x_phy_mii_read ZHAO Gang
` (3 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: ZHAO Gang @ 2013-11-20 7:55 UTC (permalink / raw)
To: Mark Einon, Greg Kroah-Hartman; +Cc: linux-kernel
As TODO file suggested, drop packet instead of return NETDEV_TX_BUSY
when tx failed.
et131x_tx calls function et131x_send_packets, I put the work of
et131x_send_packets directly into et131x_tx, and made some changes to
let the code more readable.
Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
---
drivers/staging/et131x/et131x.c | 84 +++++++++++------------------------------
1 file changed, 22 insertions(+), 62 deletions(-)
diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 836a4db..cda037a 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -3123,55 +3123,6 @@ static int send_packet(struct sk_buff *skb, struct et131x_adapter *adapter)
return 0;
}
-/* et131x_send_packets - This function is called by the OS to send packets
- * @skb: the packet(s) to send
- * @netdev:device on which to TX the above packet(s)
- *
- * Return 0 in almost all cases; non-zero value in extreme hard failure only
- */
-static int et131x_send_packets(struct sk_buff *skb, struct net_device *netdev)
-{
- int status = 0;
- struct et131x_adapter *adapter = netdev_priv(netdev);
-
- /* Send these packets
- *
- * NOTE: The Linux Tx entry point is only given one packet at a time
- * to Tx, so the PacketCount and it's array used makes no sense here
- */
-
- /* TCB is not available */
- if (adapter->tx_ring.used >= NUM_TCB) {
- /* NOTE: If there's an error on send, no need to queue the
- * packet under Linux; if we just send an error up to the
- * netif layer, it will resend the skb to us.
- */
- status = -ENOMEM;
- } else {
- /* We need to see if the link is up; if it's not, make the
- * netif layer think we're good and drop the packet
- */
- if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
- !netif_carrier_ok(netdev)) {
- dev_kfree_skb_any(skb);
- skb = NULL;
-
- adapter->net_stats.tx_dropped++;
- } else {
- status = send_packet(skb, adapter);
- if (status != 0 && status != -ENOMEM) {
- /* On any other error, make netif think we're
- * OK and drop the packet
- */
- dev_kfree_skb_any(skb);
- skb = NULL;
- adapter->net_stats.tx_dropped++;
- }
- }
- }
- return status;
-}
-
/* free_send_packet - Recycle a struct tcb
* @adapter: pointer to our adapter
* @tcb: pointer to struct tcb
@@ -4537,12 +4488,9 @@ static void et131x_multicast(struct net_device *netdev)
/* et131x_tx - The handler to tx a packet on the device
* @skb: data to be Tx'd
* @netdev: device on which data is to be Tx'd
- *
- * Returns 0 on success, errno on failure (as defined in errno.h)
*/
-static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
+static netdev_tx_t et131x_tx(struct sk_buff *skb, struct net_device *netdev)
{
- int status = 0;
struct et131x_adapter *adapter = netdev_priv(netdev);
/* stop the queue if it's getting full */
@@ -4553,17 +4501,29 @@ static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
/* Save the timestamp for the TX timeout watchdog */
netdev->trans_start = jiffies;
- /* Call the device-specific data Tx routine */
- status = et131x_send_packets(skb, netdev);
+ /* TCB is not available */
+ if (adapter->tx_ring.used >= NUM_TCB)
+ goto drop;
- /* Check status and manage the netif queue if necessary */
- if (status != 0) {
- if (status == -ENOMEM)
- status = NETDEV_TX_BUSY;
- else
- status = NETDEV_TX_OK;
+ /* We need to see if the link is up; if it's not, make the
+ * netif layer think we're good and drop the packet
+ */
+ if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
+ !netif_carrier_ok(netdev)) {
+ goto drop;
+ } else {
+ if (!send_packet(skb, adapter))
+ goto out;
+ /* On error (send_packet returns != 0), make netif
+ * think we're OK and drop the packet
+ */
}
- return status;
+
+drop:
+ dev_kfree_skb_any(skb);
+ adapter->net_stats.tx_dropped++;
+out:
+ return NETDEV_TX_OK;
}
/* et131x_tx_timeout - Timeout handler
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 3/5] staging: et131x: stop read when hit max delay in et131x_phy_mii_read
2013-11-20 7:53 [PATCH v3 0/5] staging: et131x: patches for et131x driver ZHAO Gang
2013-11-20 7:54 ` [PATCH v3 1/5] staging: et131x: clean up code ZHAO Gang
2013-11-20 7:55 ` [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx ZHAO Gang
@ 2013-11-20 7:56 ` ZHAO Gang
2013-11-21 21:19 ` Mark Einon
2013-11-20 7:57 ` [PATCH v3 4/5] staging: et131x: remove spinlock adapter->lock ZHAO Gang
` (2 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: ZHAO Gang @ 2013-11-20 7:56 UTC (permalink / raw)
To: Mark Einon, Greg Kroah-Hartman; +Cc: linux-kernel
stop read and return error when hit max delay time.
Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
---
drivers/staging/et131x/et131x.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index cda037a..6254a6b 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -1392,6 +1392,7 @@ static int et131x_phy_mii_read(struct et131x_adapter *adapter, u8 addr,
mii_indicator);
status = -EIO;
+ goto out;
}
/* If we hit here we were able to read the register and we need to
@@ -1399,6 +1400,7 @@ static int et131x_phy_mii_read(struct et131x_adapter *adapter, u8 addr,
*/
*value = readl(&mac->mii_mgmt_stat) & ET_MAC_MIIMGMT_STAT_PHYCRTL_MASK;
+out:
/* Stop the read operation */
writel(0, &mac->mii_mgmt_cmd);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 4/5] staging: et131x: remove spinlock adapter->lock
2013-11-20 7:53 [PATCH v3 0/5] staging: et131x: patches for et131x driver ZHAO Gang
` (2 preceding siblings ...)
2013-11-20 7:56 ` [PATCH v3 3/5] staging: et131x: stop read when hit max delay in et131x_phy_mii_read ZHAO Gang
@ 2013-11-20 7:57 ` ZHAO Gang
2013-11-21 21:19 ` Mark Einon
2013-11-20 7:58 ` [PATCH v3 5/5] staging: et131x: update TODO list ZHAO Gang
2013-11-21 21:18 ` [PATCH v3 0/5] staging: et131x: patches for et131x driver Mark Einon
5 siblings, 1 reply; 18+ messages in thread
From: ZHAO Gang @ 2013-11-20 7:57 UTC (permalink / raw)
To: Mark Einon, Greg Kroah-Hartman; +Cc: linux-kernel
adapter->lock is only used in et131x_multicast(), which is eventually
called by network stack function __dev_set_rx_mode(). __dev_set_rx_mode()
is always called by (net_device *)dev->addr_list_lock hold, to protect from
concurrent access. So adapter->lock is redundant.
Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
---
drivers/staging/et131x/et131x.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 6254a6b..66530b1 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -486,8 +486,6 @@ struct et131x_adapter {
u8 eeprom_data[2];
/* Spinlocks */
- spinlock_t lock;
-
spinlock_t tcb_send_qlock;
spinlock_t tcb_ready_qlock;
spinlock_t send_hw_lock;
@@ -3867,7 +3865,6 @@ static struct et131x_adapter *et131x_adapter_init(struct net_device *netdev,
adapter->netdev = netdev;
/* Initialize spinlocks here */
- spin_lock_init(&adapter->lock);
spin_lock_init(&adapter->tcb_send_qlock);
spin_lock_init(&adapter->tcb_ready_qlock);
spin_lock_init(&adapter->send_hw_lock);
@@ -4429,8 +4426,6 @@ static void et131x_multicast(struct net_device *netdev)
struct netdev_hw_addr *ha;
int i;
- spin_lock_irqsave(&adapter->lock, flags);
-
/* Before we modify the platform-independent filter flags, store them
* locally. This allows us to determine if anything's changed and if
* we even need to bother the hardware
@@ -4484,7 +4479,6 @@ static void et131x_multicast(struct net_device *netdev)
/* Call the device's filter function */
et131x_set_packet_filter(adapter);
}
- spin_unlock_irqrestore(&adapter->lock, flags);
}
/* et131x_tx - The handler to tx a packet on the device
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 5/5] staging: et131x: update TODO list
2013-11-20 7:53 [PATCH v3 0/5] staging: et131x: patches for et131x driver ZHAO Gang
` (3 preceding siblings ...)
2013-11-20 7:57 ` [PATCH v3 4/5] staging: et131x: remove spinlock adapter->lock ZHAO Gang
@ 2013-11-20 7:58 ` ZHAO Gang
2013-11-21 21:20 ` Mark Einon
2013-11-21 21:18 ` [PATCH v3 0/5] staging: et131x: patches for et131x driver Mark Einon
5 siblings, 1 reply; 18+ messages in thread
From: ZHAO Gang @ 2013-11-20 7:58 UTC (permalink / raw)
To: Mark Einon, Greg Kroah-Hartman; +Cc: linux-kernel
remove items that have been done
Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
---
drivers/staging/et131x/README | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/staging/et131x/README b/drivers/staging/et131x/README
index 8da96a6..00a34ea 100644
--- a/drivers/staging/et131x/README
+++ b/drivers/staging/et131x/README
@@ -11,12 +11,7 @@ TODO:
- Look at reducing the number of spinlocks
- Simplify code in nic_rx_pkts(), when determining multicast_pkts_rcvd
- Implement NAPI support
- - In et131x_tx(), don't return NETDEV_TX_BUSY, just drop the packet with kfree_skb().
- Reduce the number of split lines by careful consideration of variable names etc.
- - Do this in et131x.c:
- struct fbr_lookup *fbr;
- fbr = rx_local->fbr[id];
- Then replace all the instances of "rx_local->fbr[id]" with fbr.
Please send patches to:
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/5] staging: et131x: patches for et131x driver
2013-11-20 7:53 [PATCH v3 0/5] staging: et131x: patches for et131x driver ZHAO Gang
` (4 preceding siblings ...)
2013-11-20 7:58 ` [PATCH v3 5/5] staging: et131x: update TODO list ZHAO Gang
@ 2013-11-21 21:18 ` Mark Einon
2013-11-22 2:12 ` ZHAO Gang
5 siblings, 1 reply; 18+ messages in thread
From: Mark Einon @ 2013-11-21 21:18 UTC (permalink / raw)
To: ZHAO Gang, Greg Kroah-Hartman, linux-kernel, devel
On Wed, Nov 20, 2013 at 03:53:50PM +0800, ZHAO Gang wrote:
> This patch set should apply to current staging-next tree
>
> ZHAO Gang (5):
> staging: et131x: clean up code
> staging: et131x: drop packet when error occurs in et131x_tx
> staging: et131x: stop read when hit max delay in et131x_phy_mii_read
> staging: et131x: remove spinlock adapter->lock
> staging: et131x: update TODO list
>
> drivers/staging/et131x/README | 5 -
> drivers/staging/et131x/et131x.c | 301 +++++++++++++++++-----------------------
> 2 files changed, 127 insertions(+), 179 deletions(-)
>
> --
Hi Zhao,
Thanks for the patches. A few pointers for the future:
- Use scripts/get_maintainer.pl to discover to which mailing lists /
maintainers patches should be sent. I've cc'd devel@driverdev.osuosl.org
which was missing from your original submission.
- Please don't roll up patches you've previously sent into newer
patchsets, even if they haven't been applied by the tree maintainer
yet. This just means that the same lines of code get reviewed twice,
wasting the reviewer's time.
Cheers,
Mark
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/5] staging: et131x: clean up code
2013-11-20 7:54 ` [PATCH v3 1/5] staging: et131x: clean up code ZHAO Gang
@ 2013-11-21 21:18 ` Mark Einon
0 siblings, 0 replies; 18+ messages in thread
From: Mark Einon @ 2013-11-21 21:18 UTC (permalink / raw)
To: ZHAO Gang, Greg Kroah-Hartman, linux-kernel, devel
On Wed, Nov 20, 2013 at 03:54:29PM +0800, ZHAO Gang wrote:
> 1. change function name: et1310_phy_power_down -> et1310_phy_power_switch
> change function name to better describe its functionality.
>
> 2. as TODO file suggested, do this sort of things to reduce split lines
> struct fbr_lookup *fbr;
> fbr = rx_local->fbr[id];
> Then replace all the instances of "rx_local->fbr[id]" with fbr.
>
> 3. delete unnecessary variable in function et131x_init
> variable u32 numrfd is not necessary in this function.
>
> 4. some code style change
>
> Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
> ---
> drivers/staging/et131x/et131x.c | 209 ++++++++++++++++++++--------------------
> 1 file changed, 103 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> index d9446c4..836a4db 100644
> --- a/drivers/staging/et131x/et131x.c
> +++ b/drivers/staging/et131x/et131x.c
> @@ -1679,7 +1679,7 @@ static int et131x_mdio_reset(struct mii_bus *bus)
> return 0;
> }
>
> -/* et1310_phy_power_down - PHY power control
> +/* et1310_phy_power_switch - PHY power control
> * @adapter: device to control
> * @down: true for off/false for back on
> *
> @@ -1688,7 +1688,7 @@ static int et131x_mdio_reset(struct mii_bus *bus)
> * Can't you see that this code processed
> * Phy power, phy power..
> */
> -static void et1310_phy_power_down(struct et131x_adapter *adapter, bool down)
> +static void et1310_phy_power_switch(struct et131x_adapter *adapter, bool down)
> {
> u16 data;
>
> @@ -1822,6 +1822,9 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
> u32 __iomem *base_hi;
> u32 __iomem *base_lo;
>
> + struct fbr_lookup *fbr;
> + fbr = rx_local->fbr[id];
> +
> if (id == 0) {
> num_des = &rx_dma->fbr0_num_des;
> full_offset = &rx_dma->fbr0_full_offset;
> @@ -1837,12 +1840,10 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
> }
>
> /* Now's the best time to initialize FBR contents */
> - fbr_entry =
> - (struct fbr_desc *) rx_local->fbr[id]->ring_virtaddr;
> - for (entry = 0;
> - entry < rx_local->fbr[id]->num_entries; entry++) {
> - fbr_entry->addr_hi = rx_local->fbr[id]->bus_high[entry];
> - fbr_entry->addr_lo = rx_local->fbr[id]->bus_low[entry];
> + fbr_entry = (struct fbr_desc *)fbr->ring_virtaddr;
> + for (entry = 0; entry < fbr->num_entries; entry++) {
> + fbr_entry->addr_hi = fbr->bus_high[entry];
> + fbr_entry->addr_lo = fbr->bus_low[entry];
> fbr_entry->word2 = entry;
> fbr_entry++;
> }
> @@ -1850,19 +1851,16 @@ static void et131x_config_rx_dma_regs(struct et131x_adapter *adapter)
> /* Set the address and parameters of Free buffer ring 1 and 0
> * into the 1310's registers
> */
> - writel(upper_32_bits(rx_local->fbr[id]->ring_physaddr),
> - base_hi);
> - writel(lower_32_bits(rx_local->fbr[id]->ring_physaddr),
> - base_lo);
> - writel(rx_local->fbr[id]->num_entries - 1, num_des);
> + writel(upper_32_bits(fbr->ring_physaddr), base_hi);
> + writel(lower_32_bits(fbr->ring_physaddr), base_lo);
> + writel(fbr->num_entries - 1, num_des);
> writel(ET_DMA10_WRAP, full_offset);
>
> /* This variable tracks the free buffer ring 1 full position,
> * so it has to match the above.
> */
> - rx_local->fbr[id]->local_full = ET_DMA10_WRAP;
> - writel(((rx_local->fbr[id]->num_entries *
> - LO_MARK_PERCENT_FOR_RX) / 100) - 1,
> + fbr->local_full = ET_DMA10_WRAP;
> + writel((fbr->num_entries * LO_MARK_PERCENT_FOR_RX / 100) - 1,
> min_des);
> }
>
> @@ -1938,7 +1936,7 @@ static void et131x_adapter_setup(struct et131x_adapter *adapter)
>
> et1310_config_macstat_regs(adapter);
>
> - et1310_phy_power_down(adapter, 0);
> + et1310_phy_power_switch(adapter, 0);
> et131x_xcvr_init(adapter);
> }
>
> @@ -2204,6 +2202,7 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
> u32 pktstat_ringsize;
> u32 fbr_chunksize;
> struct rx_ring *rx_ring;
> + struct fbr_lookup *fbr;
>
> /* Setup some convenience pointers */
> rx_ring = &adapter->rx_ring;
> @@ -2252,15 +2251,15 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
> adapter->rx_ring.fbr[1]->num_entries;
>
> for (id = 0; id < NUM_FBRS; id++) {
> + fbr = rx_ring->fbr[id];
> +
> /* Allocate an area of memory for Free Buffer Ring */
> - bufsize =
> - (sizeof(struct fbr_desc) * rx_ring->fbr[id]->num_entries);
> - rx_ring->fbr[id]->ring_virtaddr =
> - dma_alloc_coherent(&adapter->pdev->dev,
> - bufsize,
> - &rx_ring->fbr[id]->ring_physaddr,
> - GFP_KERNEL);
> - if (!rx_ring->fbr[id]->ring_virtaddr) {
> + bufsize = sizeof(struct fbr_desc) * fbr->num_entries;
> + fbr->ring_virtaddr = dma_alloc_coherent(&adapter->pdev->dev,
> + bufsize,
> + &fbr->ring_physaddr,
> + GFP_KERNEL);
> + if (!fbr->ring_virtaddr) {
> dev_err(&adapter->pdev->dev,
> "Cannot alloc memory for Free Buffer Ring %d\n", id);
> return -ENOMEM;
> @@ -2268,25 +2267,26 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
> }
>
> for (id = 0; id < NUM_FBRS; id++) {
> - fbr_chunksize = (FBR_CHUNKS * rx_ring->fbr[id]->buffsize);
> + fbr = rx_ring->fbr[id];
> + fbr_chunksize = (FBR_CHUNKS * fbr->buffsize);
>
> - for (i = 0;
> - i < (rx_ring->fbr[id]->num_entries / FBR_CHUNKS); i++) {
> + for (i = 0; i < (fbr->num_entries / FBR_CHUNKS); i++) {
> dma_addr_t fbr_tmp_physaddr;
>
> - rx_ring->fbr[id]->mem_virtaddrs[i] = dma_alloc_coherent(
> - &adapter->pdev->dev, fbr_chunksize,
> - &rx_ring->fbr[id]->mem_physaddrs[i],
> - GFP_KERNEL);
> + fbr->mem_virtaddrs[i] =
> + dma_alloc_coherent(&adapter->pdev->dev,
> + fbr_chunksize,
> + &fbr->mem_physaddrs[i],
> + GFP_KERNEL);
>
> - if (!rx_ring->fbr[id]->mem_virtaddrs[i]) {
> + if (!fbr->mem_virtaddrs[i]) {
> dev_err(&adapter->pdev->dev,
> "Could not alloc memory\n");
> return -ENOMEM;
> }
>
> /* See NOTE in "Save Physical Address" comment above */
> - fbr_tmp_physaddr = rx_ring->fbr[id]->mem_physaddrs[i];
> + fbr_tmp_physaddr = fbr->mem_physaddrs[i];
>
> for (j = 0; j < FBR_CHUNKS; j++) {
> u32 index = (i * FBR_CHUNKS) + j;
> @@ -2294,19 +2294,18 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
> /* Save the Virtual address of this index for
> * quick access later
> */
> - rx_ring->fbr[id]->virt[index] =
> - (u8 *) rx_ring->fbr[id]->mem_virtaddrs[i] +
> - (j * rx_ring->fbr[id]->buffsize);
> + fbr->virt[index] = (u8 *)fbr->mem_virtaddrs[i] +
> + (j * fbr->buffsize);
>
> /* now store the physical address in the
> * descriptor so the device can access it
> */
> - rx_ring->fbr[id]->bus_high[index] =
> + fbr->bus_high[index] =
> upper_32_bits(fbr_tmp_physaddr);
> - rx_ring->fbr[id]->bus_low[index] =
> + fbr->bus_low[index] =
> lower_32_bits(fbr_tmp_physaddr);
>
> - fbr_tmp_physaddr += rx_ring->fbr[id]->buffsize;
> + fbr_tmp_physaddr += fbr->buffsize;
> }
> }
> }
> @@ -2322,7 +2321,7 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
>
> if (!rx_ring->ps_ring_virtaddr) {
> dev_err(&adapter->pdev->dev,
> - "Cannot alloc memory for Packet Status Ring\n");
> + "Cannot alloc memory for Packet Status Ring\n");
> return -ENOMEM;
> }
> pr_info("Packet Status Ring %llx\n",
> @@ -2341,7 +2340,7 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
> GFP_KERNEL);
> if (!rx_ring->rx_status_block) {
> dev_err(&adapter->pdev->dev,
> - "Cannot alloc memory for Status Block\n");
> + "Cannot alloc memory for Status Block\n");
> return -ENOMEM;
> }
> rx_ring->num_rfd = NIC_DEFAULT_NUM_RFD;
> @@ -2365,6 +2364,7 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
> u32 pktstat_ringsize;
> struct rfd *rfd;
> struct rx_ring *rx_ring;
> + struct fbr_lookup *fbr;
>
> /* Setup some convenience pointers */
> rx_ring = &adapter->rx_ring;
> @@ -2383,34 +2383,33 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
>
> /* Free Free Buffer Rings */
> for (id = 0; id < NUM_FBRS; id++) {
> - if (!rx_ring->fbr[id]->ring_virtaddr)
> + fbr = rx_ring->fbr[id];
> +
> + if (!fbr->ring_virtaddr)
> continue;
>
> /* First the packet memory */
> - for (index = 0;
> - index < (rx_ring->fbr[id]->num_entries / FBR_CHUNKS);
> + for (index = 0; index < (fbr->num_entries / FBR_CHUNKS);
> index++) {
> - if (rx_ring->fbr[id]->mem_virtaddrs[index]) {
> - bufsize =
> - rx_ring->fbr[id]->buffsize * FBR_CHUNKS;
> + if (fbr->mem_virtaddrs[index]) {
> + bufsize = fbr->buffsize * FBR_CHUNKS;
>
> dma_free_coherent(&adapter->pdev->dev,
> - bufsize,
> - rx_ring->fbr[id]->mem_virtaddrs[index],
> - rx_ring->fbr[id]->mem_physaddrs[index]);
> + bufsize,
> + fbr->mem_virtaddrs[index],
> + fbr->mem_physaddrs[index]);
>
> - rx_ring->fbr[id]->mem_virtaddrs[index] = NULL;
> + fbr->mem_virtaddrs[index] = NULL;
> }
> }
>
> - bufsize =
> - sizeof(struct fbr_desc) * rx_ring->fbr[id]->num_entries;
> + bufsize = sizeof(struct fbr_desc) * fbr->num_entries;
>
> dma_free_coherent(&adapter->pdev->dev, bufsize,
> - rx_ring->fbr[id]->ring_virtaddr,
> - rx_ring->fbr[id]->ring_physaddr);
> + fbr->ring_virtaddr,
> + fbr->ring_physaddr);
>
> - rx_ring->fbr[id]->ring_virtaddr = NULL;
> + fbr->ring_virtaddr = NULL;
> }
>
> /* Free Packet Status Ring */
> @@ -2419,8 +2418,8 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
> adapter->rx_ring.psr_num_entries;
>
> dma_free_coherent(&adapter->pdev->dev, pktstat_ringsize,
> - rx_ring->ps_ring_virtaddr,
> - rx_ring->ps_ring_physaddr);
> + rx_ring->ps_ring_virtaddr,
> + rx_ring->ps_ring_physaddr);
>
> rx_ring->ps_ring_virtaddr = NULL;
> }
> @@ -2428,8 +2427,10 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
> /* Free area of memory for the writeback of status information */
> if (rx_ring->rx_status_block) {
> dma_free_coherent(&adapter->pdev->dev,
> - sizeof(struct rx_status_block),
> - rx_ring->rx_status_block, rx_ring->rx_status_bus);
> + sizeof(struct rx_status_block),
> + rx_ring->rx_status_block,
> + rx_ring->rx_status_bus);
> +
> rx_ring->rx_status_block = NULL;
> }
>
> @@ -2450,7 +2451,6 @@ static int et131x_init_recv(struct et131x_adapter *adapter)
> {
> struct rfd *rfd;
> u32 rfdct;
> - u32 numrfd = 0;
> struct rx_ring *rx_ring;
>
> /* Setup some convenience pointers */
> @@ -2467,9 +2467,8 @@ static int et131x_init_recv(struct et131x_adapter *adapter)
> /* Add this RFD to the recv_list */
> list_add_tail(&rfd->list_node, &rx_ring->recv_list);
>
> - /* Increment both the available RFD's, and the total RFD's. */
> + /* Increment the available RFD's */
> rx_ring->num_ready_recv++;
> - numrfd++;
> }
>
> return 0;
> @@ -2511,7 +2510,10 @@ static void nic_return_rfd(struct et131x_adapter *adapter, struct rfd *rfd)
> */
> if (buff_index < rx_local->fbr[ring_index]->num_entries) {
> u32 __iomem *offset;
> + u32 free_buff_ring;
> struct fbr_desc *next;
> + struct fbr_lookup *fbr;
> + fbr = rx_local->fbr[ring_index];
>
> spin_lock_irqsave(&adapter->fbr_lock, flags);
>
> @@ -2520,27 +2522,25 @@ static void nic_return_rfd(struct et131x_adapter *adapter, struct rfd *rfd)
> else
> offset = &rx_dma->fbr1_full_offset;
>
> - next = (struct fbr_desc *)
> - (rx_local->fbr[ring_index]->ring_virtaddr) +
> - INDEX10(rx_local->fbr[ring_index]->local_full);
> + next = (struct fbr_desc *)(fbr->ring_virtaddr) +
> + INDEX10(fbr->local_full);
>
> /* Handle the Free Buffer Ring advancement here. Write
> * the PA / Buffer Index for the returned buffer into
> * the oldest (next to be freed)FBR entry
> */
> - next->addr_hi = rx_local->fbr[ring_index]->bus_high[buff_index];
> - next->addr_lo = rx_local->fbr[ring_index]->bus_low[buff_index];
> + next->addr_hi = fbr->bus_high[buff_index];
> + next->addr_lo = fbr->bus_low[buff_index];
> next->word2 = buff_index;
>
> - writel(bump_free_buff_ring(
> - &rx_local->fbr[ring_index]->local_full,
> - rx_local->fbr[ring_index]->num_entries - 1),
> - offset);
> + free_buff_ring = bump_free_buff_ring(&fbr->local_full,
> + fbr->num_entries - 1);
> + writel(free_buff_ring, offset);
>
> spin_unlock_irqrestore(&adapter->fbr_lock, flags);
> } else {
> dev_err(&adapter->pdev->dev,
> - "%s illegal Buffer Index returned\n", __func__);
> + "%s illegal Buffer Index returned\n", __func__);
> }
>
> /* The processing on this RFD is done, so put it back on the tail of
> @@ -2569,17 +2569,18 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
> struct rx_ring *rx_local = &adapter->rx_ring;
> struct rx_status_block *status;
> struct pkt_stat_desc *psr;
> + struct list_head *element;
> + struct fbr_lookup *fbr;
> + struct sk_buff *skb;
> struct rfd *rfd;
> - u32 i;
> - u8 *buf;
> unsigned long flags;
> - struct list_head *element;
> - u8 ring_index;
> - u16 buff_index;
> u32 len;
> u32 word0;
> u32 word1;
> - struct sk_buff *skb;
> + u32 i;
> + u16 buff_index;
> + u8 ring_index;
> + u8 *buf;
>
> /* RX Status block is written by the DMA engine prior to every
> * interrupt. It contains the next to be used entry in the Packet
> @@ -2601,6 +2602,7 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
> */
> len = psr->word1 & 0xFFFF;
> ring_index = (psr->word1 >> 26) & 0x03;
> + fbr = rx_local->fbr[ring_index];
> buff_index = (psr->word1 >> 16) & 0x3FF;
> word0 = psr->word0;
>
> @@ -2616,11 +2618,11 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
>
> writel(rx_local->local_psr_full, &adapter->regs->rxdma.psr_full_offset);
>
> - if (ring_index > 1 ||
> - buff_index > rx_local->fbr[ring_index]->num_entries - 1) {
> + if (ring_index > 1 || buff_index > fbr->num_entries - 1) {
> /* Illegal buffer or ring index cannot be used by S/W*/
> dev_err(&adapter->pdev->dev,
> - "NICRxPkts PSR Entry %d indicates length of %d and/or bad bi(%d)\n",
> + "NICRxPkts PSR Entry %d indicates"
> + " length of %d and/or bad bi(%d)\n",
Hi Zhao, please don't split debug strings like this - keep them whole, that way the code can be searched for them easily.
> rx_local->local_psr_full & 0xFFF, len, buff_index);
> return NULL;
> }
> @@ -2670,7 +2672,7 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
> && !(adapter->packet_filter & ET131X_PACKET_TYPE_PROMISCUOUS)
> && !(adapter->packet_filter &
> ET131X_PACKET_TYPE_ALL_MULTICAST)) {
> - buf = rx_local->fbr[ring_index]->virt[buff_index];
> + buf = fbr->virt[buff_index];
>
> /* Loop through our list to see if the destination
> * address of this packet matches one in our list.
> @@ -2723,9 +2725,7 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
>
> adapter->net_stats.rx_bytes += rfd->len;
>
> - memcpy(skb_put(skb, rfd->len),
> - rx_local->fbr[ring_index]->virt[buff_index],
> - rfd->len);
> + memcpy(skb_put(skb, rfd->len), fbr->virt[buff_index], rfd->len);
>
> skb->protocol = eth_type_trans(skb, adapter->netdev);
> skb->ip_summed = CHECKSUM_NONE;
> @@ -2813,10 +2813,10 @@ static int et131x_tx_dma_memory_alloc(struct et131x_adapter *adapter)
>
> desc_size = (sizeof(struct tx_desc) * NUM_DESC_PER_RING_TX);
> tx_ring->tx_desc_ring =
> - (struct tx_desc *) dma_alloc_coherent(&adapter->pdev->dev,
> - desc_size,
> - &tx_ring->tx_desc_ring_pa,
> - GFP_KERNEL);
> + (struct tx_desc *)dma_alloc_coherent(&adapter->pdev->dev,
> + desc_size,
> + &tx_ring->tx_desc_ring_pa,
> + GFP_KERNEL);
> if (!adapter->tx_ring.tx_desc_ring) {
> dev_err(&adapter->pdev->dev,
> "Cannot alloc memory for Tx Ring\n");
> @@ -2832,9 +2832,9 @@ static int et131x_tx_dma_memory_alloc(struct et131x_adapter *adapter)
> */
> /* Allocate memory for the Tx status block */
> tx_ring->tx_status = dma_alloc_coherent(&adapter->pdev->dev,
> - sizeof(u32),
> - &tx_ring->tx_status_pa,
> - GFP_KERNEL);
> + sizeof(u32),
> + &tx_ring->tx_status_pa,
> + GFP_KERNEL);
> if (!adapter->tx_ring.tx_status_pa) {
> dev_err(&adapter->pdev->dev,
> "Cannot alloc memory for Tx status block\n");
> @@ -2855,19 +2855,17 @@ static void et131x_tx_dma_memory_free(struct et131x_adapter *adapter)
> if (adapter->tx_ring.tx_desc_ring) {
> /* Free memory relating to Tx rings here */
> desc_size = (sizeof(struct tx_desc) * NUM_DESC_PER_RING_TX);
> - dma_free_coherent(&adapter->pdev->dev,
> - desc_size,
> - adapter->tx_ring.tx_desc_ring,
> - adapter->tx_ring.tx_desc_ring_pa);
> + dma_free_coherent(&adapter->pdev->dev, desc_size,
> + adapter->tx_ring.tx_desc_ring,
> + adapter->tx_ring.tx_desc_ring_pa);
The indenting is good, but it's more readable to keep each parameter on
it's own line if they're getting split up - keep this as 4 lines.
The rest of the changes look ok - care to fix these two issues and re-send?
Cheers,
Mark
> adapter->tx_ring.tx_desc_ring = NULL;
> }
>
> /* Free memory for the Tx status block */
> if (adapter->tx_ring.tx_status) {
> - dma_free_coherent(&adapter->pdev->dev,
> - sizeof(u32),
> - adapter->tx_ring.tx_status,
> - adapter->tx_ring.tx_status_pa);
> + dma_free_coherent(&adapter->pdev->dev, sizeof(u32),
> + adapter->tx_ring.tx_status,
> + adapter->tx_ring.tx_status_pa);
>
> adapter->tx_ring.tx_status = NULL;
> }
> @@ -3204,9 +3202,8 @@ static inline void free_send_packet(struct et131x_adapter *adapter,
> * they point to
> */
> do {
> - desc = (struct tx_desc *)
> - (adapter->tx_ring.tx_desc_ring +
> - INDEX10(tcb->index_start));
> + desc = (adapter->tx_ring.tx_desc_ring +
> + INDEX10(tcb->index_start));
>
> dma_addr = desc->addr_lo;
> dma_addr |= (u64)desc->addr_hi << 32;
> @@ -3222,7 +3219,7 @@ static inline void free_send_packet(struct et131x_adapter *adapter,
> tcb->index_start ^= ET_DMA10_WRAP;
> }
> } while (desc != (adapter->tx_ring.tx_desc_ring +
> - INDEX10(tcb->index)));
> + INDEX10(tcb->index)));
>
> dev_kfree_skb_any(tcb->skb);
> }
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx
2013-11-20 7:55 ` [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx ZHAO Gang
@ 2013-11-21 21:19 ` Mark Einon
2013-11-22 8:57 ` Dan Carpenter
2013-11-22 9:17 ` Denis Kirjanov
0 siblings, 2 replies; 18+ messages in thread
From: Mark Einon @ 2013-11-21 21:19 UTC (permalink / raw)
To: ZHAO Gang, Greg Kroah-Hartman, linux-kernel, devel
On Wed, Nov 20, 2013 at 03:55:27PM +0800, ZHAO Gang wrote:
> As TODO file suggested, drop packet instead of return NETDEV_TX_BUSY
> when tx failed.
>
> et131x_tx calls function et131x_send_packets, I put the work of
> et131x_send_packets directly into et131x_tx, and made some changes to
> let the code more readable.
>
> Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
Acked-by: Mark Einon <mark.einon@gmail.com>
> ---
> drivers/staging/et131x/et131x.c | 84 +++++++++++------------------------------
> 1 file changed, 22 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> index 836a4db..cda037a 100644
> --- a/drivers/staging/et131x/et131x.c
> +++ b/drivers/staging/et131x/et131x.c
> @@ -3123,55 +3123,6 @@ static int send_packet(struct sk_buff *skb, struct et131x_adapter *adapter)
> return 0;
> }
>
> -/* et131x_send_packets - This function is called by the OS to send packets
> - * @skb: the packet(s) to send
> - * @netdev:device on which to TX the above packet(s)
> - *
> - * Return 0 in almost all cases; non-zero value in extreme hard failure only
> - */
> -static int et131x_send_packets(struct sk_buff *skb, struct net_device *netdev)
> -{
> - int status = 0;
> - struct et131x_adapter *adapter = netdev_priv(netdev);
> -
> - /* Send these packets
> - *
> - * NOTE: The Linux Tx entry point is only given one packet at a time
> - * to Tx, so the PacketCount and it's array used makes no sense here
> - */
> -
> - /* TCB is not available */
> - if (adapter->tx_ring.used >= NUM_TCB) {
> - /* NOTE: If there's an error on send, no need to queue the
> - * packet under Linux; if we just send an error up to the
> - * netif layer, it will resend the skb to us.
> - */
> - status = -ENOMEM;
> - } else {
> - /* We need to see if the link is up; if it's not, make the
> - * netif layer think we're good and drop the packet
> - */
> - if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
> - !netif_carrier_ok(netdev)) {
> - dev_kfree_skb_any(skb);
> - skb = NULL;
> -
> - adapter->net_stats.tx_dropped++;
> - } else {
> - status = send_packet(skb, adapter);
> - if (status != 0 && status != -ENOMEM) {
> - /* On any other error, make netif think we're
> - * OK and drop the packet
> - */
> - dev_kfree_skb_any(skb);
> - skb = NULL;
> - adapter->net_stats.tx_dropped++;
> - }
> - }
> - }
> - return status;
> -}
> -
> /* free_send_packet - Recycle a struct tcb
> * @adapter: pointer to our adapter
> * @tcb: pointer to struct tcb
> @@ -4537,12 +4488,9 @@ static void et131x_multicast(struct net_device *netdev)
> /* et131x_tx - The handler to tx a packet on the device
> * @skb: data to be Tx'd
> * @netdev: device on which data is to be Tx'd
> - *
> - * Returns 0 on success, errno on failure (as defined in errno.h)
> */
> -static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
> +static netdev_tx_t et131x_tx(struct sk_buff *skb, struct net_device *netdev)
> {
> - int status = 0;
> struct et131x_adapter *adapter = netdev_priv(netdev);
>
> /* stop the queue if it's getting full */
> @@ -4553,17 +4501,29 @@ static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
> /* Save the timestamp for the TX timeout watchdog */
> netdev->trans_start = jiffies;
>
> - /* Call the device-specific data Tx routine */
> - status = et131x_send_packets(skb, netdev);
> + /* TCB is not available */
> + if (adapter->tx_ring.used >= NUM_TCB)
> + goto drop;
>
> - /* Check status and manage the netif queue if necessary */
> - if (status != 0) {
> - if (status == -ENOMEM)
> - status = NETDEV_TX_BUSY;
> - else
> - status = NETDEV_TX_OK;
> + /* We need to see if the link is up; if it's not, make the
> + * netif layer think we're good and drop the packet
> + */
> + if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
> + !netif_carrier_ok(netdev)) {
> + goto drop;
> + } else {
> + if (!send_packet(skb, adapter))
> + goto out;
> + /* On error (send_packet returns != 0), make netif
> + * think we're OK and drop the packet
> + */
> }
> - return status;
> +
> +drop:
> + dev_kfree_skb_any(skb);
> + adapter->net_stats.tx_dropped++;
> +out:
> + return NETDEV_TX_OK;
> }
>
> /* et131x_tx_timeout - Timeout handler
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 3/5] staging: et131x: stop read when hit max delay in et131x_phy_mii_read
2013-11-20 7:56 ` [PATCH v3 3/5] staging: et131x: stop read when hit max delay in et131x_phy_mii_read ZHAO Gang
@ 2013-11-21 21:19 ` Mark Einon
0 siblings, 0 replies; 18+ messages in thread
From: Mark Einon @ 2013-11-21 21:19 UTC (permalink / raw)
To: ZHAO Gang, Greg Kroah-Hartman, linux-kernel, devel
On Wed, Nov 20, 2013 at 03:56:32PM +0800, ZHAO Gang wrote:
> stop read and return error when hit max delay time.
>
> Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
Acked-by: Mark Einon <mark.einon@gmail.com>
> ---
> drivers/staging/et131x/et131x.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> index cda037a..6254a6b 100644
> --- a/drivers/staging/et131x/et131x.c
> +++ b/drivers/staging/et131x/et131x.c
> @@ -1392,6 +1392,7 @@ static int et131x_phy_mii_read(struct et131x_adapter *adapter, u8 addr,
> mii_indicator);
>
> status = -EIO;
> + goto out;
> }
>
> /* If we hit here we were able to read the register and we need to
> @@ -1399,6 +1400,7 @@ static int et131x_phy_mii_read(struct et131x_adapter *adapter, u8 addr,
> */
> *value = readl(&mac->mii_mgmt_stat) & ET_MAC_MIIMGMT_STAT_PHYCRTL_MASK;
>
> +out:
> /* Stop the read operation */
> writel(0, &mac->mii_mgmt_cmd);
>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/5] staging: et131x: remove spinlock adapter->lock
2013-11-20 7:57 ` [PATCH v3 4/5] staging: et131x: remove spinlock adapter->lock ZHAO Gang
@ 2013-11-21 21:19 ` Mark Einon
0 siblings, 0 replies; 18+ messages in thread
From: Mark Einon @ 2013-11-21 21:19 UTC (permalink / raw)
To: ZHAO Gang, Greg Kroah-Hartman, linux-kernel, devel
On Wed, Nov 20, 2013 at 03:57:15PM +0800, ZHAO Gang wrote:
> adapter->lock is only used in et131x_multicast(), which is eventually
> called by network stack function __dev_set_rx_mode(). __dev_set_rx_mode()
> is always called by (net_device *)dev->addr_list_lock hold, to protect from
> concurrent access. So adapter->lock is redundant.
>
> Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
Acked-by: Mark Einon <mark.einon@gmail.com>
> ---
> drivers/staging/et131x/et131x.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> index 6254a6b..66530b1 100644
> --- a/drivers/staging/et131x/et131x.c
> +++ b/drivers/staging/et131x/et131x.c
> @@ -486,8 +486,6 @@ struct et131x_adapter {
> u8 eeprom_data[2];
>
> /* Spinlocks */
> - spinlock_t lock;
> -
> spinlock_t tcb_send_qlock;
> spinlock_t tcb_ready_qlock;
> spinlock_t send_hw_lock;
> @@ -3867,7 +3865,6 @@ static struct et131x_adapter *et131x_adapter_init(struct net_device *netdev,
> adapter->netdev = netdev;
>
> /* Initialize spinlocks here */
> - spin_lock_init(&adapter->lock);
> spin_lock_init(&adapter->tcb_send_qlock);
> spin_lock_init(&adapter->tcb_ready_qlock);
> spin_lock_init(&adapter->send_hw_lock);
> @@ -4429,8 +4426,6 @@ static void et131x_multicast(struct net_device *netdev)
> struct netdev_hw_addr *ha;
> int i;
>
> - spin_lock_irqsave(&adapter->lock, flags);
> -
> /* Before we modify the platform-independent filter flags, store them
> * locally. This allows us to determine if anything's changed and if
> * we even need to bother the hardware
> @@ -4484,7 +4479,6 @@ static void et131x_multicast(struct net_device *netdev)
> /* Call the device's filter function */
> et131x_set_packet_filter(adapter);
> }
> - spin_unlock_irqrestore(&adapter->lock, flags);
> }
>
> /* et131x_tx - The handler to tx a packet on the device
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 5/5] staging: et131x: update TODO list
2013-11-20 7:58 ` [PATCH v3 5/5] staging: et131x: update TODO list ZHAO Gang
@ 2013-11-21 21:20 ` Mark Einon
0 siblings, 0 replies; 18+ messages in thread
From: Mark Einon @ 2013-11-21 21:20 UTC (permalink / raw)
To: ZHAO Gang, Greg Kroah-Hartman, linux-kernel, devel
On Wed, Nov 20, 2013 at 03:58:37PM +0800, ZHAO Gang wrote:
> remove items that have been done
>
> Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
If the two small issues in patch 1/5 are fixed:
Acked-by: Mark Einon <mark.einon@gmail.com>
> ---
> drivers/staging/et131x/README | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/staging/et131x/README b/drivers/staging/et131x/README
> index 8da96a6..00a34ea 100644
> --- a/drivers/staging/et131x/README
> +++ b/drivers/staging/et131x/README
> @@ -11,12 +11,7 @@ TODO:
> - Look at reducing the number of spinlocks
> - Simplify code in nic_rx_pkts(), when determining multicast_pkts_rcvd
> - Implement NAPI support
> - - In et131x_tx(), don't return NETDEV_TX_BUSY, just drop the packet with kfree_skb().
> - Reduce the number of split lines by careful consideration of variable names etc.
> - - Do this in et131x.c:
> - struct fbr_lookup *fbr;
> - fbr = rx_local->fbr[id];
> - Then replace all the instances of "rx_local->fbr[id]" with fbr.
>
> Please send patches to:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/5] staging: et131x: patches for et131x driver
2013-11-21 21:18 ` [PATCH v3 0/5] staging: et131x: patches for et131x driver Mark Einon
@ 2013-11-22 2:12 ` ZHAO Gang
0 siblings, 0 replies; 18+ messages in thread
From: ZHAO Gang @ 2013-11-22 2:12 UTC (permalink / raw)
To: Mark Einon; +Cc: Greg Kroah-Hartman, LKML, devel
On Fri, Nov 22, 2013 at 5:18 AM, Mark Einon <mark.einon@gmail.com> wrote:
> On Wed, Nov 20, 2013 at 03:53:50PM +0800, ZHAO Gang wrote:
>> This patch set should apply to current staging-next tree
>>
>> ZHAO Gang (5):
>> staging: et131x: clean up code
>> staging: et131x: drop packet when error occurs in et131x_tx
>> staging: et131x: stop read when hit max delay in et131x_phy_mii_read
>> staging: et131x: remove spinlock adapter->lock
>> staging: et131x: update TODO list
>>
>> drivers/staging/et131x/README | 5 -
>> drivers/staging/et131x/et131x.c | 301 +++++++++++++++++-----------------------
>> 2 files changed, 127 insertions(+), 179 deletions(-)
>>
>> --
>
> Hi Zhao,
>
> Thanks for the patches. A few pointers for the future:
>
> - Use scripts/get_maintainer.pl to discover to which mailing lists /
> maintainers patches should be sent. I've cc'd devel@driverdev.osuosl.org
> which was missing from your original submission.
I will add it to cc list next time.
>
> - Please don't roll up patches you've previously sent into newer
> patchsets, even if they haven't been applied by the tree maintainer
> yet. This just means that the same lines of code get reviewed twice,
> wasting the reviewer's time.
Greg also told me this problem, I will not do this any more.
>
> Cheers,
>
> Mark
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx
2013-11-21 21:19 ` Mark Einon
@ 2013-11-22 8:57 ` Dan Carpenter
2013-11-22 9:17 ` Denis Kirjanov
1 sibling, 0 replies; 18+ messages in thread
From: Dan Carpenter @ 2013-11-22 8:57 UTC (permalink / raw)
To: Mark Einon; +Cc: ZHAO Gang, Greg Kroah-Hartman, linux-kernel, devel
On Thu, Nov 21, 2013 at 09:19:21PM +0000, Mark Einon wrote:
> On Wed, Nov 20, 2013 at 03:55:27PM +0800, ZHAO Gang wrote:
> > As TODO file suggested, drop packet instead of return NETDEV_TX_BUSY
> > when tx failed.
> >
> > et131x_tx calls function et131x_send_packets, I put the work of
> > et131x_send_packets directly into et131x_tx, and made some changes to
> > let the code more readable.
> >
> > Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
>
> Acked-by: Mark Einon <mark.einon@gmail.com>
>
> > ---
> > drivers/staging/et131x/et131x.c | 84 +++++++++++------------------------------
> > 1 file changed, 22 insertions(+), 62 deletions(-)
> >
> > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> > index 836a4db..cda037a 100644
> > --- a/drivers/staging/et131x/et131x.c
> > +++ b/drivers/staging/et131x/et131x.c
> > @@ -3123,55 +3123,6 @@ static int send_packet(struct sk_buff *skb, struct et131x_adapter *adapter)
> > return 0;
> > }
> >
> > -/* et131x_send_packets - This function is called by the OS to send packets
> > - * @skb: the packet(s) to send
> > - * @netdev:device on which to TX the above packet(s)
> > - *
> > - * Return 0 in almost all cases; non-zero value in extreme hard failure only
> > - */
> > -static int et131x_send_packets(struct sk_buff *skb, struct net_device *netdev)
> > -{
> > - int status = 0;
> > - struct et131x_adapter *adapter = netdev_priv(netdev);
> > -
> > - /* Send these packets
> > - *
> > - * NOTE: The Linux Tx entry point is only given one packet at a time
> > - * to Tx, so the PacketCount and it's array used makes no sense here
> > - */
> > -
> > - /* TCB is not available */
> > - if (adapter->tx_ring.used >= NUM_TCB) {
> > - /* NOTE: If there's an error on send, no need to queue the
> > - * packet under Linux; if we just send an error up to the
> > - * netif layer, it will resend the skb to us.
> > - */
> > - status = -ENOMEM;
> > - } else {
> > - /* We need to see if the link is up; if it's not, make the
> > - * netif layer think we're good and drop the packet
> > - */
> > - if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
> > - !netif_carrier_ok(netdev)) {
> > - dev_kfree_skb_any(skb);
> > - skb = NULL;
> > -
> > - adapter->net_stats.tx_dropped++;
> > - } else {
> > - status = send_packet(skb, adapter);
> > - if (status != 0 && status != -ENOMEM) {
> > - /* On any other error, make netif think we're
> > - * OK and drop the packet
> > - */
> > - dev_kfree_skb_any(skb);
> > - skb = NULL;
> > - adapter->net_stats.tx_dropped++;
> > - }
> > - }
> > - }
> > - return status;
> > -}
> > -
> > /* free_send_packet - Recycle a struct tcb
> > * @adapter: pointer to our adapter
> > * @tcb: pointer to struct tcb
> > @@ -4537,12 +4488,9 @@ static void et131x_multicast(struct net_device *netdev)
> > /* et131x_tx - The handler to tx a packet on the device
> > * @skb: data to be Tx'd
> > * @netdev: device on which data is to be Tx'd
> > - *
> > - * Returns 0 on success, errno on failure (as defined in errno.h)
> > */
> > -static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
> > +static netdev_tx_t et131x_tx(struct sk_buff *skb, struct net_device *netdev)
> > {
> > - int status = 0;
> > struct et131x_adapter *adapter = netdev_priv(netdev);
> >
> > /* stop the queue if it's getting full */
> > @@ -4553,17 +4501,29 @@ static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
> > /* Save the timestamp for the TX timeout watchdog */
> > netdev->trans_start = jiffies;
> >
> > - /* Call the device-specific data Tx routine */
> > - status = et131x_send_packets(skb, netdev);
> > + /* TCB is not available */
> > + if (adapter->tx_ring.used >= NUM_TCB)
> > + goto drop;
> >
> > - /* Check status and manage the netif queue if necessary */
> > - if (status != 0) {
> > - if (status == -ENOMEM)
> > - status = NETDEV_TX_BUSY;
> > - else
> > - status = NETDEV_TX_OK;
> > + /* We need to see if the link is up; if it's not, make the
> > + * netif layer think we're good and drop the packet
> > + */
> > + if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
> > + !netif_carrier_ok(netdev)) {
> > + goto drop;
> > + } else {
> > + if (!send_packet(skb, adapter))
> > + goto out;
> > + /* On error (send_packet returns != 0), make netif
> > + * think we're OK and drop the packet
> > + */
> > }
> > - return status;
> > +
> > +drop:
> > + dev_kfree_skb_any(skb);
> > + adapter->net_stats.tx_dropped++;
> > +out:
> > + return NETDEV_TX_OK;
> > }
This is a lot of spaghetti. Just write it like this:
if (adapter->tx_ring.used >= NUM_TCB)
goto drop;
if (adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK)
goto drop;
if (!netif_carrier_ok(netdev))
goto drop;
status = send_packet(skb, adapter);
if (status)
goto drop;
return NETDEV_TX_OK;
drop:
dev_kfree_skb_any(skb);
adapter->net_stats.tx_dropped++;
/* We return success deliberately to make the netif layer happy */
return NETDEV_TX_OK;
In the original code the success path was if condition false, then if
condition true, then follow a goto then return. In the new code it is
straight line path to success with no nested if else statements.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx
2013-11-21 21:19 ` Mark Einon
2013-11-22 8:57 ` Dan Carpenter
@ 2013-11-22 9:17 ` Denis Kirjanov
2013-11-22 11:47 ` ZHAO Gang
1 sibling, 1 reply; 18+ messages in thread
From: Denis Kirjanov @ 2013-11-22 9:17 UTC (permalink / raw)
To: Mark Einon; +Cc: ZHAO Gang, Greg Kroah-Hartman, linux-kernel, devel
On 11/22/13, Mark Einon <mark.einon@gmail.com> wrote:
> On Wed, Nov 20, 2013 at 03:55:27PM +0800, ZHAO Gang wrote:
>> As TODO file suggested, drop packet instead of return NETDEV_TX_BUSY
>> when tx failed.
>>
>> et131x_tx calls function et131x_send_packets, I put the work of
>> et131x_send_packets directly into et131x_tx, and made some changes to
>> let the code more readable.
>>
>> Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
>
> Acked-by: Mark Einon <mark.einon@gmail.com>
>
>> ---
>> drivers/staging/et131x/et131x.c | 84
>> +++++++++++------------------------------
>> 1 file changed, 22 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/staging/et131x/et131x.c
>> b/drivers/staging/et131x/et131x.c
>> index 836a4db..cda037a 100644
>> --- a/drivers/staging/et131x/et131x.c
>> +++ b/drivers/staging/et131x/et131x.c
>> @@ -3123,55 +3123,6 @@ static int send_packet(struct sk_buff *skb, struct
>> et131x_adapter *adapter)
>> return 0;
>> }
>>
>> -/* et131x_send_packets - This function is called by the OS to send
>> packets
>> - * @skb: the packet(s) to send
>> - * @netdev:device on which to TX the above packet(s)
>> - *
>> - * Return 0 in almost all cases; non-zero value in extreme hard failure
>> only
>> - */
>> -static int et131x_send_packets(struct sk_buff *skb, struct net_device
>> *netdev)
>> -{
>> - int status = 0;
>> - struct et131x_adapter *adapter = netdev_priv(netdev);
>> -
>> - /* Send these packets
>> - *
>> - * NOTE: The Linux Tx entry point is only given one packet at a time
>> - * to Tx, so the PacketCount and it's array used makes no sense here
>> - */
>> -
>> - /* TCB is not available */
>> - if (adapter->tx_ring.used >= NUM_TCB) {
>> - /* NOTE: If there's an error on send, no need to queue the
>> - * packet under Linux; if we just send an error up to the
>> - * netif layer, it will resend the skb to us.
>> - */
>> - status = -ENOMEM;
>> - } else {
>> - /* We need to see if the link is up; if it's not, make the
>> - * netif layer think we're good and drop the packet
>> - */
>> - if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
>> - !netif_carrier_ok(netdev)) {
>> - dev_kfree_skb_any(skb);
>> - skb = NULL;
>> -
>> - adapter->net_stats.tx_dropped++;
>> - } else {
>> - status = send_packet(skb, adapter);
>> - if (status != 0 && status != -ENOMEM) {
>> - /* On any other error, make netif think we're
>> - * OK and drop the packet
>> - */
>> - dev_kfree_skb_any(skb);
>> - skb = NULL;
>> - adapter->net_stats.tx_dropped++;
>> - }
>> - }
>> - }
>> - return status;
>> -}
>> -
>> /* free_send_packet - Recycle a struct tcb
>> * @adapter: pointer to our adapter
>> * @tcb: pointer to struct tcb
>> @@ -4537,12 +4488,9 @@ static void et131x_multicast(struct net_device
>> *netdev)
>> /* et131x_tx - The handler to tx a packet on the device
>> * @skb: data to be Tx'd
>> * @netdev: device on which data is to be Tx'd
>> - *
>> - * Returns 0 on success, errno on failure (as defined in errno.h)
>> */
>> -static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
>> +static netdev_tx_t et131x_tx(struct sk_buff *skb, struct net_device
>> *netdev)
>> {
>> - int status = 0;
>> struct et131x_adapter *adapter = netdev_priv(netdev);
>>
>> /* stop the queue if it's getting full */
>> @@ -4553,17 +4501,29 @@ static int et131x_tx(struct sk_buff *skb, struct
>> net_device *netdev)
>> /* Save the timestamp for the TX timeout watchdog */
>> netdev->trans_start = jiffies;
>>
>> - /* Call the device-specific data Tx routine */
>> - status = et131x_send_packets(skb, netdev);
>> + /* TCB is not available */
>> + if (adapter->tx_ring.used >= NUM_TCB)
>> + goto drop;
That's wrong. You have to properly manage tx queue and not just drop packets
>> - /* Check status and manage the netif queue if necessary */
>> - if (status != 0) {
>> - if (status == -ENOMEM)
>> - status = NETDEV_TX_BUSY;
>> - else
>> - status = NETDEV_TX_OK;
>> + /* We need to see if the link is up; if it's not, make the
>> + * netif layer think we're good and drop the packet
>> + */
>> + if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
>> + !netif_carrier_ok(netdev)) {
>> + goto drop;
>> + } else {
>> + if (!send_packet(skb, adapter))
>> + goto out;
>> + /* On error (send_packet returns != 0), make netif
>> + * think we're OK and drop the packet
>> + */
>> }
>> - return status;
>> +
>> +drop:
>> + dev_kfree_skb_any(skb);
>> + adapter->net_stats.tx_dropped++;
>> +out:
>> + return NETDEV_TX_OK;
>> }
>>
>> /* et131x_tx_timeout - Timeout handler
>> --
>> 1.8.3.1
>>
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>
--
Regards,
Denis
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx
2013-11-22 9:17 ` Denis Kirjanov
@ 2013-11-22 11:47 ` ZHAO Gang
2013-11-22 11:56 ` Denis Kirjanov
0 siblings, 1 reply; 18+ messages in thread
From: ZHAO Gang @ 2013-11-22 11:47 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: Mark Einon, Greg Kroah-Hartman, LKML, STAGING SUBSYSTEM
On Fri, Nov 22, 2013 at 5:17 PM, Denis Kirjanov <kirjanov@gmail.com> wrote:
> On 11/22/13, Mark Einon <mark.einon@gmail.com> wrote:
>> On Wed, Nov 20, 2013 at 03:55:27PM +0800, ZHAO Gang wrote:
>>> As TODO file suggested, drop packet instead of return NETDEV_TX_BUSY
>>> when tx failed.
>>>
>>> et131x_tx calls function et131x_send_packets, I put the work of
>>> et131x_send_packets directly into et131x_tx, and made some changes to
>>> let the code more readable.
>>>
>>> Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
>>
>> Acked-by: Mark Einon <mark.einon@gmail.com>
>>
>>> ---
>>> drivers/staging/et131x/et131x.c | 84
>>> +++++++++++------------------------------
>>> 1 file changed, 22 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/drivers/staging/et131x/et131x.c
>>> b/drivers/staging/et131x/et131x.c
>>> index 836a4db..cda037a 100644
>>> --- a/drivers/staging/et131x/et131x.c
>>> +++ b/drivers/staging/et131x/et131x.c
>>> @@ -3123,55 +3123,6 @@ static int send_packet(struct sk_buff *skb, struct
>>> et131x_adapter *adapter)
>>> return 0;
>>> }
>>>
>>> -/* et131x_send_packets - This function is called by the OS to send
>>> packets
>>> - * @skb: the packet(s) to send
>>> - * @netdev:device on which to TX the above packet(s)
>>> - *
>>> - * Return 0 in almost all cases; non-zero value in extreme hard failure
>>> only
>>> - */
>>> -static int et131x_send_packets(struct sk_buff *skb, struct net_device
>>> *netdev)
>>> -{
>>> - int status = 0;
>>> - struct et131x_adapter *adapter = netdev_priv(netdev);
>>> -
>>> - /* Send these packets
>>> - *
>>> - * NOTE: The Linux Tx entry point is only given one packet at a time
>>> - * to Tx, so the PacketCount and it's array used makes no sense here
>>> - */
>>> -
>>> - /* TCB is not available */
>>> - if (adapter->tx_ring.used >= NUM_TCB) {
>>> - /* NOTE: If there's an error on send, no need to queue the
>>> - * packet under Linux; if we just send an error up to the
>>> - * netif layer, it will resend the skb to us.
>>> - */
>>> - status = -ENOMEM;
>>> - } else {
>>> - /* We need to see if the link is up; if it's not, make the
>>> - * netif layer think we're good and drop the packet
>>> - */
>>> - if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
>>> - !netif_carrier_ok(netdev)) {
>>> - dev_kfree_skb_any(skb);
>>> - skb = NULL;
>>> -
>>> - adapter->net_stats.tx_dropped++;
>>> - } else {
>>> - status = send_packet(skb, adapter);
>>> - if (status != 0 && status != -ENOMEM) {
>>> - /* On any other error, make netif think we're
>>> - * OK and drop the packet
>>> - */
>>> - dev_kfree_skb_any(skb);
>>> - skb = NULL;
>>> - adapter->net_stats.tx_dropped++;
>>> - }
>>> - }
>>> - }
>>> - return status;
>>> -}
>>> -
>>> /* free_send_packet - Recycle a struct tcb
>>> * @adapter: pointer to our adapter
>>> * @tcb: pointer to struct tcb
>>> @@ -4537,12 +4488,9 @@ static void et131x_multicast(struct net_device
>>> *netdev)
>>> /* et131x_tx - The handler to tx a packet on the device
>>> * @skb: data to be Tx'd
>>> * @netdev: device on which data is to be Tx'd
>>> - *
>>> - * Returns 0 on success, errno on failure (as defined in errno.h)
>>> */
>>> -static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
>>> +static netdev_tx_t et131x_tx(struct sk_buff *skb, struct net_device
>>> *netdev)
>>> {
>>> - int status = 0;
>>> struct et131x_adapter *adapter = netdev_priv(netdev);
>>>
>>> /* stop the queue if it's getting full */
>>> @@ -4553,17 +4501,29 @@ static int et131x_tx(struct sk_buff *skb, struct
>>> net_device *netdev)
>>> /* Save the timestamp for the TX timeout watchdog */
>>> netdev->trans_start = jiffies;
>>>
>>> - /* Call the device-specific data Tx routine */
>>> - status = et131x_send_packets(skb, netdev);
>>> + /* TCB is not available */
>>> + if (adapter->tx_ring.used >= NUM_TCB)
>>> + goto drop;
>
> That's wrong. You have to properly manage tx queue and not just drop packets
Could you kindly tell me what should be done to tx queue?
>
>>> - /* Check status and manage the netif queue if necessary */
>>> - if (status != 0) {
>>> - if (status == -ENOMEM)
>>> - status = NETDEV_TX_BUSY;
>>> - else
>>> - status = NETDEV_TX_OK;
>>> + /* We need to see if the link is up; if it's not, make the
>>> + * netif layer think we're good and drop the packet
>>> + */
>>> + if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
>>> + !netif_carrier_ok(netdev)) {
>>> + goto drop;
>>> + } else {
>>> + if (!send_packet(skb, adapter))
>>> + goto out;
>>> + /* On error (send_packet returns != 0), make netif
>>> + * think we're OK and drop the packet
>>> + */
>>> }
>>> - return status;
>>> +
>>> +drop:
>>> + dev_kfree_skb_any(skb);
>>> + adapter->net_stats.tx_dropped++;
>>> +out:
>>> + return NETDEV_TX_OK;
>>> }
>>>
>>> /* et131x_tx_timeout - Timeout handler
>>> --
>>> 1.8.3.1
>>>
>> _______________________________________________
>> devel mailing list
>> devel@linuxdriverproject.org
>> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>>
>
>
> --
> Regards,
> Denis
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx
2013-11-22 11:47 ` ZHAO Gang
@ 2013-11-22 11:56 ` Denis Kirjanov
2013-11-22 12:26 ` ZHAO Gang
0 siblings, 1 reply; 18+ messages in thread
From: Denis Kirjanov @ 2013-11-22 11:56 UTC (permalink / raw)
To: ZHAO Gang; +Cc: Mark Einon, Greg Kroah-Hartman, LKML, STAGING SUBSYSTEM
If you have no free TX descriptors that means that something went
wrong and it's a BUG. You have to tell the stack to stop sending
packets using netif_stop_queue() and reenable transmissions once tx
descriptors will be available. There are a lot of live examples in the
source tree.
On 11/22/13, ZHAO Gang <gamerh2o@gmail.com> wrote:
> On Fri, Nov 22, 2013 at 5:17 PM, Denis Kirjanov <kirjanov@gmail.com> wrote:
>> On 11/22/13, Mark Einon <mark.einon@gmail.com> wrote:
>>> On Wed, Nov 20, 2013 at 03:55:27PM +0800, ZHAO Gang wrote:
>>>> As TODO file suggested, drop packet instead of return NETDEV_TX_BUSY
>>>> when tx failed.
>>>>
>>>> et131x_tx calls function et131x_send_packets, I put the work of
>>>> et131x_send_packets directly into et131x_tx, and made some changes to
>>>> let the code more readable.
>>>>
>>>> Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
>>>
>>> Acked-by: Mark Einon <mark.einon@gmail.com>
>>>
>>>> ---
>>>> drivers/staging/et131x/et131x.c | 84
>>>> +++++++++++------------------------------
>>>> 1 file changed, 22 insertions(+), 62 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/et131x/et131x.c
>>>> b/drivers/staging/et131x/et131x.c
>>>> index 836a4db..cda037a 100644
>>>> --- a/drivers/staging/et131x/et131x.c
>>>> +++ b/drivers/staging/et131x/et131x.c
>>>> @@ -3123,55 +3123,6 @@ static int send_packet(struct sk_buff *skb,
>>>> struct
>>>> et131x_adapter *adapter)
>>>> return 0;
>>>> }
>>>>
>>>> -/* et131x_send_packets - This function is called by the OS to send
>>>> packets
>>>> - * @skb: the packet(s) to send
>>>> - * @netdev:device on which to TX the above packet(s)
>>>> - *
>>>> - * Return 0 in almost all cases; non-zero value in extreme hard
>>>> failure
>>>> only
>>>> - */
>>>> -static int et131x_send_packets(struct sk_buff *skb, struct net_device
>>>> *netdev)
>>>> -{
>>>> - int status = 0;
>>>> - struct et131x_adapter *adapter = netdev_priv(netdev);
>>>> -
>>>> - /* Send these packets
>>>> - *
>>>> - * NOTE: The Linux Tx entry point is only given one packet at a
>>>> time
>>>> - * to Tx, so the PacketCount and it's array used makes no sense
>>>> here
>>>> - */
>>>> -
>>>> - /* TCB is not available */
>>>> - if (adapter->tx_ring.used >= NUM_TCB) {
>>>> - /* NOTE: If there's an error on send, no need to queue the
>>>> - * packet under Linux; if we just send an error up to the
>>>> - * netif layer, it will resend the skb to us.
>>>> - */
>>>> - status = -ENOMEM;
>>>> - } else {
>>>> - /* We need to see if the link is up; if it's not, make the
>>>> - * netif layer think we're good and drop the packet
>>>> - */
>>>> - if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
>>>> - !netif_carrier_ok(netdev)) {
>>>> - dev_kfree_skb_any(skb);
>>>> - skb = NULL;
>>>> -
>>>> - adapter->net_stats.tx_dropped++;
>>>> - } else {
>>>> - status = send_packet(skb, adapter);
>>>> - if (status != 0 && status != -ENOMEM) {
>>>> - /* On any other error, make netif think
>>>> we're
>>>> - * OK and drop the packet
>>>> - */
>>>> - dev_kfree_skb_any(skb);
>>>> - skb = NULL;
>>>> - adapter->net_stats.tx_dropped++;
>>>> - }
>>>> - }
>>>> - }
>>>> - return status;
>>>> -}
>>>> -
>>>> /* free_send_packet - Recycle a struct tcb
>>>> * @adapter: pointer to our adapter
>>>> * @tcb: pointer to struct tcb
>>>> @@ -4537,12 +4488,9 @@ static void et131x_multicast(struct net_device
>>>> *netdev)
>>>> /* et131x_tx - The handler to tx a packet on the device
>>>> * @skb: data to be Tx'd
>>>> * @netdev: device on which data is to be Tx'd
>>>> - *
>>>> - * Returns 0 on success, errno on failure (as defined in errno.h)
>>>> */
>>>> -static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
>>>> +static netdev_tx_t et131x_tx(struct sk_buff *skb, struct net_device
>>>> *netdev)
>>>> {
>>>> - int status = 0;
>>>> struct et131x_adapter *adapter = netdev_priv(netdev);
>>>>
>>>> /* stop the queue if it's getting full */
>>>> @@ -4553,17 +4501,29 @@ static int et131x_tx(struct sk_buff *skb,
>>>> struct
>>>> net_device *netdev)
>>>> /* Save the timestamp for the TX timeout watchdog */
>>>> netdev->trans_start = jiffies;
>>>>
>>>> - /* Call the device-specific data Tx routine */
>>>> - status = et131x_send_packets(skb, netdev);
>>>> + /* TCB is not available */
>>>> + if (adapter->tx_ring.used >= NUM_TCB)
>>>> + goto drop;
>>
>> That's wrong. You have to properly manage tx queue and not just drop
>> packets
>
> Could you kindly tell me what should be done to tx queue?
>
>>
>>>> - /* Check status and manage the netif queue if necessary */
>>>> - if (status != 0) {
>>>> - if (status == -ENOMEM)
>>>> - status = NETDEV_TX_BUSY;
>>>> - else
>>>> - status = NETDEV_TX_OK;
>>>> + /* We need to see if the link is up; if it's not, make the
>>>> + * netif layer think we're good and drop the packet
>>>> + */
>>>> + if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
>>>> + !netif_carrier_ok(netdev)) {
>>>> + goto drop;
>>>> + } else {
>>>> + if (!send_packet(skb, adapter))
>>>> + goto out;
>>>> + /* On error (send_packet returns != 0), make netif
>>>> + * think we're OK and drop the packet
>>>> + */
>>>> }
>>>> - return status;
>>>> +
>>>> +drop:
>>>> + dev_kfree_skb_any(skb);
>>>> + adapter->net_stats.tx_dropped++;
>>>> +out:
>>>> + return NETDEV_TX_OK;
>>>> }
>>>>
>>>> /* et131x_tx_timeout - Timeout handler
>>>> --
>>>> 1.8.3.1
>>>>
>>> _______________________________________________
>>> devel mailing list
>>> devel@linuxdriverproject.org
>>> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>>>
>>
>>
>> --
>> Regards,
>> Denis
>
--
Regards,
Denis
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx
2013-11-22 11:56 ` Denis Kirjanov
@ 2013-11-22 12:26 ` ZHAO Gang
0 siblings, 0 replies; 18+ messages in thread
From: ZHAO Gang @ 2013-11-22 12:26 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: Mark Einon, Greg Kroah-Hartman, LKML, STAGING SUBSYSTEM
On Fri, Nov 22, 2013 at 7:56 PM, Denis Kirjanov <kirjanov@gmail.com> wrote:
> If you have no free TX descriptors that means that something went
> wrong and it's a BUG. You have to tell the stack to stop sending
> packets using netif_stop_queue() and reenable transmissions once tx
> descriptors will be available. There are a lot of live examples in the
> source tree.
>
The stop queue code dose not show in the diff, actually it is at the
beginning of et131x_tx:
/* stop the queue if it's getting full */
if (adapter->tx_ring.used >= NUM_TCB - 1 &&
!netif_queue_stopped(netdev))
netif_stop_queue(netdev);
I think the logic is when there is only one descriptor left, stop the
queue and transmit this packet, if tx queue is full, it must have been
stopped, so just drop the packet.
Cheers
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2013-11-22 12:26 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-20 7:53 [PATCH v3 0/5] staging: et131x: patches for et131x driver ZHAO Gang
2013-11-20 7:54 ` [PATCH v3 1/5] staging: et131x: clean up code ZHAO Gang
2013-11-21 21:18 ` Mark Einon
2013-11-20 7:55 ` [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx ZHAO Gang
2013-11-21 21:19 ` Mark Einon
2013-11-22 8:57 ` Dan Carpenter
2013-11-22 9:17 ` Denis Kirjanov
2013-11-22 11:47 ` ZHAO Gang
2013-11-22 11:56 ` Denis Kirjanov
2013-11-22 12:26 ` ZHAO Gang
2013-11-20 7:56 ` [PATCH v3 3/5] staging: et131x: stop read when hit max delay in et131x_phy_mii_read ZHAO Gang
2013-11-21 21:19 ` Mark Einon
2013-11-20 7:57 ` [PATCH v3 4/5] staging: et131x: remove spinlock adapter->lock ZHAO Gang
2013-11-21 21:19 ` Mark Einon
2013-11-20 7:58 ` [PATCH v3 5/5] staging: et131x: update TODO list ZHAO Gang
2013-11-21 21:20 ` Mark Einon
2013-11-21 21:18 ` [PATCH v3 0/5] staging: et131x: patches for et131x driver Mark Einon
2013-11-22 2:12 ` ZHAO Gang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox