Hi Sergei-san,
(add your private email address to CC.)
> From: Yoshihiro Shimoda, Sent: Tuesday, May 26, 2020 6:47 PM
>
> According to the report of [1], this driver is possible to cause
> the following error in ravb_tx_timeout_work().
>
> ravb e6800000.ethernet ethernet: failed to switch device to config mode
>
> This error means that the hardware could not change the state
> from "Operation" to "Configuration" while some tx queue is operating.
> After that, ravb_config() in ravb_dmac_init() will fail, and then
> any descriptors will be not allocaled anymore so that NULL porinter
> dereference happens after that on ravb_start_xmit().
>
> Such a case is possible to be caused because this driver supports
> two queues (NC and BE) and the ravb_stop_dma() is possible to return
> without any stopping process if TCCR or CSR register indicates
> the hardware is operating for TX.
>
> To fix the issue, just try to wake the subqueue on
> ravb_tx_timeout_work() if the descriptors are not full instead
> of stop all transfers (all queues of TX and RX).
>
> [1]
> https://lore.kernel.org/linux-renesas-soc/20200518045452.2390-1-dirk.be...@de.bosch.com/
>
> Reported-by: Dirk Behme <dirk.be...@de.bosch.com>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
> ---
> I'm guessing that this issue is possible to happen if:
> - ravb_start_xmit() calls netif_stop_subqueue(), and
> - ravb_poll() will not be called with some reason, and
> - netif_wake_subqueue() will be not called, and then
> - dev_watchdog() in net/sched/sch_generic.c calls ndo_tx_timeout().
>
> However, unfortunately, I didn't reproduce the issue yet.
> To be honest, I'm also guessing other queues (SR) of this hardware
> which out-of tree driver manages are possible to reproduce this issue,
> but I didn't try such environment for now...
>
> So, I marked RFC on this patch now.
I'm afraid, but do you have any comments about this patch?
Best regards,
Yoshihiro Shimoda
> drivers/net/ethernet/renesas/ravb.h | 1 -
> drivers/net/ethernet/renesas/ravb_main.c | 48
> ++++++++++----------------------
> 2 files changed, 14 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb.h
> b/drivers/net/ethernet/renesas/ravb.h
> index 9f88b5d..42cf41a 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1021,7 +1021,6 @@ struct ravb_private {
> u32 cur_tx[NUM_TX_QUEUE];
> u32 dirty_tx[NUM_TX_QUEUE];
> struct napi_struct napi[NUM_RX_QUEUE];
> - struct work_struct work;
> /* MII transceiver section. */
> struct mii_bus *mii_bus; /* MDIO bus control */
> int link;
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> b/drivers/net/ethernet/renesas/ravb_main.c
> index 067ad25..45e1ecd 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1428,44 +1428,25 @@ static int ravb_open(struct net_device *ndev)
> static void ravb_tx_timeout(struct net_device *ndev, unsigned int txqueue)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> + unsigned long flags;
> + bool wake = false;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + if (priv->cur_tx[txqueue] - priv->dirty_tx[txqueue] <=
> + (priv->num_tx_ring[txqueue] - 1) * priv->num_tx_desc)
> + wake = true;
>
> netif_err(priv, tx_err, ndev,
> - "transmit timed out, status %08x, resetting...\n",
> - ravb_read(ndev, ISS));
> + "transmit timed out (%d %d), status %08x %08x %08x\n",
> + txqueue, wake, ravb_read(ndev, ISS), ravb_read(ndev, TCCR),
> + ravb_read(ndev, CSR));
> +
> + if (wake)
> + netif_wake_subqueue(ndev, txqueue);
>
> /* tx_errors count up */
> ndev->stats.tx_errors++;
> -
> - schedule_work(&priv->work);
> -}
> -
> -static void ravb_tx_timeout_work(struct work_struct *work)
> -{
> - struct ravb_private *priv = container_of(work, struct ravb_private,
> - work);
> - struct net_device *ndev = priv->ndev;
> -
> - netif_tx_stop_all_queues(ndev);
> -
> - /* Stop PTP Clock driver */
> - if (priv->chip_id == RCAR_GEN2)
> - ravb_ptp_stop(ndev);
> -
> - /* Wait for DMA stopping */
> - ravb_stop_dma(ndev);
> -
> - ravb_ring_free(ndev, RAVB_BE);
> - ravb_ring_free(ndev, RAVB_NC);
> -
> - /* Device init */
> - ravb_dmac_init(ndev);
> - ravb_emac_init(ndev);
> -
> - /* Initialise PTP Clock driver */
> - if (priv->chip_id == RCAR_GEN2)
> - ravb_ptp_init(ndev, priv->pdev);
> -
> - netif_tx_start_all_queues(ndev);
> + spin_unlock_irqrestore(&priv->lock, flags);
> }
>
> /* Packet transmit function for Ethernet AVB */
> @@ -2046,7 +2027,6 @@ static int ravb_probe(struct platform_device *pdev)
> }
>
> spin_lock_init(&priv->lock);
> - INIT_WORK(&priv->work, ravb_tx_timeout_work);
>
> error = of_get_phy_mode(np, &priv->phy_interface);
> if (error && error != -ENODEV)
> --
> 2.7.4
