Re: [PATCH v4] can: Convert to runtime_pm

From: Marc Kleine-Budde
Date: Mon Jan 12 2015 - 08:25:55 EST

On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@xxxxxxxxxxxxxx]
>> Sent: Sunday, January 11, 2015 9:11 PM
>> To: Appana Durga Kedareswara Rao
>> Cc: linux-can@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; Soren Brinkmann; grant.likely@xxxxxxxxxx;
>> wg@xxxxxxxxxxxxxx
>> 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: signature.asc
Description: OpenPGP digital signature