On 06/03/2025 at 18:12, Andy Shevchenko wrote: > On Wed, Mar 05, 2025 at 09:50:27PM +0000, David Laight wrote: >> On Wed, 5 Mar 2025 21:56:22 +0200 >> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: >>> On Thu, Mar 06, 2025 at 02:17:18AM +0900, Vincent Mailhol wrote: >>>> On 06/03/2025 at 00:48, Andy Shevchenko wrote: >>>>> On Wed, Mar 05, 2025 at 11:48:10PM +0900, Vincent Mailhol wrote: >>>>>> On 05/03/2025 at 23:33, Andy Shevchenko wrote: >>>>>>> On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote: > > ... > >>>>>>>> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b)) >>>>>>>> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b)) >>>>>>> >>>>>>> Why not u8 and u16? This inconsistency needs to be well justified. >>>>>> >>>>>> Because of the C integer promotion rules, if casted to u8 or u16, the >>>>>> expression will immediately become a signed integer as soon as it is get >>>>>> used. For example, if casted to u8 >>>>>> >>>>>> BIT_U8(0) + BIT_U8(1) >>>>>> >>>>>> would be a signed integer. And that may surprise people. >>>>> >>>>> Yes, but wouldn't be better to put it more explicitly like >>>>> >>>>> #define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (u8)BIT(b) + 0 + UL(0)) // + ULL(0) ? >>>> >>>> OK, the final result would be unsigned. But, I do not follow how this is >>>> more explicit. >>>> >>>> Also, why doing: >>>> >>>> (u8)BIT(b) + 0 + UL(0) >>>> >>>> and not just: >>>> >>>> (u8)BIT(b) + UL(0) >>>> >>>> ? >>>> >>>> What is that intermediary '+ 0' for? >>>> >>>> I am sorry, but I am having a hard time understanding how casting to u8 >>>> and then doing an addition with an unsigned long is more explicit than >>>> directly doing a cast to the desired type. >>> >>> Reading this again, I think we don't need it at all. u8, aka unsigned char, >>> will be promoted to int, but it will be int with a value < 256, can't be signed >>> as far as I understand this correctly. >> >> The value can't be negative, but the type will be a signed one. > > Yes, that's what I mentioned above: "int with the value < 256". > >> Anything comparing types (and there are a few) will treat it as signed. >> It really is bad practise to even pretend you can have an expression >> (rather that a variable) that has a type smaller than 'int'. >> It wouldn't surprise me if even an 'a = b' assignment promotes 'b' to int. > > We have tons of code with u8/u16, what you are proposing here is like > "let's get rid of those types and replace all of them by int/unsigned int". > We have ISAs that are byte-oriented despite being 32- or 64-bit platforms. > >> So it is even questionable whether BIT8() and BIT16() should even exist at all. > > The point is to check the boundaries and not in the returned value per se. +1 I will also add that this adds to the readability of the code. In a driver, if I see: #define REG_FOO1_MASK GENMASK(6, 2) #define REG_FOO2_MASK GENMASK(12, 7) it does not tell me much about the register. Whereas if I see: #define REG_FOO1_MASK GENMASK_U16(6, 2) #define REG_FOO2_MASK GENMASK_U16(12, 7) then I know that this is for a 16 bit register. >> There can be reasons to return 'unsigned int' rather than 'unsigned long'. >> But with the type definitions that Linux uses (and can't really be changed) >> you can have BIT32() that is 'unsigned int' and BIT64() that is 'unsigned long >> long'. These are then the same on 32bit and 64bit. So, at the end, my goal when introducing that unsigned int cast was not to confuse people. This had the opposite effect. Nearly all the reviewers pointed at that cast. I will revert this in the v5. The U8 and U16 variants of both GENMASK and BIT will return an u8 and u16 respectively. And unless someone manages to convince Yury otherwise, I will keep it as such. Yours sincerely, Vincent Mailhol
