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

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

[Andy Shevchenko]

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

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

[Tomas Winkler]

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

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_handler,
+						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_handler,
+						mei_txe_irq_thread_handler,
+						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_handler,
+						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_handler,
+						mei_txe_irq_thread_handler,
+						IRQF_SHARED, KBUILD_MODNAME,
+						dev);
 	if (err) {
 		dev_err(&pdev->dev, "request_threaded_irq failed: irq = %d.\n",
 				pdev->irq);
-- 
2.7.4

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

[Andy Shevchenko]

On Fri, Jan 20, 2017 at 7:22 PM, Tomas Winkler <tomas.winkler@intel.com> 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

Perhaps you may leave SoB.
But let me review the code.

> 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>

> - * Return: The mei_device_device pointer on success, NULL on failure.
> + * Return: The mei_device pointer on success, NULL on failure.

This is not related.

> -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)

It's not exactly in devm_ namespace. (They have no corresponding
devm_*_fini, etc.)
Better to leave same name in spite of calling devm_* inside.

> -struct mei_device *mei_txe_dev_init(struct pci_dev *pdev)
> +struct mei_device *devm_mei_txe_init(struct pci_dev *pdev)

Ditto.

>  end:

> +       pci_set_drvdata(pdev, NULL);

Not needed.

>  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);

> +       pci_set_drvdata(pdev, NULL);

Ditto.

> -       free_irq(pdev->irq, dev);
> +       devm_free_irq(&pdev->dev, pdev->irq, dev);
>         pci_disable_msi(pdev);

All three not needed

>         return 0;

> @@ -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);

First line?

> +       memcpy(hw->mem_addr, pcim_iomap_table(pdev), sizeof(hw->mem_addr));

Why?
It is kept by PCI core, you don't need a copy.

>  end:

> +       pci_set_drvdata(pdev, NULL);

Redundant.

>  static void mei_txe_remove(struct pci_dev *pdev)
>  {

> +       pci_set_drvdata(pdev, NULL);

Ditto.

> @@ -256,7 +210,7 @@ static int mei_txe_pci_suspend(struct device *device)

> -       free_irq(pdev->irq, dev);
> +       devm_free_irq(&pdev->dev, pdev->irq, dev);
>         pci_disable_msi(pdev);

All are redundant.

-- 
With Best Regards,
Andy Shevchenko

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

[Winkler, Tomas]

> 
> On Fri, Jan 20, 2017 at 7:22 PM, Tomas Winkler <tomas.winkler@intel.com>
> 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
> 
> Perhaps you may leave SoB.
> But let me review the code.

No problem, though we also had some internal code before, you were just first to send it out ;)

> 
> > 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>
> 
> > - * Return: The mei_device_device pointer on success, NULL on failure.
> > + * Return: The mei_device pointer on success, NULL on failure.
> 
> This is not related.
> 
> > -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)
> 
> It's not exactly in devm_ namespace. (They have no corresponding
> devm_*_fini, etc.) Better to leave same name in spite of calling devm_* inside.

Okay, got it.
> 
> > -struct mei_device *mei_txe_dev_init(struct pci_dev *pdev)
> > +struct mei_device *devm_mei_txe_init(struct pci_dev *pdev)
> 
> Ditto.
> 
> >  end:
> 
> > +       pci_set_drvdata(pdev, NULL);
> 
> Not needed.
Please explain, we rely on pci_get_drvdata() returning NULL when unregistered. 
> 
> >  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);
> 
> > +       pci_set_drvdata(pdev, NULL);
> 
> Ditto.
> 
> > -       free_irq(pdev->irq, dev);
> > +       devm_free_irq(&pdev->dev, pdev->irq, dev);
> >         pci_disable_msi(pdev);
> 
> All three not needed
I believe we need it on suspend as we are going over  irq request again in resume.  Please provide more info you if you still insist. 
> 
> >         return 0;
> 
> > @@ -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);
> 
> First line?
Please be more verbose.
> 
> > +       memcpy(hw->mem_addr, pcim_iomap_table(pdev),
> > + sizeof(hw->mem_addr));
> 
> Why?
> It is kept by PCI core, you don't need a copy.

There is no simple accessor for that, it's easier to copy the two dwords then going over the function calls. 

> >  end:
> 
> > +       pci_set_drvdata(pdev, NULL);
> 
> Redundant.
> 
> >  static void mei_txe_remove(struct pci_dev *pdev)  {
> 
> > +       pci_set_drvdata(pdev, NULL);
> 
> Ditto.
> 
> > @@ -256,7 +210,7 @@ static int mei_txe_pci_suspend(struct device
> > *device)
> 
> > -       free_irq(pdev->irq, dev);
> > +       devm_free_irq(&pdev->dev, pdev->irq, dev);
> >         pci_disable_msi(pdev);
> 
> All are redundant.

Thanks
Tomas 

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

[Andy Shevchenko]

On Sat, 2017-01-21 at 10:12 +0000, Winkler, Tomas wrote:

> > 
> > > -struct mei_device *mei_txe_dev_init(struct pci_dev *pdev)
> > > +struct mei_device *devm_mei_txe_init(struct pci_dev *pdev)
> > 
> > Ditto.
> > 
> > >  end:
> > > +       pci_set_drvdata(pdev, NULL);
> > 
> > Not needed.
> 
> Please explain, we rely on pci_get_drvdata() returning NULL when
> unregistered. 

PCI core will take care about this one. Actually device core for any of
user of struct device.
See __device_release_driver() for the details.

> > 
> > > -       free_irq(pdev->irq, dev);
> > > +       devm_free_irq(&pdev->dev, pdev->irq, dev);
> > >         pci_disable_msi(pdev);
> > 
> > All three not needed
> 
> I believe we need it on suspend as we are going over  irq request
> again in resume.  Please provide more info you if you still insist. 

Ah, sorry, I missed that these are suspend/resume hooks.

So, Can you elaborate a bit why you need to disable interrupts during
system suspend?

(Basically in this case better not to use devm_request_*irq() at all)

> > 
> > >         return 0;
> > > @@ -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);
> > 
> > First line?
> 
> Please be more verbose.

Use reversed tree for definition block.

The longest lines with the assignment = first;
Then lines without assignment;
Then return code variable;

Flags for spin_lock -- depends.

> > 
> > > +       memcpy(hw->mem_addr, pcim_iomap_table(pdev),
> > > + sizeof(hw->mem_addr));
> > 
> > Why?
> > It is kept by PCI core, you don't need a copy.
> 
> There is no simple accessor for that, it's easier to copy the two
> dwords then going over the function calls. 

I'm not sure you need a copy. That function call just return the pointer
to the table.

I remember 8250_pci used to have similar approach, now it's using
whatever is kept by PCI core.

It's less error prone.


> > > @@ -256,7 +210,7 @@ static int mei_txe_pci_suspend(struct device
> > > *device)
> > > -       free_irq(pdev->irq, dev);
> > > +       devm_free_irq(&pdev->dev, pdev->irq, dev);
> > >         pci_disable_msi(pdev);
> > 
> > All are redundant.

Yeah, same clarification as for above case with system sleep.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Winkler, Tomas]

> 
> On Sat, 2017-01-21 at 10:12 +0000, Winkler, Tomas wrote:
> 
> > >
> > > > -struct mei_device *mei_txe_dev_init(struct pci_dev *pdev)
> > > > +struct mei_device *devm_mei_txe_init(struct pci_dev *pdev)
> > >
> > > Ditto.
> > >
> > > >  end:
> > > > +       pci_set_drvdata(pdev, NULL);
> > >
> > > Not needed.
> >
> > Please explain, we rely on pci_get_drvdata() returning NULL when
> > unregistered.
> 
> PCI core will take care about this one. Actually device core for any of user of
> struct device.
> See __device_release_driver() for the details.

Okay, got it. 
> 
> > >
> > > > -       free_irq(pdev->irq, dev);
> > > > +       devm_free_irq(&pdev->dev, pdev->irq, dev);
> > > >         pci_disable_msi(pdev);
> > >
> > > All three not needed
> >
> > I believe we need it on suspend as we are going over  irq request
> > again in resume.  Please provide more info you if you still insist.
> 
> Ah, sorry, I missed that these are suspend/resume hooks.
> 
> So, Can you elaborate a bit why you need to disable interrupts during system
> suspend?
> 
> (Basically in this case better not to use devm_request_*irq() at all)

MEI is used for manageability so the device might be alive also in S3 on some platforms, 
anyhow this might be reviewed  more as we do disable interrupts explicitly on suspend.
So far the current code has passed suspend/resume stress tests.

> > >
> > > >         return 0;
> > > > @@ -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);
> > >
> > > First line?
> >
> > Please be more verbose.
> 
> Use reversed tree for definition block.
> 
> The longest lines with the assignment = first; Then lines without assignment;
> Then return code variable;
> 
> Flags for spin_lock -- depends.

I haven't seen this rule in  coding style doc, this is the first I'm seeing such request.
W/o offence I prefer the current style. 

> 
> > >
> > > > +       memcpy(hw->mem_addr, pcim_iomap_table(pdev),
> > > > +sizeof(hw->mem_addr));
> > >
> > > Why?
> > > It is kept by PCI core, you don't need a copy.
> >
> > There is no simple accessor for that, it's easier to copy the two
> > dwords then going over the function calls.
> 
> I'm not sure you need a copy. That function call just return the pointer to the
> table.
> 
> I remember 8250_pci used to have similar approach, now it's using whatever is
> kept by PCI core.
> 
> It's less error prone.

Yep, we  definitely can use a pointer here . 

> 
> > > > @@ -256,7 +210,7 @@ static int mei_txe_pci_suspend(struct device
> > > > *device)
> > > > -       free_irq(pdev->irq, dev);
> > > > +       devm_free_irq(&pdev->dev, pdev->irq, dev);
> > > >         pci_disable_msi(pdev);
> > >
> > > All are redundant.
> 
> Yeah, same clarification as for above case with system sleep.
> 

Thanks for the review, will post v2, tomorrow as this will requires some more stress testing
Thanks
Tomas 

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

[Andy Shevchenko]

On Tue, Jan 24, 2017 at 12:33 AM, Winkler, Tomas
<tomas.winkler@intel.com> wrote:
>> On Sat, 2017-01-21 at 10:12 +0000, Winkler, Tomas wrote:

>> > > > -       free_irq(pdev->irq, dev);
>> > > > +       devm_free_irq(&pdev->dev, pdev->irq, dev);
>> > > >         pci_disable_msi(pdev);
>> > >
>> > > All three not needed
>> >
>> > I believe we need it on suspend as we are going over  irq request
>> > again in resume.  Please provide more info you if you still insist.
>>
>> Ah, sorry, I missed that these are suspend/resume hooks.
>>
>> So, Can you elaborate a bit why you need to disable interrupts during system
>> suspend?
>>
>> (Basically in this case better not to use devm_request_*irq() at all)
>
> MEI is used for manageability so the device might be alive also in S3 on some platforms,
> anyhow this might be reviewed  more as we do disable interrupts explicitly on suspend.
> So far the current code has passed suspend/resume stress tests.

OK, so, I would recommend to use old variant with plain
request_threaded_irq() / free_irq().
Those (IRQ related) functions somehow mistakenly got devm_*()
variation. It's not first time where devm_*irq() is inconvenient in
some ways.

>> Use reversed tree for definition block.
>>
>> The longest lines with the assignment = first; Then lines without assignment;
>> Then return code variable;
>>
>> Flags for spin_lock -- depends.
>
> I haven't seen this rule in  coding style doc, this is the first I'm seeing such request.
> W/o offence I prefer the current style.

Yes, this is matter of style -- your choice.

>> > > > @@ -256,7 +210,7 @@ static int mei_txe_pci_suspend(struct device
>> > > > *device)
>> > > > -       free_irq(pdev->irq, dev);
>> > > > +       devm_free_irq(&pdev->dev, pdev->irq, dev);
>> > > >         pci_disable_msi(pdev);
>> > >
>> > > All are redundant.

> Thanks for the review, will post v2, tomorrow as this will requires some more stress testing

Take your time.

-- 
With Best Regards,
Andy Shevchenko