If the device tree is messed up, then potentially the "protocol" string
could potentially be uninitialized.  Add a check to prevent that.

Fixes: 059f545832be ("spi: add support for microchip "soft" spi controller")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/spi/spi-microchip-core-spi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index b8738190cdcb..e65036cc62f3 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -320,6 +320,8 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 	 */
 	ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
 				      &protocol);
+	if (ret)
+		return ret;
 	if (strcmp(protocol, "motorola") != 0)
 		return dev_err_probe(&pdev->dev, -EINVAL,
 				     "CoreSPI: protocol '%s' not supported by this driver\n",
-- 
2.51.0

On Fri, Nov 21, 2025 at 04:35:01PM +0300, Dan Carpenter wrote:
> If the device tree is messed up, then potentially the "protocol" string
> could potentially be uninitialized.  Add a check to prevent that.
> 
> Fixes: 059f545832be ("spi: add support for microchip "soft" spi controller")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/spi/spi-microchip-core-spi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index b8738190cdcb..e65036cc62f3 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -320,6 +320,8 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>  	 */
>  	ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
>  				      &protocol);
> +	if (ret)
> +		return ret;
>  	if (strcmp(protocol, "motorola") != 0)
>  		return dev_err_probe(&pdev->dev, -EINVAL,
>  				     "CoreSPI: protocol '%s' not supported by this driver\n",

This should probably also complain about not being able to get the
property, otherwise nobody is going to be able to figure out what's
wrong if we actually hit the error case.

On Fri, Nov 21, 2025 at 02:18:49PM +0000, Mark Brown wrote:
> On Fri, Nov 21, 2025 at 04:35:01PM +0300, Dan Carpenter wrote:
> > If the device tree is messed up, then potentially the "protocol" string
> > could potentially be uninitialized.  Add a check to prevent that.
> > 
> > Fixes: 059f545832be ("spi: add support for microchip "soft" spi controller")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  drivers/spi/spi-microchip-core-spi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> > index b8738190cdcb..e65036cc62f3 100644
> > --- a/drivers/spi/spi-microchip-core-spi.c
> > +++ b/drivers/spi/spi-microchip-core-spi.c
> > @@ -320,6 +320,8 @@ static int mchp_corespi_probe(struct platform_device *pdev)
> >  	 */
> >  	ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
> >  				      &protocol);
> > +	if (ret)
> > +		return ret;
> >  	if (strcmp(protocol, "motorola") != 0)
> >  		return dev_err_probe(&pdev->dev, -EINVAL,
> >  				     "CoreSPI: protocol '%s' not supported by this driver\n",
> 
> This should probably also complain about not being able to get the
> property, otherwise nobody is going to be able to figure out what's
> wrong if we actually hit the error case.

The one thing to be careful of is that the property has a default, so
EINVAL needs to be treated differently, so the decision tree is
something like:
if (ret == _EINVAL)
	<do nothing>
else if (ret)
	abort complaining about malformed
else if (!motorola)
	abort complaining about unsupported mode
else
	<do nothing>

obviously that can just become two clauses, but you get the idea.

On Fri, Nov 21, 2025 at 04:20:41PM +0000, Conor Dooley wrote:
> On Fri, Nov 21, 2025 at 02:18:49PM +0000, Mark Brown wrote:
> > On Fri, Nov 21, 2025 at 04:35:01PM +0300, Dan Carpenter wrote:
> > > If the device tree is messed up, then potentially the "protocol" string
> > > could potentially be uninitialized.  Add a check to prevent that.
> > > 
> > > Fixes: 059f545832be ("spi: add support for microchip "soft" spi controller")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > ---
> > >  drivers/spi/spi-microchip-core-spi.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> > > index b8738190cdcb..e65036cc62f3 100644
> > > --- a/drivers/spi/spi-microchip-core-spi.c
> > > +++ b/drivers/spi/spi-microchip-core-spi.c
> > > @@ -320,6 +320,8 @@ static int mchp_corespi_probe(struct platform_device *pdev)
> > >  	 */
> > >  	ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
> > >  				      &protocol);
> > > +	if (ret)
> > > +		return ret;
> > >  	if (strcmp(protocol, "motorola") != 0)
> > >  		return dev_err_probe(&pdev->dev, -EINVAL,
> > >  				     "CoreSPI: protocol '%s' not supported by this driver\n",
> > 
> > This should probably also complain about not being able to get the
> > property, otherwise nobody is going to be able to figure out what's
> > wrong if we actually hit the error case.
> 
> The one thing to be careful of is that the property has a default, so
> EINVAL needs to be treated differently, so the decision tree is
> something like:
> if (ret == _EINVAL)
> 	<do nothing>
> else if (ret)
> 	abort complaining about malformed
> else if (!motorola)
> 	abort complaining about unsupported mode
> else
> 	<do nothing>
> 
> obviously that can just become two clauses, but you get the idea.

Sure.  I've sent a v2 which defaults to motorola.

regards,
dan carpenter