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

[Greg KH <gregkh@linuxfoundation.org>]

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@atmel.com>
> >>>>>>
> >>>>>>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