netdev - Re: [RFC PATCH 01/17] net: ipa: Correct ipa_status

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <CF2NYEUKQORV.3GXBPSZ0DT1BM@skynet-linux>
Date:   Mon, 18 Oct 2021 21:42:04 +0530
From:   "Sireesh Kodali" <sireeshkodali1@...il.com>
To:     "Alex Elder" <elder@...e.org>, <phone-devel@...r.kernel.org>,
        <~postmarketos/upstreaming@...ts.sr.ht>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <elder@...nel.org>
Cc:     "Vladimir Lypak" <vladimir.lypak@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        "Jakub Kicinski" <kuba@...nel.org>
Subject: Re: [RFC PATCH 01/17] net: ipa: Correct ipa_status_opcode
 enumeration

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