On Sun, Jun 26, 2005 at 06:05:44PM -0500, Doug Warzecha wrote:
> This patch adds the Dell Systems Management Base driver.
>
> The Dell Systems Management Base driver is a character driver that
> implements ioctls for Dell systems management software to use to
> communicate with the driver.
Ugh. Why new ioctls? Can't most of these be done other ways? Like
sysfs or open/write/read/close?
Adding new ioctls is generally frowned apon greatly.
> +static struct pci_dev tvm_dev;
Woah, you are creating your own pci_dev? No. Only the pci core can do
that. Lots of bad things happen if you are doing this. I'm amazed that
your code even works this way, please fix it.
> + pr_debug("%s: phys: %x size: %u\n",
> + __FUNCTION__, tvm_dma_buf_phys_addr, tvm_dma_buf_size);
Use dev_dbg() and friends instead of pr_debug(). Also, use dev_err()
and dev_warn() for your other calls to printk().
> + case ESM_TVM_READ_MEM:
> + if (down_interruptible(&tvm_lock))
> + return -ERESTARTSYS;
> + ireq->hdr.status = tvm_read_dma_buf(&ireq->data.tvm_mem_read);
> + up(&tvm_lock);
> + break;
> +
> + case ESM_TVM_WRITE_MEM:
> + if (down_interruptible(&tvm_lock))
> + return -ERESTARTSYS;
> + ireq->hdr.status = tvm_write_dma_buf(&ireq->data.tvm_mem_write);
> + up(&tvm_lock);
> + break;
Can't these two calls be done by lookin in the existing /proc/iomem
file? Or, if not that one, some other /proc/*mem file? Not sure
exactly what you are doing with this read/write stuff...
> + case ESM_TVM_ALLOC_MEM:
> + if (down_interruptible(&tvm_lock))
> + return -ERESTARTSYS;
Why use down_interruptible() everywhere? Can this stuff be called from
an interrupt somehow?
> +static int dcdbas_do_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct dcdbas_ioctl_req *ubuf = (struct dcdbas_ioctl_req *)arg;
> + struct dcdbas_ioctl_req *kbuf = NULL;
> + struct dcdbas_ioctl_hdr hdr;
> + unsigned long size;
> + int ret;
So, any user can do any of these ioctl calls? Isn't that a little bit
insecure?
Also, please run this through sparse and fix the warnings it give you.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
