Re: [PATCH 16/16] staging: ks7010: extract JIFFIES_TO_WAIT definition for common code
On Fri, Apr 06, 2018 at 02:08:31PM +0200, Sergio Paracuellos wrote: > On Fri, Apr 6, 2018 at 1:55 PM, Dan Carpenter > wrote: > > On Fri, Apr 06, 2018 at 12:38:23PM +0200, Sergio Paracuellos wrote: > 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. Ah... I had forgotten you don't have the hardware. That's fine then. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 16/16] staging: ks7010: extract JIFFIES_TO_WAIT definition for common code
On Fri, Apr 6, 2018 at 1:55 PM, Dan Carpenter 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
>> ---
>> 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 16/16] staging: ks7010: extract JIFFIES_TO_WAIT definition for common code
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
> ---
> 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 16/16] staging: ks7010: extract JIFFIES_TO_WAIT definition for common code
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.
Signed-off-by: Sergio Paracuellos
---
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)
+
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)) {
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);
}
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
