netdev - RE: [PATCH v5 net-next 10/13] net: enetc: extract enetc_int_vector_init/destroy() from enetc_alloc

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <PAXPR04MB8510865086A2629DCF58B20C884F2@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Fri, 25 Oct 2024 03:27:14 +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 10/13] net: enetc: extract
 enetc_int_vector_init/destroy() from enetc_alloc_msix()

> On Thu, Oct 24, 2024 at 02:53:25PM +0800, Wei Fang wrote:
> > From: Clark Wang <xiaoning.wang@....com>
> >
> > Extract enetc_int_vector_init() and enetc_int_vector_destroy() from
> > enetc_alloc_msix() so that the code is more concise and readable. In
> > addition, slightly different from before, the cleanup helper function
> > is used to manage dynamically allocated memory resources.
> >
> > Signed-off-by: Clark Wang <xiaoning.wang@....com>
> > Signed-off-by: Wei Fang <wei.fang@....com>
> > Reviewed-by: Frank Li <Frank.Li@....com>
> > Reviewed-by: Claudiu Manoil <claudiu.manoil@....com>
> > ---
> > v5: no changes
> > ---
> >  drivers/net/ethernet/freescale/enetc/enetc.c | 172 ++++++++++---------
> >  1 file changed, 87 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 032d8eadd003..bd725561b8a2 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -2965,6 +2965,87 @@ int enetc_ioctl(struct net_device *ndev, struct
> ifreq *rq, int cmd)
> >  }
> >  EXPORT_SYMBOL_GPL(enetc_ioctl);
> >
> > +static int enetc_int_vector_init(struct enetc_ndev_priv *priv, int i,
> > +				 int v_tx_rings)
> > +{
> > +	struct enetc_int_vector *v __free(kfree);
> 
> Please limit yourself to refactoring the code as-is into a separate function.

Okay

> This function does not benefit in any way from the use of __free() and
> no_free_ptr(). The established norm is that the normal teardown path
> should be identical to the error unwind path, minus the last step.
> Combining normal function calls with devres/scope-based cleanup/whatever
> other "look, you don't get to care about error handling!!!" APIs there may be
> makes that much more difficult to reason about. If the function is really
> simple and you really don't get to care about error handling by using __free(),
> then maybe its usage is tolerable, but that is hardly the general case.
> The more intricate the error handling becomes, the less useful __free() is,
> and the more it starts getting in the way of a human correctness reviewer.
> 
> FWIW, Documentation/process/maintainer-netdev.rst, section "Using
> device-managed and cleanup.h constructs", appears to mostly state the
> same position as me here.
> 
> Obviously, here, the established cleanup norm isn't followed anyway, but
> a patch which brings it in line would be appreciated. I think that a
> multi-patch approach, where the code is first moved and just moved, and
> then successively transformed in obviously correct and easy to review
> steps, would be best.
> 
> Since you're quite close currently to the 15-patch limit, I would try to
> create a patch set just for preparing the enetc drivers, and introduce
> the i.mx95 support in a separate set which has mostly "+" lines in its diff.
> That way, there is also some time to not rush the ongoing IERB/PRB
> dt-binding discussion, while you can progress on pure driver refactoring.
> 

Thanks for your suggestion.