[igt-dev] [PATCH i-g-t v3 1/1] lib/igt_device_scan: clear igt_devs.devs_scanned in igt_devices_free

[Tauro, Riana]

On 10/12/2022 6:24 PM, Kamil Konieczny wrote:
> Hi Riana,
> 
> I have few nits for this patch. First is subject, it should be
> avoided to write your code in C, so better title for patch is:
> 
> lib/igt_device_scan: fix igt_devices_free
> 
> On 2022-10-11 at 11:04:18 +0530, Riana Tauro wrote:
>> 1) igt_devices_scan returns with empty list if devs_scanned
> - ^
> No need for numbering your changes, it is better to send two
> patches.
> 
>>     is true and function is called after igt_devices_free
>>     clear igt_devs.devs_scanned in igt_devices_free.
> 
> This is hard to read at first, maybe some re-wording like:
> 
> igt_devices_scan returns with empty list if it is called after
> anyone calls igt_devices_free. Fixed this with clearing
> igt_devs.devs_scanned inside igt_devices_free.
> 
> btw you did second fix inside that function, e.g. free list
> so imho it should be described:
> 
> Also while at this, fix memory leak in it.
> 
>>
>> 2) remove redudant code from igt_devices_scan and replace it with
>>     igt_devices_free
>>
> 
> As Janusz pointed out, it will be better to send second patch
> with refactoring so please in future split fixes and other
> changes.
> 
> I will merge this patch with these fixups to subject and commit
> message.
> 
> Please also read CONTRIBUTING.md from igt project and see
> some other helpfull notes:
> 
> https://www.ozlabs.org/~akpm/stuff/tpp.txt
> 
> https://kernelnewbies.org/PatchPhilosophy
> 
> https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst
> 
> Regards,
> Kamil


Thank you Kamil for the helpful notes and links.Will follow these links 
Thank you for merging the patch.

Thanks,
Riana
>> v2: optimize code in igt_devices_scan (Zbigniew)
>> v3: update commit message (Janusz)
>>
>> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
>> Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
>> ---
>>   lib/igt_device_scan.c | 20 ++++----------------
>>   1 file changed, 4 insertions(+), 16 deletions(-)
>>
>> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>> index eb6b45b8..8b70e375 100644
>> --- a/lib/igt_device_scan.c
>> +++ b/lib/igt_device_scan.c
>> @@ -1027,9 +1027,11 @@ void igt_devices_free(void)
>>   	}
>>   
>>   	igt_list_for_each_entry_safe(dev, tmp, &igt_devs.all, link) {
>> +		igt_list_del(&dev->link);
>>   		igt_device_free(dev);
>>   		free(dev);
>>   	}
>> +	igt_devs.devs_scanned = false;
>>   }
>>   
>>   /**
>> @@ -1043,22 +1045,8 @@ void igt_devices_free(void)
>>    */
>>   void igt_devices_scan(bool force)
>>   {
>> -	if (force && igt_devs.devs_scanned) {
>> -		struct igt_device *dev, *tmp;
>> -
>> -		igt_list_for_each_entry_safe(dev, tmp, &igt_devs.filtered,
>> -					     link) {
>> -			igt_list_del(&dev->link);
>> -			free(dev);
>> -		}
>> -		igt_list_for_each_entry_safe(dev, tmp, &igt_devs.all, link) {
>> -			igt_list_del(&dev->link);
>> -			igt_device_free(dev);
>> -			free(dev);
>> -		}
>> -
>> -		igt_devs.devs_scanned = false;
>> -	}
>> +	if (force && igt_devs.devs_scanned)
>> +		igt_devices_free();
>>   
>>   	if (igt_devs.devs_scanned)
>>   		return;
>> -- 
>> 2.25.1
>>