1.1.4-2313 aacraid driver for LiNUX 2.4.25-pre4

PATCH In review Xose Vazquez Perez 4 participants 6 messages Jan 7, 2004 – Jan 8, 2004 2 messages not in the archive SCSI

lore

Share & tools

lore

Xose Vazquez Perez

author

2004-01-07 18:13 UTC #0 lore raw

PATCH 1/1 1.1.4-2313 aacraid driver for LiNUX 2.4.25-pre4

hi,

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Arjan van de Ven

2004-01-07 18:42 UTC #2 lore raw

Quoted 4 lines · Xose Vazquez Perez

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

Christoph Hellwig

2004-01-07 21:00 UTC #3 lore raw

In reply to a message not in the archive

Quoted 4 lines · Xose Vazquez Perez

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
[email protected]
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/

Xose Vazquez Perez

2004-01-08 15:58 UTC #4 lore raw

Quoted 2 lines · Christoph Hellwig

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.

Quoted 2 lines

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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Christoph Hellwig

2004-01-08 16:04 UTC #6 lore raw

Quoted 8 lines · Xose Vazquez Perez

Quoted 2 lines · Christoph Hellwig
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?

Xose Vazquez Perez

2004-01-08 16:36 UTC #7 lore raw

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

Quoted 2 lines · Christoph Hellwig

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

A lot of vendor's drivers maintainers, do that. And the driver is automagically rejected ;-). This is the reason why LiNUX SCSI drivers are obsolete, and some of them have important bugs.