Hi Andy,
On 1/27/26 4:38 PM, Andy Shevchenko wrote:
> On Tue, Jan 27, 2026 at 03:58:13PM +0200, Jani Nikula wrote:
>> On Tue, 27 Jan 2026, Cristian Ciocaltea <[email protected]>
>> wrote:
>>> Currently DIV_ROUND_CLOSEST() is only available for the kernel via
>>> include/linux/math.h.
>>>
>>> Expose it to userland as well by adding __KERNEL_DIV_ROUND_CLOSEST() as
>>> a common definition in uapi.
>>>
>>> Additionally, ensure it allows building ISO C applications by switching
>>> from the 'typeof' GNU extension to the ISO-friendly __typeof__.
>>
>> I am not convinced that it's a good idea to make the implementation of
>> kernel DIV_ROUND_CLOSEST() part of the kernel UAPI, which is what this
>> change effectively does.
>>
>> I'd at least like to get an ack from Andy Shevchenko first (Cc'd).
>
> Thanks for Cc'ing me!
>
> So, the history of the DIV_ROUND_UP() to appear in UAPI is a response to
> the ethtool change that missed the fact that this was a kernel internal macro.
> Giving a precedent there is no technical issues to add DIV_ROUND_CLOSEST()
> to UAPI as proposed. Main question here is: Does DRM headers in question
> (that are going to use it) really need this?
>
> Interestingly that DRM also started using __KERNEL_DIV_ROUND_UP() in UAPI
> at some point, which kinda makes an argument for allowing the other one.
>
> Also fun fact: this series plead for a new macro for division while ignoring
> existing (UAPI) macros for masks and bits.
Yeah, I initially planned to make use of those, but gave up after realizing the
FIELD_{PREP|GET} were not exposed. I reconsidered my position now that you
mentioned it. :-)
>
> 0xffffU is effectively __GENMASK(15, 0). (And if you change the code, avoid
> using variables inside GENMASK() macros, it may generate an awful code, the
> GENMASK($HI, $LO) << foo is preferred over GENMASK(foo + $DELTA, foo) case.
> GENMASK(foo - 1, 0) OTOH is fine, however be always careful against overflows
> with left shifts, as BIT(foo) - 1 may not work for foo == 32, while GENMASK()
> may not work for foo == 0).
Thanks for the heads up!
>
> So, I have no objections for either choice
> Acked-by: Andy Shevchenko <[email protected]>
>
> ...
>
> But if you go that direction, please, fix up the style.
>
>>> +/*
>>> + * Divide positive or negative dividend by positive or negative divisor
>>> + * and round to closest integer. Result is undefined for negative
>>> + * divisors if the dividend variable type is unsigned and for negative
>>> + * dividends if the divisor variable type is unsigned.
>>> + */
>>> +#define __KERNEL_DIV_ROUND_CLOSEST(x, divisor)( \
>>> +{ \
>
> Use ({ on this line together...
>
>>> + __typeof__(x) __x = x; \
>>> + __typeof__(divisor) __d = divisor; \
>
> + blank line here.
>
>>> + (((__typeof__(x))-1) > 0 || \
>>> + ((__typeof__(divisor))-1) > 0 || \
>>> + (((__x) > 0) == ((__d) > 0))) ? \
>>> + (((__x) + ((__d) / 2)) / (__d)) : \
>>> + (((__x) - ((__d) / 2)) / (__d)); \
>>> +} \
>>> +)
>
> ...as here join }) to be a single line.
>
> + blank line.
>
>>> #endif /* _UAPI_LINUX_CONST_H */
Thanks for the review! I've addressed all your comments (hopefully) in v6:
https://lore.kernel.org/all/[email protected]/
Regards,
Cristian
