On Tue, Dec 04, 2018 at 08:13:10AM +0000, Cheng Fei Phung wrote: > > > For further details, please refer to https://github.com/KastnerRG/riffa/pull/31 > > That is not permanent, please provide the details here. > > This patch helps to enable bi-directional PCIe communication at PCIe gen2 speed grade > Major change in this patch is the enabling of chnl_recv() scatter-gather list first in the case of loopback, > as you can see in https://github.com/promach/riffa/blob/full_duplex/driver/linux/riffa_driver.c#L832-L836 > > Some other changes include splitting TX and RX into two separate FSMs in two always blocks as you can see in chnl_tester.v > Also, I need to give credit to @marzoul for maintaining the kernel functions which I have already included in this patch altogether. > > However, this patch degrades bandwidth measurement result with respect to Xillybus > and riffa original driver (slower with an approximate factor of 3 at the same given data batch length) Put all of this in your patch description, and again, properly line-wrap your lines at 72 columns. I'm getting tired of asking this... > > Please fix this all up and submit a v3, after at least commenting on the > > things asked previously. > > Please allows time to clear up my confusion first regarding the following > questions about subvendor ID and subdevice ID before v3 patch submission > > > > Also, you still have this line which prevents me from being able to > > accept this patch, as I talked about previously: > > > + {PCI_DEVICE(VENDOR_ID1, PCI_ANY_ID)}, > > > Do NOT just bind to all PCI devices from a vendor, that will break other > > drivers in the system. > > This line is for subvendor ID and subdevice ID. I do not think lspci helps here. What do you mean by this? You just told the kernel that ALL devices with PCI vendor id of VENDOR_ID1 (horrible description) are handled properly by this driver. I do not think that is what you really want, correct? > Could you suggest how to get around PCI_ANY_ID for subdevice ID ? > Same question for subvendor ID. Look at include/linux/pci.h. Use the PCI_DEVICE_SUB() macro here or one of the other macros defined in that file to properly describe your device as being unique and not "ALL DEVICES FROM A VENDOR". > > Also, do not use a random MAJOR number that you just picked out of no > > where, that too will break working systems and needs to be fixed. Use > > the dynamic allocation method, or better yet, just use a misc device. > > What do you exactly mean by misc device ? Look at include/linux/miscdevice.h for the details. > > How do you plan to proceed ? > > please keep the driver in the staging state until it can really match > Xillybus or Riffa original driver's bandwidth benchmark. > > I am actually a bit worried with circ_queue structure and its corresponding > push() and pull() functions which systemtap complains to have some > significant timing overhead, but to be frank, this is not the root cause of > why the full duplex version of the driver is slower. Yes, that's kind of obvious by looking at that code :) Again, fix up the driver to use the in-kernel data structures for this type of thing and you will see a much better improvement in performance. good luck! greg k-h _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
