Received-SPF: pass (google.com: domain of linux-kernel+bounces-14212-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <2bf1b0cb-86af-4e00-a0aa-23e3944617a2@linux.ibm.com>
Date: Tue, 2 Jan 2024 10:53:15 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [EXT] Re: [PATCH 1/2] net/iucv: Improve unlocking in
 iucv_enable()
Content-Language: en-US
To: Suman Ghosh <sumang@marvell.com>, Markus Elfring <Markus.Elfring@web.de>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Wenjia Zhang <wenjia@linux.ibm.com>
Cc: LKML <linux-kernel@vger.kernel.org>
References: <cde82080-c715-473c-97ac-6ef66bba6d64@web.de>
 <81f7db31-a258-4dc8-b6e1-c1ef1844a9d2@web.de>
 <SJ0PR18MB5216C27127E46490951A1E5DDB61A@SJ0PR18MB5216.namprd18.prod.outlook.com>
 <8123a895-c7dd-4a75-94bc-6f61639621eb@web.de>
 <SJ0PR18MB52168AC4B874C0B99BD37039DB61A@SJ0PR18MB5216.namprd18.prod.outlook.com>
From: Alexandra Winter <wintera@linux.ibm.com>
In-Reply-To: <SJ0PR18MB52168AC4B874C0B99BD37039DB61A@SJ0PR18MB5216.namprd18.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 02.01.24 09:27, Suman Ghosh wrote:
>>> [Suman] This looks confusing. What is the issue with retaining the
>> original change?
>>
>> I propose to reduce the number of cpus_read_unlock() calls (in the
>> source code).
>>
>> Regards,
>> Markus
> [Suman] Then I think we should do something like this. Changing the code flow back-and-forth using "goto" does not seem correct.

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. 
For some reason Markus wants to reduce the number of cpus_read_unlock() calls (why?),
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;
 }