Re: [PATCH 2/2] virt: vmgenid: add support for generation counter

["Chalios, Babis" <bchalios@amazon.es>]

On 3/8/22 17:30, Greg KH wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Wed, Aug 03, 2022 at 05:21:27PM +0200, bchalios@amazon.es wrote:
>> +static const struct file_operations fops = {
>> +     .owner = THIS_MODULE,
>> +     .open = vmgenid_open,
>> +     .read = vmgenid_read,
>> +     .mmap = vmgenid_mmap,
>> +     .llseek = noop_llseek,
> Where is this new user/kernel api being documented?
>
> See, put it in the code please, no one knows to look in a random file in
> Documentation/
Agreed! Will move it in the code.
>
>
>> +};
>> +
>> +static struct miscdevice vmgenid_misc = {
>> +     .minor = MISC_DYNAMIC_MINOR,
>> +     .name = "vmgenid",
>> +     .fops = &fops,
>>   };
>>
>>   static int parse_vmgenid_address(struct acpi_device *device, acpi_string object_name,
>> @@ -57,7 +124,7 @@ static int vmgenid_add(struct acpi_device *device)
>>        phys_addr_t phys_addr;
>>        int ret;
>>
>> -     state = devm_kmalloc(&device->dev, sizeof(*state), GFP_KERNEL);
>> +     state = devm_kzalloc(&device->dev, sizeof(*state), GFP_KERNEL);
>>        if (!state)
>>                return -ENOMEM;
>>
>> @@ -74,6 +141,27 @@ static int vmgenid_add(struct acpi_device *device)
>>
>>        device->driver_data = state;
>>
>> +     /* Backwards compatibility. If CTRA is not there we just don't expose
>> +      * the char device
>> +      */
>> +     ret = parse_vmgenid_address(device, "CTRA", &state->gen_cntr_addr);
>> +     if (ret)
>> +             return 0;
>> +
>> +     state->next_counter = devm_memremap(&device->dev, state->gen_cntr_addr,
>> +                     sizeof(u32), MEMREMAP_WB);
>> +     if (IS_ERR(state->next_counter))
>> +             return 0;
>> +
>> +     memcpy(&state->misc, &vmgenid_misc, sizeof(state->misc));
>> +     ret = misc_register(&state->misc);
>> +     if (ret) {
>> +             devm_memunmap(&device->dev, state->next_counter);
>> +             return 0;
> Why are you not returning an error?  Why is this ok?
>
> And why call devm_memunmap() directly?  That kind of defeats the purpose
> of using devm_memremap(), right?
The intention here was to not fail the whole ACPI device if registering 
the misc device fails. My rationale
was that the ACPI device is used for other things as well (re-seeding RNG).

>
> thanks,
>
> greg k-h

Cheers,
Babis
Amazon Spain Services sociedad limitada unipersonal, Calle Ramirez de Prado 5, 28045 Madrid. Registro Mercantil de Madrid . Tomo 22458 . Folio 102 . Hoja M-401234 . CIF B84570936