linux-kernel - Re: [PATCH v2 09/10] PCI: keystone: Exit ks_pcie_probe() for the default switch-case of "mode"

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: <jgw5f33ptz6dutffxaid4kxnllsdyanqy5obsotn3bmhxibxdt@2zzlh7mbfoi2>
Date: Sat, 20 Sep 2025 13:57:34 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Siddharth Vadapalli <s-vadapalli@...com>
Cc: lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org, 
	bhelgaas@...gle.com, jingoohan1@...il.com, christian.bruel@...s.st.com, 
	qiang.yu@....qualcomm.com, mayank.rana@....qualcomm.com, thippeswamy.havalige@....com, 
	shradha.t@...sung.com, quic_schintav@...cinc.com, inochiama@...il.com, 
	cassel@...nel.org, kishon@...nel.org, sergio.paracuellos@...il.com, 
	18255117159@....com, rongqianfeng@...o.com, jirislaby@...nel.org, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, srk@...com
Subject: Re: [PATCH v2 09/10] PCI: keystone: Exit ks_pcie_probe() for the
 default switch-case of "mode"

On Sat, Sep 20, 2025 at 01:39:22PM +0530, Siddharth Vadapalli wrote:
> On Sat, Sep 20, 2025 at 12:06:46AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Sep 12, 2025 at 05:46:20PM +0530, Siddharth Vadapalli wrote:
> > > In ks_pcie_probe(), the switch-case for the "mode" is used to configure
> > > the PCIe Controller for either Root-Complex or Endpoint mode of operation.
> > > Prior to the switch-case statement for "mode" an invalid mode will result
> > > in probe failure only if "dw_pcie_ver_is_ge(pci, 480A)" is true, which
> > > is the case for the AM654 platform. On the other hand, when that is not
> > > the case, "ks_pcie_set_mode()" will be invoked, which does not validate
> > > the mode. As a result, it is possible for the switch-case statement for
> > > "mode" to receive an invalid mode. Currently, an error message is displayed
> > > in the "default" case where "mode" is neither "DW_PCIE_RC_TYPE" nor
> > > "DW_PCIE_EP_TYPE", but the probe succeeds. However, since the configuration
> > > required for Root-Complex and Endpoint mode have not been performed, the
> > > Controller is not operational.
> > > 
> > > Fix this by exiting "ks_pcie_probe()" with the return value of "-EINVAL"
> > > in addition to displaying the existing error message.
> > > 
> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@...com>
> > 
> > Fixes tag? And probably CC stable since the controller seems to be not
> > operations without this fix.
> 
> While I had mentioned the rationale for not including the 'Fixes tag' in
> the v1 patch below the tearline, I forgot to add it in this patch. I will
> quote the same below:
> 
>     NOTE: A "Fixes" tag is ommitted on purpose since the fix is not crucial:
>     1. It doesn't fix a crash or any fatal error
>     2. It doesn't enable controller functionality by fixing the issue
> 

Fixes tag should be added irrespective of the cruciality of the bug.

>     Therefore, the patch may not be worth backporting.
> 

Agree with this though.

> Prior to this patch, the probe succeeded and the controller was
> unusable. Post this patch, the probe will fail and the controller is
> still unusable. Behavior is identical from a usability perspective but
> the user is aware of the issue since the probe fails.
> 

Ok. I think the description could reworded to make it more clear. The actual
issue is that the default case lacks setting the errno, leading to probe
success. But the addition of ks_pcie_set_mode() and others seem to cause little
bit confusion.

- Mani

> > 
> > - Mani
> > 
> > > ---
> > > 
> > > v1: https://lore.kernel.org/r/20250903124505.365913-11-s-vadapalli@ti.com/
> > > No changes since v1.
> > > 
> > >  drivers/pci/controller/dwc/pci-keystone.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> > > index 2da9feaaf9ee..e85942b4f6be 100644
> > > --- a/drivers/pci/controller/dwc/pci-keystone.c
> > > +++ b/drivers/pci/controller/dwc/pci-keystone.c
> > > @@ -1414,6 +1414,8 @@ static int ks_pcie_probe(struct platform_device *pdev)
> > >  		break;
> > >  	default:
> > >  		dev_err(dev, "INVALID device type %d\n", mode);
> > > +		ret = -EINVAL;
> > > +		goto err_get_sync;
> > >  	}
> > >  
> > >  	ks_pcie_enable_error_irq(ks_pcie);
> 
> Regards,
> Siddharth.

-- 
மணிவண்ணன் சதாசிவம்