Messages in this thread Patch in this message | | | From | "Bagalkote, Sreenivas" <> | Subject | RE: [PATCH][RELEASE] megaraid 2.10.2 Driver | Date | Mon, 22 Mar 2004 19:02:39 -0500 |
| |
Jeff,
>>For upstream, this should just be CONFIG_COMPAT I presume.
For 2.6 kernels, this would be just CONFIG_COMPAT. Or have I misunderstood your comment?
>>I don't see how this construct will work in all cases. Hence my >>CONFIG_COMPAT command above.
We saw the need for ioctl compatibility in __x86_64__ cases so far. What other cases will this not work in?
>>Bug -- always set dma mask. Do not conditionally _not_ call >>pci_set_dma_mask(), for the 64-bit case.
The code does not __not__ call pci_set_dma_mask() conditionally. It is always calling with either 64-bit or 32-bit mask.
>>ummmm what??? uxferaddr is u32. why are you casting it to a pointer?
Both copy_to_user and copy_from_user take pointers, don't they?
Thank you, Sreenivas
-----Original Message----- From: Jeff Garzik [mailto:jgarzik@pobox.com] Sent: Monday, March 22, 2004 6:08 PM To: Bagalkote, Sreenivas Cc: 'linux-kernel@vger.kernel.org'; 'linux-scsi@vger.kernel.org' Subject: Re: [PATCH][RELEASE] megaraid 2.10.2 Driver
Bagalkote, Sreenivas wrote: > Hello, > @@ -45,6 +46,10 @@ > > #include "megaraid2.h" > > +#ifdef LSI_CONFIG_COMPAT > +#include <asm/ioctl32.h> > +#endif > +
For upstream, this should just be CONFIG_COMPAT I presume.
> MODULE_AUTHOR ("LSI Logic Corporation"); > MODULE_DESCRIPTION ("LSI Logic MegaRAID driver"); > MODULE_LICENSE ("GPL"); > @@ -206,6 +211,10 @@ > */ > major = register_chrdev(0, "megadev", &megadev_fops); > > + if (!major) { > + printk(KERN_WARNING > + "megaraid: failed to register char > device.\n"); > + } > /* > * Register the Shutdown Notification hook in kernel > */ > @@ -214,6 +223,13 @@ > "MegaRAID Shutdown routine not > registered!!\n"); > } > > +#ifdef LSI_CONFIG_COMPAT > + /* > + * Register the 32-bit ioctl conversion > + */ > + register_ioctl32_conversion(MEGAIOCCMD, > megadev_compat_ioctl); > +#endif > +
ditto
> @@ -620,12 +638,15 @@ > > /* Set the Mode of addressing to 64 bit if we can */ > if((adapter->flag & BOARD_64BIT)&&(sizeof(dma_addr_t) == 8)) > { > - pci_set_dma_mask(pdev, 0xffffffffffffffffULL); > - adapter->has_64bit_addr = 1; > + if (pci_set_dma_mask(pdev, 0xffffffffffffffffULL) == > 0) > + adapter->has_64bit_addr = 1; > } > - else { > - pci_set_dma_mask(pdev, 0xffffffff); > - adapter->has_64bit_addr = 0; > + if (!adapter->has_64bit_addr) { > + if (pci_set_dma_mask(pdev, 0xffffffff) != 0) { > + printk("megaraid%d: DMA not available.\n", > + host->host_no); > + goto fail_attach; > + }
Bug -- always set dma mask. Do not conditionally _not_ call pci_set_dma_mask(), for the 64-bit case.
Minor: add ULL to the constant.
> @@ -2549,7 +2575,9 @@ > /* > * Unregister the character device interface to the driver. > */ > - unregister_chrdev(major, "megadev"); > + if (major) { > + unregister_chrdev(major, "megadev"); > + }
register_chrdev() returns a negative errno value on error, such as -EBUSY.
> @@ -4434,8 +4332,9 @@ > /* > * Get the user data > */ > - if( copy_from_user(data, (char *)uxferaddr, > - pthru->dataxferlen) > ) { > + if( copy_from_user(data, > + (char *)((ulong)uxferaddr), > + pthru->dataxferlen) ) {
ummmm what??? uxferaddr is u32. why are you casting it to a pointer?
> @@ -4460,8 +4359,8 @@ > * Is data going up-stream > */ > if( pthru->dataxferlen && (uioc.flags & UIOC_RD) ) { > - if( copy_to_user((char *)uxferaddr, data, > - pthru->dataxferlen) > ) { > + if( copy_to_user((char *)((ulong)uxferaddr), > + data, pthru->dataxferlen) ) > {
ditto
> diff -Naur old/drivers/scsi/megaraid2.h new/drivers/scsi/megaraid2.h > --- old/drivers/scsi/megaraid2.h 2004-03-22 17:28:38.000000000 -0500 > +++ new/drivers/scsi/megaraid2.h 2004-03-22 13:10:48.000000000 -0500 > #ifndef PCI_VENDOR_ID_LSI_LOGIC > #define PCI_VENDOR_ID_LSI_LOGIC 0x1000 > #endif
this can be removed.
> +#if defined (CONFIG_COMPAT) || defined ( __x86_64__) > +#define LSI_CONFIG_COMPAT > +#endif > +#ifdef LSI_CONFIG_COMPAT > +static int megadev_compat_ioctl(unsigned int, unsigned int, unsigned long, > + struct file *); > +#endif I don't see how this construct will work in all cases. Hence my CONFIG_COMPAT command above.
Jeff
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |