netdev - Re: [PATCH net-next 07/11] net: stmmac: platform: provide stmmac_pltfr_remove_no

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: <CAMRc=McXAdvnxyULwhK_0+oLdo6s32q9bU06Upec-a3-zhrqyw@mail.gmail.com>
Date: Thu, 22 Jun 2023 14:25:40 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Simon Horman <simon.horman@...igine.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@...com>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Junxiao Chang <junxiao.chang@...el.com>, 
	Vinod Koul <vkoul@...nel.org>, Bhupesh Sharma <bhupesh.sharma@...aro.org>, netdev@...r.kernel.org, 
	linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH net-next 07/11] net: stmmac: platform: provide stmmac_pltfr_remove_no_dt()

On Thu, Jun 22, 2023 at 2:15 PM Simon Horman <simon.horman@...igine.com> wrote:
>
> On Wed, Jun 21, 2023 at 05:36:46PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> >
> > Add a variant of stmmac_pltfr_remove() that only frees resources
> > allocated by stmmac_pltfr_probe() and - unlike stmmac_pltfr_remove() -
> > does not call stmmac_remove_config_dt().
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > ---
> >  .../ethernet/stmicro/stmmac/stmmac_platform.c | 20 +++++++++++++++++--
> >  .../ethernet/stmicro/stmmac/stmmac_platform.h |  1 +
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
>
> Hi Bartosz,
>
> some minor feedback from my side as it looks like there will be a v2 anyway.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index df417cdab8c1..58d5c5cc2269 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -762,6 +762,23 @@ int stmmac_pltfr_probe(struct platform_device *pdev,
> >  }
> >  EXPORT_SYMBOL_GPL(stmmac_pltfr_probe);
> >
> > +/**
> > + * stmmac_pltfr_remove_no_dt
> > + * @pdev: pointer to the platform device
> > + * Description: This undoes the effects of stmmac_pltfr_probe() by removing the
> > + * driver and calling the platform's exit() callback.
> > + */
> > +void stmmac_pltfr_remove_no_dt(struct platform_device *pdev)
> > +{
> > +     struct net_device *ndev = platform_get_drvdata(pdev);
> > +     struct stmmac_priv *priv = netdev_priv(ndev);
> > +     struct plat_stmmacenet_data *plat = priv->plat;
>
> nit: please use reverse xmas tree - longest line to shortest - for
>      new Networking code.
>
>      e.g.:
>
>         struct net_device *ndev = platform_get_drvdata(pdev);
>         struct stmmac_priv *priv = netdev_priv(ndev);
>         struct plat_stmmacenet_data *plat = plat;
>
>         plat = priv->plat;
>

I normally stick to this convention but here, you need 5 lines for the
same effect and you make it more confusing by initializing some of the
variables at their declaration and some not. In other places in this
driver the same approach is used i.e. not adhering to reverse xmas
tree when all variables are initialized when declared.

Bart

> > +
> > +     stmmac_dvr_remove(&pdev->dev);
> > +     stmmac_pltfr_exit(pdev, plat);
> > +}
> > +EXPORT_SYMBOL_GPL(stmmac_pltfr_remove_no_dt);
> > +
> >  /**
> >   * stmmac_pltfr_remove
> >   * @pdev: platform device pointer
>
> ...