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

[zhangfei]

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