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

[Greg KH]

On Wed, Aug 03, 2022 at 05:21:27PM +0200, bchalios@xxxxxxxxx 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/ 


> +};
> +
> +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?

thanks,

greg k-h