Re: [RFC PATCH 01/17] net: ipa: Correct ipa_status_opcode enumeration

[Sireesh Kodali]

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@xxxxxxxxx>
> > 
> > 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@xxxxxxxxx>
> > Signed-off-by: Sireesh Kodali <sireeshkodali1@xxxxxxxxx>
> > ---
> >   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 */
> >