linux-kernel - Re: [PATCH 1/2] mmc: omap_hsmmc: stop using .enable and .disable method.

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 26 Mar 2015 08:38:00 +0100
From:	Ulf Hansson <ulf.hansson@...aro.org>
To:	NeilBrown <neilb@...e.de>
Cc:	NeilBrown <neil@...wn.name>, Tony Lindgren <tony@...mide.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Chris Ball <chris@...ntf.net>,
	linux-omap <linux-omap@...r.kernel.org>,
	linux-mmc <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] mmc: omap_hsmmc: stop using .enable and .disable method.

On 26 March 2015 at 02:18, NeilBrown <neilb@...e.de> wrote:
> On Thu, 26 Mar 2015 08:43:37 +1100 NeilBrown <neil@...wn.name> wrote:
>
>> enable and disable are only used to get and put
>> runtime pm references.  .set_ios already does this
>> itself, and other drivers just do it in set_ios
>> and .request without using enable/disable.
>>
>> So add pm_runtime get/put to omap_hsmmc_request(),
>> and discard the enable/disable methods.
>>
>> Signed-off-by: NeilBrown <neil@...wn.name>
>
> Sorry, that patch wasn't much good.  This one I have really tested properly
> and checked that the 'put' match the 'get's and  that the device to regularly
> go to sleep as expected.
>
> NeilBrown
>
>
> From: NeilBrown <neil@...wn.name>
> Date: Thu, 26 Mar 2015 08:28:43 +1100
> Subject: [PATCH] mmc: omap_hsmmc: stop using .enable and .disable method.
>
> enable and disable are only used to get and put
> runtime pm references.  .set_ios already does this
> itself, and other drivers just do it in set_ios
> and .request without using enable/disable.
>
> So add pm_runtime get/put to omap_hsmmc_request(),
> and discard the enable/disable methods.
>
> Signed-off-by: NeilBrown <neil@...wn.name>
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index f84cfb01716d..2cb81538a612 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -882,6 +882,8 @@ static void omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct mmc_req
>                 return;
>         host->mrq = NULL;
>         mmc_request_done(host->mmc, mrq);
> +       pm_runtime_mark_last_busy(host->dev);
> +       pm_runtime_put_autosuspend(host->dev);
>  }

How about adding a function like this:

_omap_hsmmc_request_done(struct omap_hsmmc_host *host, struct mmc_req)
{
  mmc_request_done(host->mmc, mrq);
  pm_runtime_mark_last_busy(host->dev);
  pm_runtime_put_autosuspend(host->dev);
}

Then just invoke it from those places were needed?

>
>  /*
> @@ -1305,6 +1307,8 @@ static void omap_hsmmc_dma_callback(void *param)
>
>                 host->mrq = NULL;
>                 mmc_request_done(host->mmc, mrq);
> +               pm_runtime_mark_last_busy(host->dev);
> +               pm_runtime_put_autosuspend(host->dev);
>         }
>  }
>
> @@ -1537,6 +1541,7 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>
>         BUG_ON(host->req_in_progress);
>         BUG_ON(host->dma_ch != -1);
> +       pm_runtime_get_sync(host->dev);
>         if (host->protect_card) {
>                 if (host->reqs_blocked < 3) {
>                         /*
> @@ -1553,6 +1558,8 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>                         req->data->error = -EBADF;
>                 req->cmd->retries = 0;
>                 mmc_request_done(mmc, req);
> +               pm_runtime_mark_last_busy(host->dev);
> +               pm_runtime_put_autosuspend(host->dev);
>                 return;
>         } else if (host->reqs_blocked)
>                 host->reqs_blocked = 0;
> @@ -1566,6 +1573,8 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>                         req->data->error = err;
>                 host->mrq = NULL;
>                 mmc_request_done(mmc, req);
> +               pm_runtime_mark_last_busy(host->dev);
> +               pm_runtime_put_autosuspend(host->dev);
>                 return;
>         }
>         if (req->sbc && !(host->flags & AUTO_CMD23)) {
> @@ -1778,25 +1787,6 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
>         set_sd_bus_power(host);
>  }
>
> -static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
> -{
> -       struct omap_hsmmc_host *host = mmc_priv(mmc);
> -
> -       pm_runtime_get_sync(host->dev);
> -
> -       return 0;
> -}
> -
> -static int omap_hsmmc_disable_fclk(struct mmc_host *mmc)
> -{
> -       struct omap_hsmmc_host *host = mmc_priv(mmc);
> -
> -       pm_runtime_mark_last_busy(host->dev);
> -       pm_runtime_put_autosuspend(host->dev);
> -
> -       return 0;
> -}
> -
>  static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
>                                      unsigned int direction, int blk_size)
>  {
> @@ -1808,8 +1798,6 @@ static int omap_hsmmc_multi_io_quirk(struct mmc_card *card,
>  }
>
>  static struct mmc_host_ops omap_hsmmc_ops = {
> -       .enable = omap_hsmmc_enable_fclk,
> -       .disable = omap_hsmmc_disable_fclk,
>         .post_req = omap_hsmmc_post_req,
>         .pre_req = omap_hsmmc_pre_req,
>         .request = omap_hsmmc_request,

Finally, I think you have forgotten to deal with pm_runtime_get|put*()
in omap_hsmmc_enable_sdio_irq().

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/