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