linux-kernel - Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <363ed3cd-2a08-26a0-31e3-2ee4d01f71c3@gmail.com>
Date:   Wed, 29 Dec 2021 15:25:26 +0100
From:   Matthias Brugger <matthias.bgg@...il.com>
To:     miles.chen@...iatek.com
Cc:     airlied@...ux.ie, chunkuang.hu@...nel.org, daniel@...ll.ch,
        dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, p.zabel@...gutronix.de
Subject: Re: [PATCH] drm/mediatek: Fix unused-but-set variable warning



On 29/12/2021 04:04, miles.chen@...iatek.com wrote:
> Hi,
> 
> On 28/12/2021 10:25, Miles Chen wrote:
>> Fix unused-but-set variable warning:
>>> drivers/gpu/drm/mediatek/mtk_cec.c:85:6: warning: variable 'tmp' set but not used [-Wunused-but-set-variable]
>>>
>>
>> Actually we ignore the value passed to mtk_cec_mask. In case of
>> mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
>>
>>
>> We are not setting CEC_32K_PDN. I wonder which side effect will it have to set
>> that bit.
> 
> I am confused about "not setting CEC_32K_PDN" part,
> in case mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);
> CEC_32K_PDN (BIT(19)) is set.
> 
> for exmaple:
> CEC_32K_PDN is BIT(19)
> PDN is BIT(16)
> say tmp = 0xffffffff;

You mean reading the register returns 0xffffffff, correct?

> 
> mask = PDN | CEC_32K_PDN;

mask = 0x48000

> val = 0 | CEC_32K_PDN;

val = 0x40000

> 
> tmp = fff6ffff, mask = 90000
> val = 80000, tmp = fffeffff
> 
> u32 tmp = readl(cec->regs + offset) & ~mask; // tmp = fff6ffff

tmp = 0xffffffff & ~(0x48000) // tmp = 0xffb7ffff

> tmp |= val & mask; // tmp = fffeffff

tmp |= 0x40000 & 0x48000 // tmp = 0xff7fffff

> writel(val, cec->regs + offset); // val = 80000, tmp = fffeffff

val = 0x40000, tmp = 0xff7fffff

> 
> in both val and tmp case, CEC_32K_PDN is set.
> 

You are right, in both cases the bit is set, but the funciton does not do what 
it is supposed to do.

With you fix:
With the mask we define bits that
a) are set to zero if not present in val
b) set to one, when part of val, independent of what the value was in the 
register read.

mtk_cec_mask(cec, CEC_CKGEN, 0 | CEC_32K_PDN, PDN | CEC_32K_PDN);


Will set CEC_32K_PDN to one and clear PDN in the register.

mtk_cec_mask(cec, RX_GEN_WD, 0, HDMI_PORD_INT_32K_CLR | RX_INT_32K_CLR |
  HDMI_HTPLG_INT_32K_CLR | HDMI_PORD_INT_32K_EN |
  RX_INT_32K_EN | HDMI_HTPLG_INT_32K_EN);

Will just clear all bits of the mask.

Without your patch, we will just write the val to the register and don't care 
what the register value was before that.

We should somehow mention that in the commit message, as it's not only about a 
not used variable, it actually has an influence on the value we write(-back) to 
the register.

Regards,
Matthias

>> Anyway, if it's the right thing to do, we should add:
>>
>> Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
> 
> I will add the Fixes tag, thanks.
>