Re: [PATCH] staging: rtl8188eu: Remove an unused variable and some lines of code

[Dan Carpenter <dan.carpenter@oracle.com>]

On Fri, Jul 02, 2021 at 02:14:45PM +0200, Fabio M. De Francesco wrote:
> On Friday, July 2, 2021 10:35:21 AM CEST Dan Carpenter wrote:
> > On Fri, Jul 02, 2021 at 10:48:40AM +0300, Dan Carpenter wrote:
> > > On Thu, Jul 01, 2021 at 04:47:07PM +0200, Fabio M. De Francesco wrote:
> > > > Remove set but unused iw_operation_mode[]. Remove all the lines of
> > > > code from the function rtw_wx_set_rate, except the "return 0;" line
> > > > to not break userland code that somewhat uses this IOCTL.
> > > > 
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > [...]
> > > 
> 
> Dear Dan,
> 
> > > Just delete this whole file.  It doesn't do anything now.
> > 
> > Sorry, I meant function, not file.  *chortle*.  :P
> 
> No worries, it is clear it was unintended.
> 
> Back to the function... As you may suspect :-) I know practically nothing 
> neither of Linux device drivers or of whatever else kernel, so I take your 
> words for good. ASAP, I'll send a v2 of this patch.
> 
> However, I usually like to understand what I make (just for fun and... more).
> 
> That rtw_wx_set_rate() is the implementation of the SIOCSIWRATE IOCTL command. 
> I hope that I have not misunderstood it, have I? 

Correct.

> 
> However, we know that this function does practically nothing and then simply 
> returns 0 to the user. That's exactly the reason why I deleted all its lines 
> (except one). 

It used to do nothing in a much more complicated way before commit
1aef69ecacda ("staging: rtl8188eu: Remove function rtw_setdatarate_cmd()")

> 
> If I am a user of that command I get a "success" return code (0) and I don't  
> notice that it won't be able to set the bit rate. However everything should 
> still keep running (I suppose using the default bit rate of the hardware; who 
> really knows?). 

It will still do nothing but now instead of returning success it will
return -ENOTSUPP.  This is done in wireless_process_ioctl().

> 
> Now it's time for two questions: 
> 
> 1) what happens if that command is used by some users that (hopelessly) expect 
> the function to set the bit rate? I mean: if the function is not anymore in 
> the list of the IOCTL commands of the rtw_handlers array will still the user 
> program compile, link, and don't crash at runtime? 
> 

Userspace programs are supposed to be written so that they work with
every wifi driver, so they should be able to handle -ENOTSUPP.  If this
breaks a userspace application then we will have to change it back.  But
returning -ENOTSUPP is the correct behavior, so let's first try to do
the correct thing and then think about working around bugs in userspace
if we find them.

> 2) how should I delete the association of SIOCSIWRATE with rtw_wx_set_rate() 
> in the rtw_handlers array?
> 	- delete the entry and shift the array one position up?
> 	- set the SIOCSIWRATE entry to NULL?

The IW_HANDLER() macro puts the function in the correct position in the
array.  So just delete it.  Everything else will remain unchanged.

regards,
dan carpenter