[U-Boot] [PATCH v3 0/9] Fixup set_bit/clear_bit definition and usage

v3: - Use linux/bitops.h instead of asm/bitops.h - checkpatch.pl
- Updated commit logs to make intended usage of __set_bit() and __clear_bit() clearer with respect to generic_set_bit() - Bin Meng
- Added Patch #8 changelong to cover-letter - Lukasz Majewski Initial patch fixes compile error for x86 by ifdeffing the local set_bit() clear_bit()
Second patch renames to _set_bit() and _clear_bit() respectively
Third patch - on further discussion we agree to use generic_set_bit() and generic_clear_bit().
Using generic_set_bit() and generic_clear_bit() prompts me to do a general tidy-up of set_bit() and clear_bit() for USB gadget and to ensure where architectures provide __set_bit() and/or __clear_bit() that said functions are appropriately stitched into generic_set_bit() and generic_clear_bit() respectively.
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.
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() nds32: 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(-)

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().
This patch defines PLATFORM__SET_BIT which means that __set_bit() in x86 bitops.h will be called whenever generic_set_bit() is called - as opposed to the default cross-platform generic_set_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Simon Glass sjg@chromium.org Cc: Bin Meng bmeng.cn@gmail.com --- arch/x86/include/asm/bitops.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h index f97dc66439..196fcf9d3f 100644 --- a/arch/x86/include/asm/bitops.h +++ b/arch/x86/include/asm/bitops.h @@ -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

On Mon, Apr 30, 2018 at 03:56:02PM +0100, Bryan O'Donoghue wrote:
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().
This patch defines PLATFORM__SET_BIT which means that __set_bit() in x86 bitops.h will be called whenever generic_set_bit() is called - as opposed to the default cross-platform generic_set_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Simon Glass sjg@chromium.org Cc: Bin Meng bmeng.cn@gmail.com
Applied to u-boot/master, thanks!

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().
This patch defines PLATFORM__SET_BIT which means that __set_bit() in x86 bitops.h will be called whenever generic_set_bit() is called - as opposed to the default cross-platform generic_set_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Rick Chen rick@andestech.com Cc: Greentime Hu green.hu@gmail.com --- arch/riscv/include/asm/bitops.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h index 55d420fdfb..0149e5c696 100644 --- a/arch/riscv/include/asm/bitops.h +++ b/arch/riscv/include/asm/bitops.h @@ -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;

On Mon, Apr 30, 2018 at 03:56:03PM +0100, Bryan O'Donoghue wrote:
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().
This patch defines PLATFORM__SET_BIT which means that __set_bit() in x86 bitops.h will be called whenever generic_set_bit() is called - as opposed to the default cross-platform generic_set_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Rick Chen rick@andestech.com Cc: Greentime Hu green.hu@gmail.com
Applied to u-boot/master, thanks!

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().
This patch defines PLATFORM__CLEAR_BIT which means that __clear_bit() in riscv bitops.h will be called whenever generic_clear_bit() is called - as opposed to the default cross-platform generic_clear_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Rick Chen rick@andestech.com Cc: Greentime Hu green.hu@gmail.com --- arch/riscv/include/asm/bitops.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h index 0149e5c696..536629bbec 100644 --- a/arch/riscv/include/asm/bitops.h +++ b/arch/riscv/include/asm/bitops.h @@ -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;

On Mon, Apr 30, 2018 at 03:56:04PM +0100, Bryan O'Donoghue wrote:
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().
This patch defines PLATFORM__CLEAR_BIT which means that __clear_bit() in riscv bitops.h will be called whenever generic_clear_bit() is called - as opposed to the default cross-platform generic_clear_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Rick Chen rick@andestech.com Cc: Greentime Hu green.hu@gmail.com
Applied to u-boot/master, thanks!

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().
This patch defines PLATFORM__SET_BIT which means that __set_bit() in nios2 bitops.h will be called whenever generic_set_bit() is called - as opposed to the default cross-platform generic_set_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Thomas Chou thomas@wytron.com.tw --- arch/nios2/include/asm/bitops/non-atomic.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/nios2/include/asm/bitops/non-atomic.h b/arch/nios2/include/asm/bitops/non-atomic.h index 697cc2b7e0..9dd9d923e1 100644 --- a/arch/nios2/include/asm/bitops/non-atomic.h +++ b/arch/nios2/include/asm/bitops/non-atomic.h @@ -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);

On Mon, Apr 30, 2018 at 03:56:05PM +0100, Bryan O'Donoghue wrote:
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().
This patch defines PLATFORM__SET_BIT which means that __set_bit() in nios2 bitops.h will be called whenever generic_set_bit() is called - as opposed to the default cross-platform generic_set_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Thomas Chou thomas@wytron.com.tw
Applied to u-boot/master, thanks!

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().
This patch defines PLATFORM__CLEAR_BIT which means that __clear_bit() in nios2 bitops.h will be called whenever generic_clear_bit() is called - as opposed to the default cross-platform generic_clear_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Thomas Chou thomas@wytron.com.tw --- arch/nios2/include/asm/bitops/non-atomic.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/nios2/include/asm/bitops/non-atomic.h b/arch/nios2/include/asm/bitops/non-atomic.h index 9dd9d923e1..f746819b43 100644 --- a/arch/nios2/include/asm/bitops/non-atomic.h +++ b/arch/nios2/include/asm/bitops/non-atomic.h @@ -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

On Mon, Apr 30, 2018 at 03:56:06PM +0100, Bryan O'Donoghue wrote:
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().
This patch defines PLATFORM__CLEAR_BIT which means that __clear_bit() in nios2 bitops.h will be called whenever generic_clear_bit() is called - as opposed to the default cross-platform generic_clear_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Thomas Chou thomas@wytron.com.tw
Applied to u-boot/master, thanks!

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().
This patch defines PLATFORM__SET_BIT which means that __set_bit() in nds32 bitops.h will be called whenever generic_set_bit() is called - as opposed to the default cross-platform generic_set_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Macpaul Lin macpaul@andestech.com --- arch/nds32/include/asm/bitops.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/nds32/include/asm/bitops.h b/arch/nds32/include/asm/bitops.h index 7ee37c37bc..ecd0032664 100644 --- a/arch/nds32/include/asm/bitops.h +++ b/arch/nds32/include/asm/bitops.h @@ -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)

On Mon, Apr 30, 2018 at 03:56:07PM +0100, Bryan O'Donoghue wrote:
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().
This patch defines PLATFORM__SET_BIT which means that __set_bit() in nds32 bitops.h will be called whenever generic_set_bit() is called - as opposed to the default cross-platform generic_set_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Macpaul Lin macpaul@andestech.com
Applied to u-boot/master, thanks!

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().
This patch defines PLATFORM__CLEAR_BIT which means that __clear_bit() in nds32 bitops.h will be called whenever generic_clear_bit() is called - as opposed to the default cross-platform generic_clear_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Macpaul Lin macpaul@andestech.com --- arch/nds32/include/asm/bitops.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/nds32/include/asm/bitops.h b/arch/nds32/include/asm/bitops.h index ecd0032664..f1cdcf3e65 100644 --- a/arch/nds32/include/asm/bitops.h +++ b/arch/nds32/include/asm/bitops.h @@ -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)

On Mon, Apr 30, 2018 at 03:56:08PM +0100, Bryan O'Donoghue wrote:
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().
This patch defines PLATFORM__CLEAR_BIT which means that __clear_bit() in nds32 bitops.h will be called whenever generic_clear_bit() is called - as opposed to the default cross-platform generic_clear_bit().
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Macpaul Lin macpaul@andestech.com
Applied to u-boot/master, thanks!

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
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Lukasz Majewski lukma@denx.de Cc: Marek Vasut marex@denx.de --- drivers/usb/gadget/f_mass_storage.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index 1ecb92ac6b..209932df45 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -252,6 +252,7 @@ #include <usb_mass_storage.h>
#include <asm/unaligned.h> +#include <linux/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) {

On Mon, Apr 30, 2018 at 03:56:09PM +0100, Bryan O'Donoghue wrote:
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
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Lukasz Majewski lukma@denx.de Cc: Marek Vasut marex@denx.de
Applied to u-boot/master, thanks!

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.
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Lukasz Majewski lukma@denx.de Cc: Marek Vasut marex@denx.de --- drivers/usb/gadget/composite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index a87639def9..9229d9ee30 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -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);

On Mon, Apr 30, 2018 at 03:56:10PM +0100, Bryan O'Donoghue wrote:
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.
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Cc: Lukasz Majewski lukma@denx.de Cc: Marek Vasut marex@denx.de
Applied to u-boot/master, thanks!

On Mon, Apr 30, 2018 at 03:56:01PM +0100, Bryan O'Donoghue wrote:
v3:
Use linux/bitops.h instead of asm/bitops.h
- checkpatch.pl
Updated commit logs to make intended usage of __set_bit() and __clear_bit() clearer with respect to generic_set_bit() - Bin Meng
Added Patch #8 changelong to cover-letter - Lukasz Majewski Initial patch fixes compile error for x86 by ifdeffing the local set_bit() clear_bit()
Second patch renames to _set_bit() and _clear_bit() respectively
Third patch - on further discussion we agree to use generic_set_bit() and generic_clear_bit().
Using generic_set_bit() and generic_clear_bit() prompts me to do a general tidy-up of set_bit() and clear_bit() for USB gadget and to ensure where architectures provide __set_bit() and/or __clear_bit() that said functions are appropriately stitched into generic_set_bit() and generic_clear_bit() respectively.
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.
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() nds32: 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(-)
This seems right, but I see a whole lot of CC and no Acks/Reviews, so I'm going to wait a bit more before grabbing it. It would be great to see a travis-ci link before I try myself, thanks!
-- 2.17.0
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
participants (2)
-
Bryan O'Donoghue
-
Tom Rini