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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
