[PATCH] bus: sunxi-rsb: Fix a small memory leak

[PATCH] bus: sunxi-rsb: Fix a small memory leak

[Dan Carpenter]

The problem is in __devm_regmap_init_sunxi_rsb().  If the call to
__devm_regmap_init() fails, then we leak the "ctx" allocation.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/bus/sunxi-rsb.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 1b76d9585902..5ec25c8164d1 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -417,17 +417,9 @@ static int regmap_sunxi_rsb_reg_write(void *context, unsigned int reg,
 	return sunxi_rsb_write(rdev->rsb, rdev->rtaddr, reg, &val, ctx->size);
 }
 
-static void regmap_sunxi_rsb_free_ctx(void *context)
-{
-	struct sunxi_rsb_ctx *ctx = context;
-
-	kfree(ctx);
-}
-
 static struct regmap_bus regmap_sunxi_rsb = {
 	.reg_write = regmap_sunxi_rsb_reg_write,
 	.reg_read = regmap_sunxi_rsb_reg_read,
-	.free_context = regmap_sunxi_rsb_free_ctx,
 	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
 	.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
 };
@@ -446,7 +438,7 @@ static struct sunxi_rsb_ctx *regmap_sunxi_rsb_init_ctx(struct sunxi_rsb_device *
 		return ERR_PTR(-EINVAL);
 	}
 
-	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	ctx = devm_kzalloc(&rdev->dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus

[Dan Carpenter]

I totally don't understand the sunxi_rsb_device_create() function.  It's
basically a no-op.  There is a lot of code to deal with sunxi_rsb_device
devices but there is no function to create them so it seems like dead
code.

What am I missing?

drivers/bus/sunxi-rsb.c
   198  static struct sunxi_rsb_device *sunxi_rsb_device_create(struct sunxi_rsb *rsb,
   199                  struct device_node *node, u16 hwaddr, u8 rtaddr)
   200  {
   201          int err;
   202          struct sunxi_rsb_device *rdev;
   203  
   204          rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
   205          if (!rdev)
   206                  return ERR_PTR(-ENOMEM);
   207  
   208          rdev->rsb = rsb;
   209          rdev->hwaddr = hwaddr;
   210          rdev->rtaddr = rtaddr;
   211          rdev->dev.bus = &sunxi_rsb_bus;
   212          rdev->dev.parent = rsb->dev;
   213          rdev->dev.of_node = node;
   214          rdev->dev.release = sunxi_rsb_dev_release;
   215  
   216          dev_set_name(&rdev->dev, "%s-%x", RSB_CTRL_NAME, hwaddr);
   217  
   218          err = device_register(&rdev->dev);
   219          if (err < 0) {
   220                  dev_err(&rdev->dev, "Can't add %s, status %d\n",
   221                          dev_name(&rdev->dev), err);
   222                  goto err_device_add;
   223          }
   224  
   225          dev_dbg(&rdev->dev, "device %s registered\n", dev_name(&rdev->dev));
   226  
   227  err_device_add:
   228          put_device(&rdev->dev);
   229  
   230          return ERR_PTR(err);

We call put_device() on the success path and return NULL.  The caller
checks to see if it is an error pointer, and prints an error message.
NULL isn't an error pointer so it doesn't cause any problems but it also
doesn't do anything.

The caller doesn't save the returned devices either so there seems to
be a bunch of code missing...

   231  }

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus

[Robin Murphy]

On 18/12/2018 08:36, Dan Carpenter wrote:
> I totally don't understand the sunxi_rsb_device_create() function.  It's
> basically a no-op.  There is a lot of code to deal with sunxi_rsb_device
> devices but there is no function to create them so it seems like dead
> code.
> 
> What am I missing?

By the sound of it, the considerable amount of stuff happening behind 
that device_register() call ;)

The general shape of this function looks like fairly typical bus code to 
me, however I'm not sure off-hand that it's actually correct to drop the 
initial device reference in the success path (I suspect that should 
probably be kept around to be dropped in the corresponding 
device_unregister() call), and AFAICS there's no reason it shouldn't 
just return a normal int error code.

Robin.

> drivers/bus/sunxi-rsb.c
>     198  static struct sunxi_rsb_device *sunxi_rsb_device_create(struct sunxi_rsb *rsb,
>     199                  struct device_node *node, u16 hwaddr, u8 rtaddr)
>     200  {
>     201          int err;
>     202          struct sunxi_rsb_device *rdev;
>     203
>     204          rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
>     205          if (!rdev)
>     206                  return ERR_PTR(-ENOMEM);
>     207
>     208          rdev->rsb = rsb;
>     209          rdev->hwaddr = hwaddr;
>     210          rdev->rtaddr = rtaddr;
>     211          rdev->dev.bus = &sunxi_rsb_bus;
>     212          rdev->dev.parent = rsb->dev;
>     213          rdev->dev.of_node = node;
>     214          rdev->dev.release = sunxi_rsb_dev_release;
>     215
>     216          dev_set_name(&rdev->dev, "%s-%x", RSB_CTRL_NAME, hwaddr);
>     217
>     218          err = device_register(&rdev->dev);
>     219          if (err < 0) {
>     220                  dev_err(&rdev->dev, "Can't add %s, status %d\n",
>     221                          dev_name(&rdev->dev), err);
>     222                  goto err_device_add;
>     223          }
>     224
>     225          dev_dbg(&rdev->dev, "device %s registered\n", dev_name(&rdev->dev));
>     226
>     227  err_device_add:
>     228          put_device(&rdev->dev);
>     229
>     230          return ERR_PTR(err);
> 
> We call put_device() on the success path and return NULL.  The caller
> checks to see if it is an error pointer, and prints an error message.
> NULL isn't an error pointer so it doesn't cause any problems but it also
> doesn't do anything.
> 
> The caller doesn't save the returned devices either so there seems to
> be a bunch of code missing...
> 
>     231  }
> 
> regards,
> dan carpenter
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus

[Chen-Yu Tsai]

Hi,

On Tue, Dec 18, 2018 at 4:36 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> I totally don't understand the sunxi_rsb_device_create() function.  It's
> basically a no-op.  There is a lot of code to deal with sunxi_rsb_device
> devices but there is no function to create them so it seems like dead
> code.
>
> What am I missing?
>
> drivers/bus/sunxi-rsb.c
>    198  static struct sunxi_rsb_device *sunxi_rsb_device_create(struct sunxi_rsb *rsb,
>    199                  struct device_node *node, u16 hwaddr, u8 rtaddr)
>    200  {
>    201          int err;
>    202          struct sunxi_rsb_device *rdev;
>    203
>    204          rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
>    205          if (!rdev)
>    206                  return ERR_PTR(-ENOMEM);
>    207
>    208          rdev->rsb = rsb;
>    209          rdev->hwaddr = hwaddr;
>    210          rdev->rtaddr = rtaddr;
>    211          rdev->dev.bus = &sunxi_rsb_bus;
>    212          rdev->dev.parent = rsb->dev;
>    213          rdev->dev.of_node = node;
>    214          rdev->dev.release = sunxi_rsb_dev_release;
>    215
>    216          dev_set_name(&rdev->dev, "%s-%x", RSB_CTRL_NAME, hwaddr);
>    217
>    218          err = device_register(&rdev->dev);
>    219          if (err < 0) {
>    220                  dev_err(&rdev->dev, "Can't add %s, status %d\n",
>    221                          dev_name(&rdev->dev), err);
>    222                  goto err_device_add;
>    223          }
>    224
>    225          dev_dbg(&rdev->dev, "device %s registered\n", dev_name(&rdev->dev));
>    226
>    227  err_device_add:
>    228          put_device(&rdev->dev);
>    229
>    230          return ERR_PTR(err);
>
> We call put_device() on the success path and return NULL.  The caller
> checks to see if it is an error pointer, and prints an error message.
> NULL isn't an error pointer so it doesn't cause any problems but it also
> doesn't do anything.

That definitely looks like a bug. I can't recall any specifics from the
time I wrote this though.

> The caller doesn't save the returned devices either so there seems to
> be a bunch of code missing...

There's no need for that as the code doesn't keep a separate list of
registered devices. All slave devices are direct childs of the bus
controller, and the only time the bus code needs a reference to them
is to remove them. For comparison, the SPI subsystem does the same.

Regards
ChenYu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] bus: sunxi-rsb: Fix a small memory leak

[Chen-Yu Tsai]

Hi,

On Tue, Dec 18, 2018 at 4:28 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> The problem is in __devm_regmap_init_sunxi_rsb().  If the call to
> __devm_regmap_init() fails, then we leak the "ctx" allocation.

This seems to be the same pattern with regmap implementations that
allocate context data, the other two being:

    - drivers/base/regmap/regmap-mmio.c
    - drivers/bluetooth/btintel.c

(CC-ing Mark) Maybe it's worth fixing in regmap itself?

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/bus/sunxi-rsb.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> index 1b76d9585902..5ec25c8164d1 100644
> --- a/drivers/bus/sunxi-rsb.c
> +++ b/drivers/bus/sunxi-rsb.c
> @@ -417,17 +417,9 @@ static int regmap_sunxi_rsb_reg_write(void *context, unsigned int reg,
>         return sunxi_rsb_write(rdev->rsb, rdev->rtaddr, reg, &val, ctx->size);
>  }
>
> -static void regmap_sunxi_rsb_free_ctx(void *context)
> -{
> -       struct sunxi_rsb_ctx *ctx = context;
> -
> -       kfree(ctx);
> -}
> -
>  static struct regmap_bus regmap_sunxi_rsb = {
>         .reg_write = regmap_sunxi_rsb_reg_write,
>         .reg_read = regmap_sunxi_rsb_reg_read,
> -       .free_context = regmap_sunxi_rsb_free_ctx,
>         .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
>         .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
>  };
> @@ -446,7 +438,7 @@ static struct sunxi_rsb_ctx *regmap_sunxi_rsb_init_ctx(struct sunxi_rsb_device *
>                 return ERR_PTR(-EINVAL);
>         }
>
> -       ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +       ctx = devm_kzalloc(&rdev->dev, sizeof(*ctx), GFP_KERNEL);

This decouples the lifetime of ctx from the regmap itself. IMO it
would be better to check the return value of __devm_regmap_init()
and act accordingly instead.

ChenYu

>         if (!ctx)
>                 return ERR_PTR(-ENOMEM);
>
> --
> 2.17.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel