On Tue, Feb 13, 2018 at 11:17:42PM +0100, Pali Rohár wrote: > Hi! This would be ratter longer email, but I hope I write everything > needed. > Hi! Sorry for the late reply. > On Tuesday 13 February 2018 22:32:14 Laszlo Toth wrote: > > Answer to Pali from the prev. thread (before splitting): > > > > "Hi! > > I agree, there are a lot of 0s just to overwrite a 0 with 0 > > after memset. > > So filling the buffer args could be improved regardless of my patch, > > after that the merged function wouldn't have so many. > > But for now I just changed them to one call. This way it won't have > > to pass buffer ptr twice to 2 functions "chained" together and > > already full of params to send a request. > > Main problem is that I still do not see a big benefit of it. > > Or to say it in opposite way, why it is worse to have two functions and > need to pass buffer pointer to both functions (for obvious reason) as a > first parameter? > I think it's easier to overlook some values like the 0x1 -> 0 case after &buffer. Not a big benefit though, I agree. > > There is one called and job done. > > Apparently job is not done. There are also output parameters filled by > SMM (remote side) which caller wants to inspect. Job is: prepare, > fill, execute, fetch and process (check output/result). > I meant job as sending. Point was that currently sending is a fill + send, so it's two functions for one thing to do. > > You're right in general, but this one is kinda special: > > it does what you want _and_ holds the values you need to do that, too. > > So this isn't a heavily used and modified function call I think, more > > like a define without an actual define. > > > > With these in mind I think it's better like this, you just check the > > last line and done. That's how I found the typo in the other commit > > as after &buffer it was invisible. I thought it might help." > > If you are going to write a new call or modify existing (for any reason) > you need to look at every one argument in that function. And if you want > to understand what is that function doing (again for any reason, > including "verification that code is written correctly"), you need to map > each positional argument to its meaning. > I don't see the problem here. It's separated to multiple lines, we can check them easily when writing new ones. Moreover you have to do this now as well I think. > And I think generally functions with lot of parameters are reading > harder then function with few (3-4 arguments). > Agreed. > We can also open another question, why your proposed function takes > buffer structure which has members for input arguments, but also takes > input arguments as function parameters? Seems a bit illogical and > probably can confuse some people. > > Good point. > Just another look at it: > > Buffer consists of 3 parts: > > 1) input parameters > 2) output parameters > 3) type of the SMM call (selector/class or how it is called in Dell terminology) > > For sending current buffer to SMM there is a one function for it. And > part of it you specify also type of the SMM call. > > Another (helper) function was created to fill input parameters into > buffer, so user would not need to manually clear buffer (via memset) and > then assign needed parameters. > > And until now there was no need to create (helper) function to do > something with output parameters. If somebody needs them, just access > it. > > I think this is just classic decomposition for solving some problem into > smaller pieces (fill input parameters, send request). > > It can be compared to problem "write buffer to file". You have name of > file, access mode for file, permissions (in case to create new file), > buffer which you want to write and size of buffer. But you do not have > one function which do everything and takes lot of parameters. Rather you > have a function to create or open file and function to send data to > opened file. For each logic one function. > > > > And another point of view: > > Looking at code > > dell_send_request(&buffer, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT); > > I can deduce just from function names and parameters that it sends some > buffer to keyboard backlight service. Without reading implementation of > that function. > > After your change there is a code > > dell_send_request(&buffer, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT, 0, 0, 0, 0); > > I can just deduce it does something with buffer, probably send it into > keyboard backlight service, but I have absolutely no idea what are there > doing four zeros and why there are needed. > > To make it more readable I need to do something like this: > > #define ARGUMENT_1_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0 > #define ARGUMENT_2_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0 > #define ARGUMENT_3_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0 > #define ARGUMENT_4_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0 > > dell_send_request( > &buffer, > CLASS_KBD_BACKLIGHT, > SELECT_KBD_BACKLIGHT, > ARGUMENT_1_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION, > ARGUMENT_2_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION, > ARGUMENT_3_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION, > ARGUMENT_4_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION > ); > > Then I can deduce from code that it send request for asking feature > information about keyboard backlight. And also I know what are > arguments, what is definition of SMM request. > > But it is too verbose, long and unusual. > > If I want to prevent writing such verbose code, I can logically split > one function call into more. And one logic part of this are input > arguments. > You're right, mine won't be better. The two functions look like the less bad thing now. Please ignore the patch. Maybe I'll take another look at it if I have some spare time. Thanks for the detailed reply, Laszlo > -- > Pali Rohár > pali.rohar@xxxxxxxxx
