lkml.org 
[lkml]   [2021]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH 01/17] net: ipa: Correct ipa_status_opcode enumeration
    From
    Date
    On Thu Oct 14, 2021 at 3:58 AM IST, Alex Elder wrote:
    > On 9/19/21 10:07 PM, Sireesh Kodali wrote:
    > > From: Vladimir Lypak <vladimir.lypak@gmail.com>
    > >
    > > The values in the enumaration were defined as bitmasks (base 2 exponents of
    > > actual opcodes). Meanwhile, it's used not as bitmask
    > > ipa_endpoint_status_skip and ipa_status_formet_packet functions (compared
    > > directly with opcode from status packet). This commit converts these values
    > > to actual hardware constansts.
    > >
    > > Signed-off-by: Vladimir Lypak <vladimir.lypak@gmail.com>
    > > Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com>
    > > ---
    > > drivers/net/ipa/ipa_endpoint.c | 8 ++++----
    > > 1 file changed, 4 insertions(+), 4 deletions(-)
    > >
    > > diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
    > > index 5528d97110d5..29227de6661f 100644
    > > --- a/drivers/net/ipa/ipa_endpoint.c
    > > +++ b/drivers/net/ipa/ipa_endpoint.c
    > > @@ -41,10 +41,10 @@
    > >
    > > /** enum ipa_status_opcode - status element opcode hardware values */
    > > enum ipa_status_opcode {
    > > - IPA_STATUS_OPCODE_PACKET = 0x01,
    > > - IPA_STATUS_OPCODE_DROPPED_PACKET = 0x04,
    > > - IPA_STATUS_OPCODE_SUSPENDED_PACKET = 0x08,
    > > - IPA_STATUS_OPCODE_PACKET_2ND_PASS = 0x40,
    > > + IPA_STATUS_OPCODE_PACKET = 0,
    > > + IPA_STATUS_OPCODE_DROPPED_PACKET = 2,
    > > + IPA_STATUS_OPCODE_SUSPENDED_PACKET = 3,
    > > + IPA_STATUS_OPCODE_PACKET_2ND_PASS = 6,
    >
    > I haven't looked at how these symbols are used (whether you
    > changed it at all), but I'm pretty sure this is wrong.
    >
    > The downstream tends to define "soft" symbols that must
    > be mapped to their hardware equivalent values. So for
    > example you might find a function ipa_pkt_status_parse()
    > that translates between the hardware status structure
    > and the abstracted "soft" status structure. In that
    > function you see, for example, that hardware status
    > opcode 0x1 is translated to IPAHAL_PKT_STATUS_OPCODE_PACKET,
    > which downstream is defined to have value 0.
    >
    > In many places the upstream code eliminates that layer
    > of indirection where possible. So enumerated constants
    > are assigned specific values that match what the hardware
    > uses.
    >

    Looking at these again, I realised this patch is indeed wrong...
    The status values are different on v2 and v3+. I guess the correct
    approach here would be to use an inline function and pick the correct
    status opcode, like how its handled for register defintions.

    Regards,
    Sireesh

    > -Alex
    >
    > > };
    > >
    > > /** enum ipa_status_exception - status element exception type */
    > >

    \
     
     \ /
      Last update: 2021-10-18 18:17    [W:2.847 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site