[PATCH 0/4] Introduce generic set_bit_le()

[PATCH 0/4] Introduce generic set_bit_le()

[Takuya Yoshikawa]

KVM is using test_and_set_bit_le() for this missing function; this patch
series corrects this usage.

As some drivers have their own definitions of set_bit_le(), which seem
to be incompatible with the generic one, renaming is also needed.

Note: the whole series is against linux-next.

Takuya Yoshikawa (4):
  drivers/net/ethernet/sfc: Add efx_ prefix to set_bit_le()
  drivers/net/ethernet/dec/tulip: Add tulip_ prefix to set_bit_le()
  bitops: Introduce generic set_bit_le()
  KVM: Replace test_and_set_bit_le() in mark_page_dirty_in_slot() with set_bit_le()

 drivers/net/ethernet/dec/tulip/de2104x.c    |    7 +++----
 drivers/net/ethernet/dec/tulip/tulip_core.c |    7 +++----
 drivers/net/ethernet/sfc/efx.c              |    4 ++--
 drivers/net/ethernet/sfc/net_driver.h       |    4 ++--
 drivers/net/ethernet/sfc/nic.c              |    4 ++--
 include/asm-generic/bitops/le.h             |    5 +++++
 virt/kvm/kvm_main.c                         |    3 +--
 7 files changed, 18 insertions(+), 16 deletions(-)

-- 
1.7.5.4

[PATCH 1/4] drivers/net/ethernet/sfc: Add efx_ prefix to set_bit_le()

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Needed to introduce generic set_bit_le().

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/ethernet/sfc/efx.c        |    4 ++--
 drivers/net/ethernet/sfc/net_driver.h |    4 ++--
 drivers/net/ethernet/sfc/nic.c        |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index b95f2e1..de11449 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1976,14 +1976,14 @@ static void efx_set_rx_mode(struct net_device *net_dev)
 		netdev_for_each_mc_addr(ha, net_dev) {
 			crc = ether_crc_le(ETH_ALEN, ha->addr);
 			bit = crc & (EFX_MCAST_HASH_ENTRIES - 1);
-			set_bit_le(bit, mc_hash->byte);
+			efx_set_bit_le(bit, mc_hash->byte);
 		}
 
 		/* Broadcast packets go through the multicast hash filter.
 		 * ether_crc_le() of the broadcast address is 0xbe2612ff
 		 * so we always add bit 0xff to the mask.
 		 */
-		set_bit_le(0xff, mc_hash->byte);
+		efx_set_bit_le(0xff, mc_hash->byte);
 	}
 
 	if (efx->port_enabled)
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 0e57535..5e084bf 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -1081,13 +1081,13 @@ static inline struct efx_rx_buffer *efx_rx_buffer(struct efx_rx_queue *rx_queue,
 }
 
 /* Set bit in a little-endian bitfield */
-static inline void set_bit_le(unsigned nr, unsigned char *addr)
+static inline void efx_set_bit_le(unsigned nr, unsigned char *addr)
 {
 	addr[nr / 8] |= (1 << (nr % 8));
 }
 
 /* Clear bit in a little-endian bitfield */
-static inline void clear_bit_le(unsigned nr, unsigned char *addr)
+static inline void efx_clear_bit_le(unsigned nr, unsigned char *addr)
 {
 	addr[nr / 8] &= ~(1 << (nr % 8));
 }
diff --git a/drivers/net/ethernet/sfc/nic.c b/drivers/net/ethernet/sfc/nic.c
index 4a9a5be..3abde7b 100644
--- a/drivers/net/ethernet/sfc/nic.c
+++ b/drivers/net/ethernet/sfc/nic.c
@@ -473,9 +473,9 @@ void efx_nic_init_tx(struct efx_tx_queue *tx_queue)
 
 		efx_reado(efx, &reg, FR_AA_TX_CHKSM_CFG);
 		if (tx_queue->queue & EFX_TXQ_TYPE_OFFLOAD)
-			clear_bit_le(tx_queue->queue, (void *)&reg);
+			efx_clear_bit_le(tx_queue->queue, (void *)&reg);
 		else
-			set_bit_le(tx_queue->queue, (void *)&reg);
+			efx_set_bit_le(tx_queue->queue, (void *)&reg);
 		efx_writeo(efx, &reg, FR_AA_TX_CHKSM_CFG);
 	}
 
-- 
1.7.5.4

[PATCH 2/4] drivers/net/ethernet/dec/tulip: Add tulip_ prefix to set_bit_le()

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Needed to introduce generic set_bit_le().

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Cc: Grant Grundler <grundler@parisc-linux.org>
---
 drivers/net/ethernet/dec/tulip/de2104x.c    |    7 +++----
 drivers/net/ethernet/dec/tulip/tulip_core.c |    7 +++----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/dec/tulip/de2104x.c b/drivers/net/ethernet/dec/tulip/de2104x.c
index 61cc093..e635f1a 100644
--- a/drivers/net/ethernet/dec/tulip/de2104x.c
+++ b/drivers/net/ethernet/dec/tulip/de2104x.c
@@ -661,8 +661,7 @@ static netdev_tx_t de_start_xmit (struct sk_buff *skb,
    new frame, not around filling de->setup_frame.  This is non-deterministic
    when re-entered but still correct. */
 
-#undef set_bit_le
-#define set_bit_le(i,p) do { ((char *)(p))[(i)/8] |= (1<<((i)%8)); } while(0)
+#define tulip_set_bit_le(i,p) do { ((char *)(p))[(i)/8] |= (1<<((i)%8)); } while(0)
 
 static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
 {
@@ -673,12 +672,12 @@ static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
 	u16 *eaddrs;
 
 	memset(hash_table, 0, sizeof(hash_table));
-	set_bit_le(255, hash_table); 			/* Broadcast entry */
+	tulip_set_bit_le(255, hash_table);		/* Broadcast entry */
 	/* This should work on big-endian machines as well. */
 	netdev_for_each_mc_addr(ha, dev) {
 		int index = ether_crc_le(ETH_ALEN, ha->addr) & 0x1ff;
 
-		set_bit_le(index, hash_table);
+		tulip_set_bit_le(index, hash_table);
 	}
 
 	for (i = 0; i < 32; i++) {
diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c
index c4f37ac..3a1ebd02 100644
--- a/drivers/net/ethernet/dec/tulip/tulip_core.c
+++ b/drivers/net/ethernet/dec/tulip/tulip_core.c
@@ -1010,8 +1010,7 @@ static int private_ioctl (struct net_device *dev, struct ifreq *rq, int cmd)
    new frame, not around filling tp->setup_frame.  This is non-deterministic
    when re-entered but still correct. */
 
-#undef set_bit_le
-#define set_bit_le(i,p) do { ((char *)(p))[(i)/8] |= (1<<((i)%8)); } while(0)
+#define tulip_set_bit_le(i,p) do { ((char *)(p))[(i)/8] |= (1<<((i)%8)); } while(0)
 
 static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
 {
@@ -1022,12 +1021,12 @@ static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
 	u16 *eaddrs;
 
 	memset(hash_table, 0, sizeof(hash_table));
-	set_bit_le(255, hash_table); 			/* Broadcast entry */
+	tulip_set_bit_le(255, hash_table);		/* Broadcast entry */
 	/* This should work on big-endian machines as well. */
 	netdev_for_each_mc_addr(ha, dev) {
 		int index = ether_crc_le(ETH_ALEN, ha->addr) & 0x1ff;
 
-		set_bit_le(index, hash_table);
+		tulip_set_bit_le(index, hash_table);
 	}
 	for (i = 0; i < 32; i++) {
 		*setup_frm++ = hash_table[i];
-- 
1.7.5.4

[PATCH 3/4] bitops: Introduce generic set_bit_le()

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Needed to replace test_and_set_bit_le() in virt/kvm/kvm_main.c which is
being used for this missing function.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 include/asm-generic/bitops/le.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/asm-generic/bitops/le.h b/include/asm-generic/bitops/le.h
index f95c663..3e72143 100644
--- a/include/asm-generic/bitops/le.h
+++ b/include/asm-generic/bitops/le.h
@@ -54,6 +54,11 @@ static inline int test_bit_le(int nr, const void *addr)
 	return test_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }
 
+static inline void set_bit_le(int nr, void *addr)
+{
+	set_bit(nr ^ BITOP_LE_SWIZZLE, addr);
+}
+
 static inline void __set_bit_le(int nr, void *addr)
 {
 	__set_bit(nr ^ BITOP_LE_SWIZZLE, addr);
-- 
1.7.5.4

[PATCH 4/4] KVM: Replace test_and_set_bit_le() in mark_page_dirty_in_slot() with set_bit_le()

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Now that we have defined generic set_bit_le() we do not need to use
test_and_set_bit_le() for atomically setting a bit.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 virt/kvm/kvm_main.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 02cb440..560c502 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1485,8 +1485,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm, struct kvm_memory_slot *memslot,
 	if (memslot && memslot->dirty_bitmap) {
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
 
-		/* TODO: introduce set_bit_le() and use it */
-		test_and_set_bit_le(rel_gfn, memslot->dirty_bitmap);
+		set_bit_le(rel_gfn, memslot->dirty_bitmap);
 	}
 }
 
-- 
1.7.5.4

Re: [PATCH 1/4] drivers/net/ethernet/sfc: Add efx_ prefix to set_bit_le()

[Arnd Bergmann]

On Monday 11 June 2012, Takuya Yoshikawa wrote:
> 
>  /* Set bit in a little-endian bitfield */
> -static inline void set_bit_le(unsigned nr, unsigned char *addr)
> +static inline void efx_set_bit_le(unsigned nr, unsigned char *addr)
>  {
>         addr[nr / 8] |= (1 << (nr % 8));
>  }
>  
>  /* Clear bit in a little-endian bitfield */
> -static inline void clear_bit_le(unsigned nr, unsigned char *addr)
> +static inline void efx_clear_bit_le(unsigned nr, unsigned char *addr)
>  {
>         addr[nr / 8] &= ~(1 << (nr % 8));
>  }

Hmm, any reason why we're not just using the existing non-atomic
__set_bit_le() here? I think the helpers in sfc and tulip can
just get removed if you use those.

	Arnd

Re: [PATCH 3/4] bitops: Introduce generic set_bit_le()

[Arnd Bergmann]

On Monday 11 June 2012, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> 
> Needed to replace test_and_set_bit_le() in virt/kvm/kvm_main.c which is
> being used for this missing function.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> Cc: Arnd Bergmann <arnd@arndb.de>

I would recommend adding the corresponding clear_bit_le() along with
set_bit_le, so the next person who needs that doesn't have to make
yet another patch.

	Arnd

Re: [PATCH 2/4] drivers/net/ethernet/dec/tulip: Add tulip_ prefix to set_bit_le()

[Grant Grundler]

On Mon, Jun 11, 2012 at 5:30 AM, Takuya Yoshikawa
<yoshikawa.takuya@oss.ntt.co.jp> wrote:
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
>
> Needed to introduce generic set_bit_le().
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> Cc: Grant Grundler <grundler@parisc-linux.org>
> ---
>  drivers/net/ethernet/dec/tulip/de2104x.c    |    7 +++----
>  drivers/net/ethernet/dec/tulip/tulip_core.c |    7 +++----
>  2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/de2104x.c b/drivers/net/ethernet/dec/tulip/de2104x.c
> index 61cc093..e635f1a 100644
> --- a/drivers/net/ethernet/dec/tulip/de2104x.c
> +++ b/drivers/net/ethernet/dec/tulip/de2104x.c
> @@ -661,8 +661,7 @@ static netdev_tx_t de_start_xmit (struct sk_buff *skb,
>    new frame, not around filling de->setup_frame.  This is non-deterministic
>    when re-entered but still correct. */
>
> -#undef set_bit_le
> -#define set_bit_le(i,p) do { ((char *)(p))[(i)/8] |= (1<<((i)%8)); } while(0)
> +#define tulip_set_bit_le(i,p) do { ((char *)(p))[(i)/8] |= (1<<((i)%8)); } while(0)

I am ok with this patch going in. This code predates the clear
understanding of bit_ops that we have now and will continue to work
with this patch.

If you have time to follow Arnd's suggestion to use __set_bit_le()
(and removing both local definitions), that would be better. Either
way:

Acked-by: Grant Grundler <grundler@parisc-linux.org>

thanks!
grant

>
>  static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
>  {
> @@ -673,12 +672,12 @@ static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
>        u16 *eaddrs;
>
>        memset(hash_table, 0, sizeof(hash_table));
> -       set_bit_le(255, hash_table);                    /* Broadcast entry */
> +       tulip_set_bit_le(255, hash_table);              /* Broadcast entry */
>        /* This should work on big-endian machines as well. */
>        netdev_for_each_mc_addr(ha, dev) {
>                int index = ether_crc_le(ETH_ALEN, ha->addr) & 0x1ff;
>
> -               set_bit_le(index, hash_table);
> +               tulip_set_bit_le(index, hash_table);
>        }
>
>        for (i = 0; i < 32; i++) {
> diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c
> index c4f37ac..3a1ebd02 100644
> --- a/drivers/net/ethernet/dec/tulip/tulip_core.c
> +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c
> @@ -1010,8 +1010,7 @@ static int private_ioctl (struct net_device *dev, struct ifreq *rq, int cmd)
>    new frame, not around filling tp->setup_frame.  This is non-deterministic
>    when re-entered but still correct. */
>
> -#undef set_bit_le
> -#define set_bit_le(i,p) do { ((char *)(p))[(i)/8] |= (1<<((i)%8)); } while(0)
> +#define tulip_set_bit_le(i,p) do { ((char *)(p))[(i)/8] |= (1<<((i)%8)); } while(0)
>
>  static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
>  {
> @@ -1022,12 +1021,12 @@ static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
>        u16 *eaddrs;
>
>        memset(hash_table, 0, sizeof(hash_table));
> -       set_bit_le(255, hash_table);                    /* Broadcast entry */
> +       tulip_set_bit_le(255, hash_table);              /* Broadcast entry */
>        /* This should work on big-endian machines as well. */
>        netdev_for_each_mc_addr(ha, dev) {
>                int index = ether_crc_le(ETH_ALEN, ha->addr) & 0x1ff;
>
> -               set_bit_le(index, hash_table);
> +               tulip_set_bit_le(index, hash_table);
>        }
>        for (i = 0; i < 32; i++) {
>                *setup_frm++ = hash_table[i];
> --
> 1.7.5.4
>

Re: [PATCH 1/4] drivers/net/ethernet/sfc: Add efx_ prefix to set_bit_le()

[Takuya Yoshikawa]

On Mon, 11 Jun 2012 14:09:15 +0000
Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 11 June 2012, Takuya Yoshikawa wrote:
> > 
> >  /* Set bit in a little-endian bitfield */
> > -static inline void set_bit_le(unsigned nr, unsigned char *addr)
> > +static inline void efx_set_bit_le(unsigned nr, unsigned char *addr)
> >  {
> >         addr[nr / 8] |= (1 << (nr % 8));
> >  }
> >  
> >  /* Clear bit in a little-endian bitfield */
> > -static inline void clear_bit_le(unsigned nr, unsigned char *addr)
> > +static inline void efx_clear_bit_le(unsigned nr, unsigned char *addr)
> >  {
> >         addr[nr / 8] &= ~(1 << (nr % 8));
> >  }
> 
> Hmm, any reason why we're not just using the existing non-atomic
> __set_bit_le() here? I think the helpers in sfc and tulip can
> just get removed if you use those.

__set_bit_le() assumes long word alignment and does endian conversion
when needed.

To be honest, I am a bit scared of changing drivers which I cannot test
on real hardware.

	Takuya

Re: [PATCH 3/4] bitops: Introduce generic set_bit_le()

[Takuya Yoshikawa]

On Mon, 11 Jun 2012 14:10:26 +0000
Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 11 June 2012, Takuya Yoshikawa wrote:
> > From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> > 
> > Needed to replace test_and_set_bit_le() in virt/kvm/kvm_main.c which is
> > being used for this missing function.
> > 
> > Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> 
> I would recommend adding the corresponding clear_bit_le() along with
> set_bit_le, so the next person who needs that doesn't have to make
> yet another patch.

I will do in the next version.

Now I have also noticed that I need to add the same code to powerpc's
bitops file.

	Takuya

Re: [PATCH 1/4] drivers/net/ethernet/sfc: Add efx_ prefix to set_bit_le()

[Arnd Bergmann]

On Tuesday 12 June 2012, Takuya Yoshikawa wrote:
> > 
> > Hmm, any reason why we're not just using the existing non-atomic
> > __set_bit_le() here? I think the helpers in sfc and tulip can
> > just get removed if you use those.
> 
> __set_bit_le() assumes long word alignment and does endian conversion
> when needed.

Ah, I see.
 
> To be honest, I am a bit scared of changing drivers which I cannot test
> on real hardware.

Ok, let's take your patches as they are then.

With the change to add clear_bit_le:

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH] sfc: Use standard __{clear,set}_bit_le() functions

[Ben Hutchings]

There are now standard functions for dealing with little-endian bit
arrays, so use them instead of our own implementations.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
Please use this version instead.

Ben.

 drivers/net/ethernet/sfc/efx.c        |    4 ++--
 drivers/net/ethernet/sfc/net_driver.h |   12 ------------
 drivers/net/ethernet/sfc/nic.c        |    4 ++--
 3 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index b95f2e1..ca2a348 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1976,14 +1976,14 @@ static void efx_set_rx_mode(struct net_device *net_dev)
 		netdev_for_each_mc_addr(ha, net_dev) {
 			crc = ether_crc_le(ETH_ALEN, ha->addr);
 			bit = crc & (EFX_MCAST_HASH_ENTRIES - 1);
-			set_bit_le(bit, mc_hash->byte);
+			__set_bit_le(bit, mc_hash);
 		}
 
 		/* Broadcast packets go through the multicast hash filter.
 		 * ether_crc_le() of the broadcast address is 0xbe2612ff
 		 * so we always add bit 0xff to the mask.
 		 */
-		set_bit_le(0xff, mc_hash->byte);
+		__set_bit_le(0xff, mc_hash);
 	}
 
 	if (efx->port_enabled)
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 0e57535..6f1a7f7 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -1080,18 +1080,6 @@ static inline struct efx_rx_buffer *efx_rx_buffer(struct efx_rx_queue *rx_queue,
 	return &rx_queue->buffer[index];
 }
 
-/* Set bit in a little-endian bitfield */
-static inline void set_bit_le(unsigned nr, unsigned char *addr)
-{
-	addr[nr / 8] |= (1 << (nr % 8));
-}
-
-/* Clear bit in a little-endian bitfield */
-static inline void clear_bit_le(unsigned nr, unsigned char *addr)
-{
-	addr[nr / 8] &= ~(1 << (nr % 8));
-}
-
 
 /**
  * EFX_MAX_FRAME_LEN - calculate maximum frame length
diff --git a/drivers/net/ethernet/sfc/nic.c b/drivers/net/ethernet/sfc/nic.c
index 4a9a5be..bb0172d 100644
--- a/drivers/net/ethernet/sfc/nic.c
+++ b/drivers/net/ethernet/sfc/nic.c
@@ -473,9 +473,9 @@ void efx_nic_init_tx(struct efx_tx_queue *tx_queue)
 
 		efx_reado(efx, &reg, FR_AA_TX_CHKSM_CFG);
 		if (tx_queue->queue & EFX_TXQ_TYPE_OFFLOAD)
-			clear_bit_le(tx_queue->queue, (void *)&reg);
+			__clear_bit_le(tx_queue->queue, &reg);
 		else
-			set_bit_le(tx_queue->queue, (void *)&reg);
+			__set_bit_le(tx_queue->queue, &reg);
 		efx_writeo(efx, &reg, FR_AA_TX_CHKSM_CFG);
 	}
 
-- 
1.7.7.6


-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.