Hi Shuo and Greg,
On 9/10/2020 9:28 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 10, 2020 at 02:19:00PM +0800, Shuo A Liu wrote:
>> On Wed 9.Sep'20 at 11:45:16 +0200, Greg Kroah-Hartman wrote:
>>> On Wed, Sep 09, 2020 at 05:08:25PM +0800, [email protected] wrote:
...
>>>> /**
>>>> * struct acrn_vm - Properties of ACRN User VM.
>>>> + * @dev: The struct device this VM belongs to
>>>> + * @list: Entry within global list of all VMs
>>>> * @vmid: User VM ID
>>>> + * @vcpu_num: Number of virtual CPUs in the VM
>>>> + * @flags: Flags (ACRN_VM_FLAG_*) of the VM. This is VM flag
>>>> management
>>>> + * in HSM which is different from the
>>>> &acrn_vm_creation.vm_flag.
>>>> */
>>>> struct acrn_vm {
>>>> - u16 vmid;
>>>> + struct device *dev;
>>>> + struct list_head list;
>>>> + u16 vmid;
>>>> + int vcpu_num;
>>>> + unsigned long flags;
>>>> };
>>>>
>>>> +struct acrn_vm *acrn_vm_create(struct acrn_vm *vm,
>>>> + struct acrn_vm_creation *vm_param);
>>>> +int acrn_vm_destroy(struct acrn_vm *vm);
>>>> +
>>>> #endif /* __ACRN_HSM_DRV_H */
>>>> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
>>>> index 28a3052ffa55..bc85a3c14f87 100644
>>>> --- a/drivers/virt/acrn/hsm.c
>>>> +++ b/drivers/virt/acrn/hsm.c
>>>> @@ -19,6 +19,8 @@
>>>>
>>>> #include "acrn_drv.h"
>>>>
>>>> +static struct miscdevice acrn_dev;
>>>> +
>>>> /*
>>>> * When /dev/acrn_hsm is opened, a 'struct acrn_vm' object is created to
>>>> * represent a VM instance and continues to be associated with the opened
>>>> file
>>>> @@ -34,14 +36,77 @@ static int acrn_dev_open(struct inode *inode, struct
>>>> file *filp)
>>>> return -ENOMEM;
>>>>
>>>> vm->vmid = ACRN_INVALID_VMID;
>>>> + vm->dev = get_device(acrn_dev.this_device);
>>>
>>> You are grabbing a reference on a static structure?
>>
>> acrn_dev is static, but acrn_dev.this_device is not.
>
> But you don't control that device, the misc device core does.
>
>>>
>>> Ugh, who reviewed this code before it was sent out???
>>
>> This part was just newly added according to your suggestion.. Please
>> refer to
>> https://lore.kernel.org/lkml/[email protected]/
>
> What you were doing there was wrong, and what you are doing here is just
> odd.
>
> Step back please, and describe exactly what you are trying to do. And
> then explain how grabbing a reference to the device reference count for
> the misc device is going to help solve that issue.
>
>From what I understand this current path was entered after Greg's
original suggestion from
https://lore.kernel.org/lkml/[email protected]
to replace the pr_xxx() calls with the more informative dev_xxx() ones.
Perhaps all that is needed to follow the original suggestion is to
replace the pr_xxx() calls with dev_xxx(acrn_dev.this_device, ...) ?
(An example of this is drivers/input/serio/userio.c and some others)
The device (acrn_dev.this_device) can be trusted to exist for the
lifetime of the driver since it is removed on module unload where the
misc device is unregistered.
Reinette
