Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 2/2] uacce: add uacce driver
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Arnd Bergmann <arnd@arndb.de>, jonathan.cameron@huawei.com,
        kenneth-lee-2012@foxmail.com, Wangzhou <wangzhou1@hisilicon.com>,
        linux-accelerators@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        Kenneth Lee <liguozhu@hisilicon.com>,
        Zaibo Xu <xuzaibo@huawei.com>
References: <1566998876-31770-1-git-send-email-zhangfei.gao@linaro.org>
 <1566998876-31770-3-git-send-email-zhangfei.gao@linaro.org>
 <20190828152201.GA10163@kroah.com>
 <5c2b0889-ea05-1ecd-fe5b-40611bd31945@linaro.org>
 <20190829095439.GA13915@kroah.com>
From:   zhangfei <zhangfei.gao@linaro.org>
Message-ID: <0fe1ac96-4332-45f2-ba1e-2283420b65fe@linaro.org>
Date:   Fri, 30 Aug 2019 16:48:53 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20190829095439.GA13915@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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).
Yes, cdev's refcount is increased by open, from user space.

Not sure whether I understand correctly, Is this correct?

@@ -819,9 +819,10 @@ static int uacce_create_chrdev(struct uacce *uacce)
         if (ret < 0)
                 return ret;

-       cdev_init(&uacce->cdev, &uacce_fops);
+       uacce->cdev = cdev_alloc();
+       uacce->cdev->ops = &uacce_fops;
         uacce->dev_id = ret;
-       uacce->cdev.owner = THIS_MODULE;
+       uacce->cdev->owner = THIS_MODULE;
         device_initialize(&uacce->dev);
         uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
         uacce->dev.class = uacce_class;
@@ -829,7 +830,7 @@ static int uacce_create_chrdev(struct uacce *uacce)
         uacce->dev.parent = uacce->pdev;
         uacce->dev.release = uacce_release;
         dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
-       ret = cdev_device_add(&uacce->cdev, &uacce->dev);
+       ret = cdev_device_add(uacce->cdev, &uacce->dev);
         if (ret)
                 goto err_with_idr;

@@ -843,7 +844,7 @@ static int uacce_create_chrdev(struct uacce *uacce)

  static void uacce_destroy_chrdev(struct uacce *uacce)
  {
-       cdev_device_del(&uacce->cdev, &uacce->dev);
+       cdev_device_del(uacce->cdev, &uacce->dev);
         put_device(&uacce->dev);
  }

diff --git a/include/linux/uacce.h b/include/linux/uacce.h
index 1892b94..39a2c4b 100644
--- a/include/linux/uacce.h
+++ b/include/linux/uacce.h
@@ -155,7 +155,7 @@ struct uacce {
         struct device *pdev;
         bool is_vf;
         u32 dev_id;
-       struct cdev cdev;
+       struct cdev *cdev;

And use struct uacce_device instead of struct uacce

-struct uacce *uacce_register(struct device *parent,
-                            struct uacce_interface *interface);
+struct uacce_device *uacce_register(struct device *parent,
+                                   struct uacce_interface *interface);

>
>> They are allocated with uacce when uacce_register and freed when
>> uacce_unregister.
> And that will not work.
>
>> To make it clear, how about adding this.
>>
>> +static void uacce_release(struct device *dev)
>> +{
>> +       struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
>> +
>> +       idr_remove(&uacce_idr, uacce->dev_id);
>> +       kfree(uacce);
>> +}
>> +
>>   static int uacce_create_chrdev(struct uacce *uacce)
>>   {
>>          int ret;
>> @@ -819,6 +827,7 @@ static int uacce_create_chrdev(struct uacce *uacce)
>>          uacce->dev.class = uacce_class;
>>          uacce->dev.groups = uacce_dev_attr_groups;
>>          uacce->dev.parent = uacce->pdev;
>> +       uacce->dev.release = uacce_release;
> You have to have a release function today, otherwise you will get nasty
> kernel messages from the log.  I don't know why you aren't seeing that
> already.
Yes, kernel report warning after using put_device.
>
>>          dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
>>          ret = cdev_device_add(&uacce->cdev, &uacce->dev);
>>          if (ret)
>> @@ -835,7 +844,7 @@ static int uacce_create_chrdev(struct uacce *uacce)
>>   static void uacce_destroy_chrdev(struct uacce *uacce)
>>   {
>>          cdev_device_del(&uacce->cdev, &uacce->dev);
>> -       idr_remove(&uacce_idr, uacce->dev_id);
>> +       put_device(&uacce->dev);
>>   }
>>
>>   static int uacce_dev_match(struct device *dev, void *data)
>> @@ -1042,8 +1051,6 @@ void uacce_unregister(struct uacce *uacce)
>>          uacce_destroy_chrdev(uacce);
>>
>>          mutex_unlock(&uacce_mutex);
>> -
>> -       kfree(uacce);
>>   }
>>
>>
>> uacce_destroy_chrdev->put_device(&uacce->dev)->uacce_release->kfree(uacce).
>>
>> And find there are many examples in driver/
>> $ grep -rn cdev_device_add drivers/
>> drivers/rtc/class.c:362:        err = cdev_device_add(&rtc->char_dev,
>> &rtc->dev);
>> rivers/gpio/gpiolib.c:1181:    status = cdev_device_add(&gdev->chrdev,
>> &gdev->dev);
>> drivers/soc/qcom/rmtfs_mem.c:223:       ret =
>> cdev_device_add(&rmtfs_mem->cdev, &rmtfs_mem->dev);
>> drivers/input/joydev.c:989:     error = cdev_device_add(&joydev->cdev,
>> &joydev->dev);
>> drivers/input/mousedev.c:907:   error = cdev_device_add(&mousedev->cdev,
>> &mousedev->dev);
>> drivers/input/evdev.c:1419:     error = cdev_device_add(&evdev->cdev,
>> &evdev->dev);
> Are you sure these all have the full structures inbedded in them?
>
>> like drivers/input/evdev.c,
>> evdev is alloced with initialization of dev and cdev,
>> and evdev is freed in release ops evdev_free
>> struct evdev {
>>          struct device dev;
>>          struct cdev cdev;
>>          ~
> Ick, that too is totally wrong and needs to be fixed.
>
> Please don't copy incorrect code, that's why we review stuff :)
>
OK, understand. Thanks Greg.