> I share Suman's concern that jumping backwards goto is confusing.
> But I think the Coccinelle finding of freeing a null-pointer should be addressed (see patch 2/2)
> Thank you Markus for reporting it.
>
> The allocation does require holding the cpus_read_lock.
How does this information fit to your following suggestion to adjust the lock scope?
> For some reason Markus wants to reduce the number of cpus_read_unlock() calls (why?),
One cpus_read_unlock() call is required here.
Would you like to benefit more from a smaller executable code size?
> so what about something like this for both issues:
>
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 0ed6e34d6edd..1030403b826b 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -542,24 +542,22 @@ static int iucv_enable(void)
> size_t alloc_size;
> int cpu, rc;
>
> - cpus_read_lock();
> - rc = -ENOMEM;
> alloc_size = iucv_max_pathid * sizeof(struct iucv_path);
> iucv_path_table = kzalloc(alloc_size, GFP_KERNEL);
> if (!iucv_path_table)
> - goto out;
> + return -ENOMEM;
> /* Declare per cpu buffers. */
> - rc = -EIO;
> + cpus_read_lock();
> for_each_online_cpu(cpu)
> smp_call_function_single(cpu, iucv_declare_cpu, NULL, 1);
> - if (cpumask_empty(&iucv_buffer_cpumask))
> + if (cpumask_empty(&iucv_buffer_cpumask)) {
> /* No cpu could declare an iucv buffer. */
> - goto out;
> - cpus_read_unlock();
> - return 0;
> -out:
> - kfree(iucv_path_table);
> - iucv_path_table = NULL;
> + kfree(iucv_path_table);
> + iucv_path_table = NULL;
> + rc = -EIO;
> + } else {
> + rc = 0;
> + }
> cpus_read_unlock();
> return rc;
> }
I suggest to reconsider patch squashing a bit more.
Regards,
Markus
