bus: sunxi-rsb: Fix a small memory leak
Subsystems (2)
PATCH 1/1 bus: sunxi-rsb: Fix a small memory leak
The problem is in __devm_regmap_init_sunxi_rsb(). If the call to __devm_regmap_init() fails, then we leak the "ctx" allocation.
--- drivers/bus/sunxi-rsb.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
drivers/bus/sunxi-rsb.c +1 -9
@@ -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 );
Signature · 1 line
2.17.1
[bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus
Quoted 6 lines · 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 withsunxi_rsb_devicedevices 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.
Quoted 45 lines
drivers/bus/sunxi-rsb.c198 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 }
Quoted 8 lines
regards,
dan carpenter_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [bug report] bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus
In reply to Dan CarpenterHi,
Quoted 43 lines · 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 withsunxi_rsb_devicedevices 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 structsunxi_rsb_device*sunxi_rsb_device_create(structsunxi_rsb*rsb,
199 structdevice_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);
Quoted 4 lines
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.
Quoted 2 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi,
Quoted 3 lines · 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.
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?
Quoted 28 lines
Signed-off-by Dan Carpenter <[email protected]>--- drivers/bus/sunxi-rsb.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)drivers/bus/sunxi-rsb.c +0
-8@@ -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 , };
drivers/bus/sunxi-rsb.c static struct sunxi_rsb_ctx *regmap_sunxi_rsb_init_ctx(struct sunxi_rsb_device * · line 441
Quoted 5 lines
drivers/bus/sunxi-rsb.c +1
-1@@ -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
Quoted 6 lines
drivers/bus/sunxi-rsb.c +0
-1if (!ctx)return ERR_PTR (-ENOMEM ); --2.17.1