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

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

[Salyzyn, Mark]

Responses (defenses?) embedded below preceded by an `mgs>'

Sincererly -- Mark Salyzyn

-----Original Message-----
From: linux-scsi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[mailto:linux-scsi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Arjan van de Ven
Sent: Wednesday, January 07, 2004 1:42 PM
To: Xose Vazquez Perez
Cc: linux-aacraid-devel-XtjxT7Vmt5ZskZv2Y/7f+AC/G2K4zDHf@public.gmane.org; linux-scsi
Subject: Re: [PATCH] 1.1.4-2313 aacraid driver for LiNUX 2.4.25-pre4

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

mgs> In Adaptec's version of the code, this is part of a kernel versioning
mgs> ifdef, to use the `host_lock' passed in, or the io_request_lock global
mgs> Better here than at each locale. However, I think I chose the wrong
mgs> side of the ifdef, doesn't 2.4.25-pre4 use host->host_lock?


+#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

mgs> Okely Dokely

+        *      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

mgs> If the system has less than 4G of memory, it is in the interest of
mgs> performance to use the 32 bit variants of the adapter packets. Smaller
mgs> frames, and capable of a larger number of scatter-gather elements.

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


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

mgs> The management tools issue ioctls to query for the information. RAID
mgs> drivers require management tool support.

+# 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);

mgs> this is `good enough' for the driver, s1 in the driver context is
mgs> not a value of null.

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

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

mgs> This is in here for compatibility reasons (should be in compat.h),
mgs> could be dropped as min is defined by the include framework. This
mgs> `minimal' definition is good enough for the driver since locally
mgs> both a and b are `constants'.

 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

mgs> Only the __x86_64__ architecture has the register_ioctl32_conversion
mgs> handler. This is reflection of what has been tested with the currently
mgs> available management tools. The only 64 bit architecture that has
mgs> the management tools compiled as a 64 bit applications is for the AMD64
mgs> right now. I have not yet determined how to figure out if a 32 bit
mgs> legacy app or a 64 bit updated app has issued the ioctl call to the
mgs> driver under other architectures.

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

mgs> Throwing the baby out with the bathwater? ;-/ I *know* that a bunch of
mgs> small updates would be easier to swallow, but alas the aacraid driver
mgs> in the 2.4 tree has been neglected for some time. Being at the front
mgs> line at Adaptec for bug reports against this driver, I would hope that
mgs> I would acquire a critical response and then tailor a more refined
mgs> meta-patch later.

_______________________________________________
Linux-aacraid-devel mailing list
Linux-aacraid-devel-8PEkshWhKlo@public.gmane.org
http://lists.us.dell.com/mailman/listinfo/linux-aacraid-devel
Please read the FAQ at http://lists.us.dell.com/faq or search the list archives at http://lists.us.dell.com/htdig/

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

[Arjan van de Ven]

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


On Wed, Jan 07, 2004 at 03:12:44PM -0500, Salyzyn, Mark wrote:
> +#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
> 
> mgs> In Adaptec's version of the code, this is part of a kernel versioning
> mgs> ifdef, to use the `host_lock' passed in, or the io_request_lock global
> mgs> Better here than at each locale. However, I think I chose the wrong
> mgs> side of the ifdef, doesn't 2.4.25-pre4 use host->host_lock?

there are other, cleaner ways to achieve this imo

> +        * 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
> 
> mgs> If the system has less than 4G of memory, it is in the interest of
> mgs> performance to use the 32 bit variants of the adapter packets. Smaller
> mgs> frames, and capable of a larger number of scatter-gather elements.

oh sure; imo the pci_set_dma_mask() function should check this, not the
driver.

> why on earth is that in a driver?????
> 
> mgs> The management tools issue ioctls to query for the information. RAID
> mgs> drivers require management tool support.


sounds like a really really broken ioctl interface then. Is the source of
these tools public so that we can fix them ?

> 
> +# 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);
> 
> mgs> this is `good enough' for the driver, s1 in the driver context is
> mgs> not a value of null.

ehm read again. My example was not of the foo==NULL case but that your
define had 2 statements where the "else" will only apply to the first

> mgs> This is in here for compatibility reasons (should be in compat.h),
> mgs> could be dropped as min is defined by the include framework. This
> mgs> `minimal' definition is good enough for the driver since locally
> mgs> both a and b are `constants'.

then use the exact 2.4/2.6  definition in a compat.h or so...


> mgs> Only the __x86_64__ architecture has the register_ioctl32_conversion
> mgs> handler. This is reflection of what has been tested with the currently
> mgs> available management tools. The only 64 bit architecture that has
> mgs> the management tools compiled as a 64 bit applications is for the AMD64
> mgs> right now. I have not yet determined how to figure out if a 32 bit
> mgs> legacy app or a 64 bit updated app has issued the ioctl call to the
> mgs> driver under other architectures.

yep that's what I call fishy ;)
broken ioctls for a broken interface ;(


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

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

[Xose Vazquez Perez]

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

hi,

here goes latest aacraid driver, it was sent for 2.4 kernel
inclusion. CHANGELOG has an extensive list of changes.

[-- Attachment #2: aacraid.diff.gz --]
[-- Type: application/x-gzip, Size: 42196 bytes --]

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

[Arjan van de Ven]

[-- 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 --]

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

[Christoph Hellwig]

On Wed, Jan 07, 2004 at 07:12:25PM +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.

That's a huge mess adding lots of broken crap and hooks for binary only modules
through ioctls (urgg..).

If you want to help getting this driver merged please split out reviewable
pieces and send them to linux-scsi.

_______________________________________________
Linux-aacraid-devel mailing list
Linux-aacraid-devel-8PEkshWhKlo@public.gmane.org
http://lists.us.dell.com/mailman/listinfo/linux-aacraid-devel
Please read the FAQ at http://lists.us.dell.com/faq or search the list archives at http://lists.us.dell.com/htdig/

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

[Xose Vazquez Perez]

Christoph Hellwig wrote:

> That's a huge mess adding lots of broken crap and hooks for binary only modules
> through ioctls (urgg..).

yes, but the driver was sent to Marcelo without to pass linux-scsi revision, like
others drivers. And I think that is very important a NET review before the inclusion
in kernel.org tree.

> If you want to help getting this driver merged please split out reviewable
> pieces and send them to linux-scsi.

Mark Salyzyn(adaptec) and Mark Haverkamp(osdl) already are working on it.

_______________________________________________
Linux-aacraid-devel mailing list
Linux-aacraid-devel-8PEkshWhKlo@public.gmane.org
http://lists.us.dell.com/mailman/listinfo/linux-aacraid-devel
Please read the FAQ at http://lists.us.dell.com/faq or search the list archives at http://lists.us.dell.com/htdig/

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

[Christoph Hellwig]

On Thu, Jan 08, 2004 at 04:58:50PM +0100, Xose Vazquez Perez wrote:
> Christoph Hellwig wrote:
> 
> > That's a huge mess adding lots of broken crap and hooks for binary only modules
> > through ioctls (urgg..).
> 
> yes, but the driver was sent to Marcelo without to pass linux-scsi revision, like
> others drivers. And I think that is very important a NET review before the inclusion
> in kernel.org tree.

Well, the review is pretty short.  The driver is an unreadable POS that
introduces bugs and backdoors for binary modules.

If marcelo is going to merge it anyway I'll urge him to back it out.

And who the heck thinks it's a good idea to submit scsi driver directly
instead of through the proper lists?

_______________________________________________
Linux-aacraid-devel mailing list
Linux-aacraid-devel-8PEkshWhKlo@public.gmane.org
http://lists.us.dell.com/mailman/listinfo/linux-aacraid-devel
Please read the FAQ at http://lists.us.dell.com/faq or search the list archives at http://lists.us.dell.com/htdig/