Messages in this thread | | | Date | Sun, 26 Jun 2005 20:10:55 -0700 | From | randy_dunlap <> | Subject | Re: [PATCH][RFC 2] char: Add Dell Systems Management Base driver |
| |
On Sun, 26 Jun 2005 18:05:44 -0500 Doug Warzecha wrote:
| diff -uprN linux-2.6.12-rc6.orig/drivers/char/dcdbas.c linux-2.6.12-rc6/drivers/char/dcdbas.c | --- linux-2.6.12-rc6.orig/drivers/char/dcdbas.c 1969-12-31 18:00:00.000000000 -0600 | +++ linux-2.6.12-rc6/drivers/char/dcdbas.c 2005-06-26 17:48:57.239013248 -0500 | @@ -0,0 +1,686 @@ | + | +#define DRIVER_NAME "dcdbas" | +#define DRIVER_VERSION "5.6.0-1" | +#define DRIVER_DESCRIPTION "Systems Management Base Driver" | + | +static int driver_major; | +static atomic_t hold_on_shutdown;
Why does hold_on_shutdown need to be atomic_t?
| +/** | + * dcdbas_dispatch_ioctl - dispatch IOCTL request | + * @ireq: IOCTL request | + */ | +static int dcdbas_dispatch_ioctl(struct dcdbas_ioctl_req *ireq) | +{ | + int retval = 0;
Is it OK that lots of these don't alter retval for their return status?
| + pr_debug("%s: req_type: %u\n", __FUNCTION__, ireq->hdr.req_type); | + | + switch (ireq->hdr.req_type) { | + 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; | + | + case ESM_TVM_ALLOC_MEM: | + if (down_interruptible(&tvm_lock)) | + return -ERESTARTSYS; | + ireq->hdr.status = tvm_alloc_dma_buf(&ireq->data.tvm_mem_alloc); | + up(&tvm_lock); | + break; | + | + case ESM_TVM_HC_ACTION: | + if (down_interruptible(&tvm_lock)) | + return -ERESTARTSYS; | + ireq->hdr.status = tvm_set_hc_action(&ireq->data.tvm_hc_action); | + up(&tvm_lock); | + break; | + | + case ESM_CALLINTF_REQ: | + retval = callintf_generate_smi(ireq); | + break; | + | + case ESM_HOLD_OS_ON_SHUTDOWN: | + /* firmware is going to perform host control action */ | + atomic_set(&hold_on_shutdown, 1); | + ireq->hdr.status = ESM_STATUS_CMD_SUCCESS; | + break; | + | + case ESM_CANCEL_HOLD_OS_ON_SHUTDOWN: | + atomic_set(&hold_on_shutdown, 0); | + ireq->hdr.status = ESM_STATUS_CMD_SUCCESS; | + break; | + | + default: | + pr_debug("%s: unsupported req_type\n", __FUNCTION__); | + ireq->hdr.status = ESM_STATUS_CMD_NOT_IMPLEMENTED; | + break; | + } | + | + return retval; | +} | + | +/** | + * dcdbas_do_ioctl - process ioctl request | + * @filp: file object for device | + * @cmd: IOCTL command | + * @arg: IOCTL request data | + */ | +static int dcdbas_do_ioctl(struct file *filp, unsigned int cmd, | + unsigned long arg) | +{ | + struct dcdbas_ioctl_req *ubuf = (struct dcdbas_ioctl_req *)arg;
Add __user annotations? and check them with sparse.
| + struct dcdbas_ioctl_req *kbuf = NULL; | + struct dcdbas_ioctl_hdr hdr; | + unsigned long size; | + int ret; | +}
| +/** | + * dcdbas_reboot_notify - handle reboot notification | + * @nb: info about registered reboot notifier | + * @code: notification code | + * @unused: unused argument | + */ | +static int dcdbas_reboot_notify(struct notifier_block *nb, unsigned long code, | + void *unused) | +{ | + static unsigned int notify_cnt = 0; | + | + switch (code) { | + case SYS_DOWN: | + case SYS_HALT: | + case SYS_POWER_OFF: | + if (atomic_read(&hold_on_shutdown)) { | + /* firmware is going to perform host control action */ | + if (++notify_cnt == 2) { | + printk(KERN_WARNING | + "Please wait for shutdown " | + "action to complete...\n"); | + dcdbas_host_control(); | + } | + register_reboot_notifier(nb);
Why call register_reboot_notifier() again?
| + } | + break; | + } | + | + return NOTIFY_DONE; | +}
| diff -uprN linux-2.6.12-rc6.orig/drivers/char/dcdbas.h linux-2.6.12-rc6/drivers/char/dcdbas.h | --- linux-2.6.12-rc6.orig/drivers/char/dcdbas.h 1969-12-31 18:00:00.000000000 -0600 | +++ linux-2.6.12-rc6/drivers/char/dcdbas.h 2005-06-26 15:06:08.000000000 -0500 | @@ -0,0 +1,161 @@ | +/* | + * IOCTL status values | + */ | +#define ESM_STATUS_CMD_UNSUCCESSFUL (-1) | +#define ESM_STATUS_CMD_SUCCESS (0) | +#define ESM_STATUS_CMD_NOT_IMPLEMENTED (1) | +#define ESM_STATUS_CMD_BAD (2) | +#define ESM_STATUS_CMD_TIMEOUT (3) | +#define ESM_STATUS_CMD_NO_SUCH_DEVICE (7) | +#define ESM_STATUS_CMD_DEVICE_BAD (9)
Why is CMD_UNSUCCESSFUL the only IOCTL status value that is negative? IOW, could all of them except for CMD_SUCCESS be negative?
--- ~Randy - 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/
| |