[PATCH 0/5] Introduce generic set_bit_le() -v2

[PATCH 0/5] Introduce generic set_bit_le() -v2

[Takuya Yoshikawa]

  [ Andrew, can you take this or should I send to other person?
    Note: the whole series is against linux-next. ]

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(), a bit of
preparation is also needed.

  Although these are differently implemented, especially for big-endian
  case, than the generic __set_bit_le(), it should not be a problem to
  use the latter since both maintainers prefer it.

Changes from v1:
  - sfc:     Ben made a patch
  - tulip:   followed suggestion by Grant
  - bitops:  added clear_bit_le -- suggested by Arnd
  - powerpc: added the same code


Ben Hutchings (1):
  sfc: Use standard __{clear,set}_bit_le() functions

Takuya Yoshikawa (4):
  drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function
  bitops: Introduce generic {clear,set}_bit_le()
  powerpc: bitops: Introduce {clear,set}_bit_le()
  KVM: Replace test_and_set_bit_le() in mark_page_dirty_in_slot() with set_bit_le()

 arch/powerpc/include/asm/bitops.h           |   10 ++++++++++
 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       |   12 ------------
 drivers/net/ethernet/sfc/nic.c              |    4 ++--
 include/asm-generic/bitops/le.h             |   10 ++++++++++
 virt/kvm/kvm_main.c                         |    3 +--
 8 files changed, 29 insertions(+), 28 deletions(-)

-- 
1.7.5.4

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

[Takuya Yoshikawa]

From: Ben Hutchings <bhutchings@solarflare.com>

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>
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 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.5.4

[PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Takuya Yoshikawa]

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

To introduce generic set_bit_le() later, we remove our own definition
and use a proper non-atomic bitops function: __set_bit_le().

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Acked-by: 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, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/dec/tulip/de2104x.c b/drivers/net/ethernet/dec/tulip/de2104x.c
index 61cc093..77335853 100644
--- a/drivers/net/ethernet/dec/tulip/de2104x.c
+++ b/drivers/net/ethernet/dec/tulip/de2104x.c
@@ -661,9 +661,6 @@ 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)
-
 static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
 {
 	struct de_private *de = netdev_priv(dev);
@@ -673,12 +670,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 */
+	__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);
+		__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..885700a 100644
--- a/drivers/net/ethernet/dec/tulip/tulip_core.c
+++ b/drivers/net/ethernet/dec/tulip/tulip_core.c
@@ -1010,9 +1010,6 @@ 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)
-
 static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
 {
 	struct tulip_private *tp = netdev_priv(dev);
@@ -1022,12 +1019,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 */
+	__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);
+		__set_bit_le(index, hash_table);
 	}
 	for (i = 0; i < 32; i++) {
 		*setup_frm++ = hash_table[i];
-- 
1.7.5.4

[PATCH 3/5] bitops: Introduce generic {clear,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>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
 include/asm-generic/bitops/le.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/asm-generic/bitops/le.h b/include/asm-generic/bitops/le.h
index f95c663..6173154 100644
--- a/include/asm-generic/bitops/le.h
+++ b/include/asm-generic/bitops/le.h
@@ -54,6 +54,16 @@ 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 clear_bit_le(int nr, void *addr)
+{
+	clear_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/5] powerpc: bitops: Introduce {clear,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: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/include/asm/bitops.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/bitops.h b/arch/powerpc/include/asm/bitops.h
index efdc926..dc2cf9c 100644
--- a/arch/powerpc/include/asm/bitops.h
+++ b/arch/powerpc/include/asm/bitops.h
@@ -288,6 +288,16 @@ static __inline__ int test_bit_le(unsigned long nr,
 	return (tmp[nr >> 3] >> (nr & 7)) & 1;
 }
 
+static inline void set_bit_le(int nr, void *addr)
+{
+	set_bit(nr ^ BITOP_LE_SWIZZLE, addr);
+}
+
+static inline void clear_bit_le(int nr, void *addr)
+{
+	clear_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 5/5] 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>
Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
---
 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 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Akinobu Mita]

2012/6/13 Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>:
> From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
>
> To introduce generic set_bit_le() later, we remove our own definition
> and use a proper non-atomic bitops function: __set_bit_le().
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> Acked-by: 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, 4 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/de2104x.c b/drivers/net/ethernet/dec/tulip/de2104x.c
> index 61cc093..77335853 100644
> --- a/drivers/net/ethernet/dec/tulip/de2104x.c
> +++ b/drivers/net/ethernet/dec/tulip/de2104x.c
> @@ -661,9 +661,6 @@ 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)
> -
>  static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
>  {
>        struct de_private *de = netdev_priv(dev);
> @@ -673,12 +670,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 */
> +       __set_bit_le(255, hash_table);                  /* Broadcast entry */

Should this hash_table be converted from u16 hash_table[32] to
DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
on long-word boundary?

Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Takuya Yoshikawa]

On Wed, 13 Jun 2012 18:43:40 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> Should this hash_table be converted from u16 hash_table[32] to
> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
> on long-word boundary?

I think hash_table is already long-word aligned because it is placed
right after a pointer.

	Takuya

Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Akinobu Mita]

2012/6/13 Takuya Yoshikawa <takuya.yoshikawa@gmail.com>:
> On Wed, 13 Jun 2012 18:43:40 +0900
> Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
>> Should this hash_table be converted from u16 hash_table[32] to
>> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
>> on long-word boundary?
>
> I think hash_table is already long-word aligned because it is placed
> right after a pointer.

I recommend converting to proper bitmap.  Because such an implicit
assumption is easily broken by someone touching this function.

Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Takuya Yoshikawa]

On Wed, 13 Jun 2012 22:31:13 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> >> Should this hash_table be converted from u16 hash_table[32] to
> >> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
> >> on long-word boundary?
> >
> > I think hash_table is already long-word aligned because it is placed
> > right after a pointer.
> 
> I recommend converting to proper bitmap.  Because such an implicit
> assumption is easily broken by someone touching this function.

Do you mean something like:
	DECLARE_BITMAP(__hash_table, 16 * 32);
	u16 *hash_table = (u16 *)__hash_table;
?

Grant, what do you think about this?

	Takuya


===
drivers/net/ethernet/dec/tulip/tulip_core.c:

static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
{
	struct tulip_private *tp = netdev_priv(dev);
	u16 hash_table[32];
	...
}

drivers/net/ethernet/dec/tulip/de2104x.c:

static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
{
	struct de_private *de = netdev_priv(dev);
	u16 hash_table[32];
	...
}

Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Ben Hutchings]

On Wed, 2012-06-13 at 23:00 +0900, Takuya Yoshikawa wrote:
> On Wed, 13 Jun 2012 22:31:13 +0900
> Akinobu Mita <akinobu.mita@gmail.com> wrote:
> 
> > >> Should this hash_table be converted from u16 hash_table[32] to
> > >> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
> > >> on long-word boundary?
> > >
> > > I think hash_table is already long-word aligned because it is placed
> > > right after a pointer.
> > 
> > I recommend converting to proper bitmap.  Because such an implicit
> > assumption is easily broken by someone touching this function.
> 
> Do you mean something like:
> 	DECLARE_BITMAP(__hash_table, 16 * 32);
> 	u16 *hash_table = (u16 *)__hash_table;
> ?
[...]

Could this driver perhaps use:

	union hash_table {
		DECLARE_BITMAP(bits, 16 * 32);
		__le16 words[32];
	};

Ben.

-- 
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.

Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Grant Grundler]

On Wed, Jun 13, 2012 at 7:00 AM, Takuya Yoshikawa
<takuya.yoshikawa@gmail.com> wrote:
> On Wed, 13 Jun 2012 22:31:13 +0900
> Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
>> >> Should this hash_table be converted from u16 hash_table[32] to
>> >> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
>> >> on long-word boundary?
>> >
>> > I think hash_table is already long-word aligned because it is placed
>> > right after a pointer.
>>
>> I recommend converting to proper bitmap.  Because such an implicit
>> assumption is easily broken by someone touching this function.
>
> Do you mean something like:
>        DECLARE_BITMAP(__hash_table, 16 * 32);
>        u16 *hash_table = (u16 *)__hash_table;
> ?
>
> Grant, what do you think about this?

Hi Takuya,
two thoughts:
1) while I agree with Akinobu and thank him for pointing out a
_potential_ alignment problem, this is a separate issue and your
existing patch should go in anyway. There are probably other drivers
with _potential_ alignment issues. Akinobu could get credit for
finding them by submitting patches after reviewing calls to set_bit
and set_bit_le() - similar to what you are doing now.

2) I generally do not like declaring one type and then using an alias
of a different type to reference the same memory address. We have a
simple alternative since hash_table[] is indexed directly only in one
hunk of code:
        for (i = 0; i < 32; i++) {
                *setup_frm++ = ((u16 *)hash_table)[i];
                *setup_frm++ = ((u16 *)hash_table)[i];
        }

thanks,
grant

>
>        Takuya
>
>
> ===
> drivers/net/ethernet/dec/tulip/tulip_core.c:
>
> static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
> {
>        struct tulip_private *tp = netdev_priv(dev);
>        u16 hash_table[32];
>        ...
> }
>
> drivers/net/ethernet/dec/tulip/de2104x.c:
>
> static void build_setup_frame_hash(u16 *setup_frm, struct net_device *dev)
> {
>        struct de_private *de = netdev_priv(dev);
>        u16 hash_table[32];
>        ...
> }

Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Takuya Yoshikawa]

On Wed, 13 Jun 2012 08:21:18 -0700
Grant Grundler <grantgrundler@gmail.com> wrote:

> >> >> Should this hash_table be converted from u16 hash_table[32] to
> >> >> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
> >> >> on long-word boundary?
> >> >
> >> > I think hash_table is already long-word aligned because it is placed
> >> > right after a pointer.
> >>
> >> I recommend converting to proper bitmap.  Because such an implicit
> >> assumption is easily broken by someone touching this function.
> >
> > Do you mean something like:
> >        DECLARE_BITMAP(__hash_table, 16 * 32);
> >        u16 *hash_table = (u16 *)__hash_table;
> > ?
> >
> > Grant, what do you think about this?
> 
> Hi Takuya,
> two thoughts:
> 1) while I agree with Akinobu and thank him for pointing out a
> _potential_ alignment problem, this is a separate issue and your
> existing patch should go in anyway. There are probably other drivers
> with _potential_ alignment issues. Akinobu could get credit for
> finding them by submitting patches after reviewing calls to set_bit
> and set_bit_le() - similar to what you are doing now.

I prefer approach 1.

hash_table is local in build_setup_frame_hash(), so if further
improvement is also required, we can do that locally there later.

Thanks,
	Takuya


> 2) I generally do not like declaring one type and then using an alias
> of a different type to reference the same memory address. We have a
> simple alternative since hash_table[] is indexed directly only in one
> hunk of code:
>         for (i = 0; i < 32; i++) {
>                 *setup_frm++ = ((u16 *)hash_table)[i];
>                 *setup_frm++ = ((u16 *)hash_table)[i];
>         }

Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Akinobu Mita]

2012/6/14 Takuya Yoshikawa <takuya.yoshikawa@gmail.com>:
> On Wed, 13 Jun 2012 08:21:18 -0700
> Grant Grundler <grantgrundler@gmail.com> wrote:
>
>> >> >> Should this hash_table be converted from u16 hash_table[32] to
>> >> >> DECLARE_BITMAP(hash_table, 16 * 32) to ensure that it is aligned
>> >> >> on long-word boundary?
>> >> >
>> >> > I think hash_table is already long-word aligned because it is placed
>> >> > right after a pointer.
>> >>
>> >> I recommend converting to proper bitmap.  Because such an implicit
>> >> assumption is easily broken by someone touching this function.
>> >
>> > Do you mean something like:
>> >        DECLARE_BITMAP(__hash_table, 16 * 32);
>> >        u16 *hash_table = (u16 *)__hash_table;
>> > ?
>> >
>> > Grant, what do you think about this?
>>
>> Hi Takuya,
>> two thoughts:
>> 1) while I agree with Akinobu and thank him for pointing out a
>> _potential_ alignment problem, this is a separate issue and your
>> existing patch should go in anyway. There are probably other drivers
>> with _potential_ alignment issues. Akinobu could get credit for
>> finding them by submitting patches after reviewing calls to set_bit
>> and set_bit_le() - similar to what you are doing now.
>
> I prefer approach 1.
>
> hash_table is local in build_setup_frame_hash(), so if further
> improvement is also required, we can do that locally there later.

This potential alignment problem is introduced by this patch.  Because
the original set_bit_le() in tulip driver can handle unaligned bitmap.
This is why I recommended it should be fixed in this patch.

But please just ignore me if I'm too much paranoid.  And I'll handle
this issue if no one wants to do it.

Re: [PATCH 2/5] drivers/net/ethernet/dec/tulip: Use standard __set_bit_le() function

[Takuya Yoshikawa]

On Thu, 14 Jun 2012 18:36:42 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:

> >> 1) while I agree with Akinobu and thank him for pointing out a
> >> _potential_ alignment problem, this is a separate issue and your
> >> existing patch should go in anyway. There are probably other drivers
> >> with _potential_ alignment issues. Akinobu could get credit for
> >> finding them by submitting patches after reviewing calls to set_bit
> >> and set_bit_le() - similar to what you are doing now.
> >
> > I prefer approach 1.
> >
> > hash_table is local in build_setup_frame_hash(), so if further
> > improvement is also required, we can do that locally there later.
> 
> This potential alignment problem is introduced by this patch.  Because
> the original set_bit_le() in tulip driver can handle unaligned bitmap.
> This is why I recommended it should be fixed in this patch.

The original set_bit_le() was used only in build_setup_frame_hash().

If it's clear that the table is aligned locally in the function, I do
not think the __potential__ problem is introduced by this patch.

As you can see from my response to Arnd in v1 thread, I knew the
alignment requirement at that time and checked the definition of
hash_table before using __set_bit_le().

> But please just ignore me if I'm too much paranoid.  And I'll handle
> this issue if no one wants to do it.

I'm open to suggestions.

But now that the maintainer who can test the driver on real hardware
has suggested this patch should go in, I won't change the patch without
any real issue.

I would thank you if you improve this driver later on top of that.

Thanks,
	Takuya

Re: [PATCH 4/5] powerpc: bitops: Introduce {clear,set}_bit_le()

[Benjamin Herrenschmidt]

On Wed, 2012-06-13 at 13:04 +0900, 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>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> ---
>  arch/powerpc/include/asm/bitops.h |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/bitops.h b/arch/powerpc/include/asm/bitops.h
> index efdc926..dc2cf9c 100644
> --- a/arch/powerpc/include/asm/bitops.h
> +++ b/arch/powerpc/include/asm/bitops.h
> @@ -288,6 +288,16 @@ static __inline__ int test_bit_le(unsigned long nr,
>  	return (tmp[nr >> 3] >> (nr & 7)) & 1;
>  }
>  
> +static inline void set_bit_le(int nr, void *addr)
> +{
> +	set_bit(nr ^ BITOP_LE_SWIZZLE, addr);
> +}
> +
> +static inline void clear_bit_le(int nr, void *addr)
> +{
> +	clear_bit(nr ^ BITOP_LE_SWIZZLE, addr);
> +}
> +
>  static inline void __set_bit_le(int nr, void *addr)
>  {
>  	__set_bit(nr ^ BITOP_LE_SWIZZLE, addr);