On Fri, Apr 6, 2018 at 1:55 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> 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?
Well, kind of. It seems that my english is not the best :-).
>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
>> ---
>> 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().
I see, thanks for pointing me this out. I'll take this into account in
the future.
>
>> +
>> 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().
>
>> }
I don't have hardware yet to test this and see what happends because
the original
code driver from renesas is as it is written here.
Anyway I am goint to send v2 using msec_to_jiffies() at first and
analyze the behaviour
you are describing when can test with real hardware.
>
> regards,
> dan carpenter
Best regards,
Sergio Paracuellos
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
