Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <cd309d68-65ac-0ce9-c941-520ac6402f8e@ozlabs.ru>
Date:   Tue, 20 Jul 2021 19:41:02 +1000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:90.0) Gecko/20100101
 Thunderbird/90.0
Subject: Re: [PATCH v5 02/11] powerpc/kernel/iommu: Add new
 iommu_table_in_use() helper
Content-Language: en-US
To:     =?UTF-8?Q?Leonardo_Br=c3=a1s?= <leobras.c@gmail.com>,
        Frederic Barrat <fbarrat@linux.ibm.com>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        David Gibson <david@gibson.dropbear.id.au>,
        kernel test robot <lkp@intel.com>,
        Nicolin Chen <nicoleotsuka@gmail.com>
Cc:     linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
References: <20210716082755.428187-1-leobras.c@gmail.com>
 <20210716082755.428187-3-leobras.c@gmail.com>
 <29c199f3-63a8-3edb-b29e-de157431d89f@linux.ibm.com>
 <283e44591010dcb0690769afaef147add3b36978.camel@gmail.com>
From:   Alexey Kardashevskiy <aik@ozlabs.ru>
In-Reply-To: <283e44591010dcb0690769afaef147add3b36978.camel@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 20/07/2021 15:38, Leonardo Brás wrote:
> Hello Fred, thanks for this feedback!
> 
> Sorry if I miss anything, this snippet was written for v1 over an year
> ago, and I have not taken a look at it ever since.
> 
> On Mon, 2021-07-19 at 15:53 +0200, Frederic Barrat wrote:
>>
>>
>> On 16/07/2021 10:27, Leonardo Bras wrote:
>>> @@ -1099,18 +1105,13 @@ int iommu_take_ownership(struct iommu_table
>>> *tbl)
>>>          for (i = 0; i < tbl->nr_pools; i++)
>>>                  spin_lock_nest_lock(&tbl->pools[i].lock, &tbl-
>>>> large_pool.lock);
>>>    
>>> -       iommu_table_release_pages(tbl);
>>> -
>>> -       if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
>>> +       if (iommu_table_in_use(tbl)) {
>>>                  pr_err("iommu_tce: it_map is not empty");
>>>                  ret = -EBUSY;
>>> -               /* Undo iommu_table_release_pages, i.e. restore
>>> bit#0, etc */
>>> -               iommu_table_reserve_pages(tbl, tbl-
>>>> it_reserved_start,
>>> -                               tbl->it_reserved_end);
>>> -       } else {
>>> -               memset(tbl->it_map, 0xff, sz);
>>>          }
>>>    
>>> +       memset(tbl->it_map, 0xff, sz);
>>> +
>>
>>
>> So if the table is not empty, we fail (EBUSY) but we now also
>> completely
>> overwrite the bitmap. It was in an unexpected state, but we're making
>> it
>> worse. Or am I missing something?
> 
> IIRC there was a reason to do that at the time, but TBH I don't really
> remember it, and by looking at the code right now you seem to be
> correct about this causing trouble.
> 
> I will send a v6 fixing it soon.
> Please review the remaining patches for some issue I may be missing.
> 
> Alexey, any comments on that?


Agree with Fred, this is a bug, EBUSY is not that unexpected :-/ Thanks,


> 
>>
>>     Fred
>>
> 
> Again, thank you for reviewing Fred!
> Best regards,
> Leonardo Bras
> 
> 
> 
> 
> 

-- 
Alexey