Re: [char-misc-next] mei: simplify error handling via devres function.

From: Andy Shevchenko
Date: Fri Jan 20 2017 - 11:36:16 EST

Next message: Djalal Harouni: "Re: [PATCH v4 2/2] procfs/tasks: add a simple per-task procfs hidepid= field"
Previous message: David Miller: "Re: [RFC PATCH] net: stmicro: eQOS IP Core"
In reply to: Tomas Winkler: "[char-misc-next] mei: simplify error handling via devres function."
Next in thread: Andy Shevchenko: "Re: [char-misc-next] mei: simplify error handling via devres function."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@xxxxxxxxx>
> ---
> Â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@xxxxxxxxxxxxxxx>
Intel Finland Oy

Next message: Djalal Harouni: "Re: [PATCH v4 2/2] procfs/tasks: add a simple per-task procfs hidepid= field"
Previous message: David Miller: "Re: [RFC PATCH] net: stmicro: eQOS IP Core"
In reply to: Tomas Winkler: "[char-misc-next] mei: simplify error handling via devres function."
Next in thread: Andy Shevchenko: "Re: [char-misc-next] mei: simplify error handling via devres function."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]