netdev - Re: [PATCH net 2/2] net: dsa: sja1105: always enable the send

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: <ZJ8S/Deq94MAonfG@corigine.com>
Date: Fri, 30 Jun 2023 19:38:04 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 2/2] net: dsa: sja1105: always enable the send_meta
 options

On Fri, Jun 30, 2023 at 07:53:00PM +0300, Vladimir Oltean wrote:
> Hi Simon,
> 
> On Fri, Jun 30, 2023 at 03:35:23PM +0200, Simon Horman wrote:
> > Hi Vladimir,
> > 
> > this patch isn't that big, so I'm ok with it.  But it also isn't that
> > small, so I'd just like to mention that a different approach might be a
> > small patch that enables meta frame generation unconditionally, as a fix.
> > And then, later, some cleanup, which seems to comprise most of this patch.
> > 
> > I do admit that I didn't try this. So it might not be sensible.  And as I
> > said, I am ok with this patch. But I did think it was worth mentioning.
> > 
> > Reviewed-by: Simon Horman <simon.horman@...igine.com>
> > 
> > ...
> 
> Thanks for the feedback.
> 
> You're saying that it's preferable to leave sja1105_rxtstamp_set_state()
> and sja1105_rxtstamp_get_state() where they are, accessible through
> tagger_data->rxtstamp_set_state() and tagger_data->rxtstamp_get_state(),
> but to only call tagger_data->rxtstamp_set_state() once, statically with
> "true", and to never call tagger_data->rxtstamp_get_state()?

I'm not the ultimate authority on what is and isn't acceptable.
But, yes, I was suggesting something like that.

> I probably could, but I'd have to delay the tagger_data->rxtstamp_set_state()
> call until sja1105_connect_tag_protocol(); it's not going to be available
> earlier. And this is going to be just filler code that I will then delete
> as soon as net-next reopens.

Right, so there is complexity. And dead code.
Or we could just cut to the chase, which is what you have done.

So it seems your approach is best.