| 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
| ||
|
Date: Mon, 12 Jan 2015 06:59:54 +0000
From: Appana Durga Kedareswara Rao <appana.durga.rao@...inx.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>
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
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?
> >>> 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.
Regards,
Kedar.
>
> > 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 |
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
Powered by blists - more mailing lists