Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators

[Tejun Heo <tj@kernel.org>]

Hello,

Elias Oltmanns wrote:
>> -		ata_port_for_each_link(link, ap) {
>> -			ata_link_for_each_dev(dev, link)
>> +		ata_for_each_link(link, ap, EDGE) {
>> +			ata_for_each_dev(dev, link, ALL)
> 
> Where did these short forms (EDGE, ALL) spring from? Does this code even
> compile?

+#define ata_for_each_link(link, ap, mode) \
+	for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \
+	     (link) = ata_link_next((link), (ap), ATA_LITER_##mode))
+
+#define ata_for_each_dev(dev, link, mode) \
+	for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \
+	     (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))

>> @@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct ata
>>  	struct ata_device *dev;
>>  	int cnt = 0;
>>  
>> -	ata_link_for_each_dev(dev, link)
>> +	ata_for_each_dev(dev, link, ALL)
>>  		if (dev->class == ATA_DEV_UNKNOWN)
>>  			cnt++;
>>  	return cnt;
> 
> What about making the two above (ata_link_nr_*()) static inline while
> you are at it? Or is this another one of those cases where the compiler
> knows best anyway?

Yeah, it's a static function used only in single place.  Should be
pretty straight forward for the compiler and even if it fails to
notice it, it doesn't hurt at all.

> [...]
>> Index: work/drivers/ata/libata-scsi.c
>> ===================================================================
>> --- work.orig/drivers/ata/libata-scsi.c
>> +++ work/drivers/ata/libata-scsi.c
> [...]
>> @@ -3254,9 +3254,9 @@ void ata_scsi_scan_host(struct ata_port
>>  	 * failure occurred, scan would have failed silently.  Check
>>  	 * whether all devices are attached.
>>  	 */
>> -	ata_port_for_each_link(link, ap) {
>> -		ata_link_for_each_dev(dev, link) {
>> -			if (ata_dev_enabled(dev) && !dev->sdev)
>> +	ata_for_each_link(link, ap, EDGE) {
>> +		ata_for_each_dev(dev, link, ENABLED) {
>> +			if (!dev->sdev)
>>  				goto exit_loop;
>>  		}
>>  	}
> 
> Getting rid of those braces would make things even cleaner in my
> opinion. (That's my nit picking since Sergei picked on that comment
> formatting issue shortly before I was going to ;-) ).

Yeah, maybe.  I don't know.  The reason why I added braces there in
the first place was because emacs has some problem dealing with nested
non-standard looping constructs.

Thanks.

-- 
tejun