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;
Subject: Re: [PATCH] dma-buf: st: fix error handling in test_get_fences()
To:     Arnd Bergmann <arnd@kernel.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tvrtko Ursulin <tvrtko.ursulin@intel.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>
Cc:     Arnd Bergmann <arnd@arndb.de>, linux-media@vger.kernel.org,
        dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev
References: <20211026083448.3471055-1-arnd@kernel.org>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Message-ID: <52f6a48a-cd94-da21-2cda-300fd1f78a44@amd.com>
Date:   Tue, 26 Oct 2021 12:55:47 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
In-Reply-To: <20211026083448.3471055-1-arnd@kernel.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SE5oK3l4RXV0U3BtQUFsaCtldnZ3c0pxMHQzMzNDSTlJQXQ1WTd1WERNM3BH?=
 =?utf-8?B?Mm5jZWJ3MWF4QUhGQldrR0gzdFZ4cDFxRWdjYjY2YVZHVkdLaWFuWXh4dXVY?=
 =?utf-8?B?OXV0SnNqbDcwMGRaTnFxeHRyelFkQzY2b0dDVUhvcUlsSHFXTVFJeTE5bkk1?=
 =?utf-8?B?SFhoRVo4b3Y3ZDlKV3VISlpmdzBHaU9ZMmxrem9NU1MxTWV0dU1FVmJFb2pr?=
 =?utf-8?B?WG9aZHR4dEM2M0MwUzNzWmIrUGFSL1k3MWpsaXB4MGNMZWRrR2IvTTN4Q3Zy?=
 =?utf-8?B?OEpIZVFheWFycFY1SXhoaTVVS0phRUpVUzFYaFMwUUtNU2w4MmdoTUZ5enh1?=
 =?utf-8?B?dWw0YW1XWDBqbDJhT3RUb1habXV0eUhJdDAzMUE5MDZWVDQxRFExVUdyY1Qw?=
 =?utf-8?B?Q2I0NytIVjN1ZU5oOFBhWGJrLzdUODg3cjQ3SVBZcWxSV1RaU0RiNktnYUYy?=
 =?utf-8?B?RUk5eFB5VGJiQkRoRXFvR3FQN1dOdHB1UXJXb0tERk14d2VDNG9lQmlQdzl4?=
 =?utf-8?B?c2Q3eURrTFk5YW9EUWdKYkEzSnljOGh5TDhQRG4rZVppTUlHNFBsYnYweVFU?=
 =?utf-8?B?b1oyWHcrbGE0UVBReUcyMHhldDNKT25NUjRkYjllVFp2WTdmL1pWSTJHclBn?=
 =?utf-8?B?OHdCVXRPTUp0ZmhkeEdWckp4V1JXWGRhYk4vOER2cHVGS05kTXRRc25zSGp0?=
 =?utf-8?B?N0hyeVNxVk9FRjdOY1d1UjlwMktzVUJDMCthOFM5Vk5Ibk5sOHBLQkQvYUhJ?=
 =?utf-8?B?UWovUzFEVzVWOGNSNldubVlkUmNxUDBmdkxERGk2dFNyRWFoMXNtdmQyRDhP?=
 =?utf-8?B?ZDRUdzU0b3RNVTl4UGVFQU5sS2JoZWVJRTFJamVmSi9aRkV6elk0M0NjWERr?=
 =?utf-8?B?OTJIbm94NHRnMEJFWnJPUDVzckkrNUQxN202VitnbEUwRVVlN3BvZVprVVl0?=
 =?utf-8?B?NzRPUGtET05nVkdxMEJkS1VCSm4weGFaMzRlVW9UamkvKzFPYkZJS1dUWUJp?=
 =?utf-8?B?OFpOeVdDY0VNWXBoemxTTFdLRThyZHQ0blQwQ01qRkNwQXhYME56VHFMY1hV?=
 =?utf-8?B?UFlXNk5penVwaEFMOG5wZ1B0WTdhcDV2Tk0yTHkzNnpGcXd0WXlPU0JrN1Zw?=
 =?utf-8?B?SFJQVWdHZ0UrN0tkYVRGV1EyMXVvZEtuT1BUQ2NFSWZWY2RZeVdjVVh0dm1v?=
 =?utf-8?B?S2pWYUQ0d1Ziczh2cUUrZFc3cDZEUHdsYnhlaTRzN0kwcWxIZmE0MHBEOVpX?=
 =?utf-8?B?Vk5uZytFMG9TT1dGSk54aTZJQmxyeFVLbGVkMloySDZBWVMwSVVrZ1lDb3h0?=
 =?utf-8?B?bVJpWDhVWlNSWnBNKy9CQWRqUGxXQVJRS0VyVHRLci9oU2RxM0NQN2pBUDMx?=
 =?utf-8?B?YjN0TGJuRDBJNnhlTnRXTEhadi9yZU02Q2p6elYyYkdEMXpna0oxbWVONXBV?=
 =?utf-8?B?cWs1bmxyTXh1Z3RhQ2JsdEk2S2ZMRW9sSVZtUWIvSzdlZVEvZ3gvcFJJYnU5?=
 =?utf-8?B?dTdtbDQ4V21EOVdYM3d5dWRjdzVpWEVrU205Uno4TTNRZnZTMjNEQVRzRFFJ?=
 =?utf-8?B?elkzK3pudml2eitLN3k1Z1RDTm1JWWthbW1lN0VCSFB0THV5R3RwSjhWUGp3?=
 =?utf-8?B?MTFaY25hK2lNNHVLTVh6L2RUdTZpdW8zTldIeTBzZGFqa1V2NnVNSzRvemNm?=
 =?utf-8?B?NGVOeGgvREp0ZHk2M0taZU52SHZOaFBpWW5NbHRKQ3QvYTM0dGg4MTcrdTVa?=
 =?utf-8?B?OE9wVHVkN0FXM24zakprMUl4ZGRZTVdua1pKd0NTTUlVRUlNUFFEbzJJRU9N?=
 =?utf-8?B?SXdNSnpRaTcwWXUwbSttVGQzTzdpNE1EYmVPU2ZQWDlFejBTbW15MFFpZWlS?=
 =?utf-8?B?ckJSWGlacENPVEQ5MHFmSXZ0dGJDQ2Q3SW0yT3dua0hFUW1TdTZSbk00blRE?=
 =?utf-8?B?YWRabGVRUHJaUWtkV0NNNnQ3UEtNTU44MUcyS0hGWW5YVTdGMXFPc2FqMlpo?=
 =?utf-8?B?SkNHZE9xY3ZMSWl6bGwyNkZoZlhGOWdQV284ZFowaDExRWxzOExTUXVFTm9h?=
 =?utf-8?B?Nm5CNno5OFJuSW00ak1Wa0dYMlZCdkxoSmk0Z1N2Q012NnpZUWpXallUNFV5?=
 =?utf-8?B?WmFHcnZ6by83Q1RBekVyZlZTOHh0OXIydEVmcGxDeEtZelFUY0UvTzAyNGpy?=
 =?utf-8?Q?PMXIk3V6YvvN6BpI441zyKY=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 114a9b08-ef83-401a-e35f-08d9986f2eec
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1201MB0192.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Oct 2021 10:55:54.8732
 (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: ZTmjoxK5oy8VSX1xnElsDtC95Zvoi4PshM+1sZSiRKoI8pC/fu0qYsN/Cq8lK6eO
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR12MB4570
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 26.10.21 um 10:34 schrieb Arnd Bergmann:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The new driver incorrectly unwinds after errors, as clang points out:
>
> drivers/dma-buf/st-dma-resv.c:295:7: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>                  if (r) {
>                      ^
> drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here
>          while (i--)
>                 ^
> drivers/dma-buf/st-dma-resv.c:295:3: note: remove the 'if' if its condition is always false
>                  if (r) {
>                  ^~~~~~~~
> drivers/dma-buf/st-dma-resv.c:288:6: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>          if (r) {
>              ^
> drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here
>          while (i--)
>                 ^
> drivers/dma-buf/st-dma-resv.c:288:2: note: remove the 'if' if its condition is always false
>          if (r) {
>          ^~~~~~~~
> drivers/dma-buf/st-dma-resv.c:280:10: note: initialize the variable 'i' to silence this warning
>          int r, i;
>                  ^
>                   = 0
>
> Skip cleaning up the bits that have not been allocated at this point.
>
> Fixes: 1d51775cd3f5 ("dma-buf: add dma_resv selftest v4")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I already send out a patch to fix this up, but forgot to fix both gotos.

Going to add my rb and using that one here instead.

Thanks,
Christian.

> ---
> I'm not familiar with these interfaces, so I'm just guessing where
> we should jump after an error, please double-check and fix if necessary.
> ---
>   drivers/dma-buf/st-dma-resv.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/st-dma-resv.c b/drivers/dma-buf/st-dma-resv.c
> index 6f3ba756da3e..bc32b3eedcb6 100644
> --- a/drivers/dma-buf/st-dma-resv.c
> +++ b/drivers/dma-buf/st-dma-resv.c
> @@ -287,7 +287,7 @@ static int test_get_fences(void *arg, bool shared)
>   	r = dma_resv_lock(&resv, NULL);
>   	if (r) {
>   		pr_err("Resv locking failed\n");
> -		goto err_free;
> +		goto err_resv;
>   	}
>   
>   	if (shared) {
> @@ -295,7 +295,7 @@ static int test_get_fences(void *arg, bool shared)
>   		if (r) {
>   			pr_err("Resv shared slot allocation failed\n");
>   			dma_resv_unlock(&resv);
> -			goto err_free;
> +			goto err_resv;
>   		}
>   
>   		dma_resv_add_shared_fence(&resv, f);
> @@ -336,6 +336,7 @@ static int test_get_fences(void *arg, bool shared)
>   	while (i--)
>   		dma_fence_put(fences[i]);
>   	kfree(fences);
> +err_resv:
>   	dma_resv_fini(&resv);
>   	dma_fence_put(f);
>   	return r;