On Thu, Nov 09, 2023 at 04:03:02PM -0500, Umang Jain wrote:
> +static int media_controller_register(struct bcm2835_isp_dev *dev)
> +{
> + char *name;
> + unsigned int i;
> + int ret;
> +
> + v4l2_dbg(2, debug, &dev->v4l2_dev, "Registering with media controller\n");
> + dev->mdev.dev = dev->dev;
> + strscpy(dev->mdev.model, "bcm2835-isp",
> + sizeof(dev->mdev.model));
> + strscpy(dev->mdev.bus_info, "platform:bcm2835-isp",
> + sizeof(dev->mdev.bus_info));
> + media_device_init(&dev->mdev);
> + dev->v4l2_dev.mdev = &dev->mdev;
> +
> + v4l2_dbg(2, debug, &dev->v4l2_dev, "Register entity for nodes\n");
> +
> + name = kmalloc(BCM2835_ISP_ENTITY_NAME_LEN, GFP_KERNEL);
> + if (!name) {
> + ret = -ENOMEM;
> + goto done;
Oh crap. This function doesn't clean up after itself, but instead
returns to a One Magical Cleanup Function... This style of error
handling is *ALWAYS* buggy. But in this case I only see a very minor
leak.
These two cleanups are under one ->registered flag but they are
allocated separately. So if we only complete one action and not the
second then the flag is not set and we don't call media_device_cleanup().
(This is one of many typical problems with One Magical Cleanup Function
Style).
> + if (dev->media_device_registered) {
> + media_device_unregister(&dev->mdev);
> + media_device_cleanup(&dev->mdev);
> + dev->media_device_registered = false;
> + }
It's just such a headache to review... (That's why it's so bug prone).
If you wrote it in Free the Last Thing Style then you could get rid
of a bunch of flags like ->media_device_registered and
->media_entity_registered because at that point you would always know
what you had done and hadn't done.
regards,
dan carpenter
> + }
> + snprintf(name, BCM2835_ISP_ENTITY_NAME_LEN, "bcm2835_isp0");
> + dev->entity.name = name;
> + dev->entity.obj_type = MEDIA_ENTITY_TYPE_BASE;
> + dev->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
> +
> + for (i = 0; i < BCM2835_ISP_NUM_NODES; i++) {
> + dev->pad[i].flags = node_is_output(&dev->node[i]) ?
> + MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> + }
> +
> + ret = media_entity_pads_init(&dev->entity, BCM2835_ISP_NUM_NODES,
> + dev->pad);
> + if (ret)
> + goto done;
> +
> + ret = media_device_register_entity(&dev->mdev, &dev->entity);
> + if (ret)
> + goto done;
> +
> + dev->media_entity_registered = true;
> + for (i = 0; i < BCM2835_ISP_NUM_NODES; i++) {
> ret = media_controller_register_node(dev, i);
> + if (ret)
> + goto done;
> + }
> +
> + ret = media_device_register(&dev->mdev);
> + if (!ret)
> + dev->media_device_registered = true;
> +done:
> + return ret;
> +}
