[ofw] [patch][core][l2w+genutils] Unify general utilities functions into one library and move l2w into a separate directory

[Uri Habusha]

We reconsidered this patch and decided not doing it. The library will be under mlx4\kernel directory.

-----Original Message-----
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Hefty, Sean
Sent: Saturday, February 05, 2011 12:16 AM
To: Fab Tillier; Irena Gannon; 'ofw at lists.openfabrics.org'
Subject: Re: [ofw] [patch][core][l2w+genutils] Unify general utilities functions into one library and move l2w into a separate directory

> Do we really need the genutil abstractions?  Can't we just have native
> Windows kernel code?

I agree.  If you want to start with the Linux implementation as a base, that's fine, but port the code.  Do not try to bring over the Linux kernel APIs, most of which are GPL anyway.  There is already an abstraction library in the windows stack, and that needs to go away.  We don't need a second.

Also, this patch is entirely WAY too big to even start reviewing.  All we can do is randomly pick places to look at, like these:

+static inline int pci_unmap_sg(struct pci_dev *pdev, 

+             struct scatterlist *sg,      int nents, int direction)
+{
+             UNUSED_PARAM(pdev);
+             UNUSED_PARAM(sg);
+             UNUSED_PARAM(direction);
+             return nents;
+}

This function serves absolutely no purpose.

+typedef enum _gfp {
+             __GFP_NOWARN                            = 0,         /* Suppress page allocation failure warning */
+             __GFP_HIGHMEM                          = 0,         /* high memory */
+             GFP_ATOMIC                                    = 1,         /* can't wait (i.e. DPC or higher) */
+             GFP_KERNEL                                      = 2,         /* can wait (npaged) */
+             GFP_HIGHUSER                                = 4          /* GFP_KERNEL, that can be in HIGH memory */
+}

Why are we defining Linux get free page definitions into the Windows kernel stack?

+static inline void * kmalloc( SIZE_T bsize, gfp_t gfp_mask)
+{
+             void *ptr;
+             ASSERT( KeGetCurrentIrql() <= DISPATCH_LEVEL);
+             ASSERT(bsize);
+             switch (gfp_mask) {
+                             case GFP_ATOMIC:
+                                             ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_ATOMIC );
+                                             break;
+                             case GFP_KERNEL:
+                                             ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_KERNEL );
+                                             break;
+                             case GFP_HIGHUSER:
+                                             ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_HIGH );
+                                             break;
+                             default:
+                                             cl_dbg_out("kmalloc: unsupported flag %d\n", gfp_mask);
+                                             ptr = NULL;
+                                             break;
+             }
+             return ptr;
+}

Use ExAllocatePoolWithTag directly.  Do not wrap the call using a Linux API.  The same goes for kzalloc, kfree, etc.

+static inline void setup_timer(struct timer_list *timer, void (*function)(void*), void* context)
+{
+    cl_timer_init(timer, function, context);
+}

So, we add an abstracted call around an abstracted call...?

+static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
+{
+             return cmd->sdb.length;
+}
+
+static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
+{
+             return cmd->sdb.table.nents;
+}
+
+static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
+{
+             return cmd->sdb.table.sgl;
+}

More unnecessary functions.

+#define BUFFER_SIZE   100

buffer size of what?

+uint64_t  GetTickCountInMsec();
+unsigned __int64 GetTickCountInNsec();
+uint64_t GetTimeStamp(void);
+LARGE_INTEGER TimeFromLong(ULONG HandredNanos);
+NTSTATUS Sleep(ULONG HandredNanos);

Why are these function prototypes being defined?

+// This is simply a wrapper to the LIST_ENTRY class that allows 
+// easier work with this list
+class LinkedList {

I disagree that wrapping around the list calls makes anything easier.  List functionality is already available in the tree and exists natively for kernel users.

+// A simpale static array (for now)
+class Array {

What is the purpose behind this?  C++ already provides template classes.

+++ B:/users/irena/proj1/trunk/inc/kernel/genutils/gu_defs.h (revision 6862)
...
+u32 inline CL_NTOH32( u32 x ) {

Why are complib macros being redefined in another header file, and why aren't the existing windows definition sufficient?

blah blah blah... basically, I think having any abstraction of OS specific calls is a bad idea.  It increases the amount of code and costs more to maintain.

- Sean
_______________________________________________
ofw mailing list
ofw at lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw