lkml.org 
[lkml]   [2011]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 1/7] char/mei: PCI device and char driver support.
Date
On Tuesday 22 March 2011, Oren Weil wrote:
> +static ssize_t mei_read(struct file *file, char __user *ubuf,
> + size_t length, loff_t *offset)
> +{
> + int i;
> + int rets;
> + int err;
> + int if_num = iminor(file->f_dentry->d_inode);
> + struct mei_file_private *file_ext = file->private_data;
> + struct mei_cb_private *priv_cb_pos = NULL;
> + struct mei_cb_private *priv_cb = NULL;
> + struct mei_device *dev;
> +
> + if (!mei_device)
> + return -ENODEV;
> +
> + dev = pci_get_drvdata(mei_device);
> + if (if_num != MEI_MINOR_NUMBER || !dev || !file_ext)
> + return -ENODEV;
> +

I'm very confused by this code and how you support multiple
instances of PCI devices, cb_list entries and concurrent connections:

* You look up a global pointer to mei_device, when there could
be multiple PCI devices.

* You use the minor number of the chardev as an index throughout
the code, but bail out if it does not match a constant.

* You walk a list of mei_cb_private entries when you should have
just a single one for this file. In other places, you walk a list
of mei_file_private entries.

I think you should try to document your data structures and how
they relate to each other, and remove all that turn out not to
be needed in the process.

From the brief look I had at this rather complex code, I would
assume that it could look something like this:

* In the PCI probe function, create a misc character device for
every instance you find behind each device. Remove the global
variables and constants.

* Unify mei_file_private and mei_cb_private, allocate at open
time but fill when the ioctl gets called.

Would that work, or am I missing something major?

Arnd


\
 
 \ /
  Last update: 2011-03-22 18:25    [from the cache]
©2003-2011 Jasper Spaans