RE: [PATCH 3/5] staging: wilc1000: use id value as argument

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux