| 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
| ||
|
Date: Wed, 8 May 2024 09:39:23 +0100
From: Simon Horman <horms@...nel.org>
To: Justin Lai <justinlai0215@...ltek.com>
Cc: "kuba@...nel.org" <kuba@...nel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"andrew@...n.ch" <andrew@...n.ch>,
"jiri@...nulli.us" <jiri@...nulli.us>,
Ping-Ke Shih <pkshih@...ltek.com>,
Larry Chiu <larry.chiu@...ltek.com>
Subject: Re: [PATCH net-next v17 01/13] rtase: Add pci table supported in
this module
On Mon, May 06, 2024 at 11:32:38AM +0000, Justin Lai wrote:
> >
> >
> > On Thu, May 02, 2024 at 05:18:35PM +0800, Justin Lai wrote:
> > > Add pci table supported in this module, and implement pci_driver
> > > function to initialize this driver, remove this driver, or shutdown this driver.
> > >
> > > Signed-off-by: Justin Lai <justinlai0215@...ltek.com>
> >
> > ...
> >
> > > diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > > b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > > new file mode 100644
> > > index 000000000000..5ddb5f7abfe9
> > > --- /dev/null
> > > +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > > @@ -0,0 +1,618 @@
> > > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> > > +/*
> > > + * rtase is the Linux device driver released for Realtek Automotive
> > > +Switch
> > > + * controllers with PCI-Express interface.
> > > + *
> > > + * Copyright(c) 2023 Realtek Semiconductor Corp.
> > > + *
> > > + * Below is a simplified block diagram of the chip and its relevant
> > interfaces.
> > > + *
> > > + * *************************
> > > + * * *
> > > + * * CPU network device *
> > > + * * *
> > > + * * +-------------+ *
> > > + * * | PCIE Host | *
> > > + * ***********++************
> > > + * ||
> > > + * PCIE
> > > + * ||
> > > + * ********************++**********************
> > > + * * | PCIE Endpoint | *
> > > + * * +---------------+ *
> > > + * * | GMAC | *
> > > + * * +--++--+ Realtek *
> > > + * * || RTL90xx Series *
> > > + * * || *
> > > + * * +-------------++----------------+ *
> > > + * * | | MAC | | *
> > > + * * | +-----+ | *
> > > + * * | | *
> > > + * * | Ethernet Switch Core | *
> > > + * * | | *
> > > + * * | +-----+ +-----+ | *
> > > + * * | | MAC |...........| MAC | | *
> > > + * * +---+-----+-----------+-----+---+ *
> > > + * * | PHY |...........| PHY | *
> > > + * * +--++-+ +--++-+ *
> > > + * *************||****************||***********
> >
> > Thanks for the diagram, I like it a lot :)
> >
>
> Thank you for your like :)
> > > + *
> > > + * The block of the Realtek RTL90xx series is our entire chip
> > > + architecture,
> > > + * the GMAC is connected to the switch core, and there is no PHY in
> > between.
> > > + * In addition, this driver is mainly used to control GMAC, but does
> > > + not
> > > + * control the switch core, so it is not the same as DSA.
> > > + */
> >
> > ...
> >
> > > +static int rtase_alloc_msix(struct pci_dev *pdev, struct
> > > +rtase_private *tp) {
> > > + int ret;
> > > + u16 i;
> > > +
> > > + memset(tp->msix_entry, 0x0, RTASE_NUM_MSIX * sizeof(struct
> > > + msix_entry));
> > > +
> > > + for (i = 0; i < RTASE_NUM_MSIX; i++)
> > > + tp->msix_entry[i].entry = i;
> > > +
> > > + ret = pci_enable_msix_exact(pdev, tp->msix_entry, tp->int_nums);
> > > + if (!ret) {
> >
> > In Linux Networking code it is an idiomatic practice to keep handle errors in
> > branches and use the main path of execution for the non error path.
> >
> > In this case I think that would look a bit like this:
> >
> > ret = pci_enable_msix_exact(pdev, tp->msix_entry, tp->int_nums);
> > if (ret)
> > return ret;
> >
> > ...
> >
> > return 0;
> >
> > > +
> > > + for (i = 0; i < tp->int_nums; i++)
> > > + tp->int_vector[i].irq = pci_irq_vector(pdev, i);
> >
> > pci_irq_vector() can fail, should that be handled here?
>
> Thank you for your feedback, I will confirm this part again.
> >
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int rtase_alloc_interrupt(struct pci_dev *pdev,
> > > + struct rtase_private *tp) {
> > > + int ret;
> > > +
> > > + ret = rtase_alloc_msix(pdev, tp);
> > > + if (ret) {
> > > + ret = pci_enable_msi(pdev);
> > > + if (ret)
> > > + dev_err(&pdev->dev,
> > > + "unable to alloc interrupt.(MSI)\n");
> >
> > If an error occurs then it is a good practice to unwind resource allocations
> > made within the context of this function call, as this leads to more symmetric
> > unwind paths in callers.
> >
> > In this case I think any resources consumed by rtase_alloc_msix() should be
> > released if pci_enable_msi fails. Probably using a goto label is appropriate
> > here.
> >
> > Likewise, I suggest that similar logic applies to errors within
> > rtase_alloc_msix().
> >
>
> Since msi will be enabled only when msix enable fails, when pci_enable_msi fails,
> there will be no problem of msix-related resources needing to be released,
> because the msix interrupt has not been successfully allocated.
Thanks, as long as no allocated resources have not been freed in the case of
returning an error value, then I am happy.
Powered by blists - more mailing lists