linux-kernel - Re: [PATCH v3 11/12] PCI: exynos: Add support for Tesla FSD SoC

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: <20250818182544.GA534647@bhelgaas>
Date: Mon, 18 Aug 2025 13:25:44 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Shradha Todi <shradha.t@...sung.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-phy@...ts.infradead.org, mani@...nel.org,
	lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org,
	bhelgaas@...gle.com, jingoohan1@...il.com, krzk+dt@...nel.org,
	conor+dt@...nel.org, alim.akhtar@...sung.com, vkoul@...nel.org,
	kishon@...nel.org, arnd@...db.de, m.szyprowski@...sung.com,
	jh80.chung@...sung.com, pankaj.dubey@...sung.com
Subject: Re: [PATCH v3 11/12] PCI: exynos: Add support for Tesla FSD SoC

[+to Krzysztof]

On Mon, Aug 18, 2025 at 03:00:00PM +0530, Shradha Todi wrote:
> > On Mon, Aug 11, 2025 at 09:16:37PM +0530, Shradha Todi wrote:
> > > Add host and endpoint controller driver support for FSD SoC.

> > It's kind of unfortunate that the driver uses "ep" everywhere for
> > struct exynos_pcie pointers.  It's going to be confusing because "ep"
> > is also commonly used for endpoint-related things, e.g., struct
> > dw_pcie_ep pointers.  Maybe it's not worth changing; I dunno.
> 
> I did try to rename the structure and the pointers 
> (https://lore.kernel.org/all/20230214121333.1837-9-shradha.t@samsung.com/)
> But the intention was different back then and so the idea was rejected.
> I could add a patch to only rename the pointers to something less
> confusing like "exy_pci"

The patch you mention did several renames:

  s/to_exynos_pcie/to_samsung_pcie/
  s/struct exynos_pcie/struct samsung_pcie/
  s/struct exynos_pcie *ep/struct samsung_pcie *sp/

I'm only concerned about the confusion of "ep" being used both for
"struct exynos_pcie *" and for "struct dw_pcie_ep *".

It would still be sort of an annoying patch to do something like this:

  s/struct exynos_pcie *ep/struct exynos_pcie *pcie/

But 'git grep "struct .*_pcie \*.*=" drivers/pci/controller/' says
using "pcie" in this way is quite common, so maybe it would be worth
doing.

What do you think, Krzysztof?

> > > +static irqreturn_t fsd_pcie_irq_handler(int irq, void *arg)
> > > +{
> > > +	u32 val;
> > > +	struct exynos_pcie *ep = arg;
> > > +	struct dw_pcie *pci = &ep->pci;
> > > +	struct dw_pcie_rp *pp = &pci->pp;
> > > +
> > > +	val = readl(ep->elbi_base + FSD_IRQ2_STS);
> > > +	if ((val & FSD_IRQ_MSI_ENABLE) == FSD_IRQ_MSI_ENABLE) {
> > > +		val &= FSD_IRQ_MSI_ENABLE;
> > > +		writel(val, ep->elbi_base + FSD_IRQ2_STS);
> > 
> > This looks weird because FSD_IRQ_MSI_ENABLE sounds like an *enable*
> > bit, but here you're treating it as a *status* bit.
> > 
> > As far as I can tell, you set FSD_IRQ_MSI_ENABLE once at probe-time in
> > fsd_pcie_msi_init(), then you clear it here in an IRQ handler, and it
> > will never be set again.  That seems wrong; am I missing something?
> 
> Actually the status IRQ and enable IRQ registers are different offsets
> but the bit position for MSI remains same in both cases so I just reused
> the macro.

Ah, that's what I missed, thanks!  At probe-time, fsd_pcie_msi_init()
enables it in FSD_IRQ2_EN.  Here you clear it in FSD_IRQ2_STS.

> But I understand that it's confusing so I will add another
> macro for FSD_IRQ_MSI_STATUS or just rename the macro to
> FSD_IRQ_MSI to re-use.

Using the same name just because a similar bit happens to be at the
same position in two different registers is definitely confusing.  I
think it will be better to have two macros, one for FSD_IRQ2_STS and
another for FSD_IRQ2_EN, e.g.,

  #define FSD_IRQ2_STS                         0x008
  #define   FSD_IRQ2_STS_MSI                   BIT(17)
  #define FSD_IRQ2_EN                          0x018
  #define   FSD_IRQ2_EN_MSI                    BIT(17)

Another question about the test:

  if ((val & FSD_IRQ_MSI_ENABLE) == FSD_IRQ_MSI_ENABLE) {

This assumes there are no other bits in FSD_IRQ2_STS that could be
set.  I would have expected a test like this:

  if (val & FSD_IRQ_MSI_ENABLE) {

Is there a reason to restrict it to the case when *only*
FSD_IRQ_MSI_ENABLE is set?

Bjorn