Message-ID: <e1b27721-19ff-2ae9-2885-90a7948f774f@linaro.org>
Date:   Tue, 21 May 2019 16:50:26 -0500
From:   Alex Elder <elder@...aro.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Arnd Bergmann <arnd@...db.de>
Cc:     Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>,
        David Miller <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC] net: qualcomm: rmnet: Move common struct definitions
 to include

On 5/21/19 4:08 PM, Bjorn Andersson wrote:
> On Tue 21 May 13:45 PDT 2019, Arnd Bergmann wrote:
> 
>> On Tue, May 21, 2019 at 9:35 PM Subash Abhinov Kasiviswanathan
>> <subashab@...eaurora.org> wrote:
>>>
>>> Create if_rmnet.h and move the rmnet MAP packet structs to this
>>> common include file. To account for portability, add little and
>>> big endian bitfield definitions similar to the ip & tcp headers.
>>>
>>> The definitions in the headers can now be re-used by the
>>> upcoming ipa driver series as well as qmi_wwan.
>>>
>>> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
>>> ---
>>> This patch is an alternate implementation of the series posted by Elder.
>>> This eliminates the changes needed in the rmnet packet parsing
>>> while maintaining portability.
>>> ---
>>
>> I think I'd just duplicate the structure definitions then, to avoid having
>> the bitfield definitions in a common header and using them in the new
>> driver.
>>
> 
> Doing would allow each driver to represent the bits as suitable, at the
> cost of some duplication and confusion. Confusion, because it doesn't
> resolve the question of what the right bit order actually is.

I have exchanged a few private messages with Subash.  He has said
that it is the high-order bit that indicates whether a QMAP packet
contains a command (1) or data (0).  That bit might be extracted
this way:

    u8 byte = *(u8 *)skb->data;
    bool command = !!(byte & 0x80);

Subash, if you don't mind, please acknowledge that again here
so everyone knows.

What this means is that the first patch in my series is wrong,
because I misinterpreted the documentation, which indicated
bit 0 was the command/data bit.  (I presume this is the first
bit that travels over the wire, and is not referring to the
conventionally-understood lowest bit position in the first byte.)

My plan is, as I said in a previous message:
- Remove the first patch (that switches the bit-fields)
- Adjust the subsequent patches to use correct field masks
- Re-send the series as v2, with Bjorn's Reviewed-by.

> Subash stated yesterday that bit 0 is "CD", which in the current struct
> is represented as the 8th bit, while Alex's patch changes the definition
> so that this bit is the lsb. I.e. I read Subash answer as confirming
> that patch 1/8 from Alex is correct.

I'm not sure about that but I don't want to confuse things further.

> Subash, as we're not addressing individual bits in this machine, so
> given a pointer map_hdr to a struct rmnet_map_header, which of the
> following ways would give you the correct value of pad_len:
> 
> u8 p = *(char*)map_hdr;
> pad_len = p & 0x3f;
> 
> or:
> 
> u8 p = *(char*)map_hdr;
> pad_len = p >> 2;

My new understanding is it's the latter.

> PS. I do prefer the two drivers share the definition of these
> structures...

I agree with you completely.  I don't think it makes sense to
have two definitions of the same structure.  Subash wants to
reduce the impact my changes have on the "rmnet" driver, but
duplicating things makes things worse, not better.  The IPA
driver *assumes* it is talking to the rmnet driver; their
interface definition should be common...

					-Alex

> Regards,
> Bjorn
> 

Powered by blists - more mailing lists