[PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[Haoxiang Li]

If device_add() fails, call put_device() to drop the device
reference and do the cleanp.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/b767348e-d89c-416e-acea-1ebbff3bea20@stanley.mountain/
Signed-off-by: Su Hui <suhui@nfschina.com>
Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
---
 drivers/bus/fsl-mc/fsl-mc-bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 25845c04e562..90a2107a9905 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -896,7 +896,8 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
 		dev_err(parent_dev,
 			"device_add() failed for device %s: %d\n",
 			dev_name(&mc_dev->dev), error);
-		goto error_cleanup_dev;
+		put_device(&mc_dev->dev)
+		return error;
 	}
 
 	dev_dbg(parent_dev, "added %s\n", dev_name(&mc_dev->dev));
-- 
2.25.1

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[kernel test robot]

Hi Haoxiang,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.19-rc2 next-20251219]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Haoxiang-Li/bus-fsl-mc-fix-an-error-handling-in-fsl_mc_device_add/20251222-155324
base:   linus/master
patch link:    https://lore.kernel.org/r/20251222074958.992911-1-lihaoxiang%40isrc.iscas.ac.cn
patch subject: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()
config: arm-randconfig-004-20251223 (https://download.01.org/0day-ci/archive/20251223/202512230653.PaZGjmCW-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251223/202512230653.PaZGjmCW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202512230653.PaZGjmCW-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/bus/fsl-mc/fsl-mc-bus.c: In function 'fsl_mc_device_add':
>> drivers/bus/fsl-mc/fsl-mc-bus.c:899:27: error: expected ';' before 'return'
      put_device(&mc_dev->dev)
                              ^
                              ;
      return error;
      ~~~~~~                   


vim +899 drivers/bus/fsl-mc/fsl-mc-bus.c

   778	
   779	/*
   780	 * Add a newly discovered fsl-mc device to be visible in Linux
   781	 */
   782	int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
   783			      struct fsl_mc_io *mc_io,
   784			      struct device *parent_dev,
   785			      struct fsl_mc_device **new_mc_dev)
   786	{
   787		int error;
   788		struct fsl_mc_device *mc_dev = NULL;
   789		struct fsl_mc_bus *mc_bus = NULL;
   790		struct fsl_mc_device *parent_mc_dev;
   791	
   792		if (dev_is_fsl_mc(parent_dev))
   793			parent_mc_dev = to_fsl_mc_device(parent_dev);
   794		else
   795			parent_mc_dev = NULL;
   796	
   797		if (strcmp(obj_desc->type, "dprc") == 0) {
   798			/*
   799			 * Allocate an MC bus device object:
   800			 */
   801			mc_bus = kzalloc(sizeof(*mc_bus), GFP_KERNEL);
   802			if (!mc_bus)
   803				return -ENOMEM;
   804	
   805			mutex_init(&mc_bus->scan_mutex);
   806			mc_dev = &mc_bus->mc_dev;
   807		} else {
   808			/*
   809			 * Allocate a regular fsl_mc_device object:
   810			 */
   811			mc_dev = kzalloc(sizeof(*mc_dev), GFP_KERNEL);
   812			if (!mc_dev)
   813				return -ENOMEM;
   814		}
   815	
   816		mc_dev->obj_desc = *obj_desc;
   817		mc_dev->mc_io = mc_io;
   818		device_initialize(&mc_dev->dev);
   819		mc_dev->dev.parent = parent_dev;
   820		mc_dev->dev.bus = &fsl_mc_bus_type;
   821		mc_dev->dev.release = fsl_mc_device_release;
   822		mc_dev->dev.type = fsl_mc_get_device_type(obj_desc->type);
   823		if (!mc_dev->dev.type) {
   824			error = -ENODEV;
   825			dev_err(parent_dev, "unknown device type %s\n", obj_desc->type);
   826			goto error_cleanup_dev;
   827		}
   828		dev_set_name(&mc_dev->dev, "%s.%d", obj_desc->type, obj_desc->id);
   829	
   830		if (strcmp(obj_desc->type, "dprc") == 0) {
   831			struct fsl_mc_io *mc_io2;
   832	
   833			mc_dev->flags |= FSL_MC_IS_DPRC;
   834	
   835			/*
   836			 * To get the DPRC's ICID, we need to open the DPRC
   837			 * in get_dprc_icid(). For child DPRCs, we do so using the
   838			 * parent DPRC's MC portal instead of the child DPRC's MC
   839			 * portal, in case the child DPRC is already opened with
   840			 * its own portal (e.g., the DPRC used by AIOP).
   841			 *
   842			 * NOTE: There cannot be more than one active open for a
   843			 * given MC object, using the same MC portal.
   844			 */
   845			if (parent_mc_dev) {
   846				/*
   847				 * device being added is a child DPRC device
   848				 */
   849				mc_io2 = parent_mc_dev->mc_io;
   850			} else {
   851				/*
   852				 * device being added is the root DPRC device
   853				 */
   854				if (!mc_io) {
   855					error = -EINVAL;
   856					goto error_cleanup_dev;
   857				}
   858	
   859				mc_io2 = mc_io;
   860			}
   861	
   862			error = get_dprc_icid(mc_io2, obj_desc->id, &mc_dev->icid);
   863			if (error < 0)
   864				goto error_cleanup_dev;
   865		} else {
   866			/*
   867			 * A non-DPRC object has to be a child of a DPRC, use the
   868			 * parent's ICID and interrupt domain.
   869			 */
   870			mc_dev->icid = parent_mc_dev->icid;
   871			mc_dev->dma_mask = FSL_MC_DEFAULT_DMA_MASK;
   872			mc_dev->dev.dma_mask = &mc_dev->dma_mask;
   873			mc_dev->dev.coherent_dma_mask = mc_dev->dma_mask;
   874			dev_set_msi_domain(&mc_dev->dev,
   875					   dev_get_msi_domain(&parent_mc_dev->dev));
   876		}
   877	
   878		/*
   879		 * Get MMIO regions for the device from the MC:
   880		 *
   881		 * NOTE: the root DPRC is a special case as its MMIO region is
   882		 * obtained from the device tree
   883		 */
   884		if (parent_mc_dev && obj_desc->region_count != 0) {
   885			error = fsl_mc_device_get_mmio_regions(mc_dev,
   886							       parent_mc_dev);
   887			if (error < 0)
   888				goto error_cleanup_dev;
   889		}
   890	
   891		/*
   892		 * The device-specific probe callback will get invoked by device_add()
   893		 */
   894		error = device_add(&mc_dev->dev);
   895		if (error < 0) {
   896			dev_err(parent_dev,
   897				"device_add() failed for device %s: %d\n",
   898				dev_name(&mc_dev->dev), error);
 > 899			put_device(&mc_dev->dev)
   900			return error;
   901		}
   902	
   903		dev_dbg(parent_dev, "added %s\n", dev_name(&mc_dev->dev));
   904	
   905		*new_mc_dev = mc_dev;
   906		return 0;
   907	
   908	error_cleanup_dev:
   909		kfree(mc_dev->regions);
   910		if (mc_bus)
   911			kfree(mc_bus);
   912		else
   913			kfree(mc_dev);
   914	
   915		return error;
   916	}
   917	EXPORT_SYMBOL_GPL(fsl_mc_device_add);
   918	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[kernel test robot]

Hi Haoxiang,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.19-rc2 next-20251219]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Haoxiang-Li/bus-fsl-mc-fix-an-error-handling-in-fsl_mc_device_add/20251222-155324
base:   linus/master
patch link:    https://lore.kernel.org/r/20251222074958.992911-1-lihaoxiang%40isrc.iscas.ac.cn
patch subject: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()
config: x86_64-buildonly-randconfig-001-20251223 (https://download.01.org/0day-ci/archive/20251223/202512232341.mNsrluKO-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251223/202512232341.mNsrluKO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202512232341.mNsrluKO-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/bus/fsl-mc/fsl-mc-bus.c:899:27: error: expected ';' after expression
     899 |                 put_device(&mc_dev->dev)
         |                                         ^
         |                                         ;
   1 error generated.


vim +899 drivers/bus/fsl-mc/fsl-mc-bus.c

   778	
   779	/*
   780	 * Add a newly discovered fsl-mc device to be visible in Linux
   781	 */
   782	int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
   783			      struct fsl_mc_io *mc_io,
   784			      struct device *parent_dev,
   785			      struct fsl_mc_device **new_mc_dev)
   786	{
   787		int error;
   788		struct fsl_mc_device *mc_dev = NULL;
   789		struct fsl_mc_bus *mc_bus = NULL;
   790		struct fsl_mc_device *parent_mc_dev;
   791	
   792		if (dev_is_fsl_mc(parent_dev))
   793			parent_mc_dev = to_fsl_mc_device(parent_dev);
   794		else
   795			parent_mc_dev = NULL;
   796	
   797		if (strcmp(obj_desc->type, "dprc") == 0) {
   798			/*
   799			 * Allocate an MC bus device object:
   800			 */
   801			mc_bus = kzalloc(sizeof(*mc_bus), GFP_KERNEL);
   802			if (!mc_bus)
   803				return -ENOMEM;
   804	
   805			mutex_init(&mc_bus->scan_mutex);
   806			mc_dev = &mc_bus->mc_dev;
   807		} else {
   808			/*
   809			 * Allocate a regular fsl_mc_device object:
   810			 */
   811			mc_dev = kzalloc(sizeof(*mc_dev), GFP_KERNEL);
   812			if (!mc_dev)
   813				return -ENOMEM;
   814		}
   815	
   816		mc_dev->obj_desc = *obj_desc;
   817		mc_dev->mc_io = mc_io;
   818		device_initialize(&mc_dev->dev);
   819		mc_dev->dev.parent = parent_dev;
   820		mc_dev->dev.bus = &fsl_mc_bus_type;
   821		mc_dev->dev.release = fsl_mc_device_release;
   822		mc_dev->dev.type = fsl_mc_get_device_type(obj_desc->type);
   823		if (!mc_dev->dev.type) {
   824			error = -ENODEV;
   825			dev_err(parent_dev, "unknown device type %s\n", obj_desc->type);
   826			goto error_cleanup_dev;
   827		}
   828		dev_set_name(&mc_dev->dev, "%s.%d", obj_desc->type, obj_desc->id);
   829	
   830		if (strcmp(obj_desc->type, "dprc") == 0) {
   831			struct fsl_mc_io *mc_io2;
   832	
   833			mc_dev->flags |= FSL_MC_IS_DPRC;
   834	
   835			/*
   836			 * To get the DPRC's ICID, we need to open the DPRC
   837			 * in get_dprc_icid(). For child DPRCs, we do so using the
   838			 * parent DPRC's MC portal instead of the child DPRC's MC
   839			 * portal, in case the child DPRC is already opened with
   840			 * its own portal (e.g., the DPRC used by AIOP).
   841			 *
   842			 * NOTE: There cannot be more than one active open for a
   843			 * given MC object, using the same MC portal.
   844			 */
   845			if (parent_mc_dev) {
   846				/*
   847				 * device being added is a child DPRC device
   848				 */
   849				mc_io2 = parent_mc_dev->mc_io;
   850			} else {
   851				/*
   852				 * device being added is the root DPRC device
   853				 */
   854				if (!mc_io) {
   855					error = -EINVAL;
   856					goto error_cleanup_dev;
   857				}
   858	
   859				mc_io2 = mc_io;
   860			}
   861	
   862			error = get_dprc_icid(mc_io2, obj_desc->id, &mc_dev->icid);
   863			if (error < 0)
   864				goto error_cleanup_dev;
   865		} else {
   866			/*
   867			 * A non-DPRC object has to be a child of a DPRC, use the
   868			 * parent's ICID and interrupt domain.
   869			 */
   870			mc_dev->icid = parent_mc_dev->icid;
   871			mc_dev->dma_mask = FSL_MC_DEFAULT_DMA_MASK;
   872			mc_dev->dev.dma_mask = &mc_dev->dma_mask;
   873			mc_dev->dev.coherent_dma_mask = mc_dev->dma_mask;
   874			dev_set_msi_domain(&mc_dev->dev,
   875					   dev_get_msi_domain(&parent_mc_dev->dev));
   876		}
   877	
   878		/*
   879		 * Get MMIO regions for the device from the MC:
   880		 *
   881		 * NOTE: the root DPRC is a special case as its MMIO region is
   882		 * obtained from the device tree
   883		 */
   884		if (parent_mc_dev && obj_desc->region_count != 0) {
   885			error = fsl_mc_device_get_mmio_regions(mc_dev,
   886							       parent_mc_dev);
   887			if (error < 0)
   888				goto error_cleanup_dev;
   889		}
   890	
   891		/*
   892		 * The device-specific probe callback will get invoked by device_add()
   893		 */
   894		error = device_add(&mc_dev->dev);
   895		if (error < 0) {
   896			dev_err(parent_dev,
   897				"device_add() failed for device %s: %d\n",
   898				dev_name(&mc_dev->dev), error);
 > 899			put_device(&mc_dev->dev)
   900			return error;
   901		}
   902	
   903		dev_dbg(parent_dev, "added %s\n", dev_name(&mc_dev->dev));
   904	
   905		*new_mc_dev = mc_dev;
   906		return 0;
   907	
   908	error_cleanup_dev:
   909		kfree(mc_dev->regions);
   910		if (mc_bus)
   911			kfree(mc_bus);
   912		else
   913			kfree(mc_dev);
   914	
   915		return error;
   916	}
   917	EXPORT_SYMBOL_GPL(fsl_mc_device_add);
   918	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[Christophe Leroy (CS GROUP)]

Le 22/12/2025 à 08:49, Haoxiang Li a écrit :
> If device_add() fails, call put_device() to drop the device
> reference and do the cleanp.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/b767348e-d89c-416e-acea-1ebbff3bea20@stanley.mountain/
> Signed-off-by: Su Hui <suhui@nfschina.com>
> Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
> ---
>   drivers/bus/fsl-mc/fsl-mc-bus.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> index 25845c04e562..90a2107a9905 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> @@ -896,7 +896,8 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
>   		dev_err(parent_dev,
>   			"device_add() failed for device %s: %d\n",
>   			dev_name(&mc_dev->dev), error);
> -		goto error_cleanup_dev;
> +		put_device(&mc_dev->dev)

This change has obviously not been tested, not even built.

And by droping the goto, the kfree() are not done anymore, leaking 
mc_bus/mc_dev p kzalloced areas.

> +		return error;
>   	}
>   
>   	dev_dbg(parent_dev, "added %s\n", dev_name(&mc_dev->dev));

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[Dan Carpenter]

On Tue, Dec 23, 2025 at 04:34:44PM +0100, Christophe Leroy (CS GROUP) wrote:
> 
> 
> Le 22/12/2025 à 08:49, Haoxiang Li a écrit :
> > If device_add() fails, call put_device() to drop the device
> > reference and do the cleanp.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/all/b767348e-d89c-416e-acea-1ebbff3bea20@stanley.mountain/
> > Signed-off-by: Su Hui <suhui@nfschina.com>
> > Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
> > ---
> >   drivers/bus/fsl-mc/fsl-mc-bus.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > index 25845c04e562..90a2107a9905 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > @@ -896,7 +896,8 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
> >   		dev_err(parent_dev,
> >   			"device_add() failed for device %s: %d\n",
> >   			dev_name(&mc_dev->dev), error);
> > -		goto error_cleanup_dev;
> > +		put_device(&mc_dev->dev)
> 
> This change has obviously not been tested, not even built.
> 

Yeah, it doesn't build.

> And by droping the goto, the kfree() are not done anymore, leaking
> mc_bus/mc_dev p kzalloced areas.
> 

Calling put_device() triggers fsl_mc_device_release() which does the
free.

regards,
dan carpenter

> > +		return error;
> >   	}
> >   	dev_dbg(parent_dev, "added %s\n", dev_name(&mc_dev->dev));

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[Christophe Leroy (CS GROUP)]

Le 24/12/2025 à 08:54, Dan Carpenter a écrit :
> On Tue, Dec 23, 2025 at 04:34:44PM +0100, Christophe Leroy (CS GROUP) wrote:
>>
>>
>> Le 22/12/2025 à 08:49, Haoxiang Li a écrit :
>>> If device_add() fails, call put_device() to drop the device
>>> reference and do the cleanp.
>>>
>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Closes: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fb767348e-d89c-416e-acea-1ebbff3bea20%40stanley.mountain%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cfd3865935a164c2083cc08de42c1b5a5%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C639021596902232212%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=j5%2BqptzXvrxcnaaH3PIDorAYMexoPmf3PWi5GbpVD9s%3D&reserved=0
>>> Signed-off-by: Su Hui <suhui@nfschina.com>
>>> Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
>>> ---
>>>    drivers/bus/fsl-mc/fsl-mc-bus.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
>>> index 25845c04e562..90a2107a9905 100644
>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
>>> @@ -896,7 +896,8 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
>>>    		dev_err(parent_dev,
>>>    			"device_add() failed for device %s: %d\n",
>>>    			dev_name(&mc_dev->dev), error);
>>> -		goto error_cleanup_dev;
>>> +		put_device(&mc_dev->dev)
>>
>> This change has obviously not been tested, not even built.
>>
> 
> Yeah, it doesn't build.
> 
>> And by droping the goto, the kfree() are not done anymore, leaking
>> mc_bus/mc_dev p kzalloced areas.
>>
> 
> Calling put_device() triggers fsl_mc_device_release() which does the
> free.

Ok, then this needs to be said in the commit message.

By the way I'm a bit puzzled by the device_add() doc versus the 
put_device(), because it looks like device_add() already calls 
put_device() in its error path, see 
https://elixir.bootlin.com/linux/v6.19-rc2/source/drivers/base/core.c#L3716

> 
> regards,
> dan carpenter
> 
>>> +		return error;
>>>    	}
>>>    	dev_dbg(parent_dev, "added %s\n", dev_name(&mc_dev->dev));

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[Haoxiang Li]

On Wed, 24 Dec 2025 10:57:52 +0100, Christophe Leroy wrote:
> Ok, then this needs to be said in the commit message.

I will add it in the patch v2.

> By the way I'm a bit puzzled by the device_add() doc versus the 
> put_device(), because it looks like device_add() already calls 
> put_device() in its error path, see 
> https://elixir.bootlin.com/linux/v6.19-rc2/source/drivers/base/core.c#L3716

I think this is because device_add() increment the reference in the
beginning, see
https://elixir.bootlin.com/linux/v6.19-rc2/source/drivers/base/core.c#L3580
and if device_add() fails, another put_device() should be called to decrement
the reference which is obtained by device_initialize().

Thanks,
Haoxiang Li

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[Christophe Leroy (CS GROUP)]

Le 24/12/2025 à 11:52, Haoxiang Li a écrit :
> On Wed, 24 Dec 2025 10:57:52 +0100, Christophe Leroy wrote:
>> Ok, then this needs to be said in the commit message.
> 
> I will add it in the patch v2.
> 
>> By the way I'm a bit puzzled by the device_add() doc versus the
>> put_device(), because it looks like device_add() already calls
>> put_device() in its error path, see
>> https://elixir.bootlin.com/linux/v6.19-rc2/source/drivers/base/core.c#L3716
> 
> I think this is because device_add() increment the reference in the
> beginning, see
> https://elixir.bootlin.com/linux/v6.19-rc2/source/drivers/base/core.c#L3580
> and if device_add() fails, another put_device() should be called to decrement
> the reference which is obtained by device_initialize().

Ah yes, I see.

But then all exit paths in fsl_mc_device_add() after device_initialize() 
should call put_device() ?

Then in fact the fix should instead be the following, shouldn't it ?

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c 
b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 25845c04e562..6d132144ce25 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -905,11 +905,7 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
  	return 0;

  error_cleanup_dev:
-	kfree(mc_dev->regions);
-	if (mc_bus)
-		kfree(mc_bus);
-	else
-		kfree(mc_dev);
+	put_device(&mc_dev->dev);

  	return error;
  }

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[Haoxiang Li]

On Wed, 24 Dec 2025 12:07:22 +0100, Christophe Leroy wrote:
> Ah yes, I see.
> 
> But then all exit paths in fsl_mc_device_add() after device_initialize() 
> should call put_device() ?

> Then in fact the fix should instead be the following, shouldn't it ?

> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c 
> b/drivers/bus/fsl-mc/fsl-mc-bus.c
> index 25845c04e562..6d132144ce25 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> @@ -905,11 +905,7 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
>   	return 0;
> 
>   error_cleanup_dev:
> -	kfree(mc_dev->regions);
> -	if (mc_bus)
> -		kfree(mc_bus);
> -	else
> -		kfree(mc_dev);
> +	put_device(&mc_dev->dev);
> 
>   	return error;
>   }

Yes, I think so.
However, I submit a same fix months ago and got a reply:
https://lore.kernel.org/all/2025052622-nautical-suitably-486c@gregkh/

I didn't figure out why, and look forward to experts' guidance.

Thanks,
Haoxiang Li

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[Dan Carpenter]

On Wed, Dec 24, 2025 at 10:57:52AM +0100, Christophe Leroy (CS GROUP) wrote:
> 
> 
> Le 24/12/2025 à 08:54, Dan Carpenter a écrit :
> > On Tue, Dec 23, 2025 at 04:34:44PM +0100, Christophe Leroy (CS GROUP) wrote:
> > > 
> > > 
> > > Le 22/12/2025 à 08:49, Haoxiang Li a écrit :
> > > > If device_add() fails, call put_device() to drop the device
> > > > reference and do the cleanp.
> > > > 
> > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > Closes: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fb767348e-d89c-416e-acea-1ebbff3bea20%40stanley.mountain%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cfd3865935a164c2083cc08de42c1b5a5%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C639021596902232212%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=j5%2BqptzXvrxcnaaH3PIDorAYMexoPmf3PWi5GbpVD9s%3D&reserved=0
> > > > Signed-off-by: Su Hui <suhui@nfschina.com>
> > > > Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
> > > > ---
> > > >    drivers/bus/fsl-mc/fsl-mc-bus.c | 3 ++-
> > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > > index 25845c04e562..90a2107a9905 100644
> > > > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > > > @@ -896,7 +896,8 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
> > > >    		dev_err(parent_dev,
> > > >    			"device_add() failed for device %s: %d\n",
> > > >    			dev_name(&mc_dev->dev), error);
> > > > -		goto error_cleanup_dev;
> > > > +		put_device(&mc_dev->dev)
> > > 
> > > This change has obviously not been tested, not even built.
> > > 
> > 
> > Yeah, it doesn't build.
> > 
> > > And by droping the goto, the kfree() are not done anymore, leaking
> > > mc_bus/mc_dev p kzalloced areas.
> > > 
> > 
> > Calling put_device() triggers fsl_mc_device_release() which does the
> > free.
> 
> Ok, then this needs to be said in the commit message.
> 
> By the way I'm a bit puzzled by the device_add() doc versus the
> put_device(), because it looks like device_add() already calls put_device()
> in its error path, see
> https://elixir.bootlin.com/linux/v6.19-rc2/source/drivers/base/core.c#L3716
> 

It's refcounted.  It calls dev = get_device(dev) at the start so it
gives up its own reference at the end.  We need another put_device() to
free everything.

regards,
dan carpetner

Re: [PATCH] bus: fsl-mc: fix an error handling in fsl_mc_device_add()

[Dan Carpenter]

On Wed, Dec 24, 2025 at 07:26:00PM +0800, Haoxiang Li wrote:
> On Wed, 24 Dec 2025 12:07:22 +0100, Christophe Leroy wrote:
> > Ah yes, I see.
> > 
> > But then all exit paths in fsl_mc_device_add() after device_initialize() 
> > should call put_device() ?
> 
> > Then in fact the fix should instead be the following, shouldn't it ?
> 
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c 
> > b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > index 25845c04e562..6d132144ce25 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > @@ -905,11 +905,7 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
> >   	return 0;
> > 
> >   error_cleanup_dev:
> > -	kfree(mc_dev->regions);
> > -	if (mc_bus)
> > -		kfree(mc_bus);
> > -	else
> > -		kfree(mc_dev);
> > +	put_device(&mc_dev->dev);
> > 
> >   	return error;
> >   }
> 
> Yes, I think so.
> However, I submit a same fix months ago and got a reply:
> https://lore.kernel.org/all/2025052622-nautical-suitably-486c@gregkh/
> 
> I didn't figure out why, and look forward to experts' guidance.

That's a different patch.  Perhaps it didn't apply?  *shrug*.
Christophe's patch looks correct to me.

regards,
dan carpenter