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] [day] [month] [year] [list]

Message-ID: <00b501dc10fd$f1fecc10$d5fc6430$@samsung.com>
Date: Tue, 19 Aug 2025 17:09:34 +0530
From: "Shradha Todi" <shradha.t@...sung.com>
To: "'Bjorn Helgaas'" <helgaas@...nel.org>, "'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

> > > > +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) {
> 

Thanks for pointing this out. FSD_IRQ_MSI_ENABLE is a single-bit, so there
is no functional difference in the two statements. I didn't have a specific
reason for using "== FSD_IRQ_MSI_ENABLE".
But I see that "val & FSD_IRQ_MSI_ENABLE" would have been the more
standard way to write this. I will update this for clarity.

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