All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage
@ 2018-04-27 15:08 Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 1/9] x86: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
                   ` (9 more replies)
  0 siblings, 10 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 1/9] x86: Define PLATFORM__SET_BIT for generic_set_bit()
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
@ 2018-04-27 15:08 ` Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 2/9] riscv: " Bryan O'Donoghue
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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.

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 2/9] riscv: Define PLATFORM__SET_BIT for generic_set_bit()
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 1/9] x86: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
@ 2018-04-27 15:08 ` Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 3/9] riscv: : Define PLATFORM__CLEAR_BIT for generic_clear_bit() Bryan O'Donoghue
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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.

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;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 3/9] riscv: : Define PLATFORM__CLEAR_BIT for generic_clear_bit()
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 1/9] x86: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 2/9] riscv: " Bryan O'Donoghue
@ 2018-04-27 15:08 ` Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 4/9] nios2: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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.

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;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 4/9] nios2: Define PLATFORM__SET_BIT for generic_set_bit()
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
                   ` (2 preceding siblings ...)
  2018-04-27 15:08 ` [U-Boot] [PATCH 3/9] riscv: : Define PLATFORM__CLEAR_BIT for generic_clear_bit() Bryan O'Donoghue
@ 2018-04-27 15:08 ` Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 5/9] nios2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit() Bryan O'Donoghue
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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.

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);
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 5/9] nios2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit()
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
                   ` (3 preceding siblings ...)
  2018-04-27 15:08 ` [U-Boot] [PATCH 4/9] nios2: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
@ 2018-04-27 15:08 ` Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 6/9] nds32: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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.

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 6/9] nds32: Define PLATFORM__SET_BIT for generic_set_bit()
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
                   ` (4 preceding siblings ...)
  2018-04-27 15:08 ` [U-Boot] [PATCH 5/9] nios2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit() Bryan O'Donoghue
@ 2018-04-27 15:08 ` Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH 7/9] nds2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit() Bryan O'Donoghue
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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.

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)
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 7/9] nds2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit()
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
                   ` (5 preceding siblings ...)
  2018-04-27 15:08 ` [U-Boot] [PATCH 6/9] nds32: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
@ 2018-04-27 15:08 ` Bryan O'Donoghue
  2018-04-27 15:08 ` [U-Boot] [PATCH v3 8/9] usb: f_mass_storage: Fix set_bit and clear_bit usage Bryan O'Donoghue
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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.

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)
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH v3 8/9] usb: f_mass_storage: Fix set_bit and clear_bit usage
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
                   ` (6 preceding siblings ...)
  2018-04-27 15:08 ` [U-Boot] [PATCH 7/9] nds2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit() Bryan O'Donoghue
@ 2018-04-27 15:08 ` Bryan O'Donoghue
  2018-04-27 16:20   ` Lukasz Majewski
  2018-04-27 15:08 ` [U-Boot] [PATCH 9/9] usb: composite convert __set_bit to generic_set_bit Bryan O'Donoghue
  2018-04-27 23:38 ` [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bin Meng
  9 siblings, 1 reply; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 9/9] usb: composite convert __set_bit to generic_set_bit
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
                   ` (7 preceding siblings ...)
  2018-04-27 15:08 ` [U-Boot] [PATCH v3 8/9] usb: f_mass_storage: Fix set_bit and clear_bit usage Bryan O'Donoghue
@ 2018-04-27 15:08 ` Bryan O'Donoghue
  2018-04-27 23:38 ` [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bin Meng
  9 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-27 15:08 UTC (permalink / raw)
  To: u-boot

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 d0ee7847b9..e5d802a80e 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);
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH v3 8/9] usb: f_mass_storage: Fix set_bit and clear_bit usage
  2018-04-27 15:08 ` [U-Boot] [PATCH v3 8/9] usb: f_mass_storage: Fix set_bit and clear_bit usage Bryan O'Donoghue
@ 2018-04-27 16:20   ` Lukasz Majewski
  2018-04-30 10:34     ` Bryan O'Donoghue
  0 siblings, 1 reply; 14+ messages in thread
From: Lukasz Majewski @ 2018-04-27 16:20 UTC (permalink / raw)
  To: u-boot

Hi Bryan,

> 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

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.

> 
> 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..af42136e1d
> 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 <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) {




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180427/9f121f33/attachment.sig>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage
  2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
                   ` (8 preceding siblings ...)
  2018-04-27 15:08 ` [U-Boot] [PATCH 9/9] usb: composite convert __set_bit to generic_set_bit Bryan O'Donoghue
@ 2018-04-27 23:38 ` Bin Meng
  2018-04-30 10:36   ` Bryan O'Donoghue
  9 siblings, 1 reply; 14+ messages in thread
From: Bin Meng @ 2018-04-27 23:38 UTC (permalink / raw)
  To: u-boot

Hi Bryan,

On Fri, Apr 27, 2018 at 11:08 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> 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().
>

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.

> 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(-)
>
> --

Regards,
Bin

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH v3 8/9] usb: f_mass_storage: Fix set_bit and clear_bit usage
  2018-04-27 16:20   ` Lukasz Majewski
@ 2018-04-30 10:34     ` Bryan O'Donoghue
  0 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-30 10:34 UTC (permalink / raw)
  To: u-boot

On 27/04/18 17:20, Lukasz Majewski wrote:
> Hi Bryan,
> 
>> 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
> 
> Just I'm a bit puzzled:
> 
> This is v3 8/9, followed by 9/9 (v1)?

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

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage
  2018-04-27 23:38 ` [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bin Meng
@ 2018-04-30 10:36   ` Bryan O'Donoghue
  0 siblings, 0 replies; 14+ messages in thread
From: Bryan O'Donoghue @ 2018-04-30 10:36 UTC (permalink / raw)
  To: u-boot

On 28/04/18 00:38, Bin Meng wrote:
> Hi Bryan,
> 
> On Fri, Apr 27, 2018 at 11:08 PM, Bryan O'Donoghue
> <pure.logic@nexus-software.ie> wrote:
>> 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().
>>
> 
> I am confused. So this changes enables x86's own __set_bit which is
> lock-prefixed.

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));
}

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-04-30 10:36 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-27 15:08 [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bryan O'Donoghue
2018-04-27 15:08 ` [U-Boot] [PATCH 1/9] x86: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
2018-04-27 15:08 ` [U-Boot] [PATCH 2/9] riscv: " Bryan O'Donoghue
2018-04-27 15:08 ` [U-Boot] [PATCH 3/9] riscv: : Define PLATFORM__CLEAR_BIT for generic_clear_bit() Bryan O'Donoghue
2018-04-27 15:08 ` [U-Boot] [PATCH 4/9] nios2: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
2018-04-27 15:08 ` [U-Boot] [PATCH 5/9] nios2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit() Bryan O'Donoghue
2018-04-27 15:08 ` [U-Boot] [PATCH 6/9] nds32: Define PLATFORM__SET_BIT for generic_set_bit() Bryan O'Donoghue
2018-04-27 15:08 ` [U-Boot] [PATCH 7/9] nds2: : Define PLATFORM__CLEAR_BIT for generic_clear_bit() Bryan O'Donoghue
2018-04-27 15:08 ` [U-Boot] [PATCH v3 8/9] usb: f_mass_storage: Fix set_bit and clear_bit usage Bryan O'Donoghue
2018-04-27 16:20   ` Lukasz Majewski
2018-04-30 10:34     ` Bryan O'Donoghue
2018-04-27 15:08 ` [U-Boot] [PATCH 9/9] usb: composite convert __set_bit to generic_set_bit Bryan O'Donoghue
2018-04-27 23:38 ` [U-Boot] [PATCH 0/9] Fixup set_bit/clear_bit definition and usage Bin Meng
2018-04-30 10:36   ` Bryan O'Donoghue

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.