Re: [PATCH v4] can: Convert to runtime_pm

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 6749 bytes --]

On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Sunday, January 11, 2015 9:11 PM
>> To: Appana Durga Kedareswara Rao
>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
>> wg@grandegger.com
>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>
>> 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.
>>
> 
>         if (netif_running(ndev)) {
>                         if (isr & XCAN_IXR_BSOFF_MASK) {
>                                 priv->can.state = CAN_STATE_BUS_OFF;
>                            priv->write_reg(priv, XCAN_SRR_OFFSET,
>                                         XCAN_SRR_RESET_MASK);
>                 } else if ((status & XCAN_SR_ESTAT_MASK) ==
>                                         XCAN_SR_ESTAT_MASK) {
>                         priv->can.state = CAN_STATE_ERROR_PASSIVE;
>                 } else if (status & XCAN_SR_ERRWRN_MASK) {
>                         priv->can.state = CAN_STATE_ERROR_WARNING;
>                 } else {
>                         priv->can.state = CAN_STATE_ERROR_ACTIVE;
>                 }
>         }
> 
> Is the above code snippet ok for you?

Yes, but what's the state of the hardware when it wakes up again?

> 
>>>>>             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.
> 
> If I move the  pm_runtime_disable after the set_reset_mode
> I am getting the below error.
> ERROR:
> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter: pm_runtime_get fail
> 
> If I move the pm_runtime_disable after unregister_candev everything is working fine.

Fine - but who calls xcan_get_berr_counter here? Can you add a
dump_stack() here?

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]