| Message ID | 20181218082800.GC440@kadam (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bus: sunxi-rsb: Fix a small memory leak | expand |
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
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 >
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
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 >
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);
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(-)