Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 26 Jan 2022 02:57:03 +0100
From:   Ansuel Smith <ansuelsmth@gmail.com>
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Subject: Re: [RFC PATCH v7 12/16] net: dsa: qca8k: add support for phy
 read/write with mgmt Ethernet
Message-ID: <YfCqb/qHn0XR8ONV@Ansuel-xps.localdomain>
References: <20220123013337.20945-1-ansuelsmth@gmail.com>
 <20220123013337.20945-13-ansuelsmth@gmail.com>
 <20220125150355.5ywi4fe3puxaphq3@skbuf>
 <61f08471.1c69fb81.a3d6.4d94@mx.google.com>
 <20220126014854.opnyrd56nsrk7udp@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220126014854.opnyrd56nsrk7udp@skbuf>
Precedence: bulk

On Wed, Jan 26, 2022 at 03:48:54AM +0200, Vladimir Oltean wrote:
> 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.
>

I'm just implementing this. (rolling counter)

> > > > +	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.

Hdr as header stuff since all this stuff is put in the hdr. Should I
just drop hdr and use mgmt_data directly? Or mgmt_eth?

-- 
	Ansuel