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: <b6da2cde-b489-0b9d-819f-c36cd64f9531@gmail.com>
Date:   Wed, 26 Jan 2022 13:58:52 +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:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Phillip Potter <phil@philpotter.co.uk>, 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>
 <b5bbe488-ac43-fd89-7c65-36bfa9c903a6@gmail.com> <YfEm4t+fhO4I2uDW@kroah.com>
From:   Pavel Skripkin <paskripkin@gmail.com>
In-Reply-To: <YfEm4t+fhO4I2uDW@kroah.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Greg,

On 1/26/22 13:48, Greg KH wrote:
>> 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.
> 
> No, if these were never getting called in normal operation, there's no
> need to add them back.
> 

I guess, I was not clear, sorry. I mean leave reads that were called 
during normal operations, but used only for printing debug info. (As 
Phillip has already done in v1)

Reads inside R88_DBG() and other debug macros of course should be 
removed, but other places seems dangerous without good testing. There is 
al least one place with following comment:

> 			/* Although lenc is only used in a debug statement,
> 			 * do not remove it as the rtw_read16() call consumes
> 			 * 2 bytes from the EEPROM source.
> 			 */
> 			u16 lenc = rtw_read16(adapter, REG_PKTBUF_DBG_DATA_L);

There is a chance that other places have same problem, but don't have a 
comment above it. That's why I suggested to leave all these "debug" 
reads and leave a comment for further work. It will help to easily spot 
them in future and remove or leave them with explanations why.


With regards,
Pavel Skripkin