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: <f1ff7e95-c177-09dd-9393-d0156ce81387@molgen.mpg.de>
Date:   Fri, 18 Mar 2022 06:58:25 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] drm/amd/display: Fixed the unused-but-set-variable
 warning
Content-Language: en-US
To:     Aashish Sharma <shraash@google.com>,
        Harry Wentland <harry.wentland@amd.com>,
        Leo Li <sunpeng.li@amd.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        Pan Xinhui <Xinhui.Pan@amd.com>,
        David Airlie <airlied@linux.ie>,
        Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>,
        Meenakshikumar Somasundaram <meenakshikumar.somasundaram@amd.com>,
        Jake Wang <haonan.wang2@amd.com>,
        Anson Jacob <Anson.Jacob@amd.com>,
        Guenter Roeck <groeck@chromium.org>
Cc:     linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        amd-gfx@lists.freedesktop.org, Daniel Vetter <daniel@ffwll.ch>,
        Wayne Lin <wayne.lin@amd.com>,
        Anthony Koo <Anthony.Koo@amd.com>
References: <20220317140115.541007-1-shraash@google.com>
From:   Paul Menzel <pmenzel@molgen.mpg.de>
In-Reply-To: <20220317140115.541007-1-shraash@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Dear Aashish,


Am 17.03.22 um 15:01 schrieb Aashish Sharma:

Thank you for your patch. If you are going to send a v2, please use 
imperative mood. Maybe:

drm/amd/display: Fix unused-but-set-variable warning


> Fixed this kernel test robot warning:

Maybe:

Fix the kernel test robot warning below:

> drivers/gpu/drm/amd/amdgpu/../display/dmub/inc/dmub_cmd.h:2893:12:
> warning: variable 'temp' set but not used [-Wunused-but-set-variable]
> 
> Replaced the assignment to the unused temp variable with READ_ONCE()
> macro to flush the writes.

Replace …

Excuse my ignorance regarding `READ_ONCE()`, but is that the reason you 
remove the volatile qualifier?

Some robots ask in their report to add a Found-by tag. If so, please add 
one.

> Signed-off-by: Aashish Sharma <shraash@google.com>
> ---
>   drivers/gpu/drm/amd/display/dmub/inc/dmub_cmd.h | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dmub/inc/dmub_cmd.h b/drivers/gpu/drm/amd/display/dmub/inc/dmub_cmd.h
> index 873ecd04e01d..b7981a781701 100644
> --- a/drivers/gpu/drm/amd/display/dmub/inc/dmub_cmd.h
> +++ b/drivers/gpu/drm/amd/display/dmub/inc/dmub_cmd.h
> @@ -2913,13 +2913,12 @@ static inline void dmub_rb_flush_pending(const struct dmub_rb *rb)
>   	uint32_t wptr = rb->wrpt;
>   
>   	while (rptr != wptr) {
> -		uint64_t volatile *data = (uint64_t volatile *)((uint8_t *)(rb->base_address) + rptr);
> +		uint64_t *data = (uint64_t volatile *)((uint8_t *)(rb->base_address) + rptr);
>   		//uint64_t volatile *p = (uint64_t volatile *)data;
> -		uint64_t temp;
>   		uint8_t i;
>   
>   		for (i = 0; i < DMUB_RB_CMD_SIZE / sizeof(uint64_t); i++)
> -			temp = *data++;
> +			(void)READ_ONCE(*data++);

Did you verify, that the generated code is the same now, or what the 
differences are. If it’s different from before, you should document in 
the commit message, that it’s wanted, as otherwise, it’s an invasive 
change just to fix a warning.

>   		rptr += DMUB_RB_CMD_SIZE;
>   		if (rptr >= rb->capacity)


Kind regards,

Paul