Hi Greg,
On 9/17/20 9:44 AM, Greg KH wrote:
On Wed, Sep 16, 2020 at 12:56:07PM -0700, Hemant Kumar wrote:
...
...
+
+static int mhi_uci_open(struct inode *inode, struct file *filp)
+{
+ struct uci_dev *udev = NULL;
+ unsigned int minor = iminor(inode);
+ int ret = -EIO;
+ struct uci_buf *buf_itr, *tmp;
+ struct uci_chan *dl_chan;
+ struct mhi_device *mhi_dev;
+ struct device *dev;
+
+ mutex_lock(&uci_idr_mutex);
+ udev = idr_find(&uci_idr, minor);
+ mutex_unlock(&uci_idr_mutex);
+ if (!udev) {
+ pr_err("uci dev: minor %d not found\n", minor);
Don't spam the kernel log for things that users can do :(
i will change it to a pr_debug, as it helps to debug why open() is failing.
+ ret = -ENODEV;
+ goto error_no_dev;
+ }
+
+ kref_get(&udev->ref_count);
Why grab a reference? What does that help with?
In case open() and driver remove() are racing, it helps to prevent use
after free of udev in open().
+
+ mhi_dev = udev->mhi_dev;
+ dev = &mhi_dev->dev;
+
+ mutex_lock(&udev->lock);
+ if (kref_read(&udev->ref_count) > 2) {
+ dev_dbg(dev, "Node already opened\n");
Nope, this is NOT doing what you think it is doing.
I told you before, do not try to keep a device node from being opened
multiple times, as it will always fail (think about passing file handles
around between programs...)
If userspace wants to do this, it will do it. If your driver can't
handle that, that's fine, userspace will learn not to do that. But the
kernel can not prevent this from happening.
This check is not returning error, instead just setting
filp->private_data = udev; and return 0; It is skipping channel prepare
and queuing of inbound buffers which was done by first open().
Also note that reading a kref value is a HUGE sign that the code is
incorrect, you should never care about the value of a reference. Maybe
if it is 0, but that's a special case...
In previous patch this was done using separate open reference count and
after removing that i was relying on udev ref count. MHI channel prepare
and buffer allocation for a give channel suppose to happen at open() and
only for first open() call.
Anyway, given that you ignored my previous review comments here, I'm
loath to keep reviewing this patch series. Please get others to review
it first before sending it back as I don't like being the only one doing
this type of work...
Thanks for reviewing my patch series Greg and help making it a better
driver!
thanks,
greg k-h
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
