| Message ID | 20191126091946.7970-2-nsaenzjulienne@suse.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Raspberry Pi 4 PCIe support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: > Some users need to make sure their rounding function accepts and returns > 64bit long variables regardless of the architecture. Sadly > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > new generic 64bit variant of the function and cleanup rougue custom > implementations. Is it possible to create general roundup/rounddown_pow_two() which will work correctly for any type of variables, instead of creating special variant for every type? Thanks
On Tue, 2019-11-26 at 10:19 +0100, Nicolas Saenz Julienne wrote: > Some users need to make sure their rounding function accepts and returns > 64bit long variables regardless of the architecture. Sadly > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > new generic 64bit variant of the function and cleanup rougue custom > implementations. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Small Nit: I corrected the patch subject for next version. linux/log2.h: Add roundup/rounddown_pow_two_u64() family of functions Note the change here: ^^^^ Regards, Nicolas
On Wed, 2019-11-27 at 18:06 +0000, Robin Murphy wrote: > On 26/11/2019 12:51 pm, Leon Romanovsky wrote: > > On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: > > > Some users need to make sure their rounding function accepts and returns > > > 64bit long variables regardless of the architecture. Sadly > > > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > > > new generic 64bit variant of the function and cleanup rougue custom > > > implementations. > > > > Is it possible to create general roundup/rounddown_pow_two() which will > > work correctly for any type of variables, instead of creating special > > variant for every type? > > In fact, that is sort of the case already - roundup_pow_of_two() itself > wraps ilog2() such that the constant case *is* type-independent. And > since ilog2() handles non-constant values anyway, might it be reasonable > to just take the strongly-typed __roundup_pow_of_two() helper out of the > loop as below? > > Robin > That looks way better that's for sure. Some questions. > ----->8----- > diff --git a/include/linux/log2.h b/include/linux/log2.h > index 83a4a3ca3e8a..e825f8a6e8b5 100644 > --- a/include/linux/log2.h > +++ b/include/linux/log2.h > @@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > */ > #define roundup_pow_of_two(n) \ > ( \ > - __builtin_constant_p(n) ? ( \ > - (n == 1) ? 1 : \ > - (1UL << (ilog2((n) - 1) + 1)) \ > - ) : \ > - __roundup_pow_of_two(n) \ > + (__builtin_constant_p(n) && (n == 1)) ? \ > + 1 : (1UL << (ilog2((n) - 1) + 1)) \ Then here you'd have to use ULL instead of UL, right? I want my 64bit value everywhere regardless of the CPU arch. The downside is that would affect performance to some extent (i.e. returning a 64bit value where you used to have a 32bit one)? Also, what about callers to this function on platforms with 32bit 'unsigned longs' that happen to input a 64bit value into this. IIUC we'd have a change of behaviour. Regards, Nicolas
On 27/11/2019 6:24 pm, Nicolas Saenz Julienne wrote: > On Wed, 2019-11-27 at 18:06 +0000, Robin Murphy wrote: >> On 26/11/2019 12:51 pm, Leon Romanovsky wrote: >>> On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: >>>> Some users need to make sure their rounding function accepts and returns >>>> 64bit long variables regardless of the architecture. Sadly >>>> roundup/rounddown_pow_two() takes and returns unsigned longs. Create a >>>> new generic 64bit variant of the function and cleanup rougue custom >>>> implementations. >>> >>> Is it possible to create general roundup/rounddown_pow_two() which will >>> work correctly for any type of variables, instead of creating special >>> variant for every type? >> >> In fact, that is sort of the case already - roundup_pow_of_two() itself >> wraps ilog2() such that the constant case *is* type-independent. And >> since ilog2() handles non-constant values anyway, might it be reasonable >> to just take the strongly-typed __roundup_pow_of_two() helper out of the >> loop as below? >> >> Robin >> > > That looks way better that's for sure. Some questions. > >> ----->8----- >> diff --git a/include/linux/log2.h b/include/linux/log2.h >> index 83a4a3ca3e8a..e825f8a6e8b5 100644 >> --- a/include/linux/log2.h >> +++ b/include/linux/log2.h >> @@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >> */ >> #define roundup_pow_of_two(n) \ >> ( \ >> - __builtin_constant_p(n) ? ( \ >> - (n == 1) ? 1 : \ >> - (1UL << (ilog2((n) - 1) + 1)) \ >> - ) : \ >> - __roundup_pow_of_two(n) \ >> + (__builtin_constant_p(n) && (n == 1)) ? \ >> + 1 : (1UL << (ilog2((n) - 1) + 1)) \ > > Then here you'd have to use ULL instead of UL, right? I want my 64bit value > everywhere regardless of the CPU arch. The downside is that would affect > performance to some extent (i.e. returning a 64bit value where you used to have > a 32bit one)? True, although it's possible that 1ULL might result in the same codegen if the compiler can see that the result is immediately truncated back to long anyway. Or at worst, I suppose "(typeof(n))1" could suffice, however ugly. Either way, this diff was only an illustration rather than a concrete proposal, but it might be an interesting diversion to investigate. On that note, though, you should probably be using ULL in your current patch too. > Also, what about callers to this function on platforms with 32bit 'unsigned > longs' that happen to input a 64bit value into this. IIUC we'd have a change of > behaviour. Indeed, although the change in such a case would be "start getting the expected value instead of nonsense", so it might very well be welcome ;) Robin.
On Wed, Nov 27, 2019 at 07:06:12PM +0000, Robin Murphy wrote: > On 27/11/2019 6:24 pm, Nicolas Saenz Julienne wrote: > > On Wed, 2019-11-27 at 18:06 +0000, Robin Murphy wrote: > > > On 26/11/2019 12:51 pm, Leon Romanovsky wrote: > > > > On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: > > > > > Some users need to make sure their rounding function accepts and returns > > > > > 64bit long variables regardless of the architecture. Sadly > > > > > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > > > > > new generic 64bit variant of the function and cleanup rougue custom > > > > > implementations. > > > > > > > > Is it possible to create general roundup/rounddown_pow_two() which will > > > > work correctly for any type of variables, instead of creating special > > > > variant for every type? > > > > > > In fact, that is sort of the case already - roundup_pow_of_two() itself > > > wraps ilog2() such that the constant case *is* type-independent. And > > > since ilog2() handles non-constant values anyway, might it be reasonable > > > to just take the strongly-typed __roundup_pow_of_two() helper out of the > > > loop as below? > > > > > > Robin > > > > > > > That looks way better that's for sure. Some questions. > > > > > ----->8----- > > > diff --git a/include/linux/log2.h b/include/linux/log2.h > > > index 83a4a3ca3e8a..e825f8a6e8b5 100644 > > > --- a/include/linux/log2.h > > > +++ b/include/linux/log2.h > > > @@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > > > */ > > > #define roundup_pow_of_two(n) \ > > > ( \ > > > - __builtin_constant_p(n) ? ( \ > > > - (n == 1) ? 1 : \ > > > - (1UL << (ilog2((n) - 1) + 1)) \ > > > - ) : \ > > > - __roundup_pow_of_two(n) \ > > > + (__builtin_constant_p(n) && (n == 1)) ? \ > > > + 1 : (1UL << (ilog2((n) - 1) + 1)) \ > > > > Then here you'd have to use ULL instead of UL, right? I want my 64bit value > > everywhere regardless of the CPU arch. The downside is that would affect > > performance to some extent (i.e. returning a 64bit value where you used to have > > a 32bit one)? > > True, although it's possible that 1ULL might result in the same codegen if > the compiler can see that the result is immediately truncated back to long > anyway. Or at worst, I suppose "(typeof(n))1" could suffice, however ugly. > Either way, this diff was only an illustration rather than a concrete > proposal, but it might be an interesting diversion to investigate. > > On that note, though, you should probably be using ULL in your current patch > too. > > > Also, what about callers to this function on platforms with 32bit 'unsigned > > longs' that happen to input a 64bit value into this. IIUC we'd have a change of > > behaviour. > > Indeed, although the change in such a case would be "start getting the > expected value instead of nonsense", so it might very well be welcome ;) Agree, if code overflowed with 32 bits before this change, the code was already broken. At least now, it won't overflow. > > Robin.
On Wed, 2019-11-27 at 19:06 +0000, Robin Murphy wrote: > On 27/11/2019 6:24 pm, Nicolas Saenz Julienne wrote: > > On Wed, 2019-11-27 at 18:06 +0000, Robin Murphy wrote: > > > On 26/11/2019 12:51 pm, Leon Romanovsky wrote: > > > > On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: > > > > > Some users need to make sure their rounding function accepts and > > > > > returns > > > > > 64bit long variables regardless of the architecture. Sadly > > > > > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > > > > > new generic 64bit variant of the function and cleanup rougue custom > > > > > implementations. > > > > > > > > Is it possible to create general roundup/rounddown_pow_two() which will > > > > work correctly for any type of variables, instead of creating special > > > > variant for every type? > > > > > > In fact, that is sort of the case already - roundup_pow_of_two() itself > > > wraps ilog2() such that the constant case *is* type-independent. And > > > since ilog2() handles non-constant values anyway, might it be reasonable > > > to just take the strongly-typed __roundup_pow_of_two() helper out of the > > > loop as below? > > > > > > Robin > > > > > > > That looks way better that's for sure. Some questions. > > > > > ----->8----- > > > diff --git a/include/linux/log2.h b/include/linux/log2.h > > > index 83a4a3ca3e8a..e825f8a6e8b5 100644 > > > --- a/include/linux/log2.h > > > +++ b/include/linux/log2.h > > > @@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > > > */ > > > #define roundup_pow_of_two(n) \ > > > ( \ > > > - __builtin_constant_p(n) ? ( \ > > > - (n == 1) ? 1 : \ > > > - (1UL << (ilog2((n) - 1) + 1)) \ > > > - ) : \ > > > - __roundup_pow_of_two(n) \ > > > + (__builtin_constant_p(n) && (n == 1)) ? \ > > > + 1 : (1UL << (ilog2((n) - 1) + 1)) \ > > > > Then here you'd have to use ULL instead of UL, right? I want my 64bit value > > everywhere regardless of the CPU arch. The downside is that would affect > > performance to some extent (i.e. returning a 64bit value where you used to > > have > > a 32bit one)? > > True, although it's possible that 1ULL might result in the same codegen > if the compiler can see that the result is immediately truncated back to > long anyway. Or at worst, I suppose "(typeof(n))1" could suffice, > however ugly. Either way, this diff was only an illustration rather than > a concrete proposal, but it might be an interesting diversion to > investigate. > > On that note, though, you should probably be using ULL in your current > patch too. I actually meant to, the fix got lost. Thanks for pointing it out. As I see Leon also likes this, I'll try out this implementation and come back with some results. Regards, Nicolas
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 33f71983e001..4809d3757d71 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -1090,7 +1090,7 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) * firmware. */ end = dmaaddr + size - 1; - mask = DMA_BIT_MASK(ilog2(end) + 1); + mask = roundup_pow_of_two_u64(end) - 1; dev->bus_dma_limit = end; dev->coherent_dma_mask = mask; *dev->dma_mask = mask; diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c index 024788549c25..ba5368e221f3 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c @@ -33,6 +33,7 @@ #include <linux/mlx4/device.h> #include <linux/clocksource.h> +#include <linux/log2.h> #include "mlx4_en.h" @@ -252,7 +253,7 @@ static u32 freq_to_shift(u16 freq) { u32 freq_khz = freq * 1000; u64 max_val_cycles = freq_khz * 1000 * MLX4_EN_WRAP_AROUND_SEC; - u64 max_val_cycles_rounded = 1ULL << fls64(max_val_cycles - 1); + u64 max_val_cycles_rounded = roundup_pow_of_two_u64(max_val_cycles); /* calculate max possible multiplier in order to fit in 64bit */ u64 max_mul = div64_u64(ULLONG_MAX, max_val_cycles_rounded); diff --git a/drivers/of/device.c b/drivers/of/device.c index e9127db7b067..418d7d014af1 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -149,7 +149,7 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) * set by the driver. */ end = dma_addr + size - 1; - mask = DMA_BIT_MASK(ilog2(end) + 1); + mask = roundup_pow_of_two_u64(end) - 1; dev->coherent_dma_mask &= mask; *dev->dma_mask &= mask; /* ...but only set bus limit if we found valid dma-ranges earlier */ diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c index 1c173dad67d1..f6e37eb67c68 100644 --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c @@ -10,6 +10,7 @@ #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/sizes.h> +#include <linux/log2.h> #include "pcie-cadence.h" @@ -61,11 +62,7 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, /* BAR size is 2^(aperture + 7) */ sz = max_t(size_t, epf_bar->size, CDNS_PCIE_EP_MIN_APERTURE); - /* - * roundup_pow_of_two() returns an unsigned long, which is not suited - * for 64bit values. - */ - sz = 1ULL << fls64(sz - 1); + sz = roundup_pow_of_two_u64(sz); aperture = ilog2(sz) - 7; /* 128B -> 0, 256B -> 1, 512B -> 2, ... */ if ((flags & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) { diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c index cd795f6fc1e2..2ddda5ac60c4 100644 --- a/drivers/pci/controller/cadence/pcie-cadence.c +++ b/drivers/pci/controller/cadence/pcie-cadence.c @@ -4,6 +4,7 @@ // Author: Cyrille Pitchen <cyrille.pitchen@free-electrons.com> #include <linux/kernel.h> +#include <linux/log2.h> #include "pcie-cadence.h" @@ -11,11 +12,7 @@ void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 fn, u32 r, bool is_io, u64 cpu_addr, u64 pci_addr, size_t size) { - /* - * roundup_pow_of_two() returns an unsigned long, which is not suited - * for 64bit values. - */ - u64 sz = 1ULL << fls64(size - 1); + u64 sz = roundup_pow_of_two_u64(size); int nbits = ilog2(sz); u32 addr0, addr1, desc0, desc1; diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c index d743b0a48988..343f9683b540 100644 --- a/drivers/pci/controller/pcie-rockchip-ep.c +++ b/drivers/pci/controller/pcie-rockchip-ep.c @@ -16,6 +16,7 @@ #include <linux/platform_device.h> #include <linux/pci-epf.h> #include <linux/sizes.h> +#include <linux/log2.h> #include "pcie-rockchip.h" @@ -70,7 +71,7 @@ static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn, u32 r, u32 type, u64 cpu_addr, u64 pci_addr, size_t size) { - u64 sz = 1ULL << fls64(size - 1); + u64 sz = roundup_pow_of_two_u64(size); int num_pass_bits = ilog2(sz); u32 addr0, addr1, desc0, desc1; bool is_nor_msg = (type == AXI_WRAPPER_NOR_MSG); @@ -172,11 +173,7 @@ static int rockchip_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, /* BAR size is 2^(aperture + 7) */ sz = max_t(size_t, epf_bar->size, MIN_EP_APERTURE); - /* - * roundup_pow_of_two() returns an unsigned long, which is not suited - * for 64bit values. - */ - sz = 1ULL << fls64(sz - 1); + sz = roundup_pow_of_two_u64(sz); aperture = ilog2(sz) - 7; /* 128B -> 0, 256B -> 1, 512B -> 2, ... */ if ((flags & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) { diff --git a/include/linux/log2.h b/include/linux/log2.h index 83a4a3ca3e8a..0db47b78faa2 100644 --- a/include/linux/log2.h +++ b/include/linux/log2.h @@ -67,6 +67,24 @@ unsigned long __rounddown_pow_of_two(unsigned long n) return 1UL << (fls_long(n) - 1); } +/** + * __roundup_pow_of_two_u64() - round 64bit value up to nearest power of two + * @n: value to round up + */ +static inline __attribute__((const)) u64 __roundup_pow_of_two_u64(u64 n) +{ + return 1ULL << fls64(n - 1); +} + +/** + * __rounddown_pow_of_two_u64() - round 64bit value down to nearest power of two + * @n: value to round down + */ +static inline __attribute__((const)) u64 __rounddown_pow_of_two_u64(u64 n) +{ + return 1ULL << (fls64(n) - 1); +} + /** * const_ilog2 - log base 2 of 32-bit or a 64-bit constant unsigned value * @n: parameter @@ -194,6 +212,40 @@ unsigned long __rounddown_pow_of_two(unsigned long n) __rounddown_pow_of_two(n) \ ) +/** + * roundup_pow_of_two_u64 - round the given 64bit value up to nearest power of + * two + * @n: parameter + * + * round the given value up to the nearest power of two + * - the result is undefined when n == 0 + * - this can be used to initialise global variables from constant data + */ +#define roundup_pow_of_two_u64(n) \ +( \ + __builtin_constant_p(n) ? ( \ + (n == 1) ? 1 : \ + (1UL << (ilog2((n) - 1) + 1)) \ + ) : \ + __roundup_pow_of_two_u64(n) \ +) + +/** + * rounddown_pow_of_two_u64 - round the given 64bit value down to nearest power + * of two + * @n: parameter + * + * round the given value down to the nearest power of two + * - the result is undefined when n == 0 + * - this can be used to initialise global variables from constant data + */ +#define rounddown_pow_of_two_u64(n) \ +( \ + __builtin_constant_p(n) ? ( \ + (1UL << ilog2(n))) : \ + __rounddown_pow_of_two_u64(n) \ +) + static inline __attribute_const__ int __order_base_2(unsigned long n) { diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 6af7ae83c4ad..8dd59e51b4dc 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -15,6 +15,7 @@ #include <linux/vmalloc.h> #include <linux/set_memory.h> #include <linux/swiotlb.h> +#include <linux/log2.h> /* * Most architectures use ZONE_DMA for the first 16 Megabytes, but some use it @@ -53,7 +54,7 @@ u64 dma_direct_get_required_mask(struct device *dev) { u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT); - return (1ULL << (fls64(max_dma) - 1)) * 2 - 1; + return rounddown_pow_of_two_u64(max_dma) * 2 - 1; } static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
Some users need to make sure their rounding function accepts and returns 64bit long variables regardless of the architecture. Sadly roundup/rounddown_pow_two() takes and returns unsigned longs. Create a new generic 64bit variant of the function and cleanup rougue custom implementations. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- Changes since v2: - Use u64 - Rename function to roundup/down_pow_two_u64() - Use 1ULL instead of 1UL - Include function usage in of/device.c and acpi/arm64/iort.c drivers/acpi/arm64/iort.c | 2 +- drivers/net/ethernet/mellanox/mlx4/en_clock.c | 3 +- drivers/of/device.c | 2 +- .../pci/controller/cadence/pcie-cadence-ep.c | 7 +-- drivers/pci/controller/cadence/pcie-cadence.c | 7 +-- drivers/pci/controller/pcie-rockchip-ep.c | 9 ++-- include/linux/log2.h | 52 +++++++++++++++++++ kernel/dma/direct.c | 3 +- 8 files changed, 65 insertions(+), 20 deletions(-)