Team, Thanks for all you hardworking on complex problem. Thanks Johnny, Tony, Nicolas, and especially Greg. Best regards, Austin -----Original Message----- From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] Sent: Saturday, September 05, 2015 1:24 AM To: Kim, Johnny Cc: Cho, Tony; devel@xxxxxxxxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx; Park, Chris; Kim, Rachel; Lee, Glen; Kim, Leo; Lee, Jude; Hwang, Robin; Shin, Austin; Noureldin, Adel; Abozaeid, Adham; Ferre, Nicolas Subject: Re: [PATCH 3/5] staging: wilc1000: use id value as argument On Fri, Sep 04, 2015 at 03:01:55PM +0900, johnny.kim wrote: > > > On 2015년 09월 04일 12:51, Greg KH wrote: > >On Fri, Sep 04, 2015 at 12:24:29PM +0900, johnny.kim wrote: > >> > >>On 2015년 09월 04일 00:47, Greg KH wrote: > >>>On Thu, Sep 03, 2015 at 04:00:08PM +0900, johnny.kim wrote: > >>>>Hello Greg. > >>>> > >>>>On 2015년 09월 03일 10:33, Greg KH wrote: > >>>>>On Thu, Aug 20, 2015 at 04:32:52PM +0900, Tony Cho wrote: > >>>>>>From: Johnny Kim <johnny.kim@xxxxxxxxx> > >>>>>> > >>>>>>The driver communicates with the chipset via the address of > >>>>>>handlers to distinguish async data frame. The SendConfigPkt > >>>>>>function gets the pointer address indicating the handlers as the > >>>>>>last argument, but this requires redundant typecasting and does not support the 64 bit machine. > >>>>>> > >>>>>>This patch adds the function which assigns ID values instead of > >>>>>>pointer representing the driver handler to the address and then > >>>>>>uses the ID instead of pointer as the last argument of > >>>>>>SendConfigPkt. The driver also gets the handler's address from > >>>>>>the ID in the data frame when it receives them. > >>>>>> > >>>>>I don't understand this code at all. You are randomly adding > >>>>>values to a list, and then assuming that you can use the index > >>>>>into that list for some type of representation? As this is a > >>>>>local list, why not just use the real variables instead of having > >>>>>a list and dealing with them in this very ackward manner? > >>>>> > >>>>>In other words, I don't see the need for the list at all, just > >>>>>use the real types here, you have all the needed information > >>>>>(hint, if you know the index, you really know the data as > >>>>>well...) > >>>>> > >>>>The value is needed to send it to chipset and to distinguish async > >>>>data packet mutually. > >>>What is the value, the index or some random pointer? > >>The value of current patch substitutes the corresponding index for > >>some random pointer(= address of device handler). > >Ok. > > > >>>>The length of the data field is 4byte and the data field has been > >>>>filled with the address of pointer so far. > >>>So the data field can just be any random number, as long as it is > >>>consistent? What does the chip do with the random number? > >>Current driver normally create a couple of network interface. > >Multiple network interfaces for the same hardware? > Yes. A chipset supports multiple network interface. > >>The driver can send some commands(data frame) to the network > >>interfaces at the same time and wait the results. Both driver and > >>chipset need unique value to distinguish whom the interface owner of > >>the commands is. > >>And the value always has same value while the interface is alive. > >But as you created the interfaces, just use a unique number for each. > >It could be a #define, as you know how many interfaces you will > >create, that's not a dynamic thing. No need to keep looking up > >something in an index and converting to a structure. > I think your opinion is right only if the driver send some commands to > chipset. The driver should look for the network interface > corresponding to the #define value to know owner of data frame which > received from chipset. Network interface structure dynamically is > allocated as 'ifconfig up' > command of shell. As a result, look-up table is needed. > > >>>>But this patch changes it to unique index value corresponding to > >>>>the address for 64bit address machine. If real type is used as > >>>>your opinion, new patch will have the same meaning with current code. > >>>index now, but was using a pointer before? That sounds like you > >>>are changing the functionality. > >>> > >>>confused, > >>There is a reserved field to distinguish the data frames in chipset. > >>Because the field has 4byte space, this patch creates the index > >>corresponding to the pointer and uses the index to input the > >>identifier in the size instead of the pointer value. > >> > >>I'm sorry, too. It's not easy to explain it in English. > >It's not easy to explain that in any language :) > Thanks for your generous mind. > >As you "know" the interfaces you create, just use a "fixed" number > >for them, and refer to them that way. No need to have an array and > >iterate over the whole array every time. > > > >There are lots of wrapper functions and pointers in this driver that > >need to be stripped away. I think you will find the end result of > >all of that work to be much simpler, and smaller. See the patches > >that I did for the driver this evening as an example, it removed > >code, and in doing so, also fixed a long-time bug. There's a lot of > >work to be done here still... > > > > > I know current driver has a lot of stripping code and is heavy . My > members is waiting to send patches after x64bit issue is closed. And I > improve this driver continuously. Ok, I really think this can be solved in a "cleaner" way, but due to all of the different layers in the code, it's hard to see how to do that at the moment. I'll apply the patch, but note that I do think this needs to be fixed "properly" later on before the code is merged out of staging. With this patch applied, there are still 2 build errors on x86_64, can you fix those up and send patches for them, and then we can turn off the CONFIG_BROKEN dependency. thanks, greg k-h ��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
