linux-kernel - Re: [RFC PATCH v7 12/16] net: dsa: qca8k: add support for phy read/write with mgmt Ethernet

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]

Date:   Wed, 26 Jan 2022 03:48:54 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Ansuel Smith <ansuelsmth@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [RFC PATCH v7 12/16] net: dsa: qca8k: add support for phy
 read/write with mgmt Ethernet

On Wed, Jan 26, 2022 at 12:14:55AM +0100, Ansuel Smith wrote:
> > At some point, you'll have to do something about those sequence numbers.
> > Hardcoding 200 and 400 isn't going to get you very far, it's prone to
> > errors. How about dealing with it now? If treating them as actual
> > sequence numbers isn't useful because you can't have multiple packets in
> > flight due to reordering concerns, at least create a macro for each
> > sequence number used by the driver for packet identification.
> 
> Is documenting define and adding some inline function acceptable? That
> should make the separation more clear and also prepare for a future
> implementation. The way I see an use for the seq number is something
> like a global workqueue that would handle all this stuff and be the one
> that handle the seq number.
> I mean another way would be just use a counter that will overflow and
> remove all this garbage with hardcoded seq number.
> (think will follow this path and just implement a correct seq number...)

Cleanest would be, I think, to just treat the sequence number as a
rolling counter and use it to match the request to the response.
But I didn't object to your use of fixed numbers per packet type, just
to the lack of a #define behind those numbers.

> > > +	mutex_lock(&phy_hdr_data->mutex);
> > 
> > Shouldn't qca8k_master_change() also take phy_hdr_data->mutex?
> > 
> 
> Is actually the normal mgmg_hdr_data. 
> 
> phy_hdr_data = &priv->mgmt_hdr_data;
> 
> Should I remove this and use mgmt_hdr_data directly to remove any
> confusion? 

I am not thrilled by the naming of this data structure anyway
(why "hdr"?), but yes, I also got tricked by inconsistent naming.
Please choose a consistent name and stick with it.