| 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: <CAHCN7xJq1O8nLG-4Feofh4aAVfMfTXGcHjY8XvFt4aGnwjAVtg@mail.gmail.com>
Date: Mon, 16 Dec 2019 05:17:54 -0600
From: Adam Ford <aford173@...il.com>
To: Tony Lindgren <tony@...mide.com>
Cc: Evgeniy Polyakov <zbr@...emap.net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Linux-OMAP <linux-omap@...r.kernel.org>,
"Andrew F . Davis" <afd@...com>,
Andreas Kemnade <andreas@...nade.info>,
"H . Nikolaus Schaller" <hns@...delico.com>,
Vignesh R <vigneshr@...com>
Subject: Re: [PATCH] w1: omap-hdq: Simplify driver with PM runtime autosuspend
On Sun, Dec 15, 2019 at 11:38 AM Tony Lindgren <tony@...mide.com> wrote:
>
> We've had generic code handling module sysconfig and OCP reset registers
> for omap variants for many years now and all the drivers really needs to
> do is just call runtime PM functions.
>
> Looks like the omap-hdq driver got only partially updated over the years
> to use runtime PM, and still has lots of custom PM code left.
>
> We can replace all the custom code for sysconfig, OCP reset, and PM with
> just a few lines of runtime PM autosuspend code.
>
> Note that the earlier driver specific usage count limit of four seems
> completely artificial and should not be an issue in normal use.
>
> Cc: Adam Ford <aford173@...il.com>
> Cc: Andrew F. Davis <afd@...com>
> Cc: Andreas Kemnade <andreas@...nade.info>
> Cc: H. Nikolaus Schaller <hns@...delico.com>
> Cc: Vignesh R <vigneshr@...com>
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> ---
>
>
> Can you guys please review and and test on gta04 and torpedo?
I will try to test this series early this week.
adam
>
>
> drivers/w1/masters/omap_hdq.c | 286 +++++++++++-----------------------
> 1 file changed, 90 insertions(+), 196 deletions(-)
>
> diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
> --- a/drivers/w1/masters/omap_hdq.c
> +++ b/drivers/w1/masters/omap_hdq.c
> @@ -38,12 +38,6 @@
> #define OMAP_HDQ_INT_STATUS_TXCOMPLETE BIT(2)
> #define OMAP_HDQ_INT_STATUS_RXCOMPLETE BIT(1)
> #define OMAP_HDQ_INT_STATUS_TIMEOUT BIT(0)
> -#define OMAP_HDQ_SYSCONFIG 0x14
> -#define OMAP_HDQ_SYSCONFIG_SOFTRESET BIT(1)
> -#define OMAP_HDQ_SYSCONFIG_AUTOIDLE BIT(0)
> -#define OMAP_HDQ_SYSCONFIG_NOIDLE 0x0
> -#define OMAP_HDQ_SYSSTATUS 0x18
> -#define OMAP_HDQ_SYSSTATUS_RESETDONE BIT(0)
>
> #define OMAP_HDQ_FLAG_CLEAR 0
> #define OMAP_HDQ_FLAG_SET 1
> @@ -62,17 +56,9 @@ struct hdq_data {
> void __iomem *hdq_base;
> /* lock status update */
> struct mutex hdq_mutex;
> - int hdq_usecount;
> u8 hdq_irqstatus;
> /* device lock */
> spinlock_t hdq_spinlock;
> - /*
> - * Used to control the call to omap_hdq_get and omap_hdq_put.
> - * HDQ Protocol: Write the CMD|REG_address first, followed by
> - * the data wrire or read.
> - */
> - int init_trans;
> - int rrw;
> /* mode: 0-HDQ 1-W1 */
> int mode;
>
> @@ -237,41 +223,6 @@ static void omap_w1_search_bus(void *_hdq, struct w1_master *master_dev,
> slave_found(master_dev, id);
> }
>
> -static int _omap_hdq_reset(struct hdq_data *hdq_data)
> -{
> - int ret;
> - u8 tmp_status;
> -
> - hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> - OMAP_HDQ_SYSCONFIG_SOFTRESET);
> - /*
> - * Select HDQ/1W mode & enable clocks.
> - * It is observed that INT flags can't be cleared via a read and GO/INIT
> - * won't return to zero if interrupt is disabled. So we always enable
> - * interrupt.
> - */
> - hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> - OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> - OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
> -
> - /* wait for reset to complete */
> - ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_SYSSTATUS,
> - OMAP_HDQ_SYSSTATUS_RESETDONE, OMAP_HDQ_FLAG_SET, &tmp_status);
> - if (ret)
> - dev_dbg(hdq_data->dev, "timeout waiting HDQ reset, %x",
> - tmp_status);
> - else {
> - hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> - OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> - OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
> - hdq_data->mode);
> - hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> - OMAP_HDQ_SYSCONFIG_AUTOIDLE);
> - }
> -
> - return ret;
> -}
> -
> /* Issue break pulse to the device */
> static int omap_hdq_break(struct hdq_data *hdq_data)
> {
> @@ -357,7 +308,7 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
> goto rtn;
> }
>
> - if (!hdq_data->hdq_usecount) {
> + if (pm_runtime_suspended(hdq_data->dev)) {
> ret = -EINVAL;
> goto out;
> }
> @@ -394,80 +345,6 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
>
> }
>
> -/* Enable clocks and set the controller to HDQ/1W mode */
> -static int omap_hdq_get(struct hdq_data *hdq_data)
> -{
> - int ret = 0;
> -
> - ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> - if (ret < 0) {
> - ret = -EINTR;
> - goto rtn;
> - }
> -
> - if (OMAP_HDQ_MAX_USER == hdq_data->hdq_usecount) {
> - dev_dbg(hdq_data->dev, "attempt to exceed the max use count");
> - ret = -EINVAL;
> - goto out;
> - } else {
> - hdq_data->hdq_usecount++;
> - try_module_get(THIS_MODULE);
> - if (1 == hdq_data->hdq_usecount) {
> -
> - pm_runtime_get_sync(hdq_data->dev);
> -
> - /* make sure HDQ/1W is out of reset */
> - if (!(hdq_reg_in(hdq_data, OMAP_HDQ_SYSSTATUS) &
> - OMAP_HDQ_SYSSTATUS_RESETDONE)) {
> - ret = _omap_hdq_reset(hdq_data);
> - if (ret)
> - /* back up the count */
> - hdq_data->hdq_usecount--;
> - } else {
> - /* select HDQ/1W mode & enable clocks */
> - hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> - OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> - OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
> - hdq_data->mode);
> - hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> - OMAP_HDQ_SYSCONFIG_NOIDLE);
> - hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> - }
> - }
> - }
> -
> -out:
> - mutex_unlock(&hdq_data->hdq_mutex);
> -rtn:
> - return ret;
> -}
> -
> -/* Disable clocks to the module */
> -static int omap_hdq_put(struct hdq_data *hdq_data)
> -{
> - int ret = 0;
> -
> - ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> - if (ret < 0)
> - return -EINTR;
> -
> - hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> - OMAP_HDQ_SYSCONFIG_AUTOIDLE);
> - if (0 == hdq_data->hdq_usecount) {
> - dev_dbg(hdq_data->dev, "attempt to decrement use count"
> - " when it is zero");
> - ret = -EINVAL;
> - } else {
> - hdq_data->hdq_usecount--;
> - module_put(THIS_MODULE);
> - if (0 == hdq_data->hdq_usecount)
> - pm_runtime_put_sync(hdq_data->dev);
> - }
> - mutex_unlock(&hdq_data->hdq_mutex);
> -
> - return ret;
> -}
> -
> /*
> * W1 triplet callback function - used for searching ROM addresses.
> * Registered only when controller is in 1-wire mode.
> @@ -482,7 +359,12 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
> OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK;
> u8 mask = ctrl | OMAP_HDQ_CTRL_STATUS_DIR;
>
> - omap_hdq_get(_hdq);
> + err = pm_runtime_get_sync(hdq_data->dev);
> + if (err < 0) {
> + pm_runtime_put_noidle(hdq_data->dev);
> +
> + return err;
> + }
>
> err = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> if (err < 0) {
> @@ -549,16 +431,30 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
> out:
> mutex_unlock(&hdq_data->hdq_mutex);
> rtn:
> - omap_hdq_put(_hdq);
> + pm_runtime_mark_last_busy(hdq_data->dev);
> + pm_runtime_put_autosuspend(hdq_data->dev);
> +
> return ret;
> }
>
> /* reset callback */
> static u8 omap_w1_reset_bus(void *_hdq)
> {
> - omap_hdq_get(_hdq);
> - omap_hdq_break(_hdq);
> - omap_hdq_put(_hdq);
> + struct hdq_data *hdq_data = _hdq;
> + int err;
> +
> + err = pm_runtime_get_sync(hdq_data->dev);
> + if (err < 0) {
> + pm_runtime_put_noidle(hdq_data->dev);
> +
> + return err;
> + }
> +
> + omap_hdq_break(hdq_data);
> +
> + pm_runtime_mark_last_busy(hdq_data->dev);
> + pm_runtime_put_autosuspend(hdq_data->dev);
> +
> return 0;
> }
>
> @@ -569,37 +465,25 @@ static u8 omap_w1_read_byte(void *_hdq)
> u8 val = 0;
> int ret;
>
> - /* First write to initialize the transfer */
> - if (hdq_data->init_trans == 0)
> - omap_hdq_get(hdq_data);
> + ret = pm_runtime_get_sync(hdq_data->dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(hdq_data->dev);
> +
> + return ret;
> + }
>
> ret = hdq_read_byte(hdq_data, &val);
> if (ret) {
> - ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> - if (ret < 0) {
> - dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> - return -EINTR;
> - }
> - hdq_data->init_trans = 0;
> - mutex_unlock(&hdq_data->hdq_mutex);
> - omap_hdq_put(hdq_data);
> - return -1;
> + ret = -1;
> + goto out_err;
> }
>
> hdq_disable_interrupt(hdq_data, OMAP_HDQ_CTRL_STATUS,
> ~OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
>
> - /* Write followed by a read, release the module */
> - if (hdq_data->init_trans) {
> - ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> - if (ret < 0) {
> - dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> - return -EINTR;
> - }
> - hdq_data->init_trans = 0;
> - mutex_unlock(&hdq_data->hdq_mutex);
> - omap_hdq_put(hdq_data);
> - }
> +out_err:
> + pm_runtime_mark_last_busy(hdq_data->dev);
> + pm_runtime_put_autosuspend(hdq_data->dev);
>
> return val;
> }
> @@ -611,9 +495,12 @@ static void omap_w1_write_byte(void *_hdq, u8 byte)
> int ret;
> u8 status;
>
> - /* First write to initialize the transfer */
> - if (hdq_data->init_trans == 0)
> - omap_hdq_get(hdq_data);
> + ret = pm_runtime_get_sync(hdq_data->dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(hdq_data->dev);
> +
> + return;
> + }
>
> /*
> * We need to reset the slave before
> @@ -623,31 +510,15 @@ static void omap_w1_write_byte(void *_hdq, u8 byte)
> if (byte == W1_SKIP_ROM)
> omap_hdq_break(hdq_data);
>
> - ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> - if (ret < 0) {
> - dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> - return;
> - }
> - hdq_data->init_trans++;
> - mutex_unlock(&hdq_data->hdq_mutex);
> -
> ret = hdq_write_byte(hdq_data, byte, &status);
> if (ret < 0) {
> dev_dbg(hdq_data->dev, "TX failure:Ctrl status %x\n", status);
> - return;
> + goto out_err;
> }
>
> - /* Second write, data transferred. Release the module */
> - if (hdq_data->init_trans > 1) {
> - omap_hdq_put(hdq_data);
> - ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> - if (ret < 0) {
> - dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> - return;
> - }
> - hdq_data->init_trans = 0;
> - mutex_unlock(&hdq_data->hdq_mutex);
> - }
> +out_err:
> + pm_runtime_mark_last_busy(hdq_data->dev);
> + pm_runtime_put_autosuspend(hdq_data->dev);
> }
>
> static struct w1_bus_master omap_w1_master = {
> @@ -656,6 +527,35 @@ static struct w1_bus_master omap_w1_master = {
> .reset_bus = omap_w1_reset_bus,
> };
>
> +static int __maybe_unused omap_hdq_runtime_suspend(struct device *dev)
> +{
> + struct hdq_data *hdq_data = dev_get_drvdata(dev);
> +
> + hdq_reg_out(hdq_data, 0, hdq_data->mode);
> + hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused omap_hdq_runtime_resume(struct device *dev)
> +{
> + struct hdq_data *hdq_data = dev_get_drvdata(dev);
> +
> + /* select HDQ/1W mode & enable clocks */
> + hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> + OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> + OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
> + hdq_data->mode);
> + hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops omap_hdq_pm_ops = {
> + SET_RUNTIME_PM_OPS(omap_hdq_runtime_suspend,
> + omap_hdq_runtime_resume, NULL)
> +};
> +
> static int omap_hdq_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -677,23 +577,18 @@ static int omap_hdq_probe(struct platform_device *pdev)
> if (IS_ERR(hdq_data->hdq_base))
> return PTR_ERR(hdq_data->hdq_base);
>
> - hdq_data->hdq_usecount = 0;
> - hdq_data->rrw = 0;
> mutex_init(&hdq_data->hdq_mutex);
>
> pm_runtime_enable(&pdev->dev);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 300);
> ret = pm_runtime_get_sync(&pdev->dev);
> if (ret < 0) {
> + pm_runtime_put_noidle(&pdev->dev);
> dev_dbg(&pdev->dev, "pm_runtime_get_sync failed\n");
> goto err_w1;
> }
>
> - ret = _omap_hdq_reset(hdq_data);
> - if (ret) {
> - dev_dbg(&pdev->dev, "reset failed\n");
> - goto err_irq;
> - }
> -
> rev = hdq_reg_in(hdq_data, OMAP_HDQ_REVISION);
> dev_info(&pdev->dev, "OMAP HDQ Hardware Rev %c.%c. Driver in %s mode\n",
> (rev >> 4) + '0', (rev & 0x0f) + '0', "Interrupt");
> @@ -715,7 +610,8 @@ static int omap_hdq_probe(struct platform_device *pdev)
>
> omap_hdq_break(hdq_data);
>
> - pm_runtime_put_sync(&pdev->dev);
> + pm_runtime_mark_last_busy(&pdev->dev);
> + pm_runtime_put_autosuspend(&pdev->dev);
>
> ret = of_property_read_string(pdev->dev.of_node, "ti,mode", &mode);
> if (ret < 0 || !strcmp(mode, "hdq")) {
> @@ -739,6 +635,7 @@ static int omap_hdq_probe(struct platform_device *pdev)
> err_irq:
> pm_runtime_put_sync(&pdev->dev);
> err_w1:
> + pm_runtime_dont_use_autosuspend(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
>
> return ret;
> @@ -746,23 +643,19 @@ static int omap_hdq_probe(struct platform_device *pdev)
>
> static int omap_hdq_remove(struct platform_device *pdev)
> {
> - struct hdq_data *hdq_data = platform_get_drvdata(pdev);
> -
> - mutex_lock(&hdq_data->hdq_mutex);
> + int active;
>
> - if (hdq_data->hdq_usecount) {
> - dev_dbg(&pdev->dev, "removed when use count is not zero\n");
> - mutex_unlock(&hdq_data->hdq_mutex);
> - return -EBUSY;
> - }
> + active = pm_runtime_get_sync(&pdev->dev);
> + if (active < 0)
> + pm_runtime_put_noidle(&pdev->dev);
>
> - mutex_unlock(&hdq_data->hdq_mutex);
> + w1_remove_master_device(&omap_w1_master);
>
> - /* remove module dependency */
> + pm_runtime_dont_use_autosuspend(&pdev->dev);
> + if (active >= 0)
> + pm_runtime_put_sync(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
>
> - w1_remove_master_device(&omap_w1_master);
> -
> return 0;
> }
>
> @@ -779,6 +672,7 @@ static struct platform_driver omap_hdq_driver = {
> .driver = {
> .name = "omap_hdq",
> .of_match_table = omap_hdq_dt_ids,
> + .pm = &omap_hdq_pm_ops,
> },
> };
> module_platform_driver(omap_hdq_driver);
> --
> 2.24.1
Powered by blists - more mailing lists