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

[brookxu]

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
>