Messages in this thread | | | Subject | Re: [PATCH v6 4/4] bus: mhi: Add userspace client interface driver | From | Hemant Kumar <> | Date | Fri, 18 Sep 2020 11:14:08 -0700 |
| |
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
| |