diff mbox series

cdx: Unlock on error path in rescan_store()

Message ID 8c79956b-bf8c-4511-97cc-a16833a0630f@moroto.mountain
State New
Headers
Series cdx: Unlock on error path in rescan_store() |

Commit Message

Dan Carpenter Dec. 12, 2023, 9:20 a.m. UTC 
  We added locking to this function but these two error paths were
accidentally overlooked.

Fixes: f0af81683466 ("cdx: Introduce lock to protect controller ops")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/cdx/cdx.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Christophe JAILLET Dec. 12, 2023, 5:53 p.m. UTC | #1 
Le 12/12/2023 à 10:20, Dan Carpenter a écrit :
> We added locking to this function but these two error paths were
> accidentally overlooked.
> 
> Fixes: f0af81683466 ("cdx: Introduce lock to protect controller ops")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   drivers/cdx/cdx.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> index d84d153078d7..f4f9f0c88c09 100644
> --- a/drivers/cdx/cdx.c
> +++ b/drivers/cdx/cdx.c
> @@ -572,12 +572,16 @@ static ssize_t rescan_store(const struct bus_type *bus,
>   
>   	/* Rescan all the devices */
>   	for_each_compatible_node(np, NULL, compat_node_name) {
> -		if (!np)
> -			return -EINVAL;
> +		if (!np) {
> +			count = -EINVAL;
> +			goto unlock;
> +		}
>   
>   		pd = of_find_device_by_node(np);
> -		if (!pd)
> -			return -EINVAL;
> +		if (!pd) {
> +			count = -EINVAL;
> +			goto unlock;

Unrelated to your patch, but should we have a of_node_put(np); here, on 
early exit?

CJ

> +		}
>   
>   		cdx = platform_get_drvdata(pd);
>   		if (cdx && cdx->controller_registered && cdx->ops->scan)
> @@ -585,7 +589,7 @@ static ssize_t rescan_store(const struct bus_type *bus,
>   
>   		put_device(&pd->dev);
>   	}
> -
> +unlock:
>   	mutex_unlock(&cdx_controller_lock);
>   
>   	return count;
Agarwal, Nikhil Dec. 13, 2023, 4:22 a.m. UTC | #2 
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Sent: Tuesday, December 12, 2023 2:50 PM
> To: Gangurde, Abhijit <abhijit.gangurde@amd.com>
> Cc: Gupta, Nipun <Nipun.Gupta@amd.com>; Agarwal, Nikhil
> <nikhil.agarwal@amd.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; kernel-
> janitors@vger.kernel.org
> Subject: [PATCH] cdx: Unlock on error path in rescan_store()
> 
> We added locking to this function but these two error paths were accidentally
> overlooked.
> 
> Fixes: f0af81683466 ("cdx: Introduce lock to protect controller ops")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Acked-by: Nikhil Agarwal <Nikhil.agarwal@amd.com>
Gangurde, Abhijit Dec. 13, 2023, 6:14 a.m. UTC | #3 
> Le 12/12/2023 à 10:20, Dan Carpenter a écrit :
> > We added locking to this function but these two error paths were
> > accidentally overlooked.
> >
> > Fixes: f0af81683466 ("cdx: Introduce lock to protect controller ops")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >   drivers/cdx/cdx.c | 14 +++++++++-----
> >   1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> > index d84d153078d7..f4f9f0c88c09 100644
> > --- a/drivers/cdx/cdx.c
> > +++ b/drivers/cdx/cdx.c
> > @@ -572,12 +572,16 @@ static ssize_t rescan_store(const struct bus_type
> *bus,
> >
> >   	/* Rescan all the devices */
> >   	for_each_compatible_node(np, NULL, compat_node_name) {
> > -		if (!np)
> > -			return -EINVAL;
> > +		if (!np) {
> > +			count = -EINVAL;
> > +			goto unlock;
> > +		}
> >
> >   		pd = of_find_device_by_node(np);
> > -		if (!pd)
> > -			return -EINVAL;
> > +		if (!pd) {
> > +			count = -EINVAL;
> > +			goto unlock;
> 
> Unrelated to your patch, but should we have a of_node_put(np); here, on
> early exit?

Yes. of_node_put(np) is needed here.

Thanks,
Abhijit
Dan Carpenter Dec. 13, 2023, 9:47 a.m. UTC | #4 
On Tue, Dec 12, 2023 at 06:53:13PM +0100, Christophe JAILLET wrote:
> Le 12/12/2023 à 10:20, Dan Carpenter a écrit :
> > We added locking to this function but these two error paths were
> > accidentally overlooked.
> > 
> > Fixes: f0af81683466 ("cdx: Introduce lock to protect controller ops")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >   drivers/cdx/cdx.c | 14 +++++++++-----
> >   1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> > index d84d153078d7..f4f9f0c88c09 100644
> > --- a/drivers/cdx/cdx.c
> > +++ b/drivers/cdx/cdx.c
> > @@ -572,12 +572,16 @@ static ssize_t rescan_store(const struct bus_type *bus,
> >   	/* Rescan all the devices */
> >   	for_each_compatible_node(np, NULL, compat_node_name) {
> > -		if (!np)
> > -			return -EINVAL;
> > +		if (!np) {
> > +			count = -EINVAL;
> > +			goto unlock;
> > +		}
> >   		pd = of_find_device_by_node(np);
> > -		if (!pd)
> > -			return -EINVAL;
> > +		if (!pd) {
> > +			count = -EINVAL;
> > +			goto unlock;
> 
> Unrelated to your patch, but should we have a of_node_put(np); here, on
> early exit?
> 

Let me resend this along with a patch 2/2 which adds the of_node_put().

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index d84d153078d7..f4f9f0c88c09 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -572,12 +572,16 @@  static ssize_t rescan_store(const struct bus_type *bus,
 
 	/* Rescan all the devices */
 	for_each_compatible_node(np, NULL, compat_node_name) {
-		if (!np)
-			return -EINVAL;
+		if (!np) {
+			count = -EINVAL;
+			goto unlock;
+		}
 
 		pd = of_find_device_by_node(np);
-		if (!pd)
-			return -EINVAL;
+		if (!pd) {
+			count = -EINVAL;
+			goto unlock;
+		}
 
 		cdx = platform_get_drvdata(pd);
 		if (cdx && cdx->controller_registered && cdx->ops->scan)
@@ -585,7 +589,7 @@  static ssize_t rescan_store(const struct bus_type *bus,
 
 		put_device(&pd->dev);
 	}
-
+unlock:
 	mutex_unlock(&cdx_controller_lock);
 
 	return count;