Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3] PCI: vmd: Assign a number to each VMD controller
To:     =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>
Cc:     jonathan.derrick@intel.com, lorenzo.pieralisi@arm.com,
        robh@kernel.org, bhelgaas@google.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1631675273-1934-1-git-send-email-brookxu.cn@gmail.com>
 <20210916225755.GA1511623@rocinante>
From:   brookxu <brookxu.cn@gmail.com>
Message-ID: <b87f7a38-d147-3da6-0352-ad2df7a5f55f@gmail.com>
Date:   Fri, 17 Sep 2021 09:36:08 +0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <20210916225755.GA1511623@rocinante>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


Krzysztof Wilczyński wrote on 2021/9/17 6:57 上午:
> 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?
> 

Thanks, I think we can do this.

> 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).
> 

Thanks, my mistake here.

> 	Krzysztof
>