[PATCH 16/16] staging: ks7010: extract JIFFIES_TO_WAIT definition for common code

[Dan Carpenter]

On Fri, Apr 06, 2018 at 12:38:23PM +0200, Sergio Paracuellos wrote:
> This commit extracts JIFFIES_TO_WAIT definition to be precalculated
> by preprocessor insted of just do the same operation different times
> in ks7010_rw_function.
> 

I don't understand what you're saying at all.  Are you saying that it's
more readable now?

> Signed-off-by: Sergio Paracuellos <sergio.paracuellos at gmail.com>
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
> index d689599..9e98062 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -418,6 +418,8 @@ static void ks_wlan_hw_rx(struct ks_wlan_private *priv, uint16_t size)
>  	tasklet_schedule(&priv->rx_bh_task);
>  }
>  
> +#define JIFFIES_TO_WAIT ((30 * HZ) / 1000)]

This macro is sort of bad.  First of all, it's an unnecessary
abstraction because the code is easier to read if you don't have to
scroll to the start of the function and then back.  Also if a function
has "_to_" in the name, I sort of expect that it takes an argument and
translates it to another thing, for example, cpu_to_le16(), to_attr() or
hv_vcpu_to_vcpu().

> +
>  static void ks7010_rw_function(struct work_struct *work)
>  {
>  	struct ks_wlan_private *priv;
> @@ -427,19 +429,18 @@ static void ks7010_rw_function(struct work_struct *work)
>  	priv = container_of(work, struct ks_wlan_private, rw_dwork.work);
>  
>  	/* wait after DOZE */
> -	if (time_after(priv->last_doze + ((30 * HZ) / 1000), jiffies)) {
> +	if (time_after(priv->last_doze + JIFFIES_TO_WAIT, jiffies)) {

We have the msec_to_jiffies() function:

	if (time_after(priv->last_doze + msec_to_jiffies(30), jiffies)) {

So we return here if we've dozed in the previous 30 msec...  That seems
like a very short time.

>  		netdev_dbg(priv->net_dev, "wait after DOZE\n");
>  		queue_delayed_work(priv->wq, &priv->rw_dwork, 1);
>  		return;
>  	}
>  
>  	/* wait after WAKEUP */
> -	while (time_after(priv->last_wakeup + ((30 * HZ) / 1000), jiffies)) {
> +	while (time_after(priv->last_wakeup + JIFFIES_TO_WAIT, jiffies)) {
>  		netdev_dbg(priv->net_dev, "wait after WAKEUP\n");
>  		dev_info(&priv->ks_sdio_card->func->dev,
>  			 "wake: %lu %lu\n",
> -			 priv->last_wakeup + (30 * HZ) / 1000,
> -				jiffies);
> +			 priv->last_wakeup + JIFFIES_TO_WAIT, jiffies);
>  		msleep(30);

I don't know this code but it looks really suspicious.  Each loop takes
30 msecs.  We loop until the ->last_wakeup wasn't touched in the
previous iteration through the loop.  I wonder why we're doing this?
It feels like it's going to be racy.  There doesn't seem to be any
locking to stop someone from touching ->last_wakeup as soon as the loop
exits and before the sdio_claim_host().

>  	}

regards,
dan carpenter