Lino Sanfilippo wrote:
+ skb = dev_alloc_skb(adpt->rxbuf_size + NET_IP_ALIGN);
+ if (!skb)
+ break;
+
+ /* Make buffer alignment 2 beyond a 16 byte boundary
+ * this will result in a 16 byte aligned IP header after
+ * the 14 byte MAC header is removed
+ */
+ skb_reserve(skb, NET_IP_ALIGN);
__netdev_alloc_skb_ip_align will do this for you.
Will fix.
+ curr_rxbuf->dma_addr = dma_map_single(adpt->netdev->dev.parent,
+ skb_data,
+ curr_rxbuf->length,
+ DMA_FROM_DEVICE);
Mapping can fail. You should check the result via dma_mapping_error().
There are several other places in which dma_map_single() is called and the return value
is not checked.
Will fix.
+ if (ret) {
+ netdev_err(adpt->netdev,
+ "error:%d on request_irq(%d:%s flags:0)\n", ret,
+ irq->irq, EMAC_MAC_IRQ_RES);
freeing the irq is missing
Will fix.
+ /* disable mac irq */
+ writel(DIS_INT, adpt->base + EMAC_INT_STATUS);
+ writel(0, adpt->base + EMAC_INT_MASK);
+ synchronize_irq(adpt->irq.irq);
+ free_irq(adpt->irq.irq, &adpt->irq);
+ clear_bit(EMAC_STATUS_TASK_REINIT_REQ, &adpt->status);
+
+ cancel_work_sync(&adpt->tx_ts_task);
+ spin_lock_irqsave(&adpt->tx_ts_lock, flags);
Maybe I am missing something but AFAICS tx_ts_lock is never called from irq context, so
there is no reason to disable irqs.
It might have been that way in an older version of the code, but it
appears you are correct. I will change it to a normal spinlock. Thanks.
+/* Push the received skb to upper layers */
+static void emac_receive_skb(struct emac_rx_queue *rx_q,
+ struct sk_buff *skb,
+ u16 vlan_tag, bool vlan_flag)
+{
+ if (vlan_flag) {
+ u16 vlan;
+
+ EMAC_TAG_TO_VLAN(vlan_tag, vlan);
+ __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan);
+ }
+
+ napi_gro_receive(&rx_q->napi, skb);
napi_gro_receive requires rx checksum offload. However emac_receive_skb() is also called if
hardware checksumming is disabled.
So the hardware is a little weird here. Apparently, there is a bug in
the parsing of the packet headers that is avoided if we disable hardware
checksumming.
In emac_mac_rx_process(), right before it calls emac_receive_skb(), it
does this:
if (netdev->features & NETIF_F_RXCSUM)
skb->ip_summed = RRD_L4F(&rrd) ?
CHECKSUM_NONE : CHECKSUM_UNNECESSARY;
else
skb_checksum_none_assert(skb);
RRD_L4F(&rrd) is always zero and NETIF_F_RXCSUM is set by default, so
ip_summed is set to CHECKSUM_UNNECESSARY.
So you're saying that if NETIF_F_RXCSUM is not set, then
napi_gro_receive() should not be called?
I see examples of other drivers that *appear* to call napi_gro_receive()
even when hardware checksumming is disabled.
For example, bfin_mac_rx() in adi/bfin_mac.c does this:
/*
* Disable hardware checksum for bug #5600 if writeback cache is
* enabled. Otherwize, corrupted RX packet will be sent up stack
* without error mark.
*/
#ifndef CONFIG_BFIN_EXTMEM_WRITEBACK
#define BFIN_MAC_CSUM_OFFLOAD
#endif
...
#if defined(BFIN_MAC_CSUM_OFFLOAD)
...
#endif
napi_gro_receive(&lp->napi, skb);
Shouldn't the call to napi_gro_receive() be before the #endif?
Function i40e_receive_skb() has similar code to my driver.
In fact, I have not been able to find any clear example of a driver that
intentionally avoids calling napi_gro_receive() if hardware checksumming
is disabled.
+/* Transmit the packet using specified transmit queue */
+int emac_mac_tx_buf_send(struct emac_adapter *adpt, struct emac_tx_queue *tx_q,
+ struct sk_buff *skb)
+{
+ struct emac_tpd tpd;
+ u32 prod_idx;
+
+ if (!emac_tx_has_enough_descs(tx_q, skb)) {
Drivers should avoid this situation right from the start by checking after each transmission if the max number
of possible descriptors is still available for a further transmission and stop the queue if there are not.
Ok, to be clear, you're saying I should do what bcmgenet_xmit() does.
if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
netif_tx_stop_queue(txq);
At the end of emac_mac_tx_buf_send(), I should call
emac_tpd_num_free_descs() and check to see whether the number of free
descriptors is <= (MAX_SKB_FRAGS + 1).
Furthermore there does not seem to be any function that wakes the queue up again once it has been stopped.
If I make the above fix, won't that also fix this bug?
+/* reinitialize */
+void emac_reinit_locked(struct emac_adapter *adpt)
+{
+ while (test_and_set_bit(EMAC_STATUS_RESETTING, &adpt->status))
+ msleep(20); /* Reset might take few 10s of ms */
+
+ emac_mac_down(adpt, true);
+
+ emac_sgmii_reset(adpt);
+ emac_mac_up(adpt);
emac_mac_up() may fail, so this case should be handled properly.
Ok.
+/* Change the Maximum Transfer Unit (MTU) */
+static int emac_change_mtu(struct net_device *netdev, int new_mtu)
+{
+ unsigned int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+ struct emac_adapter *adpt = netdev_priv(netdev);
+ unsigned int old_mtu = netdev->mtu;
+
+ if ((max_frame < EMAC_MIN_ETH_FRAME_SIZE) ||
+ (max_frame > EMAC_MAX_ETH_FRAME_SIZE)) {
+ netdev_err(adpt->netdev, "error: invalid MTU setting\n");
+ return -EINVAL;
+ }
+
+ if ((old_mtu != new_mtu) && netif_running(netdev)) {
Setting the new mtu in case that the interface is down is missing.
Should I just move the "netdev->mtu = new_mtu" line outside of the
if-statement?
Also the first check is not needed, since this function is only called if
there is a change of the mtu.
Will fix.
+/* Provide network statistics info for the interface */
+static struct rtnl_link_stats64 *emac_get_stats64(struct net_device *netdev,
+ struct rtnl_link_stats64 *net_stats)
+{
+ struct emac_adapter *adpt = netdev_priv(netdev);
+ unsigned int addr = REG_MAC_RX_STATUS_BIN;
+ struct emac_stats *stats = &adpt->stats;
+ u64 *stats_itr = &adpt->stats.rx_ok;
+ u32 val;
+
+ mutex_lock(&stats->lock);
It is not allowed to sleep in this function, so you have to use something else for locking,
e.g. a spinlock.
Will fix.
+ strlcpy(netdev->name, "eth%d", sizeof(netdev->name));
This is already done by alloc_etherdev.
Will fix.
Thank you very much for reviewing my code. I hope my questions haven't
been too stupid.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
