netdev - Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT

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: <e305ce76-215e-8350-4f6c-4e938924e754@aquantia.com>
Date:   Tue, 22 Jan 2019 12:44:07 +0000
From:   Igor Russkikh <Igor.Russkikh@...antia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "David S . Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Nikita Danilov <Nikita.Danilov@...antia.com>
Subject: Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR
 construct


>> -#define AQ_HW_WAIT_FOR(_B_, _US_, _N_) \
>> +#define AQ_HW_WAIT_FOR(_B_, _US_, _N_, _err_) \
>>  do { \
>>  	unsigned int AQ_HW_WAIT_FOR_i; \
>>  	for (AQ_HW_WAIT_FOR_i = _N_; (!(_B_)) && (AQ_HW_WAIT_FOR_i);\
>> @@ -31,7 +31,7 @@ do { \
>>  		udelay(_US_); \
>>  	} \
>>  	if (!AQ_HW_WAIT_FOR_i) {\
>> -		err = -ETIME; \
>> +		*(_err_) = -ETIME; \
>>  	} \
>>  } while (0)
> 
> Hi Igor
> 
> How about throwing this horrible macro away and using one of the
> readx_poll_timeout() variants.

Hi Andrew,

Thanks I was not aware of that macro.

The original macro actually differs a bit in a sense that it does not require
a 'value' local variable to be used. Also aq driver used a number of wrapper
functions to hide the actual readl and mmio access.

Thus its hard to adopt readx_poll_timeout() in its current form.

WAIT_FOR is normally used like

	AQ_HW_WAIT_FOR(hw_atl_itr_res_irq_get(self) == 0, 1000U, 10U, &err);

So there is no direct access to register offset and value. Thats an expression waiting for
logical condition to happen, not just register value to change.

I was under impression statement expressions (macro returning values) should not be used because
of compatibility, but since its in use may be its the better to rewrite this as

#define AQ_HW_WAIT_FOR(_B_, _US_, _N_) \
({ \
	unsigned int AQ_HW_WAIT_FOR_i; \
	for (AQ_HW_WAIT_FOR_i = _N_; (!(_B_)) && (AQ_HW_WAIT_FOR_i);\
	--AQ_HW_WAIT_FOR_i) {\
		udelay(_US_); \
	} \
        AQ_HW_WAIT_FOR_i ? EOK : -ETIMEDOUT;\
})

So it could be used as

	err = AQ_HW_WAIT_FOR(hw_atl_itr_res_irq_get(self) == 0, 1000U, 10U);

and may be worth renaming it to something with _poll_timeout suffix as well?

Regards, 
  Igor