On 9/29/2025 9:37 PM, Greg KH wrote:
> On Mon, Sep 29, 2025 at 03:26:57PM -0400, Joel Fernandes wrote:
>> On 9/24/2025 12:40 PM, Greg KH wrote:
>>> On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:
>> [..]
>>>
>>>> For the separate issue of enforcing endianness with respect to (across)
>>>> multiple fields, I agree with you that if the user's backend (the consumer
>>>> of
>>>> the data) is not doing such conversion, say via regmap, then that becomes a
>>>> problem. But that problem is orthogonal/different and cannot be solved
>>>> here.
>>>
>>> But that is exactly what these macros are being defined here for, so to
>>> ignore that is going to cause problems :)
>>>
>>
>> If needed, happy to add endianness support as needed by providing additional
>> options to the macro. Based on this thread, it sounds like we want see if
>> that
>> is really needed here or can be solved elsewhere (?). The mental model I
>> kind of
>> have is this macro should only be dealing with CPU native endianness, much
>> like
>> bitfields in C deal with CPU endianness. Hmm.
>
> Just don't go down the old path like drivers/net/fddi/skfp/h/supern_2.h
> does with it's definition of:
But this is not a comparable example though because in our macro we specify
individual bit numbers, not just bit width. So tx_length for example would
always start at the lower 2 bytes since we'd specify "tx_length 0:15" regardless
of whether those 2 bytes are higher in memory or lower in memory. Whether it is
higher or lower depends on CPU endianness AFAICS, and if interacting with HW is
needed, the user of the macro would do endianness conversion. Maybe we should
add a comment about such conversion requirements?
Then there is the issue of the byte ordering within a multi-byte field. That
again follows CPU endianness, and we could add a comment for the benefit of
macro user.
>
> union tx_descr {
> struct {
> #ifdef LITTLE_ENDIAN
> unsigned int tx_length:16 ; /* frame length lower/upper
> byte */
> unsigned int tx_res :8 ; /* reserved (bit 16..23) */
> unsigned int tx_xmtabt:1 ; /* transmit abort */
> unsigned int tx_nfcs :1 ; /* no frame check sequence */
> unsigned int tx_xdone :1 ; /* give up token */
> unsigned int tx_rpxm :2 ; /* byte offset */
> unsigned int tx_pat1 :2 ; /* must be TXP1 */
> unsigned int tx_more :1 ; /* more frame in chain */
> #else
> unsigned int tx_more :1 ; /* more frame in chain */
> unsigned int tx_pat1 :2 ; /* must be TXP1 */
> unsigned int tx_rpxm :2 ; /* byte offset */
> unsigned int tx_xdone :1 ; /* give up token */
> unsigned int tx_nfcs :1 ; /* no frame check sequence */
> unsigned int tx_xmtabt:1 ; /* transmit abort */
> unsigned int tx_res :8 ; /* reserved (bit 16..23) */
> unsigned int tx_length:16 ; /* frame length lower/upper
> byte */
> #endif
> } t ;
> long i ;
> } ;
>
This is indeed yuck though but afaics not what hopefully we'd be doing ;-)
thanks,
- Joel
