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
From: Takuya Yoshikawa <[email protected]>
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 <[email protected]>
---
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
From: Takuya Yoshikawa <[email protected]>
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 <[email protected]>
Cc: Arnd Bergmann <[email protected]>
---
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
From: Takuya Yoshikawa <[email protected]>
Needed to introduce generic set_bit_le().
Signed-off-by: Takuya Yoshikawa <[email protected]>
Cc: Grant Grundler <[email protected]>
---
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
From: Takuya Yoshikawa <[email protected]>
Needed to introduce generic set_bit_le().
Signed-off-by: Takuya Yoshikawa <[email protected]>
Cc: Ben Hutchings <[email protected]>
---
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, ®, FR_AA_TX_CHKSM_CFG);
if (tx_queue->queue & EFX_TXQ_TYPE_OFFLOAD)
- clear_bit_le(tx_queue->queue, (void *)®);
+ efx_clear_bit_le(tx_queue->queue, (void *)®);
else
- set_bit_le(tx_queue->queue, (void *)®);
+ efx_set_bit_le(tx_queue->queue, (void *)®);
efx_writeo(efx, ®, FR_AA_TX_CHKSM_CFG);
}
--
1.7.5.4
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
On Monday 11 June 2012, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa <[email protected]>
>
> 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 <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
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
On Mon, Jun 11, 2012 at 5:30 AM, Takuya Yoshikawa
<[email protected]> wrote:
> From: Takuya Yoshikawa <[email protected]>
>
> Needed to introduce generic set_bit_le().
>
> Signed-off-by: Takuya Yoshikawa <[email protected]>
> Cc: Grant Grundler <[email protected]>
> ---
> ?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 <[email protected]>
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
>
On Mon, 11 Jun 2012 14:09:15 +0000
Arnd Bergmann <[email protected]> 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
On Mon, 11 Jun 2012 14:10:26 +0000
Arnd Bergmann <[email protected]> wrote:
> On Monday 11 June 2012, Takuya Yoshikawa wrote:
> > From: Takuya Yoshikawa <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
>
> 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
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 <[email protected]>
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 <[email protected]>
---
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, ®, FR_AA_TX_CHKSM_CFG);
if (tx_queue->queue & EFX_TXQ_TYPE_OFFLOAD)
- clear_bit_le(tx_queue->queue, (void *)®);
+ __clear_bit_le(tx_queue->queue, ®);
else
- set_bit_le(tx_queue->queue, (void *)®);
+ __set_bit_le(tx_queue->queue, ®);
efx_writeo(efx, ®, 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.