From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Tomas Winkler <tomas.winkler@intel.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alexander Usyskin <alexander.usyskin@intel.com>,
linux-kernel@vger.kernel.org
Subject: Re: [char-misc-next] mei: simplify error handling via devres function.
Date: Fri, 20 Jan 2017 18:33:01 +0200 [thread overview]
Message-ID: <1484929981.2133.276.camel@linux.intel.com> (raw)
In-Reply-To: <1484932972-3442-1-git-send-email-tomas.winkler@intel.com>
On Fri, 2017-01-20 at 19:22 +0200, Tomas Winkler wrote:
> Use devm_ and pcim_ functions to make error handling
> simpler and code smaller and tidier.
>
> Based on original patch by
> mei: me: use managed functions pcim_* and devm_*
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> https://lkml.org/lkml/2016/2/1/339
>
I have some comments on it, but need to go right now.
So, I will look to this during weekend.
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> ---
> drivers/misc/mei/hw-me.c | 13 +++---
> drivers/misc/mei/hw-me.h | 4 +-
> drivers/misc/mei/hw-txe.c | 8 ++--
> drivers/misc/mei/hw-txe.h | 2 +-
> drivers/misc/mei/pci-me.c | 74 +++++++++---------------------
> drivers/misc/mei/pci-txe.c | 112 ++++++++++++++--------------------
> -----------
> 6 files changed, 69 insertions(+), 144 deletions(-)
>
> diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
> index a05375a3338a..b376acfca640 100644
> --- a/drivers/misc/mei/hw-me.c
> +++ b/drivers/misc/mei/hw-me.c
> @@ -1384,21 +1384,21 @@ const struct mei_cfg mei_me_pch8_sps_cfg = {
> };
>
> /**
> - * mei_me_dev_init - allocates and initializes the mei device
> structure
> + * devm_mei_me_init - allocates and initializes the mei device
> structure
> *
> * @pdev: The pci device structure
> * @cfg: per device generation config
> *
> - * Return: The mei_device_device pointer on success, NULL on failure.
> + * Return: The mei_device pointer on success, NULL on failure.
> */
> -struct mei_device *mei_me_dev_init(struct pci_dev *pdev,
> - const struct mei_cfg *cfg)
> +struct mei_device *devm_mei_me_init(struct pci_dev *pdev,
> + const struct mei_cfg *cfg)
> {
> struct mei_device *dev;
> struct mei_me_hw *hw;
>
> - dev = kzalloc(sizeof(struct mei_device) +
> - sizeof(struct mei_me_hw), GFP_KERNEL);
> + dev = devm_kzalloc(&pdev->dev, sizeof(struct mei_device) +
> + sizeof(struct mei_me_hw), GFP_KERNEL);
> if (!dev)
> return NULL;
> hw = to_me_hw(dev);
> @@ -1407,4 +1407,3 @@ struct mei_device *mei_me_dev_init(struct
> pci_dev *pdev,
> hw->cfg = cfg;
> return dev;
> }
> -
> diff --git a/drivers/misc/mei/hw-me.h b/drivers/misc/mei/hw-me.h
> index cf64847a35b9..2c80ec46b593 100644
> --- a/drivers/misc/mei/hw-me.h
> +++ b/drivers/misc/mei/hw-me.h
> @@ -70,8 +70,8 @@ extern const struct mei_cfg mei_me_pch_cpt_pbg_cfg;
> extern const struct mei_cfg mei_me_pch8_cfg;
> extern const struct mei_cfg mei_me_pch8_sps_cfg;
>
> -struct mei_device *mei_me_dev_init(struct pci_dev *pdev,
> - const struct mei_cfg *cfg);
> +struct mei_device *devm_mei_me_init(struct pci_dev *pdev,
> + const struct mei_cfg *cfg);
>
> int mei_me_pg_enter_sync(struct mei_device *dev);
> int mei_me_pg_exit_sync(struct mei_device *dev);
> diff --git a/drivers/misc/mei/hw-txe.c b/drivers/misc/mei/hw-txe.c
> index e9f8c0aeec13..fb096c32817a 100644
> --- a/drivers/misc/mei/hw-txe.c
> +++ b/drivers/misc/mei/hw-txe.c
> @@ -1196,19 +1196,19 @@ static const struct mei_hw_ops mei_txe_hw_ops
> = {
> };
>
> /**
> - * mei_txe_dev_init - allocates and initializes txe hardware specific
> structure
> + * devm_mei_txe_init - allocates and initializes txe hardware
> specific structure
> *
> * @pdev: pci device
> *
> * Return: struct mei_device * on success or NULL
> */
> -struct mei_device *mei_txe_dev_init(struct pci_dev *pdev)
> +struct mei_device *devm_mei_txe_init(struct pci_dev *pdev)
> {
> struct mei_device *dev;
> struct mei_txe_hw *hw;
>
> - dev = kzalloc(sizeof(struct mei_device) +
> - sizeof(struct mei_txe_hw), GFP_KERNEL);
> + dev = devm_kzalloc(&pdev->dev, sizeof(struct mei_device) +
> + sizeof(struct mei_txe_hw), GFP_KERNEL);
> if (!dev)
> return NULL;
>
> diff --git a/drivers/misc/mei/hw-txe.h b/drivers/misc/mei/hw-txe.h
> index ce3ed0b88b0c..7083ac255497 100644
> --- a/drivers/misc/mei/hw-txe.h
> +++ b/drivers/misc/mei/hw-txe.h
> @@ -62,7 +62,7 @@ static inline struct mei_device
> *hw_txe_to_mei(struct mei_txe_hw *hw)
> return container_of((void *)hw, struct mei_device, hw);
> }
>
> -struct mei_device *mei_txe_dev_init(struct pci_dev *pdev);
> +struct mei_device *devm_mei_txe_init(struct pci_dev *pdev);
>
> irqreturn_t mei_txe_irq_quick_handler(int irq, void *dev_id);
> irqreturn_t mei_txe_irq_thread_handler(int irq, void *dev_id);
> diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
> index f9c6ec4b98ab..3918624ca40e 100644
> --- a/drivers/misc/mei/pci-me.c
> +++ b/drivers/misc/mei/pci-me.c
> @@ -149,18 +149,18 @@ static int mei_me_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> return -ENODEV;
>
> /* enable pci dev */
> - err = pci_enable_device(pdev);
> + err = pcim_enable_device(pdev);
> if (err) {
> dev_err(&pdev->dev, "failed to enable pci
> device.\n");
> goto end;
> }
> /* set PCI host mastering */
> pci_set_master(pdev);
> - /* pci request regions for mei driver */
> - err = pci_request_regions(pdev, KBUILD_MODNAME);
> + /* pci request regions and mapping IO device memory for mei
> driver */
> + err = pcim_iomap_regions(pdev, BIT(0), KBUILD_MODNAME);
> if (err) {
> dev_err(&pdev->dev, "failed to get pci regions.\n");
> - goto disable_device;
> + goto end;
> }
>
> if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)) ||
> @@ -173,37 +173,31 @@ static int mei_me_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> }
> if (err) {
> dev_err(&pdev->dev, "No usable DMA configuration,
> aborting\n");
> - goto release_regions;
> + goto end;
> }
>
> -
> /* allocates and initializes the mei dev structure */
> - dev = mei_me_dev_init(pdev, cfg);
> + dev = devm_mei_me_init(pdev, cfg);
> if (!dev) {
> err = -ENOMEM;
> - goto release_regions;
> + goto end;
> }
> hw = to_me_hw(dev);
> - /* mapping IO device memory */
> - hw->mem_addr = pci_iomap(pdev, 0, 0);
> - if (!hw->mem_addr) {
> - dev_err(&pdev->dev, "mapping I/O device memory
> failure.\n");
> - err = -ENOMEM;
> - goto free_device;
> - }
> + hw->mem_addr = pcim_iomap_table(pdev)[0];
> +
> pci_enable_msi(pdev);
>
> /* request and enable interrupt */
> irqflags = pci_dev_msi_enabled(pdev) ? IRQF_ONESHOT :
> IRQF_SHARED;
>
> - err = request_threaded_irq(pdev->irq,
> - mei_me_irq_quick_handler,
> - mei_me_irq_thread_handler,
> - irqflags, KBUILD_MODNAME, dev);
> + err = devm_request_threaded_irq(&pdev->dev, pdev->irq,
> + mei_me_irq_quick_handler,
> + mei_me_irq_thread_handler,
> + irqflags, KBUILD_MODNAME,
> dev);
> if (err) {
> dev_err(&pdev->dev, "request_threaded_irq failure.
> irq = %d\n",
> pdev->irq);
> - goto disable_msi;
> + goto end;
> }
>
> if (mei_start(dev)) {
> @@ -241,17 +235,8 @@ static int mei_me_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> release_irq:
> mei_cancel_work(dev);
> mei_disable_interrupts(dev);
> - free_irq(pdev->irq, dev);
> -disable_msi:
> - pci_disable_msi(pdev);
> - pci_iounmap(pdev, hw->mem_addr);
> -free_device:
> - kfree(dev);
> -release_regions:
> - pci_release_regions(pdev);
> -disable_device:
> - pci_disable_device(pdev);
> end:
> + pci_set_drvdata(pdev, NULL);
> dev_err(&pdev->dev, "initialization failed.\n");
> return err;
> }
> @@ -267,7 +252,6 @@ static int mei_me_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> static void mei_me_remove(struct pci_dev *pdev)
> {
> struct mei_device *dev;
> - struct mei_me_hw *hw;
>
> dev = pci_get_drvdata(pdev);
> if (!dev)
> @@ -276,33 +260,19 @@ static void mei_me_remove(struct pci_dev *pdev)
> if (mei_pg_is_enabled(dev))
> pm_runtime_get_noresume(&pdev->dev);
>
> - hw = to_me_hw(dev);
> -
> -
> dev_dbg(&pdev->dev, "stop\n");
> mei_stop(dev);
>
> if (!pci_dev_run_wake(pdev))
> mei_me_unset_pm_domain(dev);
>
> - /* disable interrupts */
> mei_disable_interrupts(dev);
>
> - free_irq(pdev->irq, dev);
> - pci_disable_msi(pdev);
> -
> - if (hw->mem_addr)
> - pci_iounmap(pdev, hw->mem_addr);
> -
> mei_deregister(dev);
>
> - kfree(dev);
> -
> - pci_release_regions(pdev);
> - pci_disable_device(pdev);
> -
> -
> + pci_set_drvdata(pdev, NULL);
> }
> +
> #ifdef CONFIG_PM_SLEEP
> static int mei_me_pci_suspend(struct device *device)
> {
> @@ -318,7 +288,7 @@ static int mei_me_pci_suspend(struct device
> *device)
>
> mei_disable_interrupts(dev);
>
> - free_irq(pdev->irq, dev);
> + devm_free_irq(&pdev->dev, pdev->irq, dev);
> pci_disable_msi(pdev);
>
> return 0;
> @@ -340,10 +310,10 @@ static int mei_me_pci_resume(struct device
> *device)
> irqflags = pci_dev_msi_enabled(pdev) ? IRQF_ONESHOT :
> IRQF_SHARED;
>
> /* request and enable interrupt */
> - err = request_threaded_irq(pdev->irq,
> - mei_me_irq_quick_handler,
> - mei_me_irq_thread_handler,
> - irqflags, KBUILD_MODNAME, dev);
> + err = devm_request_threaded_irq(&pdev->dev, pdev->irq,
> + mei_me_irq_quick_handler,
> + mei_me_irq_thread_handler,
> + irqflags, KBUILD_MODNAME,
> dev);
>
> if (err) {
> dev_err(&pdev->dev, "request_threaded_irq failed: irq
> = %d.\n",
> diff --git a/drivers/misc/mei/pci-txe.c b/drivers/misc/mei/pci-txe.c
> index 58ffd30dcc91..be1709e5c1a6 100644
> --- a/drivers/misc/mei/pci-txe.c
> +++ b/drivers/misc/mei/pci-txe.c
> @@ -52,17 +52,6 @@ static inline void mei_txe_set_pm_domain(struct
> mei_device *dev) {}
> static inline void mei_txe_unset_pm_domain(struct mei_device *dev) {}
> #endif /* CONFIG_PM */
>
> -static void mei_txe_pci_iounmap(struct pci_dev *pdev, struct
> mei_txe_hw *hw)
> -{
> - int i;
> -
> - for (i = SEC_BAR; i < NUM_OF_MEM_BARS; i++) {
> - if (hw->mem_addr[i]) {
> - pci_iounmap(pdev, hw->mem_addr[i]);
> - hw->mem_addr[i] = NULL;
> - }
> - }
> -}
> /**
> * mei_txe_probe - Device Initialization Routine
> *
> @@ -75,22 +64,22 @@ static int mei_txe_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> {
> struct mei_device *dev;
> struct mei_txe_hw *hw;
> + const int mask = BIT(SEC_BAR) | BIT(BRIDGE_BAR);
> int err;
> - int i;
>
> /* enable pci dev */
> - err = pci_enable_device(pdev);
> + err = pcim_enable_device(pdev);
> if (err) {
> dev_err(&pdev->dev, "failed to enable pci
> device.\n");
> goto end;
> }
> /* set PCI host mastering */
> pci_set_master(pdev);
> - /* pci request regions for mei driver */
> - err = pci_request_regions(pdev, KBUILD_MODNAME);
> + /* pci request regions and mapping IO device memory for mei
> driver */
> + err = pcim_iomap_regions(pdev, mask, KBUILD_MODNAME);
> if (err) {
> dev_err(&pdev->dev, "failed to get pci regions.\n");
> - goto disable_device;
> + goto end;
> }
>
> err = pci_set_dma_mask(pdev, DMA_BIT_MASK(36));
> @@ -98,28 +87,18 @@ static int mei_txe_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> if (err) {
> dev_err(&pdev->dev, "No suitable DMA
> available.\n");
> - goto release_regions;
> + goto end;
> }
> }
>
> /* allocates and initializes the mei dev structure */
> - dev = mei_txe_dev_init(pdev);
> + dev = devm_mei_txe_init(pdev);
> if (!dev) {
> err = -ENOMEM;
> - goto release_regions;
> + goto end;
> }
> hw = to_txe_hw(dev);
> -
> - /* mapping IO device memory */
> - for (i = SEC_BAR; i < NUM_OF_MEM_BARS; i++) {
> - hw->mem_addr[i] = pci_iomap(pdev, i, 0);
> - if (!hw->mem_addr[i]) {
> - dev_err(&pdev->dev, "mapping I/O device
> memory failure.\n");
> - err = -ENOMEM;
> - goto free_device;
> - }
> - }
> -
> + memcpy(hw->mem_addr, pcim_iomap_table(pdev), sizeof(hw-
> >mem_addr));
>
> pci_enable_msi(pdev);
>
> @@ -128,19 +107,21 @@ static int mei_txe_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>
> /* request and enable interrupt */
> if (pci_dev_msi_enabled(pdev))
> - err = request_threaded_irq(pdev->irq,
> - NULL,
> - mei_txe_irq_thread_handler,
> - IRQF_ONESHOT, KBUILD_MODNAME, dev);
> + err = devm_request_threaded_irq(&pdev->dev, pdev-
> >irq,
> + NULL,
> + mei_txe_irq_thread_ha
> ndler,
> + IRQF_ONESHOT,
> KBUILD_MODNAME,
> + dev);
> else
> - err = request_threaded_irq(pdev->irq,
> - mei_txe_irq_quick_handler,
> - mei_txe_irq_thread_handler,
> - IRQF_SHARED, KBUILD_MODNAME, dev);
> + err = devm_request_threaded_irq(&pdev->dev, pdev-
> >irq,
> + mei_txe_irq_quick_han
> dler,
> + mei_txe_irq_thread_ha
> ndler,
> + IRQF_SHARED,
> KBUILD_MODNAME,
> + dev);
> if (err) {
> dev_err(&pdev->dev, "mei: request_threaded_irq
> failure. irq = %d\n",
> pdev->irq);
> - goto free_device;
> + goto end;
> }
>
> if (mei_start(dev)) {
> @@ -173,24 +154,10 @@ static int mei_txe_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> stop:
> mei_stop(dev);
> release_irq:
> -
> mei_cancel_work(dev);
> -
> - /* disable interrupts */
> mei_disable_interrupts(dev);
> -
> - free_irq(pdev->irq, dev);
> - pci_disable_msi(pdev);
> -
> -free_device:
> - mei_txe_pci_iounmap(pdev, hw);
> -
> - kfree(dev);
> -release_regions:
> - pci_release_regions(pdev);
> -disable_device:
> - pci_disable_device(pdev);
> end:
> + pci_set_drvdata(pdev, NULL);
> dev_err(&pdev->dev, "initialization failed.\n");
> return err;
> }
> @@ -206,38 +173,25 @@ static int mei_txe_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> static void mei_txe_remove(struct pci_dev *pdev)
> {
> struct mei_device *dev;
> - struct mei_txe_hw *hw;
>
> dev = pci_get_drvdata(pdev);
> if (!dev) {
> - dev_err(&pdev->dev, "mei: dev =NULL\n");
> + dev_err(&pdev->dev, "mei: dev == NULL\n");
> return;
> }
>
> pm_runtime_get_noresume(&pdev->dev);
>
> - hw = to_txe_hw(dev);
> -
> mei_stop(dev);
>
> if (!pci_dev_run_wake(pdev))
> mei_txe_unset_pm_domain(dev);
>
> - /* disable interrupts */
> mei_disable_interrupts(dev);
> - free_irq(pdev->irq, dev);
> - pci_disable_msi(pdev);
> -
> - pci_set_drvdata(pdev, NULL);
> -
> - mei_txe_pci_iounmap(pdev, hw);
>
> mei_deregister(dev);
>
> - kfree(dev);
> -
> - pci_release_regions(pdev);
> - pci_disable_device(pdev);
> + pci_set_drvdata(pdev, NULL);
> }
>
>
> @@ -256,7 +210,7 @@ static int mei_txe_pci_suspend(struct device
> *device)
>
> mei_disable_interrupts(dev);
>
> - free_irq(pdev->irq, dev);
> + devm_free_irq(&pdev->dev, pdev->irq, dev);
> pci_disable_msi(pdev);
>
> return 0;
> @@ -278,15 +232,17 @@ static int mei_txe_pci_resume(struct device
> *device)
>
> /* request and enable interrupt */
> if (pci_dev_msi_enabled(pdev))
> - err = request_threaded_irq(pdev->irq,
> - NULL,
> - mei_txe_irq_thread_handler,
> - IRQF_ONESHOT, KBUILD_MODNAME, dev);
> + err = devm_request_threaded_irq(&pdev->dev, pdev-
> >irq,
> + NULL,
> + mei_txe_irq_thread_ha
> ndler,
> + IRQF_ONESHOT,
> KBUILD_MODNAME,
> + dev);
> else
> - err = request_threaded_irq(pdev->irq,
> - mei_txe_irq_quick_handler,
> - mei_txe_irq_thread_handler,
> - IRQF_SHARED, KBUILD_MODNAME, dev);
> + err = devm_request_threaded_irq(&pdev->dev, pdev-
> >irq,
> + mei_txe_irq_quick_han
> dler,
> + mei_txe_irq_thread_ha
> ndler,
> + IRQF_SHARED,
> KBUILD_MODNAME,
> + dev);
> if (err) {
> dev_err(&pdev->dev, "request_threaded_irq failed: irq
> = %d.\n",
> pdev->irq);
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
next prev parent reply other threads:[~2017-01-20 16:36 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-20 17:22 [char-misc-next] mei: simplify error handling via devres function Tomas Winkler
2017-01-20 16:33 ` Andy Shevchenko [this message]
2017-01-20 22:00 ` Andy Shevchenko
2017-01-21 10:12 ` Winkler, Tomas
2017-01-23 14:48 ` Andy Shevchenko
2017-01-23 22:33 ` Winkler, Tomas
2017-01-23 22:40 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1484929981.2133.276.camel@linux.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=alexander.usyskin@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tomas.winkler@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.