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: <CAHp75VdtFET87R9DZbz27vEeyv4K5bn7mxDCnBVdpFVJ=j6qtg@mail.gmail.com>
Date: Mon, 5 Aug 2024 22:13:38 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Herve Codina <herve.codina@...tlin.com>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>, Simon Horman <horms@...nel.org>, Lee Jones <lee@...nel.org>, 
	Arnd Bergmann <arnd@...db.de>, Derek Kiernan <derek.kiernan@....com>, 
	Dragan Cvetic <dragan.cvetic@....com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Philipp Zabel <p.zabel@...gutronix.de>, 
	Lars Povlsen <lars.povlsen@...rochip.com>, Steen Hegelund <Steen.Hegelund@...rochip.com>, 
	Daniel Machon <daniel.machon@...rochip.com>, UNGLinuxDriver@...rochip.com, 
	Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Horatiu Vultur <horatiu.vultur@...rochip.com>, Andrew Lunn <andrew@...n.ch>, 
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	devicetree@...r.kernel.org, Allan Nielsen <allan.nielsen@...rochip.com>, 
	Luca Ceresoli <luca.ceresoli@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v4 1/8] misc: Add support for LAN966x PCI device

On Mon, Aug 5, 2024 at 12:19 PM Herve Codina <herve.codina@...tlin.com> wrote:
>
> Add a PCI driver that handles the LAN966x PCI device using a device-tree
> overlay. This overlay is applied to the PCI device DT node and allows to
> describe components that are present in the device.
>
> The memory from the device-tree is remapped to the BAR memory thanks to
> "ranges" properties computed at runtime by the PCI core during the PCI
> enumeration.
>
> The PCI device itself acts as an interrupt controller and is used as the
> parent of the internal LAN966x interrupt controller to route the
> interrupts to the assigned PCI INTx interrupt.

...

+ device.h

> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>

> +#include <linux/pci.h>

> +#include <linux/pci_ids.h>

AFAIU pci_ids..h is guaranteed to be included by pci.h, but having it
here explicitly doesn't make it worse, so up to you.

> +#include <linux/slab.h>

...

> +static irqreturn_t pci_dev_irq_handler(int irq, void *data)
> +{
> +       struct pci_dev_intr_ctrl *intr_ctrl = data;
> +       int ret;
> +
> +       ret = generic_handle_domain_irq(intr_ctrl->irq_domain, 0);
> +       return IRQ_RETVAL(!ret);

Hmm... I dunno if it was me who suggested IRQ_RETVAL() here, but it
usually makes sense for the cases where ret is not inverted.

Perhaps

  if (ret)
    return NONE;
  return HANDLED;

is slightly better in this case?

> +}

...

> +static struct pci_dev_intr_ctrl *pci_dev_create_intr_ctrl(struct pci_dev *pdev)
> +{
> +       struct pci_dev_intr_ctrl *intr_ctrl;
> +       struct fwnode_handle *fwnode;
> +       int ret;

> +       if (!pdev->irq)
> +               return ERR_PTR(-EOPNOTSUPP);

Before even trying to get it via APIs? (see below as well)
Also, when is it possible to have 0 here?

> +       fwnode = dev_fwnode(&pdev->dev);
> +       if (!fwnode)
> +               return ERR_PTR(-ENODEV);
> +
> +       intr_ctrl = kmalloc(sizeof(*intr_ctrl), GFP_KERNEL);

Hmm... Why not use __free()?

> +       if (!intr_ctrl)
> +               return ERR_PTR(-ENOMEM);
> +
> +       intr_ctrl->pci_dev = pdev;
> +
> +       intr_ctrl->irq_domain = irq_domain_create_linear(fwnode, 1, &pci_dev_irq_domain_ops,
> +                                                        intr_ctrl);
> +       if (!intr_ctrl->irq_domain) {
> +               pci_err(pdev, "Failed to create irqdomain\n");
> +               ret = -ENOMEM;
> +               goto err_free_intr_ctrl;
> +       }

> +       ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
> +       if (ret < 0) {
> +               pci_err(pdev, "Unable alloc irq vector (%d)\n", ret);
> +               goto err_remove_domain;
> +       }

I am wondering if you even need this in case you want solely INTx.

> +       intr_ctrl->irq = pci_irq_vector(pdev, 0);

Don't remember documentation by heart for this, but the implementation
suggests that it can be called without the above for retrieving INTx.

> +       ret = request_irq(intr_ctrl->irq, pci_dev_irq_handler, IRQF_SHARED,
> +                         dev_name(&pdev->dev), intr_ctrl);

pci_name() ? (IIRC the macro name)

> +       if (ret) {
> +               pci_err(pdev, "Unable to request irq %d (%d)\n", intr_ctrl->irq, ret);
> +               goto err_free_irq_vector;
> +       }
> +
> +       return intr_ctrl;
> +
> +err_free_irq_vector:
> +       pci_free_irq_vectors(pdev);
> +err_remove_domain:
> +       irq_domain_remove(intr_ctrl->irq_domain);
> +err_free_intr_ctrl:
> +       kfree(intr_ctrl);
> +       return ERR_PTR(ret);
> +}

...

> +static void devm_pci_dev_remove_intr_ctrl(void *data)
> +{

> +       struct pci_dev_intr_ctrl *intr_ctrl = data;

It can be eliminated

static void devm_pci_...(void *intr_ctrl)

> +       pci_dev_remove_intr_ctrl(intr_ctrl);
> +}

...

> +static int lan966x_pci_load_overlay(struct lan966x_pci *data)
> +{
> +       u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
> +       void *dtbo_start = __dtbo_lan966x_pci_begin;
> +       int ret;
> +
> +       ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, dev_of_node(data->dev));
> +       if (ret)
> +               return ret;
> +
> +       return 0;

return of_overlay_fdt_apply() ?

> +}

...

> +static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct lan966x_pci *data;
> +       int ret;
> +
> +       /*
> +        * On ACPI system, fwnode can point to the ACPI node.
> +        * This driver needs an of_node to be used as the device-tree overlay
> +        * target. This of_node should be set by the PCI core if it succeeds in
> +        * creating it (CONFIG_PCI_DYNAMIC_OF_NODES feature).
> +        * Check here for the validity of this of_node.
> +        */
> +       if (!dev_of_node(dev)) {

> +               dev_err(dev, "Missing of_node for device\n");
> +               return -EINVAL;

return dev_err_probe() ?

> +       }
> +
> +       /* Need to be done before devm_pci_dev_create_intr_ctrl.
> +        * It allocates an IRQ and so pdev->irq is updated.
> +        */
> +       ret = pcim_enable_device(pdev);
> +       if (ret)
> +               return ret;
> +
> +       ret = devm_pci_dev_create_intr_ctrl(pdev);
> +       if (ret)
> +               return ret;
> +
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       pci_set_drvdata(pdev, data);
> +       data->dev = dev;
> +
> +       ret = lan966x_pci_load_overlay(data);
> +       if (ret)
> +               return ret;
> +
> +       pci_set_master(pdev);
> +
> +       ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
> +       if (ret)
> +               goto err_unload_overlay;
> +
> +       return 0;
> +
> +err_unload_overlay:
> +       lan966x_pci_unload_overlay(data);
> +       return ret;
> +}

...

> +#include <dt-bindings/clock/microchip,lan966x.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/mfd/atmel-flexcom.h>
> +#include <dt-bindings/phy/phy-lan966x-serdes.h>

> +#include <dt-bindings/gpio/gpio.h>

Alphabetical order?

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ