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;
Date:   Thu, 27 Jan 2022 08:53:49 +0000
From:   Phillip Potter <phil@philpotter.co.uk>
To:     Pavel Skripkin <paskripkin@gmail.com>
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
Subject: Re: [PATCH 06/10] staging: r8188eu: remove DBG_88E calls from
 os_dep/ioctl_linux.c
Message-ID: <YfJdnTKa/gF9TEV5@equinox>
References: <20220124224415.831-1-phil@philpotter.co.uk>
 <20220124224415.831-7-phil@philpotter.co.uk>
 <a681a562-eb41-9dc8-703a-5b235c15a3a1@gmail.com>
 <YfCgQbFxfVZXw9Fg@equinox>
 <b5bbe488-ac43-fd89-7c65-36bfa9c903a6@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b5bbe488-ac43-fd89-7c65-36bfa9c903a6@gmail.com>
Precedence: bulk

On Wed, Jan 26, 2022 at 01:26:08PM +0300, Pavel Skripkin wrote:
> 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.
> 

Yeah, not a bad idea - I've tried to remove only those which don't look
like they'd have side effects (such as fwstate checks etc.), but by all
means I can put them back in with the next revision. Thanks.

Regards,
Phil