Fixup set_bit/clear_bit definition and usage

Following on from a discussion with Marek and Lukasz re: a namespace collision with set_bit and clear_bit in f_mass_storage, I noticed some inconsistencies in the definition and usage of PLATFORM__SET_BIT and PLATFORM__CLEAR_BIT as well as a similar use of __set_bit in the composite USB gadget driver.

__set_bit is lock-prefixed on x86 whereas set_bit is not and the analog driver in upstream Linux does set_bit() not __set_bit().

This series addresses all of those inconsistencies.

There are some usages of __set_bit() but those are in SoC specific GPIO code-paths and therefore don't really need to change IMO.

Bryan O'Donoghue (9):
x86: Define PLATFORM__SET_BIT for generic_set_bit()
riscv: Define PLATFORM__SET_BIT for generic_set_bit()
riscv: : Define PLATFORM__CLEAR_BIT for generic_clear_bit()
nios2: Define PLATFORM__SET_BIT for generic_set_bit()
nios2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit()
nds32: Define PLATFORM__SET_BIT for generic_set_bit()
nds2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit()
usb: f_mass_storage: Fix set_bit and clear_bit usage
usb: composite convert __set_bit to generic_set_bit

 arch/nds32/include/asm/bitops.h            |  4 ++++
 arch/nios2/include/asm/bitops/non-atomic.h |  4 ++++
 arch/riscv/include/asm/bitops.h            |  4 ++++
 arch/x86/include/asm/bitops.h              |  2 ++
 drivers/usb/gadget/composite.c             |  2 +-
 drivers/usb/gadget/f_mass_storage.c        | 25 +++-------------------
 6 files changed, 18 insertions(+), 23 deletions(-)

2.17.0

x86 bitops.h provides a __set_bit() but does not define PLATFORM__SET_BIT as a result generic_set_bit() is used instead of the architecturally provided __set_bit().

Hook the arch __set_bit() now.

---
 arch/x86/include/asm/bitops.h | 2 ++
 1 file changed, 2 insertions(+)

index f97dc66439..196fcf9d3f 100644
@@ -61,6 +61,8 @@ static __inline__ void __set_bit(int nr, volatile void * addr)
 		:"Ir" (nr));
 }
 
+#define PLATFORM__SET_BIT
+
 /**
  * clear_bit - Clears a bit in memory
  * @nr: Bit to clear

2.17.0

riscv bitops.h provides a __set_bit() but does not define PLATFORM__SET_BIT as a result generic_set_bit() is used instead of the architecturally provided __set_bit().

Hook the arch __set_bit() now.

---
 arch/riscv/include/asm/bitops.h | 2 ++
 1 file changed, 2 insertions(+)

index 55d420fdfb..0149e5c696 100644
@@ -42,6 +42,8 @@ static inline void __set_bit(int nr, void *addr)
 	*a |= mask;
 }
 
+#define PLATFORM__SET_BIT
+
 static inline void __clear_bit(int nr, void *addr)
 {
 	int *a = (int *)addr;

2.17.0

riscv bitops.h provides a __clear_bit() but does not define PLATFORM__CLEAR_BIT as a result generic_clear_bit() is used instead of the architecturally provided __clear_bit().

Hook the arch __clear_bit() now.

---
 arch/riscv/include/asm/bitops.h | 2 ++
 1 file changed, 2 insertions(+)

index 0149e5c696..536629bbec 100644
@@ -54,6 +54,8 @@ static inline void __clear_bit(int nr, void *addr)
 	*a &= ~mask;
 }
 
+#define PLATFORM__CLEAR_BIT
+
 static inline void __change_bit(int nr, void *addr)
 {
 	int mask;

2.17.0

nios2 bitops.h provides a __set_bit() but does not define PLATFORM__SET_BIT as a result generic_set_bit() is used instead of the architecturally provided __set_bit().

Hook the arch __set_bit() now.

---
 arch/nios2/include/asm/bitops/non-atomic.h | 2 ++
 1 file changed, 2 insertions(+)

index 697cc2b7e0..9dd9d923e1 100644
@@ -20,6 +20,8 @@ static inline void __set_bit(int nr, volatile unsigned long *addr)
 	*p  |= mask;
 }
 
+#define PLATFORM__SET_BIT
+
 static inline void __clear_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);

2.17.0

nios2 bitops.h provides a __clear_bit() but does not define PLATFORM__CLEAR_BIT as a result generic_clear_bit() is used instead of the architecturally provided __clear_bit().

Hook the arch __clear_bit() now.

---
 arch/nios2/include/asm/bitops/non-atomic.h | 2 ++
 1 file changed, 2 insertions(+)

index 9dd9d923e1..f746819b43 100644
@@ -30,6 +30,8 @@ static inline void __clear_bit(int nr, volatile unsigned long *addr)
 	*p &= ~mask;
 }
 
+#define PLATFORM__CLEAR_BIT
+
 /**
  * __change_bit - Toggle a bit in memory
  * @nr: the bit to change

2.17.0

nds32 bitops.h provides a __set_bit() but does not define PLATFORM__SET_BIT as a result generic_set_bit() is used instead of the architecturally provided __set_bit().

Hook the arch __set_bit() now.

---
 arch/nds32/include/asm/bitops.h | 2 ++
 1 file changed, 2 insertions(+)

index 7ee37c37bc..ecd0032664 100644
@@ -44,6 +44,8 @@ static inline void __set_bit(int nr, void *addr)
 	*a |= mask;
 }
 
+#define PLATFORM__SET_BIT
+
 extern void clear_bit(int nr, void *addr);
 
 static inline void __clear_bit(int nr, void *addr)

2.17.0

nds2 bitops.h provides a __clear_bit() but does not define PLATFORM__CLEAR_BIT as a result generic_clear_bit() is used instead of the architecturally provided __clear_bit().

Hook the arch __clear_bit() now.

---
 arch/nds32/include/asm/bitops.h | 2 ++
 1 file changed, 2 insertions(+)

index ecd0032664..f1cdcf3e65 100644
@@ -61,6 +61,8 @@ static inline void __clear_bit(int nr, void *addr)
 	local_irq_restore(flags);
 }
 
+#define PLATFORM__CLEAR_BIT
+
 extern void change_bit(int nr, void *addr);
 
 static inline void __change_bit(int nr, void *addr)

2.17.0

Compiling the f_mass_storage driver for an x86 target results in a compilation error as set_bit and clear_bit are provided by bitops.h

Looking at the provenance of the current u-boot code and the git change history in the kernel, it looks like we have a local copy of set_bit and clear_bit as a hold-over from porting the Linux driver into u-boot.

These days __set_bit and __clear_bit are optionally provided by an arch and can be used as inputs to generic_bit_set and generic_bit_clear.

This patch switches over to generic_set_bit and generic_clear_bit to accommodate.

Tested on i.MX WaRP7 and Intel Edison

---
 drivers/usb/gadget/f_mass_storage.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

index 1ecb92ac6b..af42136e1d 100644
@@ -252,6 +252,7 @@
 #include <usb_mass_storage.h>
 
 #include <asm/unaligned.h>
+#include <asm/bitops.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/composite.h>

@@ -283,26 +284,6 @@ static const char fsg_string_interface[] = "Mass Storage";
 struct kref {int x; };
 struct completion {int x; };
 
-inline void set_bit(int nr, volatile void *addr)
-{
-	int	mask;
-	unsigned int *a = (unsigned int *) addr;
-
-	a += nr >> 5;
-	mask = 1 << (nr & 0x1f);
-	*a |= mask;
-}
-
-inline void clear_bit(int nr, volatile void *addr)
-{
-	int	mask;
-	unsigned int *a = (unsigned int *) addr;
-
-	a += nr >> 5;
-	mask = 1 << (nr & 0x1f);
-	*a &= ~mask;
-}
-
 struct fsg_dev;
 struct fsg_common;
 

@@ -2086,7 +2067,7 @@ static int received_cbw(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 		 * we can simply accept and discard any data received
 		 * until the next reset. */
 		wedge_bulk_in_endpoint(fsg);
-		set_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags);
+		generic_set_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags);
 		return -EINVAL;
 	}
 

@@ -2250,7 +2231,7 @@ reset:
 	fsg->bulk_out_enabled = 1;
 	common->bulk_out_maxpacket =
 				le16_to_cpu(get_unaligned(&d->wMaxPacketSize));
-	clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags);
+	generic_clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags);
 
 	/* Allocate the requests */
 	for (i = 0; i < FSG_NUM_BUFFERS; ++i) {

2.17.0

Hi Bryan,

Just I'm a bit puzzled:

This is v3 8/9, followed by 9/9 (v1)?

And after some time you sent v2 8/9 and v2 9/9.

I suppose that the latter ones are correct?

Also it is very handy to have some kind of change log in patches - i.e.:

Changes for v2:
- Switching to generic_set|clear bit functions
- ......

The patch seems OK, but I need to test them on my machines.

I sent out two previous versions of this patch as a standalone, if you recall.

Cover letter contains, and I believe best practice is to keep the changelog in the cover-letter - not the patch itself.

##
V2:
- Fix commit log nds2 -> nds32
- Fix copy/paste error resulting in double colon "arch : : text"

V1:
Following on from a discussion with Marek and Lukasz re: a namespace collision with set_bit and clear_bit in f_mass_storage, I noticed some inconsistencies in the definition and usage of PLATFORM__SET_BIT and PLATFORM__CLEAR_BIT as well as a similar use of __set_bit in the composite USB gadget driver.

__set_bit is lock-prefixed on x86 whereas set_bit is not and the analog driver in upstream Linux does set_bit() not __set_bit().

This series addresses all of those inconsistencies.

There are some usages of __set_bit() but those are in SoC specific GPIO code-paths and therefore don't really need to change IMO.
##

Anyway I'll resend this set as a v3 since Bin Meng needs a change to the log text anyway.

---
bod

Compiling the f_mass_storage driver for an x86 target results in a compilation error as set_bit and clear_bit are provided by bitops.h

To address that situation we discussed on the list moving to genetic_set_bit() instead.

Doing a quick grep for similar situations in drivers/usb shows that the composite device is using __set_bit().

This patch switches over to generic_set_bit to maintain consistency between the two gadget drivers.

---
 drivers/usb/gadget/composite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d0ee7847b9..e5d802a80e 100644
@@ -379,7 +379,7 @@ static int set_config(struct usb_composite_dev *cdev,
 			ep = (struct usb_endpoint_descriptor *)*descriptors;
 			addr = ((ep->bEndpointAddress & 0x80) >> 3)
 			     |	(ep->bEndpointAddress & 0x0f);
-			__set_bit(addr, f->endpoints);
+			generic_set_bit(addr, f->endpoints);
 		}
 
 		result = f->set_alt(f, tmp, 0);

2.17.0

Hi Bryan,

I am confused. So this changes enables x86's own __set_bit which is lock-prefixed. But you are saying the upstream Linux is using set_bit which is not lock-prefixed. Then this is inconsistent again.

Regards,
Bin

No sorry Bin,

It's the other way around.

Let me reword this for you..

/**
* set_bit - Atomically set a bit in memory
* @nr: the bit to set
* @addr: the address to start counting from
*
* This function is atomic and may not be reordered. See __set_bit()
* if you do not require the atomic guarantees.
* Note that @nr may be almost arbitrarily large; this function is not
* restricted to acting on a single-word quantity.
*/
static __inline__ void set_bit(int nr, volatile void * addr)

{
         __asm__ __volatile__( LOCK_PREFIX
                 "btsl %1,%0"
                 :"=m" (ADDR)
                 :"Ir" (nr));
}

/**
* __set_bit - Set a bit in memory
* @nr: the bit to set
* @addr: the address to start counting from
*
* Unlike set_bit(), this function is non-atomic and may be reordered.
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
static __inline__ void __set_bit(int nr, volatile void * addr)

{
         __asm__(
                 "btsl %1,%0"
                 :"=m" (ADDR)
                 :"Ir" (nr));
}