| Message ID | 20180415234307.28132-34-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Use the BYTE-based definitions when useful | expand |
Am 16.04.2018 um 01:42 schrieb Philippe Mathieu-Daudé: > It eases code review, unit is explicit. > > Patch generated using: > > $ git grep -E '(1024|2048|4096|8192|(<<|>>).?(10|20|30))' hw/ include/hw/ > > and modified manually. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/hw/net/allwinner_emac.h | 5 +++-- > hw/net/e1000e.c | 7 ++++--- > hw/net/e1000x_common.c | 3 ++- > hw/net/eepro100.c | 7 +++---- > 4 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h > index 4cc8aab7ec..4b53b6485c 100644 > --- a/include/hw/net/allwinner_emac.h > +++ b/include/hw/net/allwinner_emac.h > @@ -23,6 +23,7 @@ > #ifndef ALLWINNER_EMAC_H > #define ALLWINNER_EMAC_H > > +#include "qemu/units.h" > #include "net/net.h" > #include "qemu/fifo8.h" > #include "hw/net/mii.h" > @@ -125,8 +126,8 @@ > #define EMAC_INT_RX (1 << 8) > > /* Due to lack of specifications, size of fifos is chosen arbitrarily */ > -#define TX_FIFO_SIZE (4 * 1024) > -#define RX_FIFO_SIZE (32 * 1024) > +#define TX_FIFO_SIZE (4 * K_BYTE) > +#define RX_FIFO_SIZE (32 * K_BYTE) > > #define NUM_TX_FIFOS 2 > #define RX_HDR_SIZE 8 > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c > index 16a9417a85..101efe7c83 100644 > --- a/hw/net/e1000e.c > +++ b/hw/net/e1000e.c > @@ -34,6 +34,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/units.h" > #include "net/net.h" > #include "net/tap.h" > #include "qemu/range.h" > @@ -81,10 +82,10 @@ typedef struct E1000EState { > #define E1000E_IO_IDX 2 > #define E1000E_MSIX_IDX 3 > > -#define E1000E_MMIO_SIZE (128 * 1024) > -#define E1000E_FLASH_SIZE (128 * 1024) > +#define E1000E_MMIO_SIZE (128 * K_BYTE) > +#define E1000E_FLASH_SIZE (128 * K_BYTE) > #define E1000E_IO_SIZE (32) > -#define E1000E_MSIX_SIZE (16 * 1024) > +#define E1000E_MSIX_SIZE (16 * K_BYTE) > > #define E1000E_MSIX_TABLE (0x0000) > #define E1000E_MSIX_PBA (0x2000) > diff --git a/hw/net/e1000x_common.c b/hw/net/e1000x_common.c > index eb0e097137..58c8db77e9 100644 > --- a/hw/net/e1000x_common.c > +++ b/hw/net/e1000x_common.c > @@ -23,6 +23,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/units.h" > #include "hw/hw.h" > #include "hw/pci/pci.h" > #include "net/net.h" > @@ -111,7 +112,7 @@ bool e1000x_is_oversized(uint32_t *mac, size_t size) > static const int maximum_ethernet_vlan_size = 1522; > /* this is the size past which hardware will > drop packets when setting LPE=1 */ > - static const int maximum_ethernet_lpe_size = 16384; > + static const int maximum_ethernet_lpe_size = 16 * K_BYTE; > > if ((size > maximum_ethernet_lpe_size || > (size > maximum_ethernet_vlan_size > diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c > index a07a63247e..a02e2b55e8 100644 > --- a/hw/net/eepro100.c > +++ b/hw/net/eepro100.c > @@ -41,6 +41,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/units.h" > #include "hw/hw.h" > #include "hw/pci/pci.h" > #include "net/net.h" > @@ -60,8 +61,6 @@ > * changed to pad short packets itself. */ > #define CONFIG_PAD_RECEIVED_FRAMES > > -#define KiB 1024 > - > /* Debug EEPRO100 card. */ > #if 0 > # define DEBUG_EEPRO100 > @@ -104,9 +103,9 @@ > /* Use 64 word EEPROM. TODO: could be a runtime option. */ > #define EEPROM_SIZE 64 > > -#define PCI_MEM_SIZE (4 * KiB) > +#define PCI_MEM_SIZE (4 * K_BYTE) > #define PCI_IO_SIZE 64 > -#define PCI_FLASH_SIZE (128 * KiB) > +#define PCI_FLASH_SIZE (128 * K_BYTE) > > #define BITS(n, m) (((0xffffffffU << (31 - n)) >> (31 - n + m)) << m) Technically this is fine, therefore Reviewed-by: Stefan Weil <sw@weilnetz.de> Practically I'd prefer replacing all K_BYTE by KiB, because that is the standard name with a precise definition: https://en.wikipedia.org/wiki/Kibibyte. Stefan
Hi Stefan, On 04/16/2018 02:23 AM, Stefan Weil wrote: > Am 16.04.2018 um 01:42 schrieb Philippe Mathieu-Daudé: >> It eases code review, unit is explicit. >> >> Patch generated using: >> >> $ git grep -E '(1024|2048|4096|8192|(<<|>>).?(10|20|30))' hw/ include/hw/ >> >> and modified manually. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> include/hw/net/allwinner_emac.h | 5 +++-- >> hw/net/e1000e.c | 7 ++++--- >> hw/net/e1000x_common.c | 3 ++- >> hw/net/eepro100.c | 7 +++---- >> 4 files changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h >> index 4cc8aab7ec..4b53b6485c 100644 >> --- a/include/hw/net/allwinner_emac.h >> +++ b/include/hw/net/allwinner_emac.h >> @@ -23,6 +23,7 @@ >> #ifndef ALLWINNER_EMAC_H >> #define ALLWINNER_EMAC_H >> >> +#include "qemu/units.h" >> #include "net/net.h" >> #include "qemu/fifo8.h" >> #include "hw/net/mii.h" >> @@ -125,8 +126,8 @@ >> #define EMAC_INT_RX (1 << 8) >> >> /* Due to lack of specifications, size of fifos is chosen arbitrarily */ >> -#define TX_FIFO_SIZE (4 * 1024) >> -#define RX_FIFO_SIZE (32 * 1024) >> +#define TX_FIFO_SIZE (4 * K_BYTE) >> +#define RX_FIFO_SIZE (32 * K_BYTE) >> >> #define NUM_TX_FIFOS 2 >> #define RX_HDR_SIZE 8 >> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c >> index 16a9417a85..101efe7c83 100644 >> --- a/hw/net/e1000e.c >> +++ b/hw/net/e1000e.c >> @@ -34,6 +34,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/units.h" >> #include "net/net.h" >> #include "net/tap.h" >> #include "qemu/range.h" >> @@ -81,10 +82,10 @@ typedef struct E1000EState { >> #define E1000E_IO_IDX 2 >> #define E1000E_MSIX_IDX 3 >> >> -#define E1000E_MMIO_SIZE (128 * 1024) >> -#define E1000E_FLASH_SIZE (128 * 1024) >> +#define E1000E_MMIO_SIZE (128 * K_BYTE) >> +#define E1000E_FLASH_SIZE (128 * K_BYTE) >> #define E1000E_IO_SIZE (32) >> -#define E1000E_MSIX_SIZE (16 * 1024) >> +#define E1000E_MSIX_SIZE (16 * K_BYTE) >> >> #define E1000E_MSIX_TABLE (0x0000) >> #define E1000E_MSIX_PBA (0x2000) >> diff --git a/hw/net/e1000x_common.c b/hw/net/e1000x_common.c >> index eb0e097137..58c8db77e9 100644 >> --- a/hw/net/e1000x_common.c >> +++ b/hw/net/e1000x_common.c >> @@ -23,6 +23,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/units.h" >> #include "hw/hw.h" >> #include "hw/pci/pci.h" >> #include "net/net.h" >> @@ -111,7 +112,7 @@ bool e1000x_is_oversized(uint32_t *mac, size_t size) >> static const int maximum_ethernet_vlan_size = 1522; >> /* this is the size past which hardware will >> drop packets when setting LPE=1 */ >> - static const int maximum_ethernet_lpe_size = 16384; >> + static const int maximum_ethernet_lpe_size = 16 * K_BYTE; >> >> if ((size > maximum_ethernet_lpe_size || >> (size > maximum_ethernet_vlan_size >> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c >> index a07a63247e..a02e2b55e8 100644 >> --- a/hw/net/eepro100.c >> +++ b/hw/net/eepro100.c >> @@ -41,6 +41,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/units.h" >> #include "hw/hw.h" >> #include "hw/pci/pci.h" >> #include "net/net.h" >> @@ -60,8 +61,6 @@ >> * changed to pad short packets itself. */ >> #define CONFIG_PAD_RECEIVED_FRAMES >> >> -#define KiB 1024 >> - >> /* Debug EEPRO100 card. */ >> #if 0 >> # define DEBUG_EEPRO100 >> @@ -104,9 +103,9 @@ >> /* Use 64 word EEPROM. TODO: could be a runtime option. */ >> #define EEPROM_SIZE 64 >> >> -#define PCI_MEM_SIZE (4 * KiB) >> +#define PCI_MEM_SIZE (4 * K_BYTE) >> #define PCI_IO_SIZE 64 >> -#define PCI_FLASH_SIZE (128 * KiB) >> +#define PCI_FLASH_SIZE (128 * K_BYTE) >> >> #define BITS(n, m) (((0xffffffffU << (31 - n)) >> (31 - n + m)) << m) > > > Technically this is fine, therefore > > Reviewed-by: Stefan Weil <sw@weilnetz.de> > > Practically I'd prefer replacing all K_BYTE by KiB, because that is the > standard name with a precise definition: > https://en.wikipedia.org/wiki/Kibibyte. "1 kibibyte is 1024 bytes. The unit symbol for the kibibyte is KiB." I like your suggestion :) Mostly because this shortens the code and keeps it readable (while respecting the SI). If the other maintainers agree with this change, I'm OK to do the cleanup. I couldn't find a rule requiring #defines be all capital letter in CODING_STYLE, is this acceptable to have a lower case "i"? Regards, Phil.
diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h index 4cc8aab7ec..4b53b6485c 100644 --- a/include/hw/net/allwinner_emac.h +++ b/include/hw/net/allwinner_emac.h @@ -23,6 +23,7 @@ #ifndef ALLWINNER_EMAC_H #define ALLWINNER_EMAC_H +#include "qemu/units.h" #include "net/net.h" #include "qemu/fifo8.h" #include "hw/net/mii.h" @@ -125,8 +126,8 @@ #define EMAC_INT_RX (1 << 8) /* Due to lack of specifications, size of fifos is chosen arbitrarily */ -#define TX_FIFO_SIZE (4 * 1024) -#define RX_FIFO_SIZE (32 * 1024) +#define TX_FIFO_SIZE (4 * K_BYTE) +#define RX_FIFO_SIZE (32 * K_BYTE) #define NUM_TX_FIFOS 2 #define RX_HDR_SIZE 8 diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index 16a9417a85..101efe7c83 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -34,6 +34,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "net/net.h" #include "net/tap.h" #include "qemu/range.h" @@ -81,10 +82,10 @@ typedef struct E1000EState { #define E1000E_IO_IDX 2 #define E1000E_MSIX_IDX 3 -#define E1000E_MMIO_SIZE (128 * 1024) -#define E1000E_FLASH_SIZE (128 * 1024) +#define E1000E_MMIO_SIZE (128 * K_BYTE) +#define E1000E_FLASH_SIZE (128 * K_BYTE) #define E1000E_IO_SIZE (32) -#define E1000E_MSIX_SIZE (16 * 1024) +#define E1000E_MSIX_SIZE (16 * K_BYTE) #define E1000E_MSIX_TABLE (0x0000) #define E1000E_MSIX_PBA (0x2000) diff --git a/hw/net/e1000x_common.c b/hw/net/e1000x_common.c index eb0e097137..58c8db77e9 100644 --- a/hw/net/e1000x_common.c +++ b/hw/net/e1000x_common.c @@ -23,6 +23,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "hw/hw.h" #include "hw/pci/pci.h" #include "net/net.h" @@ -111,7 +112,7 @@ bool e1000x_is_oversized(uint32_t *mac, size_t size) static const int maximum_ethernet_vlan_size = 1522; /* this is the size past which hardware will drop packets when setting LPE=1 */ - static const int maximum_ethernet_lpe_size = 16384; + static const int maximum_ethernet_lpe_size = 16 * K_BYTE; if ((size > maximum_ethernet_lpe_size || (size > maximum_ethernet_vlan_size diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c index a07a63247e..a02e2b55e8 100644 --- a/hw/net/eepro100.c +++ b/hw/net/eepro100.c @@ -41,6 +41,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "hw/hw.h" #include "hw/pci/pci.h" #include "net/net.h" @@ -60,8 +61,6 @@ * changed to pad short packets itself. */ #define CONFIG_PAD_RECEIVED_FRAMES -#define KiB 1024 - /* Debug EEPRO100 card. */ #if 0 # define DEBUG_EEPRO100 @@ -104,9 +103,9 @@ /* Use 64 word EEPROM. TODO: could be a runtime option. */ #define EEPROM_SIZE 64 -#define PCI_MEM_SIZE (4 * KiB) +#define PCI_MEM_SIZE (4 * K_BYTE) #define PCI_IO_SIZE 64 -#define PCI_FLASH_SIZE (128 * KiB) +#define PCI_FLASH_SIZE (128 * K_BYTE) #define BITS(n, m) (((0xffffffffU << (31 - n)) >> (31 - n + m)) << m)
It eases code review, unit is explicit. Patch generated using: $ git grep -E '(1024|2048|4096|8192|(<<|>>).?(10|20|30))' hw/ include/hw/ and modified manually. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/hw/net/allwinner_emac.h | 5 +++-- hw/net/e1000e.c | 7 ++++--- hw/net/e1000x_common.c | 3 ++- hw/net/eepro100.c | 7 +++---- 4 files changed, 12 insertions(+), 10 deletions(-)