Messages in this thread | | | Date | Wed, 26 Jan 2022 02:57:03 +0100 | From | Ansuel Smith <> | 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 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
| |