* [PATCH] bus: sunxi-rsb: Fix a small memory leak
@ 2018-12-18 8:28 Dan Carpenter
2018-12-18 8:36 ` [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus Dan Carpenter
2018-12-18 12:51 ` [PATCH] bus: sunxi-rsb: Fix a small memory leak Chen-Yu Tsai
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2018-12-18 8:28 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Chen-Yu Tsai, kernel-janitors, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus
2018-12-18 8:28 [PATCH] bus: sunxi-rsb: Fix a small memory leak Dan Carpenter
@ 2018-12-18 8:36 ` Dan Carpenter
2018-12-18 11:48 ` Robin Murphy
2018-12-18 12:27 ` Chen-Yu Tsai
2018-12-18 12:51 ` [PATCH] bus: sunxi-rsb: Fix a small memory leak Chen-Yu Tsai
1 sibling, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2018-12-18 8:36 UTC (permalink / raw)
To: wens; +Cc: Olof Johansson, Maxime Ripard, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus
2018-12-18 8:36 ` [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus Dan Carpenter
@ 2018-12-18 11:48 ` Robin Murphy
2018-12-18 12:27 ` Chen-Yu Tsai
1 sibling, 0 replies; 5+ messages in thread
From: Robin Murphy @ 2018-12-18 11:48 UTC (permalink / raw)
To: Dan Carpenter, wens; +Cc: Olof Johansson, Maxime Ripard, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus
2018-12-18 8:36 ` [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus Dan Carpenter
2018-12-18 11:48 ` Robin Murphy
@ 2018-12-18 12:27 ` Chen-Yu Tsai
1 sibling, 0 replies; 5+ messages in thread
From: Chen-Yu Tsai @ 2018-12-18 12:27 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Olof Johansson, Maxime Ripard, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bus: sunxi-rsb: Fix a small memory leak
2018-12-18 8:28 [PATCH] bus: sunxi-rsb: Fix a small memory leak Dan Carpenter
2018-12-18 8:36 ` [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus Dan Carpenter
@ 2018-12-18 12:51 ` Chen-Yu Tsai
1 sibling, 0 replies; 5+ messages in thread
From: Chen-Yu Tsai @ 2018-12-18 12:51 UTC (permalink / raw)
To: Dan Carpenter
Cc: Maxime Ripard, Mark Brown, kernel-janitors, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-18 12:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-18 8:28 [PATCH] bus: sunxi-rsb: Fix a small memory leak Dan Carpenter
2018-12-18 8:36 ` [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus Dan Carpenter
2018-12-18 11:48 ` Robin Murphy
2018-12-18 12:27 ` Chen-Yu Tsai
2018-12-18 12:51 ` [PATCH] bus: sunxi-rsb: Fix a small memory leak Chen-Yu Tsai
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).