Re: [PATCH v4] can: Convert to runtime_pm

[Marc Kleine-Budde]

On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
[...]
>>>             return ret;
>>>     }
>>>
>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>
>>>     if (netif_running(ndev)) {
>>>             priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>
>> What happens if the device was not in ACTIVE state prior to the
>> runtime_suspend?
>>
> 
> I am not sure about the state of CAN at this point of time.
> I just followed what other drivers are following for run time  suspend :).

Please check the state of the hardware if you go with bus off into
suspend and then resume.

>>>             netif_device_attach(ndev);
>>>             netif_start_queue(ndev);
>>>     }
>>>
>>>     return 0;
>>> }
>>
>>
>>>
>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct
>>> device *dev)
>>>
>>>     priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>     priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>> -   priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>
>>>     if (netif_running(ndev)) {
>>> +           priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>             netif_device_attach(ndev);
>>>             netif_start_queue(ndev);
>>>     }
>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused xcan_resume(struct
>> device *dev)
>>>     return 0;
>>>  }
>>>
>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
>> xcan_resume);
>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
>>> +   SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
>>> +   SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
>> xcan_runtime_resume,
>>> +NULL) };
>>>
>>>  /**
>>>   * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
>>> static int xcan_probe(struct platform_device *pdev)
>>>             return -ENOMEM;
>>>
>>>     priv = netdev_priv(ndev);
>>> -   priv->dev = ndev;
>>> +   priv->dev = &pdev->dev;
>>>     priv->can.bittiming_const = &xcan_bittiming_const;
>>>     priv->can.do_set_mode = xcan_do_set_mode;
>>>     priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
>> 1137,15
>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
>>>
>>>     netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>>>
>>> +   pm_runtime_set_active(&pdev->dev);
>>> +   pm_runtime_irq_safe(&pdev->dev);
>>> +   pm_runtime_enable(&pdev->dev);
>>> +   pm_runtime_get_sync(&pdev->dev);
>> Check error values?
> 
> Ok
>>> +
>>>     ret = register_candev(ndev);
>>>     if (ret) {
>>>             dev_err(&pdev->dev, "fail to register failed (err=%d)\n",
>> ret);
>>> +           pm_runtime_put(priv->dev);
>>
>> Please move the pm_runtime_put into the common error exit path.
>>
> 
> Ok
> 
>>>             goto err_unprepare_disable_busclk;
>>>     }
>>>
>>>     devm_can_led_init(ndev);
>>> -   clk_disable_unprepare(priv->bus_clk);
>>> -   clk_disable_unprepare(priv->can_clk);
>>> +
>>> +   pm_runtime_put(&pdev->dev);
>>> +
>>>     netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
>> depth:%d\n",
>>>                     priv->reg_base, ndev->irq, priv->can.clock.freq,
>>>                     priv->tx_max);
>>>
>>
>> I think you have to convert the _remove() function, too. Have a look at the
>> gpio-zynq.c driver:
>>
>>> static int zynq_gpio_remove(struct platform_device *pdev) {
>>>     struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>>>
>>>     pm_runtime_get_sync(&pdev->dev);
>>
>> However I don't understand why the get_sync() is here. Maybe SÃren can
>> help?
> 
> I converted the remove function to use the run-time PM and .
> Below is the remove code snippet.
> 
>        ret = pm_runtime_get_sync(&pdev->dev);
>         if (ret < 0) {
>                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>                                 __func__, ret);
>                 return ret;
>         }
> 
>         if (set_reset_mode(ndev) < 0)
>                 netdev_err(ndev, "mode resetting failed!\n");
> 
>         unregister_candev(ndev);
>         netif_napi_del(&priv->napi);
>         free_candev(ndev);

>         pm_runtime_disable(&pdev->dev);

Can this make a call to xcan_runtime_*()? I'm asking since the ndev has
been unregistered and already free()ed. Better move this directly after
the set_reset_mode(). This way you are symmetric to the probe() function.

> Observed the below things in the /sys/kernel/debug/clk/clk_summary.
> 
>                                 Modprobe        ifconfig up    ifconfig down   rmmod  Modprobe  ifconfig up   ifconfig down  rmmod  Modprobe  ifconfig up   ifconfig down  rmmod
> Clk_prepare count:                1             1               1               1       2       2               2               2       3       3               3               3
> Clk_enable count:                 0             1               0               1       2       2               2               2       3       3               3               3

As the numbers are increasing you have a clk_prepare_enable() leak. Your
remove() function is missing the clk_disable_unprepare() for the can and
bus clock (as you have clk_prepare_enable in probe()).

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Attachment: signature.asc
Description: OpenPGP digital signature