Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 3 Aug 2022 17:30:21 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     bchalios@amazon.es
Cc:     linux-kernel@vger.kernel.org, tytso@mit.edu, Jason@zx2c4.com,
        dwmw@amazon.co.uk, graf@amazon.de, xmarcalx@amazon.co.uk
Subject: Re: [PATCH 2/2] virt: vmgenid: add support for generation counter
Message-ID: <YuqUjUBxq6X738t/@kroah.com>
References: <20220803152127.48281-1-bchalios@amazon.es>
 <20220803152127.48281-3-bchalios@amazon.es>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220803152127.48281-3-bchalios@amazon.es>
Precedence: bulk

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/ 


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