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

[Siddharth Vadapalli <s-vadapalli@ti.com>]

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@ti.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

    Therefore, the patch may not be worth backporting.

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.

> 
> - 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.