linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe
@ 2022-01-05 11:10 Miaoqian Lin
  2022-01-06 12:42 ` kernel test robot
  2022-03-01  7:18 ` [PATCH v2] " Miaoqian Lin
  0 siblings, 2 replies; 9+ messages in thread
From: Miaoqian Lin @ 2022-01-05 11:10 UTC (permalink / raw)
  Cc: linmq006, Eugen Hristev, Mauro Carvalho Chehab, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Hans Verkuil, linux-media,
	linux-arm-kernel, linux-kernel

The pm_runtime_enable will increase power disable depth.
If the probe fails, we should use pm_runtime_disable() to balance
pm_runtime_enable().

Fixes: 0a0e265 ("media: atmel: atmel-isc: split driver into driver base and isc")
Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
---
 drivers/media/platform/atmel/atmel-sama5d2-isc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
index 1b2063cce0f7..88a81c3ada90 100644
--- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
@@ -559,6 +559,9 @@ static int atmel_isc_probe(struct platform_device *pdev)
 cleanup_subdev:
 	isc_subdev_cleanup(isc);
 
+disable_pm_runtime:
+	pm_runtime_disable(dev);
+
 unregister_v4l2_device:
 	v4l2_device_unregister(&isc->v4l2_dev);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe
  2022-01-05 11:10 [PATCH] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe Miaoqian Lin
@ 2022-01-06 12:42 ` kernel test robot
  2022-03-01  7:18 ` [PATCH v2] " Miaoqian Lin
  1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2022-01-06 12:42 UTC (permalink / raw)
  To: Miaoqian Lin
  Cc: llvm, kbuild-all, linmq006, Eugen Hristev, Mauro Carvalho Chehab,
	linux-media, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches,
	Hans Verkuil, linux-arm-kernel, linux-kernel

Hi Miaoqian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.16-rc8 next-20220105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miaoqian-Lin/media-atmel-atmel-isc-Fix-PM-disable-depth-imbalance-in-atmel_isc_probe/20220105-191228
base:   git://linuxtv.org/media_tree.git master
config: riscv-randconfig-r002-20220106 (https://download.01.org/0day-ci/archive/20220106/202201062034.BD2L4uCe-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project ca7ffe09dc6e525109e3cd570cc5182ce568be13)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/f116d8f81c642486900c9589a7c10d137d7eb0a2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Miaoqian-Lin/media-atmel-atmel-isc-Fix-PM-disable-depth-imbalance-in-atmel_isc_probe/20220105-191228
        git checkout f116d8f81c642486900c9589a7c10d137d7eb0a2
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/media/platform/atmel/ net/xfrm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/media/platform/atmel/atmel-sama5d2-isc.c:30:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/media/platform/atmel/atmel-sama5d2-isc.c:30:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/media/platform/atmel/atmel-sama5d2-isc.c:30:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:1024:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/media/platform/atmel/atmel-sama5d2-isc.c:562:1: warning: unused label 'disable_pm_runtime' [-Wunused-label]
   disable_pm_runtime:
   ^~~~~~~~~~~~~~~~~~~
   8 warnings generated.


vim +/disable_pm_runtime +562 drivers/media/platform/atmel/atmel-sama5d2-isc.c

   380	
   381	static int atmel_isc_probe(struct platform_device *pdev)
   382	{
   383		struct device *dev = &pdev->dev;
   384		struct isc_device *isc;
   385		struct resource *res;
   386		void __iomem *io_base;
   387		struct isc_subdev_entity *subdev_entity;
   388		int irq;
   389		int ret;
   390		u32 ver;
   391	
   392		isc = devm_kzalloc(dev, sizeof(*isc), GFP_KERNEL);
   393		if (!isc)
   394			return -ENOMEM;
   395	
   396		platform_set_drvdata(pdev, isc);
   397		isc->dev = dev;
   398	
   399		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   400		io_base = devm_ioremap_resource(dev, res);
   401		if (IS_ERR(io_base))
   402			return PTR_ERR(io_base);
   403	
   404		isc->regmap = devm_regmap_init_mmio(dev, io_base, &isc_regmap_config);
   405		if (IS_ERR(isc->regmap)) {
   406			ret = PTR_ERR(isc->regmap);
   407			dev_err(dev, "failed to init register map: %d\n", ret);
   408			return ret;
   409		}
   410	
   411		irq = platform_get_irq(pdev, 0);
   412		if (irq < 0)
   413			return irq;
   414	
   415		ret = devm_request_irq(dev, irq, isc_interrupt, 0,
   416				       "atmel-sama5d2-isc", isc);
   417		if (ret < 0) {
   418			dev_err(dev, "can't register ISR for IRQ %u (ret=%i)\n",
   419				irq, ret);
   420			return ret;
   421		}
   422	
   423		isc->gamma_table = isc_sama5d2_gamma_table;
   424		isc->gamma_max = 2;
   425	
   426		isc->max_width = ISC_SAMA5D2_MAX_SUPPORT_WIDTH;
   427		isc->max_height = ISC_SAMA5D2_MAX_SUPPORT_HEIGHT;
   428	
   429		isc->config_dpc = isc_sama5d2_config_dpc;
   430		isc->config_csc = isc_sama5d2_config_csc;
   431		isc->config_cbc = isc_sama5d2_config_cbc;
   432		isc->config_cc = isc_sama5d2_config_cc;
   433		isc->config_gam = isc_sama5d2_config_gam;
   434		isc->config_rlp = isc_sama5d2_config_rlp;
   435		isc->config_ctrls = isc_sama5d2_config_ctrls;
   436	
   437		isc->adapt_pipeline = isc_sama5d2_adapt_pipeline;
   438	
   439		isc->offsets.csc = ISC_SAMA5D2_CSC_OFFSET;
   440		isc->offsets.cbc = ISC_SAMA5D2_CBC_OFFSET;
   441		isc->offsets.sub422 = ISC_SAMA5D2_SUB422_OFFSET;
   442		isc->offsets.sub420 = ISC_SAMA5D2_SUB420_OFFSET;
   443		isc->offsets.rlp = ISC_SAMA5D2_RLP_OFFSET;
   444		isc->offsets.his = ISC_SAMA5D2_HIS_OFFSET;
   445		isc->offsets.dma = ISC_SAMA5D2_DMA_OFFSET;
   446		isc->offsets.version = ISC_SAMA5D2_VERSION_OFFSET;
   447		isc->offsets.his_entry = ISC_SAMA5D2_HIS_ENTRY_OFFSET;
   448	
   449		isc->controller_formats = sama5d2_controller_formats;
   450		isc->controller_formats_size = ARRAY_SIZE(sama5d2_controller_formats);
   451		isc->formats_list = sama5d2_formats_list;
   452		isc->formats_list_size = ARRAY_SIZE(sama5d2_formats_list);
   453	
   454		/* sama5d2-isc - 8 bits per beat */
   455		isc->dcfg = ISC_DCFG_YMBSIZE_BEATS8 | ISC_DCFG_CMBSIZE_BEATS8;
   456	
   457		/* sama5d2-isc : ISPCK is required and mandatory */
   458		isc->ispck_required = true;
   459	
   460		ret = isc_pipeline_init(isc);
   461		if (ret)
   462			return ret;
   463	
   464		isc->hclock = devm_clk_get(dev, "hclock");
   465		if (IS_ERR(isc->hclock)) {
   466			ret = PTR_ERR(isc->hclock);
   467			dev_err(dev, "failed to get hclock: %d\n", ret);
   468			return ret;
   469		}
   470	
   471		ret = clk_prepare_enable(isc->hclock);
   472		if (ret) {
   473			dev_err(dev, "failed to enable hclock: %d\n", ret);
   474			return ret;
   475		}
   476	
   477		ret = isc_clk_init(isc);
   478		if (ret) {
   479			dev_err(dev, "failed to init isc clock: %d\n", ret);
   480			goto unprepare_hclk;
   481		}
   482		ret = v4l2_device_register(dev, &isc->v4l2_dev);
   483		if (ret) {
   484			dev_err(dev, "unable to register v4l2 device.\n");
   485			goto unprepare_clk;
   486		}
   487	
   488		ret = isc_parse_dt(dev, isc);
   489		if (ret) {
   490			dev_err(dev, "fail to parse device tree\n");
   491			goto unregister_v4l2_device;
   492		}
   493	
   494		if (list_empty(&isc->subdev_entities)) {
   495			dev_err(dev, "no subdev found\n");
   496			ret = -ENODEV;
   497			goto unregister_v4l2_device;
   498		}
   499	
   500		list_for_each_entry(subdev_entity, &isc->subdev_entities, list) {
   501			struct v4l2_async_subdev *asd;
   502			struct fwnode_handle *fwnode =
   503				of_fwnode_handle(subdev_entity->epn);
   504	
   505			v4l2_async_nf_init(&subdev_entity->notifier);
   506	
   507			asd = v4l2_async_nf_add_fwnode_remote(&subdev_entity->notifier,
   508							      fwnode,
   509							      struct v4l2_async_subdev);
   510	
   511			of_node_put(subdev_entity->epn);
   512			subdev_entity->epn = NULL;
   513	
   514			if (IS_ERR(asd)) {
   515				ret = PTR_ERR(asd);
   516				goto cleanup_subdev;
   517			}
   518	
   519			subdev_entity->notifier.ops = &isc_async_ops;
   520	
   521			ret = v4l2_async_nf_register(&isc->v4l2_dev,
   522						     &subdev_entity->notifier);
   523			if (ret) {
   524				dev_err(dev, "fail to register async notifier\n");
   525				goto cleanup_subdev;
   526			}
   527	
   528			if (video_is_registered(&isc->video_dev))
   529				break;
   530		}
   531	
   532		pm_runtime_set_active(dev);
   533		pm_runtime_enable(dev);
   534		pm_request_idle(dev);
   535	
   536		isc->ispck = isc->isc_clks[ISC_ISPCK].clk;
   537	
   538		ret = clk_prepare_enable(isc->ispck);
   539		if (ret) {
   540			dev_err(dev, "failed to enable ispck: %d\n", ret);
   541			goto cleanup_subdev;
   542		}
   543	
   544		/* ispck should be greater or equal to hclock */
   545		ret = clk_set_rate(isc->ispck, clk_get_rate(isc->hclock));
   546		if (ret) {
   547			dev_err(dev, "failed to set ispck rate: %d\n", ret);
   548			goto unprepare_clk;
   549		}
   550	
   551		regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
   552		dev_info(dev, "Microchip ISC version %x\n", ver);
   553	
   554		return 0;
   555	
   556	unprepare_clk:
   557		clk_disable_unprepare(isc->ispck);
   558	
   559	cleanup_subdev:
   560		isc_subdev_cleanup(isc);
   561	
 > 562	disable_pm_runtime:
   563		pm_runtime_disable(dev);
   564	
   565	unregister_v4l2_device:
   566		v4l2_device_unregister(&isc->v4l2_dev);
   567	
   568	unprepare_hclk:
   569		clk_disable_unprepare(isc->hclock);
   570	
   571		isc_clk_cleanup(isc);
   572	
   573		return ret;
   574	}
   575	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe
  2022-01-05 11:10 [PATCH] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe Miaoqian Lin
  2022-01-06 12:42 ` kernel test robot
@ 2022-03-01  7:18 ` Miaoqian Lin
  2022-03-01 13:51   ` Eugen.Hristev
  2022-03-07  7:38   ` Hans Verkuil
  1 sibling, 2 replies; 9+ messages in thread
From: Miaoqian Lin @ 2022-03-01  7:18 UTC (permalink / raw)
  To: Eugen Hristev, Mauro Carvalho Chehab, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Hans Verkuil, linux-media,
	linux-arm-kernel, linux-kernel
  Cc: linmq006

The pm_runtime_enable will increase power disable depth.
If the probe fails, we should use pm_runtime_disable() to balance
pm_runtime_enable().

Fixes: 0a0e265 ("media: atmel: atmel-isc: split driver into driver base and isc")
Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
---
changes in v2:
- remove unused label.
---
 drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
index 1b2063cce0f7..7f1ebbb25437 100644
--- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
@@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev)
 cleanup_subdev:
 	isc_subdev_cleanup(isc);
 
+	pm_runtime_disable(dev);
+
 unregister_v4l2_device:
 	v4l2_device_unregister(&isc->v4l2_dev);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe
  2022-03-01  7:18 ` [PATCH v2] " Miaoqian Lin
@ 2022-03-01 13:51   ` Eugen.Hristev
  2022-03-02  5:44     ` Miaoqian Lin
  2022-03-07  7:38   ` Hans Verkuil
  1 sibling, 1 reply; 9+ messages in thread
From: Eugen.Hristev @ 2022-03-01 13:51 UTC (permalink / raw)
  To: linmq006, mchehab, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, hverkuil-cisco, linux-media, linux-arm-kernel,
	linux-kernel

On 3/1/22 9:18 AM, Miaoqian Lin wrote:
> The pm_runtime_enable will increase power disable depth.
> If the probe fails, we should use pm_runtime_disable() to balance
> pm_runtime_enable().
> 
> Fixes: 0a0e265 ("media: atmel: atmel-isc: split driver into driver base and isc")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> ---
> changes in v2:
> - remove unused label.
> ---
>   drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> index 1b2063cce0f7..7f1ebbb25437 100644
> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> @@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev)
>   cleanup_subdev:
>          isc_subdev_cleanup(isc);
> 
> +       pm_runtime_disable(dev);
> +

Hello Miaoqian Lin ,

Could you please perform the same change (or similar) in 
atmel-sama7g5-isc , as the sama7g5 ISC will perform the same and has the 
same bug.

Thank you for the patch,
Eugen

>   unregister_v4l2_device:
>          v4l2_device_unregister(&isc->v4l2_dev);
> 
> --
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe
  2022-03-01 13:51   ` Eugen.Hristev
@ 2022-03-02  5:44     ` Miaoqian Lin
  2022-03-02  6:17       ` Eugen.Hristev
  0 siblings, 1 reply; 9+ messages in thread
From: Miaoqian Lin @ 2022-03-02  5:44 UTC (permalink / raw)
  To: Eugen.Hristev
  Cc: mchehab, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	hverkuil-cisco, linux-media, linux-arm-kernel, linux-kernel

On Tue, Mar 01, 2022 at 01:51:02PM +0000, Eugen.Hristev@microchip.com wrote:
> > ---
> > changes in v2:
> > - remove unused label.
> > ---
> >   drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> > index 1b2063cce0f7..7f1ebbb25437 100644
> > --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> > +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> > @@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev)
> >   cleanup_subdev:
> >          isc_subdev_cleanup(isc);
> > 
> > +       pm_runtime_disable(dev);
> > +
> 
> Hello Miaoqian Lin ,
> 
> Could you please perform the same change (or similar) in 
> atmel-sama7g5-isc , as the sama7g5 ISC will perform the same and has the 
> same bug.
> 
Hi, Eugen Hristev:

I think you are referring to microchip_xisc_probe() function in
atmel-sama7g5-isc, and I have look into it. After it calls
pm_runtime_enable(), it only have a regular path whichs return 0 and
indicates
the probe is successful. It doesn't have error handling path.

regmap_read() function returns a negative errno in error cases, but it
is used to get Microchip XISC version. I am not sure if failure means
the probe fails.
> >   unregister_v4l2_device:
> >          v4l2_device_unregister(&isc->v4l2_dev);
> > 
> > --
> > 2.17.1
> > 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe
  2022-03-02  5:44     ` Miaoqian Lin
@ 2022-03-02  6:17       ` Eugen.Hristev
  0 siblings, 0 replies; 9+ messages in thread
From: Eugen.Hristev @ 2022-03-02  6:17 UTC (permalink / raw)
  To: linmq006
  Cc: mchehab, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	hverkuil-cisco, linux-media, linux-arm-kernel, linux-kernel

On 3/2/22 7:44 AM, Miaoqian Lin wrote:
> On Tue, Mar 01, 2022 at 01:51:02PM +0000, Eugen.Hristev@microchip.com wrote:
>>> ---
>>> changes in v2:
>>> - remove unused label.
>>> ---
>>>    drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>>> index 1b2063cce0f7..7f1ebbb25437 100644
>>> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>>> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>>> @@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev)
>>>    cleanup_subdev:
>>>           isc_subdev_cleanup(isc);
>>>
>>> +       pm_runtime_disable(dev);
>>> +
>>
>> Hello Miaoqian Lin ,
>>
>> Could you please perform the same change (or similar) in
>> atmel-sama7g5-isc , as the sama7g5 ISC will perform the same and has the
>> same bug.
>>
> Hi, Eugen Hristev:
> 
> I think you are referring to microchip_xisc_probe() function in
> atmel-sama7g5-isc, and I have look into it. After it calls
> pm_runtime_enable(), it only have a regular path whichs return 0 and
> indicates
> the probe is successful. It doesn't have error handling path.

Hi,

You are right. It will always have a success path.

Your patch is fine for me :

Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com>

Thanks !

> 
> regmap_read() function returns a negative errno in error cases, but it
> is used to get Microchip XISC version. I am not sure if failure means
> the probe fails.
>>>    unregister_v4l2_device:
>>>           v4l2_device_unregister(&isc->v4l2_dev);
>>>
>>> --
>>> 2.17.1
>>>
>>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe
  2022-03-01  7:18 ` [PATCH v2] " Miaoqian Lin
  2022-03-01 13:51   ` Eugen.Hristev
@ 2022-03-07  7:38   ` Hans Verkuil
  2022-03-07  8:16     ` [PATCH v3] " Miaoqian Lin
  1 sibling, 1 reply; 9+ messages in thread
From: Hans Verkuil @ 2022-03-07  7:38 UTC (permalink / raw)
  To: Miaoqian Lin, Eugen Hristev, Mauro Carvalho Chehab,
	Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-media,
	linux-arm-kernel, linux-kernel



On 3/1/22 08:18, Miaoqian Lin wrote:
> The pm_runtime_enable will increase power disable depth.
> If the probe fails, we should use pm_runtime_disable() to balance
> pm_runtime_enable().
> 
> Fixes: 0a0e265 ("media: atmel: atmel-isc: split driver into driver base and isc")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> ---
> changes in v2:
> - remove unused label.
> ---
>  drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> index 1b2063cce0f7..7f1ebbb25437 100644
> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> @@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev)
>  cleanup_subdev:
>  	isc_subdev_cleanup(isc);
>  
> +	pm_runtime_disable(dev);

Same issue as with the st-delta patch: there is one 'goto cleanup_subdev'
that is called before the pm_runtime_enable, so this patch just introduces
another imbalance. You need an additional goto label here and rework it a
little bit to get this right.

Regards,

	Hans

> +
>  unregister_v4l2_device:
>  	v4l2_device_unregister(&isc->v4l2_dev);
>  

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe
  2022-03-07  7:38   ` Hans Verkuil
@ 2022-03-07  8:16     ` Miaoqian Lin
  2022-05-09 19:13       ` Eugen.Hristev
  0 siblings, 1 reply; 9+ messages in thread
From: Miaoqian Lin @ 2022-03-07  8:16 UTC (permalink / raw)
  To: Eugen Hristev, Mauro Carvalho Chehab, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Hans Verkuil, linux-media,
	linux-arm-kernel, linux-kernel
  Cc: linmq006

The pm_runtime_enable will decrease power disable depth.
If the probe fails, we should use pm_runtime_disable() to balance
pm_runtime_enable().

Fixes: 0a0e265 ("media: atmel: atmel-isc: split driver into driver base and isc")
Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
---
changes in v2:
- remove unused label.
changes in v3:
- add disable_pm label and update related 'goto'
- update commit message
---
 drivers/media/platform/atmel/atmel-sama5d2-isc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
index 1b2063cce0f7..949035cdb846 100644
--- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
+++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
@@ -538,7 +538,7 @@ static int atmel_isc_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(isc->ispck);
 	if (ret) {
 		dev_err(dev, "failed to enable ispck: %d\n", ret);
-		goto cleanup_subdev;
+		goto disable_pm;
 	}
 
 	/* ispck should be greater or equal to hclock */
@@ -556,6 +556,9 @@ static int atmel_isc_probe(struct platform_device *pdev)
 unprepare_clk:
 	clk_disable_unprepare(isc->ispck);
 
+disable_pm:
+	pm_runtime_disable(dev);
+
 cleanup_subdev:
 	isc_subdev_cleanup(isc);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe
  2022-03-07  8:16     ` [PATCH v3] " Miaoqian Lin
@ 2022-05-09 19:13       ` Eugen.Hristev
  0 siblings, 0 replies; 9+ messages in thread
From: Eugen.Hristev @ 2022-05-09 19:13 UTC (permalink / raw)
  To: linmq006, mchehab, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, hverkuil-cisco, linux-media, linux-arm-kernel,
	linux-kernel

On 3/7/22 10:16 AM, Miaoqian Lin wrote:
> The pm_runtime_enable will decrease power disable depth.
> If the probe fails, we should use pm_runtime_disable() to balance
> pm_runtime_enable().
> 
> Fixes: 0a0e265 ("media: atmel: atmel-isc: split driver into driver base and isc")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>

Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com>

> ---
> changes in v2:
> - remove unused label.
> changes in v3:
> - add disable_pm label and update related 'goto'
> - update commit message
> ---
>   drivers/media/platform/atmel/atmel-sama5d2-isc.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> index 1b2063cce0f7..949035cdb846 100644
> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> @@ -538,7 +538,7 @@ static int atmel_isc_probe(struct platform_device *pdev)
>          ret = clk_prepare_enable(isc->ispck);
>          if (ret) {
>                  dev_err(dev, "failed to enable ispck: %d\n", ret);
> -               goto cleanup_subdev;
> +               goto disable_pm;
>          }
> 
>          /* ispck should be greater or equal to hclock */
> @@ -556,6 +556,9 @@ static int atmel_isc_probe(struct platform_device *pdev)
>   unprepare_clk:
>          clk_disable_unprepare(isc->ispck);
> 
> +disable_pm:
> +       pm_runtime_disable(dev);
> +
>   cleanup_subdev:
>          isc_subdev_cleanup(isc);
> 
> --
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-05-09 19:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-05 11:10 [PATCH] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe Miaoqian Lin
2022-01-06 12:42 ` kernel test robot
2022-03-01  7:18 ` [PATCH v2] " Miaoqian Lin
2022-03-01 13:51   ` Eugen.Hristev
2022-03-02  5:44     ` Miaoqian Lin
2022-03-02  6:17       ` Eugen.Hristev
2022-03-07  7:38   ` Hans Verkuil
2022-03-07  8:16     ` [PATCH v3] " Miaoqian Lin
2022-05-09 19:13       ` Eugen.Hristev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).