Re: [PATCH] 1.1.4-2313 aacraid driver for LiNUX 2.4.25-pre4

[Arjan van de Ven <arjanv-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]

On Wed, Jan 07, 2004 at 07:28:01PM +0100, Xose Vazquez Perez wrote:
> hi,
> 
> here goes latest aacraid driver, it was sent for 2.4 kernel
> inclusion. CHANGELOG has an extensive list of changes.

+#define aac_spin_lock_irqsave(host_lock, cpu_flags)            spin_lock_irqsave(&io_request_lock, cpu_flags)
+#define aac_spin_lock_irq(host_lock)                           spin_lock_irq(&io_request_lock)
+#define aac_spin_unlock_irqrestore(host_lock, cpu_flags)       spin_unlock_irqrestore(&io_request_lock, cpu_flags)
+#define aac_spin_unlock_irq(host_lock)                         spin_unlock_irq(&io_request_lock)
                                                                                                                        
ewwww


+#ifdef AAC_DETAILED_STATUS_INFO
 static char *aac_get_status_string(u32 status);
+#endif

there's no need for ifdefs around prototypes, they just clutter the code

+        *      Only enable DAC mode if the dma_addr_t is larger than 32
+        * bit addressing, and we have more than 32 bit addressing worth of
+        * memory and if the controller supports 64 bit scatter gather elements.
+        */
+       if( (sizeof(dma_addr_t) > 4) && (num_physpages > (0xFFFFFFFFULL >> PAGE_SHIFT)) && (dev->adapter_info.options &
+               dev->dac_support = 1;
        }

that looks broken, at least drivers should never need to check num_physpages etc etc


+static void get_sd_devname(int disknum, char *buffer)
+{
+.....


why on earth is that in a driver?????

+# define strlcpy(s1,s2,n) strncpy(s1,s2,n);s1[n-1]='\0'


this is very broken; consider

if (foo)
    bar(foo);
else
    strlcpy(foo,bar,3);


+# ifndef min
+#  define min(a,b) (((a)<(b))?(a):(b))
+# endif

that is quite broken..... arguments get evaluated multiple times



 struct fib_ioctl
 {
-       char    *fibctx;
-       int     wait;
-       char    *fib;
+       u32     fibctx;
+       s32     wait;
+#if (defined(__x86_64__))
+       u64     fib;
+#else
+       u32     fib;
+#endif
 };


that looks really fishy; __x86_64__ most certainly is not the only 64 bit architecture out there



this driver "update" appears to intruduce quite a few nastys... doesn't look like a good idea to me

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]