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;
Message-ID: <b5bbe488-ac43-fd89-7c65-36bfa9c903a6@gmail.com>
Date:   Wed, 26 Jan 2022 13:26:08 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH 06/10] staging: r8188eu: remove DBG_88E calls from
 os_dep/ioctl_linux.c
Content-Language: en-US
To:     Phillip Potter <phil@philpotter.co.uk>
Cc:     gregkh@linuxfoundation.org, dan.carpenter@oracle.com,
        Larry.Finger@lwfinger.net, straube.linux@gmail.com,
        martin@kaiser.cx, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
References: <20220124224415.831-1-phil@philpotter.co.uk>
 <20220124224415.831-7-phil@philpotter.co.uk>
 <a681a562-eb41-9dc8-703a-5b235c15a3a1@gmail.com> <YfCgQbFxfVZXw9Fg@equinox>
From:   Pavel Skripkin <paskripkin@gmail.com>
In-Reply-To: <YfCgQbFxfVZXw9Fg@equinox>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Phillip,

On 1/26/22 04:13, Phillip Potter wrote:

[snip]

}
>> 
>> And here you also removes the reads. I guess, some kind of magic pattern is
>> used
>> 
> 
> So these calls are macro arguments, they would never be executed under
> normal circumstances anyway, unless the rtw_debug kernel module was
> passed in as 5 or more - it is 1 by default. The DBG_88E macro would
> expand during preprocessing phase to (for example):
> 
> do {
> 	if (5 <= GlobalDebugLevel)
> 		pr_info("R8188EU: " "dbg(0x450) = 0x%x\n", rtw_read32(padapter, 0x450));
> } while (0)
> 
> As this is never executed under normal circumstances anyway, I would say
> calls like these are therefore safe to remove. Happy to be convinced
> though :-) Many thanks.
> 

I see your point, thanks for explanation.

Well, in this case, you may left all reads, that are executed during 
normal lifetime of a driver. We know, that there is at least 1 place, 
where read() call removal can break things. Might be there are couple of 
other places we don't know about.

IMHO the best thing you can do is to leave these reads and leave a 
comment like "hey, please remove me and test". One day useless reads 
should be anyway removed, since ideally rtw_read family must get 
__must_check annotation + normal error handling.

Thanks :)


With regards,
Pavel Skripkin