lkml.org 
[lkml]   [2024]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH net-next v17 01/13] rtase: Add pci table supported in this module
    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@realtek.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 :)

    > + *
    > + * 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?

    > + }
    > +
    > + 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().

    > + else
    > + tp->sw_flag |= RTASE_SWF_MSI_ENABLED;
    > + } else {
    > + tp->sw_flag |= RTASE_SWF_MSIX_ENABLED;
    > + }
    > +
    > + return ret;
    > +}

    ..

    \
     
     \ /
      Last update: 2024-05-27 18:14    [W:3.607 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site