| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <54B299A0.1000504@pengutronix.de>
Date: Sun, 11 Jan 2015 16:41:20 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Appana Durga Kedareswara Rao <appana.durga.rao@...inx.com>
CC: "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Soren Brinkmann <sorenb@...inx.com>,
"grant.likely@...aro.org" <grant.likely@...aro.org>,
"wg@...ndegger.com" <wg@...ndegger.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.
>>> 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 |
Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)
Powered by blists - more mailing lists