From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753050AbdATQgN (ORCPT ); Fri, 20 Jan 2017 11:36:13 -0500 Received: from mga03.intel.com ([134.134.136.65]:30650 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752951AbdATQgG (ORCPT ); Fri, 20 Jan 2017 11:36:06 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,259,1477983600"; d="scan'208";a="811321373" Message-ID: <1484929981.2133.276.camel@linux.intel.com> Subject: Re: [char-misc-next] mei: simplify error handling via devres function. From: Andy Shevchenko To: Tomas Winkler , Greg Kroah-Hartman Cc: Alexander Usyskin , linux-kernel@vger.kernel.org Date: Fri, 20 Jan 2017 18:33:01 +0200 In-Reply-To: <1484932972-3442-1-git-send-email-tomas.winkler@intel.com> References: <1484932972-3442-1-git-send-email-tomas.winkler@intel.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 > Signed-off-by: Tomas Winkler > Signed-off-by: Alexander Usyskin > --- >  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 Intel Finland Oy