Re: [PATCH v2 2/2] uacce: add uacce driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Greg

On 2019/8/29 下午5:54, Greg Kroah-Hartman wrote:
On Thu, Aug 29, 2019 at 05:05:13PM +0800, zhangfei wrote:
Hi, Greg

On 2019/8/28 下午11:22, Greg Kroah-Hartman wrote:
On Wed, Aug 28, 2019 at 09:27:56PM +0800, Zhangfei Gao wrote:
+struct uacce {
+	const char *drv_name;
+	const char *algs;
+	const char *api_ver;
+	unsigned int flags;
+	unsigned long qf_pg_start[UACCE_QFRT_MAX];
+	struct uacce_ops *ops;
+	struct device *pdev;
+	bool is_vf;
+	u32 dev_id;
+	struct cdev cdev;
+	struct device dev;
+	void *priv;
+	atomic_t state;
+	int prot;
+	struct mutex q_lock;
+	struct list_head qs;
+};
At a quick glance, this problem really stood out to me.  You CAN NOT
have two different objects within a structure that have different
lifetime rules and reference counts.  You do that here with both a
'struct cdev' and a 'struct device'.  Pick one or the other, but never
both.

I would recommend using a 'struct device' and then a 'struct cdev *'.
That way you get the advantage of using the driver model properly, and
then just adding your char device node pointer to "the side" which
interacts with this device.

Then you might want to call this "struct uacce_device" :)
Here the 'struct cdev' and 'struct device' have the same lifetime and
refcount.
No they do not, that's impossible as refcounts are incremented from
different places (i.e. userspace).

They are allocated with uacce when uacce_register and freed when
uacce_unregister.
And that will not work.
I am sorry, could I ask more about this part.

  * This function should be used whenever the struct cdev and the
 * struct device are members of the same structure whose lifetime is
 * managed by the struct device.

From cdev_device_add comments, looks struct cdev and stuct device
can be in the same structure like uacce, and uacce is released when
put_device(device)

Also cdev_device_del do the device_del(dev) and cdev_del(cdev).

Copy:
fs/char_dev.c
/**
 * cdev_device_add() - add a char device and it's corresponding
 *      struct device, linkink
 * @dev: the device structure
 * @cdev: the cdev structure
 *
 * cdev_device_add() adds the char device represented by @cdev to the system,
 * just as cdev_add does. It then adds @dev to the system using device_add
 * The dev_t for the char device will be taken from the struct device which
 * needs to be initialized first. This helper function correctly takes a
 * reference to the parent device so the parent will not get released until
 * all references to the cdev are released.
 *
 * This helper uses dev->devt for the device number. If it is not set
 * it will not add the cdev and it will be equivalent to device_add.
 *
 * This function should be used whenever the struct cdev and the
 * struct device are members of the same structure whose lifetime is
 * managed by the struct device.
 *
 * NOTE: Callers must assume that userspace was able to open the cdev and
 * can call cdev fops callbacks at any time, even if this function fails.
 */
int cdev_device_add(struct cdev *cdev, struct device *dev)
{
        int rc = 0;

        if (dev->devt) {
                cdev_set_parent(cdev, &dev->kobj);

                rc = cdev_add(cdev, dev->devt, 1);
                if (rc)
                        return rc;
        }

        rc = device_add(dev);
        if (rc)
                cdev_del(cdev);

        return rc;
}

Thanks



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux