On Tue, Jul 26, 2016 at 07:05:37PM -0700, k...@exchange.microsoft.com wrote:
> +/*
> + * Create a char device that can support read/write for passing
> + * the payload.
> + */
That sounds "interesting"...
> +
> +static struct completion ip_event;
> +static bool opened;
> +
> +char hvnd_ip_addr[4];
> +char hvnd_mac_addr[6];
> +bool hvnd_addr_set;
Global variables?
> +
> +int hvnd_get_ip_addr(char **ip_addr, char **mac_addr)
> +{
> + int t;
> +
> + /*
> + * Now wait for the user level daemon to get us the
> + * IP addresses bound to the MAC address.
> + */
> + if (!hvnd_addr_set) {
> + t = wait_for_completion_timeout(&ip_event, 600*HZ);
> + if (t == 0)
> + return -ETIMEDOUT;
> + }
> +
> + if (hvnd_addr_set) {
> + *ip_addr = hvnd_ip_addr;
> + *mac_addr = hvnd_mac_addr;
> + return 0;
> + }
> +
> + return -ENODATA;
> +}
> +
> +static ssize_t hvnd_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + char input[120];
> + int scaned, i;
> + unsigned int mac_addr[6], ip_addr[4];
> +
> + if (hvnd_addr_set) {
> + hvnd_error("IP/MAC address already set, ignoring input\n");
> + return count;
> + }
> +
> + if (count > sizeof(input)-1)
> + return -EINVAL;
> +
> + if (copy_from_user(input, buf, count))
> + return -EFAULT;
> +
> + input[count] = 0;
> +
> + /*
> + * Wakeup the context that may be waiting for this.
> + */
> + hvnd_debug("get user mode input: %s\n", input);
> +
> + scaned = sscanf(input,
> + "rdmaMacAddress=\"%x:%x:%x:%x:%x:%x\"
> rdmaIPv4Address=\"%u.%u.%u.%u\"",
> + &mac_addr[0],
> + &mac_addr[1],
> + &mac_addr[2],
> + &mac_addr[3],
> + &mac_addr[4],
> + &mac_addr[5],
> + &ip_addr[0],
> + &ip_addr[1],
> + &ip_addr[2],
> + &ip_addr[3]);
Oh, that's a mess, you are going to parse text in the kernel that is
passed on a char device? Please tell me that not all IB drivers are
like this...
> +
> + if (scaned == 10) {
> +
> + for (i = 0; i < 6; i++)
> + hvnd_mac_addr[i] = (char) mac_addr[i];
> + for (i = 0; i < 4; i++)
> + hvnd_ip_addr[i] = (char) ip_addr[i];
> +
> + hvnd_error("Scanned IP address: %pI4 Mac address: %pM\n",
> + hvnd_ip_addr, hvnd_mac_addr);
> +
> + hvnd_addr_set = true;
> + complete(&ip_event);
> + }
> +
> + return count;
> +}
> +
> +static int hvnd_open(struct inode *inode, struct file *f)
> +{
> + /*
> + * The user level daemon that will open this device is
> + * really an extension of this driver. We can have only
> + * active open at a time.
Do you have a pointer to that code? As it's a logical extension, you
know what the license for that code better be... :)
> + */
> + if (opened)
> + return -EBUSY;
You just raced, and lost, oops :(
There are better ways to do this, the easiest being, why do you need
"exclusive" access at all?
> +
> + /*
> + * The daemon is alive; setup the state.
> + */
> + opened = true;
> + return 0;
> +}
> +
> +static int hvnd_release(struct inode *inode, struct file *f)
> +{
> + /*
> + * The daemon has exited; reset the state.
> + */
> + opened = false;
> + return 0;
> +}
> +
> +
> +static const struct file_operations hvnd_fops = {
> + .write = hvnd_write,
> + .release = hvnd_release,
> + .open = hvnd_open,
> +};
> +
> +static struct miscdevice hvnd_misc = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "hvnd_rdma",
> + .fops = &hvnd_fops,
> +};
> +
> +static int hvnd_dev_init(void)
> +{
> + init_completion(&ip_event);
> + return misc_register(&hvnd_misc);
> +}
> +
> +static void hvnd_dev_deinit(void)
> +{
> +
> + /*
> + * The device is going away - perhaps because the
> + * host has rescinded the channel. Setup state so that
> + * user level daemon can gracefully exit if it is blocked
> + * on the read semaphore.
> + */
> + opened = false;
But if it's blocked, it's not going to get unblocked here :(
> + /*
> + * Signal the semaphore as the device is
> + * going away.
> + */
> + misc_deregister(&hvnd_misc);
> +}
Your comment doesn't match the code you are calling.
I gave up here, sorry.
Exactly why do you want a char interface? It looks like you are using
it to configure your "hardware", surely there is already other ways to
do this and not every driver needs to roll-their-own like this?
thanks,
greg k-h
