Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d5d1785d-b2a8-6913-78ba-d686d70f2a64@amd.com>
Date:   Wed, 31 Aug 2022 07:54:02 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: crypto: ccp - Release dma channels before dmaengine unrgister
Content-Language: en-US
To:     Koba Ko <koba.ko@canonical.com>
Cc:     John Allen <john.allen@amd.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220830093439.951960-1-koba.ko@canonical.com>
 <c3ea3992-df42-3a26-6d30-834368faf91c@amd.com>
 <CAJB-X+XMHYyQJLLuC24rAvQwNOOZUdMBhh12RC9UTOSCrCa=HA@mail.gmail.com>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <CAJB-X+XMHYyQJLLuC24rAvQwNOOZUdMBhh12RC9UTOSCrCa=HA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SEVnN01KYzZVMWtBR0lsQzZlZTFQR2MvZjJML2doWlBlamh6RStML2V5OW9F?=
 =?utf-8?B?MEY4M1I4azJHdTlmK1dUY1J3Nll1Tk52cXdwTlN3aWlzaGp3UVloZHFtOHc5?=
 =?utf-8?B?NnN2Q0t3OGRYM1RFNGVuSkdDYTBuWjVQWW1SakF1QzJlMEVETGdmUWxVTGxi?=
 =?utf-8?B?V0hua1d1SHFmOG12bEQrMVkwR2NFSFNFeE5WSlRQZXFKWWJXcXNlQ0NnQ3pR?=
 =?utf-8?B?UVZORnVhQkZUOWtlajVkeGNkUU1GQ0JrVWxVbUIxejBCMzFMUzlaQ1dycENC?=
 =?utf-8?B?L1o0ZzBkeDJyY2VaUE9VaUt6SzMwSDRoUzJYUXpTa3RkbTdyT0NWTWFiVWY0?=
 =?utf-8?B?aWpHSkNZTEtuRWp0UHNza2dFNFJKaU91THczNG1QM1BISFJpQ2NGRzFjVFVs?=
 =?utf-8?B?YTQvbXZOMzV2RlN5Y09oM1ZrSzlvd1NYTFVWL0p3cW85RkJiczl1QUswU3BQ?=
 =?utf-8?B?TmRsZllvODFvZEluMThCRTNuMFY2THJqdEJOclpiTE9wWkZjakN5UDIyRHZ2?=
 =?utf-8?B?Qk5WZk1QZU0xQ2dKeWZSRHQ5ak9tcnVQOG5HNFFDZ09xcVhzYStwTnpjd3VZ?=
 =?utf-8?B?d3U0eVZlekhYVEtZZWh2ZFcrbWhnS3NMQVJmc3BBV3A2eklEQlYya0tEMytk?=
 =?utf-8?B?L1Jyb1Budmcwc3F4V3ZpeDFTbGg4bTVCV0tDQXh1MmdGdkVUZTFSTmpIUmV1?=
 =?utf-8?B?dHAzOFdmU1RaR0JKREtuRFNBK0FrK1ZHeUJsVXEyRGREQzVZSm5QdEpmMW1O?=
 =?utf-8?B?VytSRmZtalBKU1p5cUpMaVB5U1puQVZUbFQ3dTNSazFjUzFnbFN5d3RpRkVu?=
 =?utf-8?B?NTJlRmoyZWRoV0dtUWttbVBXd0tBdDIzTGsxWFZKMjNqQkMwYjBiaWRxazB5?=
 =?utf-8?B?dk5lS2ttS2hzcVBuTUNYVWFIYVJ2Z2lnNmVOUitZWkZqTkNaVFUxWjRUVXFt?=
 =?utf-8?B?aVFKWG1CdG5WZUxrSHZtbVdTMHVIQ1JpVmhBTEpEZlJiYUkzaWxNMTlWMEh2?=
 =?utf-8?B?YU05UzlNa3pCWXdXbXJtVnpXS3JwZWd1eExkVzVMS1Q4QWpwZ0Rnd2c5UEp3?=
 =?utf-8?B?Qk1zQm5PanlMaUF4cmdObkM0eEFwdlpuMWpIMytvWFV5MzNMbnZkREpCZ1Vm?=
 =?utf-8?B?MVJWcTBMeHpYOFlwdXh3bTFYc0QvbTdiNnArNGt1WXJWSGRZaVViSTZnQ2h3?=
 =?utf-8?B?OU5BeWlxL3VyTjkzNEZyMTRmb1BoemcxU0t1T3krZUEwVU9zMmhOVmNQRW5l?=
 =?utf-8?B?SWs3b2RsVDQyVFpZRzVlK2Rwd0FwcU5xMDJibHpnS015clUzZ2tOQUthL3FU?=
 =?utf-8?B?VWF4eklsYjZnL1I2a0RmdUlHZ3drS1lDL0lJWm1PRERGUTJBR2FlYmRRTTZh?=
 =?utf-8?B?LzcrRDU4SlJwUjArcDd5UjIvSmVqN3hYNmdaRHZNQWpyLzNlbitROVlWeG9s?=
 =?utf-8?B?WUtYdDRlL2FTd1VUUWNCcHQ5WDUwejAwN2xVejZiUEhCdFZQbkh1Njl2UXRI?=
 =?utf-8?B?dnNqZ1NlQmZySTc1a01icmdWYURKc0E3WS9vOHVxY0w1dVJxQ2FqR3grbXli?=
 =?utf-8?B?WHRXZjdubVMvSllOTUtLOTFNdkxFa0hiOTVsR0k4aU1pa1VxdTlEVng4TVdT?=
 =?utf-8?B?bGdzS09XZm90RTlsT1QyYkc1ZDFaVjhtYndQRFFrK0N6NWpYSFdRYTJML3Yz?=
 =?utf-8?B?ejA3RTFLMStyVGVKWEtYeDZmbElEcVhtb0hicWZhSHpCRGZScUdHR3lYNGlm?=
 =?utf-8?B?b3ZSWVU0UUlSVXBpVGxvOGl1THMwa2tTUDRNeHU2ZHAwbW5BVlV1QUN1WW9Y?=
 =?utf-8?B?aTYyM2wzcWk5OXJWa1l3YTVVQmhLaXcxaTMycDFqOFlCUXlobzRUVlFLK1FN?=
 =?utf-8?B?UVNmdXIyWXExM3k5aHQway9tTWptNStSYnFyaFg1UFR5Mm1uK1ltMGtVYTlu?=
 =?utf-8?B?NjZqN01lbHlqNlZJQ0huSDZ5T282SXkyT3FSTU5OYjA0N3NkblpucjM3WERq?=
 =?utf-8?B?M0p6Y0pLL3VHMittNWVHenBValRmWUd5empGWUI0WkdQY3NPN2NkSkVmR1Zl?=
 =?utf-8?B?ZWN5aTloSGZFbVowNzcyQ2ZtdXRkUWRrcnhWTVFJU0dCV0swNDQwaklrU2NU?=
 =?utf-8?Q?5ZKa6yeoCVq61dxPqZ/7ME+Oz?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9a650602-5ddf-461a-f23b-08da8b4fe267
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Aug 2022 12:54:04.4892
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UB0tWowtsIuAocfQ8GfuNP2znfMZi3WkzgEdiAgAb/2k4xMa0e6RX+6XfdRVShihnUU2u/poSWXaQWOYNgKNbQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB6680
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 8/31/22 00:39, Koba Ko wrote:
> On Tue, Aug 30, 2022 at 11:32 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 8/30/22 04:34, Koba Ko wrote:
>>> A warning is shown during shutdown,
>>>
>>> __dma_async_device_channel_unregister called while 2 clients hold a reference
>>> WARNING: CPU: 15 PID: 1 at drivers/dma/dmaengine.c:1110
>>> __dma_async_device_channel_unregister
>>>
>>> Call dma_release_channel for occupied channles
>>> before dma_async_device_unregister.
>>>
>>> Fixes: 4cbe9bc34ed0 ("crypto: ccp - ccp_dmaengine_unregister release dma channels")
>>> Signed-off-by: Koba Ko <koba.ko@canonical.com>
>>> ---
>>>    drivers/crypto/ccp/ccp-dmaengine.c | 8 +++++++-
>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/crypto/ccp/ccp-dmaengine.c b/drivers/crypto/ccp/ccp-dmaengine.c
>>> index 7d4b4ad1db1f3..ba3eb1ac3b55d 100644
>>> --- a/drivers/crypto/ccp/ccp-dmaengine.c
>>> +++ b/drivers/crypto/ccp/ccp-dmaengine.c
>>> @@ -641,6 +641,10 @@ static void ccp_dma_release(struct ccp_device *ccp)
>>>        for (i = 0; i < ccp->cmd_q_count; i++) {
>>>                chan = ccp->ccp_dma_chan + i;
>>>                dma_chan = &chan->dma_chan;
>>> +
>>> +             if (dma_chan->client_count)
>>> +                     dma_release_channel(dma_chan);
>>> +
>>>                tasklet_kill(&chan->cleanup_tasklet);
>>>                list_del_rcu(&dma_chan->device_node);
>>>        }
>>> @@ -762,12 +766,14 @@ int ccp_dmaengine_register(struct ccp_device *ccp)
>>>    void ccp_dmaengine_unregister(struct ccp_device *ccp)
>>>    {
>>>        struct dma_device *dma_dev = &ccp->dma_dev;
>>> +     struct dma_chan *dma_chan;
>>> +     unsigned int i;
>>>
>>>        if (!dmaengine)
>>>                return;
>>>
>>> -     dma_async_device_unregister(dma_dev);
>>>        ccp_dma_release(ccp);
>>> +     dma_async_device_unregister(dma_dev);
>>
>> Seems the right way to get rid of the warn, but can there still be a race
>> where the channel is grabbed again after ccp_dma_release() and before
>> dma_async_device_unregister() is called?
> 
> After tracing the ccp driver,
> dma_device is held by the ccp device.
> the only function called ccp_dmaengine_register is ccp5_init/ccp_init,
> ccp_dma_release is called in ccp_dmaengine_unregister/ccp_dmaengine_register,
> so there's no race condition in current codes.

I'm referring to some other piece of software claiming the channel, like 
the raid driver or the AMD NTB driver. This just seems racy to me, but it 
does address the WARNing.

Clean up the kernel test robot complaints and submit a v2.

Thanks,
Tom

> 
> 
>>
>> Thanks,
>> Tom
>>
>>>
>>>        kmem_cache_destroy(ccp->dma_desc_cache);
>>>        kmem_cache_destroy(ccp->dma_cmd_cache);