confusing code....whats the point of this construct ?

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Jeff Haran wrote:

> -----Original Message-----
> From: kernelnewbies-bounces at kernelnewbies.org [mailto:kernelnewbies-bounces at kernelnewbies.org] On Behalf Of Valdis.Kletnieks at vt.edu
> Sent: Wednesday, March 11, 2015 10:00 AM
> To: Nicholas Mc Guire
> Cc: Bj??rn Mork; kernelnewbies at kernelnewbies.org
> Subject: Re: confusing code....whats the point of this construct ?
> 
> On Wed, 11 Mar 2015 17:46:56 +0100, Nicholas Mc Guire said:
> 
> > ret = ({ long __ret = (5*250);
> >         do { _cond_resched(); } while (0);
> >  	if (!({
> > 		bool __cond = (({
> 
> >Gaak.
> >
> >         ret = wait_event_timeout(mgr->tx_waitq,
> >                                  check_txmsg_state(mgr, txmsg),
> >                                  (4 * HZ));
> 
> >-EGADS - use of macro as a function violates the Principle of Least Surprise....
> >
> >I have to wonder how many other places we've got bugs waiting to happen because of this....
> 
> I don't understand the problem here. The caller passes in a condition to be evaluated in a loop. Many times that condition is quite simple (e.g. a counter being non-zero). If it was a function the caller would have to pass in a pointer to a function that does the evaluation, as in:
> 
> int bar;
> 
> int foo(void)
> {
> 	return bar;
> }
> 
> ...
> 
> 	wait_event_interruptible(..., foo, ...);
> 
> Instead of the much simpler:
> 
> 	wait_event_interruptible(..., bar, ...);
> 
> That latter seems easier to understand and require fewer instructions to be generated since there is no function call overhead.
>
for simle conditions that is true and commonly done but for
complex conditions that require locking or intermediate 
variables it is much more readable if packed up in a function like in
e.g. see drivers/gpu/drm/drm_dp_mst_topology.c:drm_dp_mst_wait_tx_reply()

static bool check_txmsg_state(struct drm_dp_mst_topology_mgr *mgr,
                              struct drm_dp_sideband_msg_tx *txmsg)
{
        bool ret;
        mutex_lock(&mgr->qlock);
        ret = (txmsg->state == DRM_DP_SIDEBAND_TX_RX ||
               txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT);
        mutex_unlock(&mgr->qlock);
        return ret;
}

drm_dp_mst_wait_tx_reply()
<snip>
        ret = wait_event_timeout(mgr->tx_waitq,
                                 check_txmsg_state(mgr, txmsg),
                                 (4 * HZ));
<snip>
 
which I find is much simpler to understand than the "inline" code
in ath10k_flush().

thx!
hofrat