Re: [PATCH v2] staging: media: ipu7: fix boot_config leak on queue_mem failure

[Dan Carpenter <error27@gmail.com>]

On Fri, Apr 17, 2026 at 11:01:29AM +0300, Dan Carpenter wrote:
> I haven't looked at this but I bet there are bugs in the error handling
> since magical cleanup functions are always buggy.

The error handling in ipu7_fw_isys_init() is frustrating to review but
it works fine.

Imagine that someone were writing this error handling methodically, using
a normal unwind ladder.
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

drivers/staging/media/ipu7/ipu7-fw-isys.c 
    81  int ipu7_fw_isys_init(struct ipu7_isys *isys)
    82  {
    83          struct syscom_queue_config *queue_configs;
    84          struct ipu7_bus_device *adev = isys->adev;
    85          struct device *dev = &adev->auxdev.dev;
    86          struct ipu7_insys_config *isys_config;
    87          struct ipu7_syscom_context *syscom;
    88          dma_addr_t isys_config_dma_addr;
    89          unsigned int i, num_queues;
    90          u32 freq;
    91          u8 major;
    92          int ret;
    93  
    94          /* Allocate and init syscom context. */
    95          syscom = devm_kzalloc(dev, sizeof(struct ipu7_syscom_context),
    96                                GFP_KERNEL);
    97          if (!syscom)
    98                  return -ENOMEM;

This is our first resource.  It's allocated with devm_ so we don't need
to free it.  Nothing to free.

    99  
   100          adev->syscom = syscom;
   101          syscom->num_input_queues = IPU_INSYS_MAX_INPUT_QUEUES;
   102          syscom->num_output_queues = IPU_INSYS_MAX_OUTPUT_QUEUES;
   103          num_queues = syscom->num_input_queues + syscom->num_output_queues;
   104          queue_configs = devm_kzalloc(dev, FW_QUEUE_CONFIG_SIZE(num_queues),
   105                                       GFP_KERNEL);

This is our second resource.  Still devm_.  Still nothing to free.

   106          if (!queue_configs) {
   107                  ipu7_fw_isys_release(isys);

And yet we call ipu7_fw_isys_release().  Fortunately it's a no-op.

   108                  return -ENOMEM;
   109          }
   110          syscom->queue_configs = queue_configs;
   111          queue_configs[IPU_INSYS_OUTPUT_MSG_QUEUE].max_capacity =
   112                  IPU_ISYS_SIZE_RECV_QUEUE;
   113          queue_configs[IPU_INSYS_OUTPUT_MSG_QUEUE].token_size_in_bytes =
   114                  sizeof(struct ipu7_insys_resp);
   115          queue_configs[IPU_INSYS_OUTPUT_LOG_QUEUE].max_capacity =
   116                  IPU_ISYS_SIZE_LOG_QUEUE;
   117          queue_configs[IPU_INSYS_OUTPUT_LOG_QUEUE].token_size_in_bytes =
   118                  sizeof(struct ipu7_insys_resp);
   119          queue_configs[IPU_INSYS_OUTPUT_RESERVED_QUEUE].max_capacity = 0;
   120          queue_configs[IPU_INSYS_OUTPUT_RESERVED_QUEUE].token_size_in_bytes = 0;
   121  
   122          queue_configs[IPU_INSYS_INPUT_DEV_QUEUE].max_capacity =
   123                  IPU_ISYS_MAX_STREAMS;
   124          queue_configs[IPU_INSYS_INPUT_DEV_QUEUE].token_size_in_bytes =
   125                  sizeof(struct ipu7_insys_send_queue_token);
   126  
   127          for (i = IPU_INSYS_INPUT_MSG_QUEUE; i < num_queues; i++) {
   128                  queue_configs[i].max_capacity = IPU_ISYS_SIZE_SEND_QUEUE;
   129                  queue_configs[i].token_size_in_bytes =
   130                          sizeof(struct ipu7_insys_send_queue_token);
   131          }
   132  
   133          /* Allocate ISYS subsys config. */
   134          isys_config = ipu7_dma_alloc(adev, sizeof(struct ipu7_insys_config),
   135                                       &isys_config_dma_addr, GFP_KERNEL, 0);
   136          if (!isys_config) {
   137                  dev_err(dev, "Failed to allocate isys subsys config.\n");
   138                  ipu7_fw_isys_release(isys);

Still nothing to free.  This is still a no-op.

   139                  return -ENOMEM;
   140          }
   141          isys->subsys_config = isys_config;

isys->subsys_config is our first resource that we have to free.

   142          isys->subsys_config_dma_addr = isys_config_dma_addr;
   143          memset(isys_config, 0, sizeof(struct ipu7_insys_config));
   144          isys_config->logger_config.use_source_severity = 0;
   145          isys_config->logger_config.use_channels_enable_bitmask = 1;
   146          isys_config->logger_config.channels_enable_bitmask =
   147                  LOGGER_CONFIG_CHANNEL_ENABLE_SYSCOM_BITMASK;
   148          isys_config->logger_config.hw_printf_buffer_base_addr = 0U;
   149          isys_config->logger_config.hw_printf_buffer_size_bytes = 0U;
   150          isys_config->wdt_config.wdt_timer1_us = 0;
   151          isys_config->wdt_config.wdt_timer2_us = 0;
   152          ret = ipu_buttress_get_isys_freq(adev->isp, &freq);

This doesn't allocate anything, but we would need to free
isys->subsys_config.  Ideally, we would have a goto free_subsys_config;
here.

   153          if (ret) {
   154                  dev_err(dev, "Failed to get ISYS frequency.\n");
   155                  ipu7_fw_isys_release(isys);

But this does free isys->subsys_config.  Good.

   156                  return ret;
   157          }
   158  
   159          ipu7_dma_sync_single(adev, isys_config_dma_addr,
   160                               sizeof(struct ipu7_insys_config));
   161  
   162          major = is_ipu8(adev->isp->hw_ver) ? 2U : 1U;
   163          ret = ipu7_boot_init_boot_config(adev, queue_configs, num_queues,
   164                                           freq, isys_config_dma_addr, major);

This allocates one or both of syscom->queue_mem and adev->boot_config.
Ideally it would clean up partial allocations and we would do the same
goto free_subsys_config; here.

   165          if (ret)
   166                  ipu7_fw_isys_release(isys);

But this does free them along with isys->subsys_config.  Everything
works.

   167  
   168          return ret;
   169  }

regards,
dan carpenter