| 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
| ||
|
Message-ID: <yksxyboq33eke57n6rz4ikzlcbfda22uj5t3zsipc6m66fgoqr@ngbjg4abymcs>
Date: Mon, 29 Sep 2025 23:05:09 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Claudiu Beznea <claudiu.beznea@...on.dev>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kwilczynski@...nel.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, geert+renesas@...der.be,
magnus.damm@...il.com, p.zabel@...gutronix.de, linux-pci@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, Wolfram Sang <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH v4 2/6] PCI: rzg3s-host: Add Renesas RZ/G3S SoC host
driver
On Fri, Sep 19, 2025 at 12:28:10PM +0300, Claudiu Beznea wrote:
>
>
> On 9/19/25 11:45, Manivannan Sadhasivam wrote:
> > On Fri, Sep 12, 2025 at 03:24:40PM +0300, Claudiu wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> >>
> >> The Renesas RZ/G3S features a PCIe IP that complies with the PCI Express
> >> Base Specification 4.0 and supports speeds of up to 5 GT/s. It functions
> >> only as a root complex, with a single-lane (x1) configuration. The
> >> controller includes Type 1 configuration registers, as well as IP
> >> specific registers (called AXI registers) required for various adjustments.
> >>
> >> Hardware manual can be downloaded from the address in the "Link" section.
> >> The following steps should be followed to access the manual:
> >> 1/ Click the "User Manual" button
> >> 2/ Click "Confirm"; this will start downloading an archive
> >> 3/ Open the downloaded archive
> >> 4/ Navigate to r01uh1014ej*-rzg3s-users-manual-hardware -> Deliverables
> >> 5/ Open the file r01uh1014ej*-rzg3s.pdf
> >>
> >> Link: https://www.renesas.com/en/products/rz-g3s?queryID=695cc067c2d89e3f271d43656ede4d12
> >> Tested-by: Wolfram Sang <wsa+renesas@...g-engineering.com>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> >> ---
> >>
> >
> > [...]
> >
> >> +static void rzg3s_pcie_update_bits(void __iomem *base, u32 offset, u32 mask,
> >> + u32 val)
> >> +{
> >> + u32 tmp;
> >> +
> >> + tmp = readl(base + offset);
> >
> > Unless there is an ordering requirement, you can safely use
> > {readl/writel}_relaxed variants throughout the driver.
>
> HW manual lists specific steps to follow when issuing requests. These steps
> are listed in chapter "34.4.2.4 Issuing Special Requests" in the manual
> pointed in patch description.
>
I'm not asking you to change the order of the register config, but just asking
you to use {readl/writel}_relaxed variants as the plain readl/writel accessors
have memory barriers builtin, which will be unnecessary if not needed.
> >
> >> + tmp &= ~mask;
> >> + tmp |= val & mask;
> >> + writel(tmp, base + offset);
> >> +}
> >> +
> >
> > [...]
> >
> >> +static void __iomem *rzg3s_pcie_child_map_bus(struct pci_bus *bus,
> >> + unsigned int devfn,
> >> + int where)
> >> +{
> >> + struct rzg3s_pcie_host *host = bus->sysdata;
> >> + unsigned int dev, func, reg;
> >> +
> >> + dev = PCI_SLOT(devfn);
> >> + func = PCI_FUNC(devfn);
> >> + reg = where & ~0x3;
> >> +
> >> + /* Set the destination */
> >> + writel(FIELD_PREP(RZG3S_PCI_REQADR1_BUS, bus->number) |
> >> + FIELD_PREP(RZG3S_PCI_REQADR1_DEV, dev) |
> >> + FIELD_PREP(RZG3S_PCI_REQADR1_FUNC, func) |
> >> + FIELD_PREP(RZG3S_PCI_REQADR1_REG, reg),
> >> + host->axi + RZG3S_PCI_REQADR1);
> >> +
> >> + /* Set byte enable */
> >> + writel(RZG3S_PCI_REQBE_BYTE_EN, host->axi + RZG3S_PCI_REQBE);
> >> +
> >> + /*
> >> + * rzg3s_pcie_child_map_bus() is used to configure the controller before
> >> + * executing requests. It is called only within this driver and not
> >> + * through subsystem calls. Since it does not return an address that
> >> + * needs to be used later, return NULL.
> >> + */
> >
> > What guarantees that the PCI core will not call this function through
> > pci_ops::map_bus?
>
> As of my code inspection the pci_ops::map_bus is currently called from:
> pci_generic_config_read()
> pci_generic_config_write()
> pci_generic_config_read32()
> pci_generic_config_write32()
>
> As of my code inspection, these are currently called from vendor specific
> drivers. I the core behavior will be changed, I can't guarantee the
> statement from the comment. Please let me know if you want me to drop the
> initialization of rzg3s_pcie_child_ops::map_bus and call
> rzg3s_pcie_child_map_bus() explicitly instead of calling it though
> rzg3s_pcie_child_ops::map_bus
>
> As mentioned in the previous review rounds, this is implemented like this
> as it was suggested in v1 review process.
>
My concern is, since you are using it as a map_bus() callback implementation,
there is no guarantee that the PCI core will not invoke it. If you do not intend
it, then I would suggest dropping the callback and use it directly.
> >
> >> + return NULL;
> >> +}
> >> +
> >> +static struct pci_ops rzg3s_pcie_child_ops = {
> >> + .read = rzg3s_pcie_child_read,
> >> + .write = rzg3s_pcie_child_write,
> >> + .map_bus = rzg3s_pcie_child_map_bus,
> >> +};
> >> +
> >> +static void __iomem *rzg3s_pcie_root_map_bus(struct pci_bus *bus,
> >> + unsigned int devfn,
> >> + int where)
> >> +{
> >> + struct rzg3s_pcie_host *host = bus->sysdata;
> >> +
> >> + if (devfn)
> >> + return NULL;
> >> +
> >> + return host->pcie + where;
> >> +}
> >> +
> >> +/* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
> >> +static int rzg3s_pcie_root_write(struct pci_bus *bus, unsigned int devfn,
> >> + int where, int size, u32 val)
> >> +{
> >> + struct rzg3s_pcie_host *host = bus->sysdata;
> >> +
> >> + /* Enable access control to the CFGU */
> >> + writel(RZG3S_PCI_PERM_CFG_HWINIT_EN, host->axi + RZG3S_PCI_PERM);
> >> +
> >
> > I'm not sure if 'host->axi' written above and the address written below are in
> > the same domain or not.
>
> host->axi and host->pci are both part of the PCI controller address space.
> I don't have more info on it than this. HW manual don't mention anything
> about this.
>
Then it should be fine.
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists