Re: [PATCH 14/14] staging: ks7010: use linux circular buffer header macros to handle tx and rx queues

[Sergio Paracuellos]

On Fri, Apr 13, 2018 at 3:27 PM, Dan Carpenter  wrote:
>
> This is the last patch in the series so I think we could just drop it
> without resending the others.

True. Ok, I'll resend the last one modified in my next series.

>
> regards,
> dan carpenter
>

Best regards,
Sergio Paracuellos
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 14/14] staging: ks7010: use linux circular buffer header macros to handle tx and rx queues

[Dan Carpenter]

This is the last patch in the series so I think we could just drop it
without resending the others.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 14/14] staging: ks7010: use linux circular buffer header macros to handle tx and rx queues

[Sergio Paracuellos]

On Fri, Apr 13, 2018 at 2:14 PM, Dan Carpenter  wrote:
> On Thu, Apr 12, 2018 at 05:50:31PM +0200, Sergio Paracuellos wrote:
>> This commit replace current custom implementation of some circular
>> buffer head and tail logic in favour of the use of macros defined
>> in linux circ_buf.h header. It also review internal names and adds
>> a new CIRC_INC macro to make code more readable. Note also that
>> CIRC_INC does not need to go inside do-while(0) block because its
>> use is only located in the four functions that make use of it
>> so it won't expand into invalid code at all.
>>
>> Signed-off-by: Sergio Paracuellos 
>> ---
>>  drivers/staging/ks7010/ks7010_sdio.c | 59 
>> ++--
>>  1 file changed, 36 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
>> b/drivers/staging/ks7010/ks7010_sdio.c
>> index 9c591e0..9676902 100644
>> --- a/drivers/staging/ks7010/ks7010_sdio.c
>> +++ b/drivers/staging/ks7010/ks7010_sdio.c
>> @@ -10,6 +10,7 @@
>>   *   published by the Free Software Foundation.
>>   */
>>
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -101,38 +102,50 @@ enum gen_com_reg_b {
>>
>>  #define KS7010_IO_BLOCK_SIZE 512
>>
>> +#define CIRC_INC(a, b) if (++a >= b) a = 0
>
> I don't like this macro.  If we're going to call it CIRC_INC() then it
> needs to be included in include/linux/circ_buf.h and not here.  I don't
> like that the argument is  "b" instead of "size" or something.  It
> should be wrapped in a do { } while(0).  There should be parens around
> "a" and "b" so I don't have to think about precedence bugs.

Ok, I'll send v2 using other macros included in
include/linux/circ_buf.h but avoiding
the use of this new one.

Thanks, Dan.

>
> regards,
> dan carpenter

Best regards,
Sergio Paracuellos
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 14/14] staging: ks7010: use linux circular buffer header macros to handle tx and rx queues

[Dan Carpenter]

On Thu, Apr 12, 2018 at 05:50:31PM +0200, Sergio Paracuellos wrote:
> This commit replace current custom implementation of some circular
> buffer head and tail logic in favour of the use of macros defined
> in linux circ_buf.h header. It also review internal names and adds
> a new CIRC_INC macro to make code more readable. Note also that
> CIRC_INC does not need to go inside do-while(0) block because its
> use is only located in the four functions that make use of it 
> so it won't expand into invalid code at all.
> 
> Signed-off-by: Sergio Paracuellos 
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 59 
> ++--
>  1 file changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> b/drivers/staging/ks7010/ks7010_sdio.c
> index 9c591e0..9676902 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -10,6 +10,7 @@
>   *   published by the Free Software Foundation.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -101,38 +102,50 @@ enum gen_com_reg_b {
>  
>  #define KS7010_IO_BLOCK_SIZE 512
>  
> +#define CIRC_INC(a, b) if (++a >= b) a = 0

I don't like this macro.  If we're going to call it CIRC_INC() then it
needs to be included in include/linux/circ_buf.h and not here.  I don't
like that the argument is  "b" instead of "size" or something.  It
should be wrapped in a do { } while(0).  There should be parens around
"a" and "b" so I don't have to think about precedence bugs.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 14/14] staging: ks7010: use linux circular buffer header macros to handle tx and rx queues

[Sergio Paracuellos]

This commit replace current custom implementation of some circular
buffer head and tail logic in favour of the use of macros defined
in linux circ_buf.h header. It also review internal names and adds
a new CIRC_INC macro to make code more readable. Note also that
CIRC_INC does not need to go inside do-while(0) block because its
use is only located in the four functions that make use of it 
so it won't expand into invalid code at all.

Signed-off-by: Sergio Paracuellos 
---
 drivers/staging/ks7010/ks7010_sdio.c | 59 ++--
 1 file changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 9c591e0..9676902 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -10,6 +10,7 @@
  *   published by the Free Software Foundation.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -101,38 +102,50 @@ enum gen_com_reg_b {
 
 #define KS7010_IO_BLOCK_SIZE 512
 
+#define CIRC_INC(a, b) if (++a >= b) a = 0
+
 static inline void inc_txqhead(struct ks_wlan_private *priv)
 {
-   priv->tx_dev.qhead = (priv->tx_dev.qhead + 1) % TX_DEVICE_BUFF_SIZE;
+   CIRC_INC(priv->tx_dev.qhead, TX_DEVICE_BUFF_SIZE);
 }
 
 static inline void inc_txqtail(struct ks_wlan_private *priv)
 {
-   priv->tx_dev.qtail = (priv->tx_dev.qtail + 1) % TX_DEVICE_BUFF_SIZE;
+   CIRC_INC(priv->tx_dev.qtail, TX_DEVICE_BUFF_SIZE);
 }
 
-static inline unsigned int cnt_txqbody(struct ks_wlan_private *priv)
+static inline bool txq_has_space(struct ks_wlan_private *priv)
 {
-   unsigned int tx_cnt = priv->tx_dev.qtail - priv->tx_dev.qhead;
-
-   return (tx_cnt + TX_DEVICE_BUFF_SIZE) % TX_DEVICE_BUFF_SIZE;
+   return (CIRC_SPACE(priv->tx_dev.qhead, priv->tx_dev.qtail,
+  TX_DEVICE_BUFF_SIZE) > 0);
 }
 
 static inline void inc_rxqhead(struct ks_wlan_private *priv)
 {
-   priv->rx_dev.qhead = (priv->rx_dev.qhead + 1) % RX_DEVICE_BUFF_SIZE;
+   CIRC_INC(priv->rx_dev.qhead, RX_DEVICE_BUFF_SIZE);
 }
 
 static inline void inc_rxqtail(struct ks_wlan_private *priv)
 {
-   priv->rx_dev.qtail = (priv->rx_dev.qtail + 1) % RX_DEVICE_BUFF_SIZE;
+   CIRC_INC(priv->rx_dev.qtail, RX_DEVICE_BUFF_SIZE);
 }
 
-static inline unsigned int cnt_rxqbody(struct ks_wlan_private *priv)
+static inline bool rxq_has_space(struct ks_wlan_private *priv)
 {
-   unsigned int rx_cnt = priv->rx_dev.qtail - priv->rx_dev.qhead;
+   return (CIRC_SPACE(priv->rx_dev.qhead, priv->rx_dev.qtail,
+  RX_DEVICE_BUFF_SIZE) > 0);
+}
 
-   return (rx_cnt + RX_DEVICE_BUFF_SIZE) % RX_DEVICE_BUFF_SIZE;
+static inline unsigned int txq_count(struct ks_wlan_private *priv)
+{
+   return CIRC_CNT_TO_END(priv->tx_dev.qhead, priv->tx_dev.qtail,
+  TX_DEVICE_BUFF_SIZE);
+}
+
+static inline unsigned int rxq_count(struct ks_wlan_private *priv)
+{
+   return CIRC_CNT_TO_END(priv->rx_dev.qhead, priv->rx_dev.qtail,
+  RX_DEVICE_BUFF_SIZE);
 }
 
 /* Read single byte from device address into byte (CMD52) */
@@ -258,11 +271,11 @@ static void _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
   atomic_read(>psstatus.status),
   atomic_read(>psstatus.confirm_wait),
   atomic_read(>psstatus.snooze_guard),
-  cnt_txqbody(priv));
+  txq_count(priv));
 
if (atomic_read(>psstatus.confirm_wait) ||
atomic_read(>psstatus.snooze_guard) ||
-   cnt_txqbody(priv)) {
+   txq_has_space(priv)) {
queue_delayed_work(priv->wq, >rw_dwork, 0);
return;
}
@@ -308,7 +321,7 @@ static int enqueue_txdev(struct ks_wlan_private *priv, 
unsigned char *p,
goto err_complete;
}
 
-   if ((TX_DEVICE_BUFF_SIZE - 1) <= cnt_txqbody(priv)) {
+   if ((TX_DEVICE_BUFF_SIZE - 1) <= txq_count(priv)) {
netdev_err(priv->net_dev, "tx buffer overflow\n");
ret = -EOVERFLOW;
goto err_complete;
@@ -366,7 +379,7 @@ static void tx_device_task(struct ks_wlan_private *priv)
struct tx_device_buffer *sp;
int ret;
 
-   if (cnt_txqbody(priv) <= 0 ||
+   if (!txq_has_space(priv) ||
atomic_read(>psstatus.status) == PS_SNOOZE)
return;
 
@@ -385,7 +398,7 @@ static void tx_device_task(struct ks_wlan_private *priv)
(*sp->complete_handler)(priv, sp->skb);
inc_txqhead(priv);
 
-   if (cnt_txqbody(priv) > 0)
+   if (txq_has_space(priv))
queue_delayed_work(priv->wq, >rw_dwork, 0);
 }
 
@@ -413,7 +426,7 @@ int ks_wlan_hw_tx(struct ks_wlan_private *priv, void *p, 
unsigned long size,
result = enqueue_txdev(priv, p, size, complete_handler, skb);
spin_unlock(>tx_dev.tx_dev_lock);
 
-   if