| 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: <54B3D1C0.200@pengutronix.de>
Date: Mon, 12 Jan 2015 14:53:04 +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>,
Michal Simek <michals@...inx.com>
Subject: Re: [PATCH v4] can: Convert to runtime_pm
On 01/12/2015 02:49 PM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@...gutronix.de]
>> Sent: Monday, January 12, 2015 6:56 PM
>> To: Appana Durga Kedareswara Rao
>> Cc: linux-can@...r.kernel.org; netdev@...r.kernel.org; linux-
>> kernel@...r.kernel.org; Soren Brinkmann; grant.likely@...aro.org;
>> wg@...ndegger.com; Michal Simek
>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>
>> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
>>> Hi Marc,
>>>
>>>> -----Original Message-----
>>>> From: Marc Kleine-Budde [mailto:mkl@...gutronix.de]
>>>> Sent: Sunday, January 11, 2015 9:11 PM
>>>> To: Appana Durga Kedareswara Rao
>>>> Cc: linux-can@...r.kernel.org; netdev@...r.kernel.org; linux-
>>>> kernel@...r.kernel.org; Soren Brinkmann; grant.likely@...aro.org;
>>>> 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.
>>>>
>>>
>>> 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?
>
> It depends on the previous state of the CAN.
> I mean In Suspend we are putting the device in sleep mode and in resume we are waking up by putting the device into the
> Configuration mode. We are not doing any reset of the core in the suspend/resume so it depends on the previous state of the CAN
> when it wakes up that's why checking for the status of the CAN in the status register here to put the device in appropriate mode.
I understand the software side, but I don't know how your hardware
behaves. This is why I'm asking.
>
>>
>>>
>>>>>>> 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?
>>
>
> I think it is getting called from the atomic context.
> When I am trying to do a rmmod I am getting the above error.
> ERROR:
> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> pm_runtime_get fail.
>
> I am getting only the above error in the console when I do rmmod.
Put a dump_stack into xcan_get_berr_counter(), then you'll see where
it's called from. However calling from atomic context should be fine.
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