Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <99bee917-d874-11af-5c78-30852e057732@linux.ibm.com>
Date:   Mon, 10 Jul 2023 08:35:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.12.0
Subject: Re: [PATCH net v2 1/3] s390/ism: Fix locking for forwarding of IRQs
 and events to clients
To:     Simon Horman <simon.horman@corigine.com>,
        Niklas Schnelle <schnelle@linux.ibm.com>
Cc:     Paolo Abeni <pabeni@redhat.com>,
        Wenjia Zhang <wenjia@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        "David S. Miller" <davem@davemloft.net>,
        Stefan Raspl <raspl@linux.ibm.com>,
        Jan Karcher <jaka@linux.ibm.com>, linux-s390@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230707104359.3324039-1-schnelle@linux.ibm.com>
 <20230707104359.3324039-2-schnelle@linux.ibm.com>
 <ZKlmeDUEZf7F8+HW@corigine.com>
Content-Language: en-US
From:   Alexandra Winter <wintera@linux.ibm.com>
In-Reply-To: <ZKlmeDUEZf7F8+HW@corigine.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 08.07.23 15:36, Simon Horman wrote:
> On Fri, Jul 07, 2023 at 12:43:57PM +0200, Niklas Schnelle wrote:
[...]
>> @@ -92,6 +102,9 @@ int ism_unregister_client(struct ism_client *client)
>>  		max_client--;
>>  	spin_unlock_irqrestore(&clients_lock, flags);
>>  	list_for_each_entry(ism, &ism_dev_list.list, list) {
>> +		spin_lock_irqsave(&ism->lock, flags);
> 
> Hi Niklas,
> 
> The lock is taken here.
> 
>> +		/* Stop forwarding IRQs and events */
>> +		ism->subs[client->id] = NULL;
>>  		for (int i = 0; i < ISM_NR_DMBS; ++i) {
>>  			if (ism->sba_client_arr[i] == client->id) {
>>  				pr_err("%s: attempt to unregister client '%s'"
>> @@ -101,6 +114,7 @@ int ism_unregister_client(struct ism_client *client)
>>  				goto out;
> 
> But it does not appear to be released
> (by the call to spin_unlock_irqrestore() below)
> if goto out is called here.
> 
>>  			}
>>  		}
>> +		spin_unlock_irqrestore(&ism->lock, flags);
>>  	}
>>  out:
>>  	mutex_unlock(&ism_dev_list.mutex);
> 

Great catch, Simon.
@Niklas, the missing unlock accidentially got moved to "[PATCH net v2 3/3] s390/ism: Do not unregister clients with registered DMBs"