Fixed this kernel test robot warning:
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.
Signed-off-by: Aashish Sharma <[email protected]>
---
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++);
rptr += DMUB_RB_CMD_SIZE;
if (rptr >= rb->capacity)
--
2.35.1.723.g4982287a31-goog
On Thu, Mar 17, 2022 at 7:01 AM Aashish Sharma <[email protected]> wrote:
>
> Fixed this kernel test robot warning:
>
> 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.
>
> Signed-off-by: Aashish Sharma <[email protected]>
> ---
> 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);
also drop the volatile in the typecast
> //uint64_t volatile *p = (uint64_t volatile *)data;
The above commented out code serves no purpose and should be removed as well.
Thanks,
Guenter
> - uint64_t temp;
> uint8_t i;
>
> for (i = 0; i < DMUB_RB_CMD_SIZE / sizeof(uint64_t); i++)
> - temp = *data++;
> + (void)READ_ONCE(*data++);
>
> rptr += DMUB_RB_CMD_SIZE;
> if (rptr >= rb->capacity)
> --
> 2.35.1.723.g4982287a31-goog
>
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 <[email protected]>
> ---
> 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
Hi Paul,
On Thu, Mar 17, 2022 at 10:58 PM Paul Menzel <[email protected]> wrote:
>
> 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:
>
Uuh, sorry, I have to take the blame for that. I am guiding Aashish
through the process of submitting patches upstream, and I completely
missed that.
> 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?
>
It is not the reason to remove volatile, it is to enable removing it.
I had suggested using it, following its description " ... Ensuring
that the compiler does not fold, spindle, or otherwise * mutilate
accesses ... ", to avoid the use of volatile, and to make it obvious
from the code that the read is intentional. My apologies if that is
the wrong approach. A simpler solution would be to just remove the
temp variable if that is preferred.
> Some robots ask in their report to add a Found-by tag. If so, please add
> one.
I think that should be "Reported-by", or more specifically
Reported-by: kernel test robot <[email protected]>
>
> > Signed-off-by: Aashish Sharma <[email protected]>
> > ---
> > 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.
>
The generated code is exactly the same on x86.
Thanks,
Guenter
> > rptr += DMUB_RB_CMD_SIZE;
> > if (rptr >= rb->capacity)
>
>
> Kind regards,
>
> Paul