netdev - RE: [PATCH v5 net-next 06/13] net: enetc: build enetc_pf

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:
 <PAXPR04MB8510D0548F3C5AD3101D5BAA88482@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Sat, 26 Oct 2024 03:23:44 +0000
From: Wei Fang <wei.fang@....com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, Claudiu Manoil <claudiu.manoil@....com>, Clark Wang
	<xiaoning.wang@....com>, Frank Li <frank.li@....com>,
	"christophe.leroy@...roup.eu" <christophe.leroy@...roup.eu>,
	"linux@...linux.org.uk" <linux@...linux.org.uk>, "bhelgaas@...gle.com"
	<bhelgaas@...gle.com>, "horms@...nel.org" <horms@...nel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>, "alexander.stein@...tq-group.com"
	<alexander.stein@...tq-group.com>
Subject: RE: [PATCH v5 net-next 06/13] net: enetc: build enetc_pf_common.c as
 a separate module

> On Fri, Oct 25, 2024 at 06:00:42AM +0300, Wei Fang wrote:
> > > Don't artificially create errors when there are really no errors to handle.
> > > Both enetc_pf_ops and enetc4_pf_ops provide .set_si_primary_mac(), so it
> > > is unnecessary to handle the case where it isn't present. Those functions
> > > return void, and void can be propagated to enetc_set_si_hw_addr() as well.
> >
> > I thought checking the pointer is safer, so you mean that pointers that are
> > definitely present in the current driver do not need to be checked?
> 
> Yes, there is no point to check for a condition which is impossible
> to trigger in the current code. The callee and the caller are tightly
> coupled (in the same driver), it's not a rigid API, so if the function
> pointers should be made optional for some future hardware IP, the error
> handling will be added when necessary. Ideally any change passes through
> review, and any inconsistency should be spotted when added.
> 
> > > A bit inconsistent that pf->ops->set_si_primary_mac() goes through a
> > > wrapper function but this doesn't.
> > >
> >
> > If we really do not need to check these callback pointers, then I think I can
> > remove the wrapper.
> 
> Fine without wrapping throughout, my comment was first and foremost
> about consistency.
> 
> > > This one looks extremely weird in the existing code, but I suppose I'm
> > > too late to the party to request you to clean up any of the PSFP code,
> > > so I'll make a note to do it myself after your work. I haven't spotted
> > > any actual bug, just weird coding patterns.
> > >
> > > No change request here. I see the netc4_pf doesn't implement
> enable_psfp(),
> > > so making it optional here is fine.
> >
> > Yes, PSFP is not supported in this patch set, I will remove it in future.
> 
> If by "I will remove it in future" you mean "once NETC4 gains PSFP
> support, I will make enable_psfp() non-optional", then ok, great.
> 
> > Currently, we have not add the PCS support, so the 10G ENETC is not
> supported
> > yet. And we also disable the 10G ENETC in DTS. Only the 1G ENETCs (without
> PCS)
> > are supported for i.MX95.
> 
> Also think about the case where the current version of the kernel
> will boot on a newer version of the device tree, which does not have
> 'status = "disabled";' for those ports. It should do something reasonable.
> In any case, "they're now disabled in the device tree" is not an argument.
> 

For this case where the kernel is lower but the device tree is newer, I think
there is no problem. It just fails in probe() and does not affect other functions.
The old kernel does not support PCS, it is reasonable to return a '- EOPNOTSUPP'
error.

> My anecdotal and vague understanding of the Arm SystemReady (IR I think)
> requirements is that the device tree is provided by the platform,
> separately from the kernel/rootfs. It relies on the device tree ABI
> being stable, backwards-compatible and forwards-compatible.
> 
> > > A message maybe, stating what's the deal? Just that users figure out
> > > quickly that it's an expected behavior, and not spend hours debugging
> > > until they find out it's not their fault?
> >
> > I will explain in the commit message that i.MX95 10G ENETC is not currently
> > supported.
> 
> By "a message, maybe" I actually meant dev_err("Message here\n"); rather
> than silent/imprecise failure.

Okay, I'll add an explicit error message.