linux-kernel - Re: [PATCH v3] PCI: vmd: Assign a number to each VMD controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210916225755.GA1511623@rocinante>
Date:   Fri, 17 Sep 2021 00:57:55 +0200
From:   Krzysztof Wilczyński <kw@...ux.com>
To:     brookxu <brookxu.cn@...il.com>
Cc:     jonathan.derrick@...el.com, lorenzo.pieralisi@....com,
        robh@...nel.org, bhelgaas@...gle.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] PCI: vmd: Assign a number to each VMD controller

Hi Xu,

Thank you for sending the patch over!

A small nitpick below, so feel free to ignore it.

[...] 
> @@ -769,28 +773,48 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	unsigned long features = (unsigned long) id->driver_data;
>  	struct vmd_dev *vmd;
> -	int err;
> +	int err = 0;
>  
> -	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20))
> -		return -ENOMEM;
> +	if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20)) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
>  
>  	vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL);
> -	if (!vmd)
> -		return -ENOMEM;
> +	if (!vmd) {
> +		err = -ENOMEM;
> +		goto out;
> +	}

I assume that you changed the above to use the newly added "out" label to
be consistent given that you also have the other label, but since there is
no clean-up to be done here, do we need this additional label?

>  	vmd->dev = dev;
> +	vmd->instance = ida_simple_get(&vmd_instance_ida, 0, 0, GFP_KERNEL);
> +	if (vmd->instance < 0) {
> +		err = vmd->instance;
> +		goto out;
> +	}

Similarly to here to the above, no clean-up to be done, and you could just
return immediately here.

What do you think?

Also, I think we might have lost a "Reviewed-by" from Jon Derrick somewhere
along the way.  Given that you only updated the commit log and the subject
like, it probably still applies (unless Jon would like to give his seal of
approval again).

	Krzysztof