Skip to contentSkip to conversation

usb: f_mass_storage: Fix compile on x86

PATCH Superseded Bryan O'Donoghue 3 participants 4 messages Apr 26, 2018 – Apr 27, 2018 USB

lore
Share & tools
lore

Bryan O'Donoghue

author
#0 lore raw

PATCH v2 1/1 usb: f_mass_storage: Fix compile on x86

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

The local version of set_bit and clear_bit are doing some IP-block specific bit-twiddling so we don't actually want to accidentally pick up the bitops.h version of set_bit and clear bit.

This patch renames to _set_bit() and _clear_bit() respectively to avoid the namespace collision.

Signed-off-by Bryan O'Donoghue <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Marek Vasut <[email protected]>
---
 drivers/usb/gadget/f_mass_storage.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/gadget/f_mass_storage.c +4 -4
index 1ecb92ac6b..524c43419f 100644
@@ -283,7 +283,7 @@ 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)
+inline void _set_bit(int nr, volatile void *addr)
 {
 	int	mask;
 	unsigned int *a = (unsigned int *) addr;

@@ -293,7 +293,7 @@ inline void set_bit(int nr, volatile void *addr)
 	*a |= mask;
 }
 
-inline void clear_bit(int nr, volatile void *addr)
+inline void _clear_bit(int nr, volatile void *addr)
 {
 	int	mask;
 	unsigned int *a = (unsigned int *) addr;

@@ -2086,7 +2086,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);
+		_set_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags);
 		return -EINVAL;
 	}
 

@@ -2250,7 +2250,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);
+	_clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags);
 
 	/* Allocate the requests */
 	for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
Signature · 1 line
2.17.0
#1 lore raw

PATCH v2 1/1 usb: f_mass_storage: Fix compile on x86

Quoted 22 lines · Bryan O'Donoghue

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

The local version of set_bit and clear_bit are doing some IP-block specific bit-twiddling so we don't actually want to accidentally pick up the bitops.h version of set_bit and clear bit.

This patch renames to _set_bit() and _clear_bit() respectively to avoid the namespace collision.

Signed-off-by Bryan O'Donoghue <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Marek Vasut <[email protected]>
---
 drivers/usb/gadget/f_mass_storage.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/gadget/f_mass_storage.c +0 -0
index 1ecb92ac6b..524c43419f 100644

drivers/usb/gadget/f_mass_storage.c static const char fsg_string_interface[] = "Mass Storage"; · line 286

Quoted 5 lines
drivers/usb/gadget/f_mass_storage.c +1 -1
@@ -283,7 +283,7 @@ 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)
+inline void _set_bit(int nr, volatile void *addr)

Do not name functions with _ prefix. Also, I think the suggestion from Lukasz was better.

Signature · 2 lines
Best regards,
Marek Vasut
author
#2 lore raw

PATCH v2 1/1 usb: f_mass_storage: Fix compile on x86

Quoted 26 lines · Marek Vasut
Quoted 25 lines · Bryan O'Donoghue

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

The local version of set_bit and clear_bit are doing some IP-block specific bit-twiddling so we don't actually want to accidentally pick up the bitops.h version of set_bit and clear bit.

This patch renames to _set_bit() and _clear_bit() respectively to avoid the namespace collision.

Signed-off-by Bryan O'Donoghue <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Marek Vasut <[email protected]>
---
  drivers/usb/gadget/f_mass_storage.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/gadget/f_mass_storage.c +0 -0
index 1ecb92ac6b..524c43419f 100644
@@ -283,7 +283,7 @@ static const char fsg_string_interface[] = "Mass Storage";
  struct kref {int x; };
  struct completion {int x; };
Quoted 6 lines

drivers/usb/gadget/f_mass_storage.c static const char fsg_string_interface[] = "Mass Storage"; · line 286

Quoted 2 lines
drivers/usb/gadget/f_mass_storage.c +1 -1
-inline void set_bit(int nr, volatile void *addr)
+inline void _set_bit(int nr, volatile void *addr)

Do not name functions with _ prefix. Also, I think the suggestion from Lukasz was better.

Funny - I had assumed we were seting the bit like this for a good reason but...

This code seems to be something that just crept in along the way

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

but on Linux the upstream driver actually does arch-specific set_bit/clear_bit

Looking at it a bit more.. I'm not quite sure how/why we are setting the bit like that

git blame drivers/usb/gadget/f_mass_storage.c

b4d36f6809f (Piotr Wilczek 2013-03-05 12:10:16 +0100 286)
inline void set_bit(int nr, volatile void *addr)

The f_mass_storage.c source file from v2.6.36 Linux kernel.

     commit 8876f5e7d3b2a320777dd4f6f5301d474c97a06c
     Author: Michal Nazarewicz <[email protected]>
     Date:   Mon Jun 21 13:57:09 2010 +0200

linux$ git checkout 8876f5e7d3b2a320777dd4f6f5301d474c97a06c -- drivers/usb/gadget/f_mass_storage.c

Doesn't have a local copy of set_bit/clear_bit at all, which is confusing - I guess bitops.h in 2013 didn't have set_bit/clear_bit..

But yeah - I guess we really should be using arch-specific get_bit/set_bit after all...

---
bod

#3 lore raw

PATCH v2 1/1 usb: f_mass_storage: Fix compile on x86

Hi Bryan,

Quoted 67 lines · Marek Vasut
Quoted 26 lines · Bryan O'Donoghue
Quoted 25 lines

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

The local version of set_bit and clear_bit are doing some IP-block specific bit-twiddling so we don't actually want to accidentally pick up the bitops.h version of set_bit and clear bit.

This patch renames to _set_bit() and _clear_bit() respectively to avoid the namespace collision.

Signed-off-by Bryan O'Donoghue <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Marek Vasut <[email protected]>
---
  drivers/usb/gadget/f_mass_storage.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
(quoted diff) +0 -0

b/drivers/usb/gadget/f_mass_storage.c index 1ecb92ac6b..524c43419f 100644 --- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c

drivers/usb/gadget/f_mass_storage.c +0 -0
@@ -283,7 +283,7 @@ static const char fsg_string_interface[] =
 "Mass Storage"; struct kref {int x; };
  struct completion {int x; };
Quoted 6 lines

drivers/usb/gadget/f_mass_storage.c static const char fsg_string_interface[] = "Mass Storage"; · line 286

Quoted 2 lines
drivers/usb/gadget/f_mass_storage.c +1 -1
-inline void set_bit(int nr, volatile void *addr)
+inline void _set_bit(int nr, volatile void *addr)

Do not name functions with _ prefix. Also, I think the suggestion from Lukasz was better.

Funny - I had assumed we were seting the bit like this for a good reason but...

This code seems to be something that just crept in along the way

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

but on Linux the upstream driver actually does arch-specific set_bit/clear_bit

Looking at it a bit more.. I'm not quite sure how/why we are setting the bit like that

git blame drivers/usb/gadget/f_mass_storage.c

b4d36f6809f (Piotr Wilczek 2013-03-05 12:10:16 +0100 286)
inline void set_bit(int nr, volatile void *addr)

The f_mass_storage.c source file from v2.6.36 Linux kernel.

     commit 8876f5e7d3b2a320777dd4f6f5301d474c97a06c
     Author: Michal Nazarewicz <[email protected]>
     Date:   Mon Jun 21 13:57:09 2010 +0200
Quoted 8 lines

linux$ git checkout 8876f5e7d3b2a320777dd4f6f5301d474c97a06c -- drivers/usb/gadget/f_mass_storage.c

Doesn't have a local copy of set_bit/clear_bit at all, which is confusing - I guess bitops.h in 2013 didn't have set_bit/clear_bit..

But yeah - I guess we really should be using arch-specific get_bit/set_bit after all...

+1

Quoted 4 lines

---
bod

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 at 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/7de660a0/attachment.sig>