Re: [PATCH][RFC 2] char: Add Dell Systems Management Base driver

[randy_dunlap <rdunlap@xenotime.net>]

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