[PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..2d0ffd3c4e16 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	/* DON'T MOVE ME: must be enable before PHY init */
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
 	if (IS_ERR(rockchip->vpcie3v3))
-		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
-			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
-					"failed to get vpcie3v3 regulator\n");
+		return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
+				     "failed to get vpcie3v3 regulator\n");
 
 	ret = regulator_enable(rockchip->vpcie3v3);
 	if (ret) {
-- 
2.20.1

[PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..2d0ffd3c4e16 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	/* DON'T MOVE ME: must be enable before PHY init */
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
 	if (IS_ERR(rockchip->vpcie3v3))
-		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
-			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
-					"failed to get vpcie3v3 regulator\n");
+		return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
+				     "failed to get vpcie3v3 regulator\n");
 
 	ret = regulator_enable(rockchip->vpcie3v3);
 	if (ret) {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..2d0ffd3c4e16 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	/* DON'T MOVE ME: must be enable before PHY init */
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
 	if (IS_ERR(rockchip->vpcie3v3))
-		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
-			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
-					"failed to get vpcie3v3 regulator\n");
+		return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
+				     "failed to get vpcie3v3 regulator\n");
 
 	ret = regulator_enable(rockchip->vpcie3v3);
 	if (ret) {
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 12:33, Dan Carpenter wrote:
> If devm_regulator_get_optional() returns an error pointer, then we
> should return it to the user.  The current code makes an exception
> for -ENODEV that will result in an error pointer dereference on the
> next line when it calls regulator_enable().  Remove the exception.

Doesn't this break the apparent intent of the regulator being optional, 
though?

Robin.

> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 20cef2e06f66..2d0ffd3c4e16 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>   	/* DON'T MOVE ME: must be enable before PHY init */
>   	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>   	if (IS_ERR(rockchip->vpcie3v3))
> -		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
> -			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> -					"failed to get vpcie3v3 regulator\n");
> +		return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> +				     "failed to get vpcie3v3 regulator\n");
>   
>   	ret = regulator_enable(rockchip->vpcie3v3);
>   	if (ret) {
> 

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 12:33, Dan Carpenter wrote:
> If devm_regulator_get_optional() returns an error pointer, then we
> should return it to the user.  The current code makes an exception
> for -ENODEV that will result in an error pointer dereference on the
> next line when it calls regulator_enable().  Remove the exception.

Doesn't this break the apparent intent of the regulator being optional, 
though?

Robin.

> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 20cef2e06f66..2d0ffd3c4e16 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>   	/* DON'T MOVE ME: must be enable before PHY init */
>   	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>   	if (IS_ERR(rockchip->vpcie3v3))
> -		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
> -			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> -					"failed to get vpcie3v3 regulator\n");
> +		return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> +				     "failed to get vpcie3v3 regulator\n");
>   
>   	ret = regulator_enable(rockchip->vpcie3v3);
>   	if (ret) {
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 12:33, Dan Carpenter wrote:
> If devm_regulator_get_optional() returns an error pointer, then we
> should return it to the user.  The current code makes an exception
> for -ENODEV that will result in an error pointer dereference on the
> next line when it calls regulator_enable().  Remove the exception.

Doesn't this break the apparent intent of the regulator being optional, 
though?

Robin.

> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 20cef2e06f66..2d0ffd3c4e16 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>   	/* DON'T MOVE ME: must be enable before PHY init */
>   	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>   	if (IS_ERR(rockchip->vpcie3v3))
> -		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
> -			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> -					"failed to get vpcie3v3 regulator\n");
> +		return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> +				     "failed to get vpcie3v3 regulator\n");
>   
>   	ret = regulator_enable(rockchip->vpcie3v3);
>   	if (ret) {
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Rob Herring]

On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-13 12:33, Dan Carpenter wrote:
> > If devm_regulator_get_optional() returns an error pointer, then we
> > should return it to the user.  The current code makes an exception
> > for -ENODEV that will result in an error pointer dereference on the
> > next line when it calls regulator_enable().  Remove the exception.
>
> Doesn't this break the apparent intent of the regulator being optional,
> though?

I'm pretty sure 'optional' means ENODEV is never returned. So there
wasn't any real problem, but the check was unnecessary.

>
> Robin.
>
> > Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index 20cef2e06f66..2d0ffd3c4e16 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >       /* DON'T MOVE ME: must be enable before PHY init */
> >       rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> >       if (IS_ERR(rockchip->vpcie3v3))
> > -             if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
> > -                     return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> > -                                     "failed to get vpcie3v3 regulator\n");
> > +             return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> > +                                  "failed to get vpcie3v3 regulator\n");
> >
> >       ret = regulator_enable(rockchip->vpcie3v3);
> >       if (ret) {
> >

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Rob Herring]

On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-13 12:33, Dan Carpenter wrote:
> > If devm_regulator_get_optional() returns an error pointer, then we
> > should return it to the user.  The current code makes an exception
> > for -ENODEV that will result in an error pointer dereference on the
> > next line when it calls regulator_enable().  Remove the exception.
>
> Doesn't this break the apparent intent of the regulator being optional,
> though?

I'm pretty sure 'optional' means ENODEV is never returned. So there
wasn't any real problem, but the check was unnecessary.

>
> Robin.
>
> > Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index 20cef2e06f66..2d0ffd3c4e16 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >       /* DON'T MOVE ME: must be enable before PHY init */
> >       rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> >       if (IS_ERR(rockchip->vpcie3v3))
> > -             if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
> > -                     return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> > -                                     "failed to get vpcie3v3 regulator\n");
> > +             return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> > +                                  "failed to get vpcie3v3 regulator\n");
> >
> >       ret = regulator_enable(rockchip->vpcie3v3);
> >       if (ret) {
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Rob Herring]

On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-13 12:33, Dan Carpenter wrote:
> > If devm_regulator_get_optional() returns an error pointer, then we
> > should return it to the user.  The current code makes an exception
> > for -ENODEV that will result in an error pointer dereference on the
> > next line when it calls regulator_enable().  Remove the exception.
>
> Doesn't this break the apparent intent of the regulator being optional,
> though?

I'm pretty sure 'optional' means ENODEV is never returned. So there
wasn't any real problem, but the check was unnecessary.

>
> Robin.
>
> > Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index 20cef2e06f66..2d0ffd3c4e16 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >       /* DON'T MOVE ME: must be enable before PHY init */
> >       rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> >       if (IS_ERR(rockchip->vpcie3v3))
> > -             if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
> > -                     return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> > -                                     "failed to get vpcie3v3 regulator\n");
> > +             return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
> > +                                  "failed to get vpcie3v3 regulator\n");
> >
> >       ret = regulator_enable(rockchip->vpcie3v3);
> >       if (ret) {
> >

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 14:34, Rob Herring wrote:
> On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-08-13 12:33, Dan Carpenter wrote:
>>> If devm_regulator_get_optional() returns an error pointer, then we
>>> should return it to the user.  The current code makes an exception
>>> for -ENODEV that will result in an error pointer dereference on the
>>> next line when it calls regulator_enable().  Remove the exception.
>>
>> Doesn't this break the apparent intent of the regulator being optional,
>> though?
> 
> I'm pretty sure 'optional' means ENODEV is never returned. So there
> wasn't any real problem, but the check was unnecessary.

In fact it's the other way round - "optional" in this case is for when 
the supply may legitimately not exist so the driver may or may not need 
to handle it, so it can return -ENODEV if a regulator isn't described by 
firmware. A non-optional regulator is assumed to represent a necessary 
supply, so if there's nothing described by firmware you get the (valid) 
dummy regulator back.

Robin.

>>> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>>    drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> index 20cef2e06f66..2d0ffd3c4e16 100644
>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> @@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>        /* DON'T MOVE ME: must be enable before PHY init */
>>>        rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>>>        if (IS_ERR(rockchip->vpcie3v3))
>>> -             if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
>>> -                     return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
>>> -                                     "failed to get vpcie3v3 regulator\n");
>>> +             return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
>>> +                                  "failed to get vpcie3v3 regulator\n");
>>>
>>>        ret = regulator_enable(rockchip->vpcie3v3);
>>>        if (ret) {
>>>

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 14:34, Rob Herring wrote:
> On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-08-13 12:33, Dan Carpenter wrote:
>>> If devm_regulator_get_optional() returns an error pointer, then we
>>> should return it to the user.  The current code makes an exception
>>> for -ENODEV that will result in an error pointer dereference on the
>>> next line when it calls regulator_enable().  Remove the exception.
>>
>> Doesn't this break the apparent intent of the regulator being optional,
>> though?
> 
> I'm pretty sure 'optional' means ENODEV is never returned. So there
> wasn't any real problem, but the check was unnecessary.

In fact it's the other way round - "optional" in this case is for when 
the supply may legitimately not exist so the driver may or may not need 
to handle it, so it can return -ENODEV if a regulator isn't described by 
firmware. A non-optional regulator is assumed to represent a necessary 
supply, so if there's nothing described by firmware you get the (valid) 
dummy regulator back.

Robin.

>>> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>>    drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> index 20cef2e06f66..2d0ffd3c4e16 100644
>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> @@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>        /* DON'T MOVE ME: must be enable before PHY init */
>>>        rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>>>        if (IS_ERR(rockchip->vpcie3v3))
>>> -             if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
>>> -                     return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
>>> -                                     "failed to get vpcie3v3 regulator\n");
>>> +             return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
>>> +                                  "failed to get vpcie3v3 regulator\n");
>>>
>>>        ret = regulator_enable(rockchip->vpcie3v3);
>>>        if (ret) {
>>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 14:34, Rob Herring wrote:
> On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-08-13 12:33, Dan Carpenter wrote:
>>> If devm_regulator_get_optional() returns an error pointer, then we
>>> should return it to the user.  The current code makes an exception
>>> for -ENODEV that will result in an error pointer dereference on the
>>> next line when it calls regulator_enable().  Remove the exception.
>>
>> Doesn't this break the apparent intent of the regulator being optional,
>> though?
> 
> I'm pretty sure 'optional' means ENODEV is never returned. So there
> wasn't any real problem, but the check was unnecessary.

In fact it's the other way round - "optional" in this case is for when 
the supply may legitimately not exist so the driver may or may not need 
to handle it, so it can return -ENODEV if a regulator isn't described by 
firmware. A non-optional regulator is assumed to represent a necessary 
supply, so if there's nothing described by firmware you get the (valid) 
dummy regulator back.

Robin.

>>> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>>    drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> index 20cef2e06f66..2d0ffd3c4e16 100644
>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> @@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>        /* DON'T MOVE ME: must be enable before PHY init */
>>>        rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>>>        if (IS_ERR(rockchip->vpcie3v3))
>>> -             if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
>>> -                     return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
>>> -                                     "failed to get vpcie3v3 regulator\n");
>>> +             return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
>>> +                                  "failed to get vpcie3v3 regulator\n");
>>>
>>>        ret = regulator_enable(rockchip->vpcie3v3);
>>>        if (ret) {
>>>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Rob Herring]

On Fri, Aug 13, 2021 at 8:47 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-13 14:34, Rob Herring wrote:
> > On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> On 2021-08-13 12:33, Dan Carpenter wrote:
> >>> If devm_regulator_get_optional() returns an error pointer, then we
> >>> should return it to the user.  The current code makes an exception
> >>> for -ENODEV that will result in an error pointer dereference on the
> >>> next line when it calls regulator_enable().  Remove the exception.
> >>
> >> Doesn't this break the apparent intent of the regulator being optional,
> >> though?
> >
> > I'm pretty sure 'optional' means ENODEV is never returned. So there
> > wasn't any real problem, but the check was unnecessary.
>
> In fact it's the other way round - "optional" in this case is for when
> the supply may legitimately not exist so the driver may or may not need
> to handle it, so it can return -ENODEV if a regulator isn't described by
> firmware. A non-optional regulator is assumed to represent a necessary
> supply, so if there's nothing described by firmware you get the (valid)
> dummy regulator back.

Ah yes, regulators is the oddball. Surely no one else will assume the
same behavior of _optional() variants across subsystems... ;)

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Rob Herring]

On Fri, Aug 13, 2021 at 8:47 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-13 14:34, Rob Herring wrote:
> > On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> On 2021-08-13 12:33, Dan Carpenter wrote:
> >>> If devm_regulator_get_optional() returns an error pointer, then we
> >>> should return it to the user.  The current code makes an exception
> >>> for -ENODEV that will result in an error pointer dereference on the
> >>> next line when it calls regulator_enable().  Remove the exception.
> >>
> >> Doesn't this break the apparent intent of the regulator being optional,
> >> though?
> >
> > I'm pretty sure 'optional' means ENODEV is never returned. So there
> > wasn't any real problem, but the check was unnecessary.
>
> In fact it's the other way round - "optional" in this case is for when
> the supply may legitimately not exist so the driver may or may not need
> to handle it, so it can return -ENODEV if a regulator isn't described by
> firmware. A non-optional regulator is assumed to represent a necessary
> supply, so if there's nothing described by firmware you get the (valid)
> dummy regulator back.

Ah yes, regulators is the oddball. Surely no one else will assume the
same behavior of _optional() variants across subsystems... ;)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Rob Herring]

On Fri, Aug 13, 2021 at 8:47 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-13 14:34, Rob Herring wrote:
> > On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> On 2021-08-13 12:33, Dan Carpenter wrote:
> >>> If devm_regulator_get_optional() returns an error pointer, then we
> >>> should return it to the user.  The current code makes an exception
> >>> for -ENODEV that will result in an error pointer dereference on the
> >>> next line when it calls regulator_enable().  Remove the exception.
> >>
> >> Doesn't this break the apparent intent of the regulator being optional,
> >> though?
> >
> > I'm pretty sure 'optional' means ENODEV is never returned. So there
> > wasn't any real problem, but the check was unnecessary.
>
> In fact it's the other way round - "optional" in this case is for when
> the supply may legitimately not exist so the driver may or may not need
> to handle it, so it can return -ENODEV if a regulator isn't described by
> firmware. A non-optional regulator is assumed to represent a necessary
> supply, so if there's nothing described by firmware you get the (valid)
> dummy regulator back.

Ah yes, regulators is the oddball. Surely no one else will assume the
same behavior of _optional() variants across subsystems... ;)

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]

On Fri, Aug 13, 2021 at 10:57:44AM -0500, Rob Herring wrote:
> On Fri, Aug 13, 2021 at 8:47 AM Robin Murphy <robin.murphy@arm.com> wrote:

> > In fact it's the other way round - "optional" in this case is for when
> > the supply may legitimately not exist so the driver may or may not need
> > to handle it, so it can return -ENODEV if a regulator isn't described by
> > firmware. A non-optional regulator is assumed to represent a necessary
> > supply, so if there's nothing described by firmware you get the (valid)
> > dummy regulator back.

> Ah yes, regulators is the oddball. Surely no one else will assume the
> same behavior of _optional() variants across subsystems... ;)

It would be silly to copy the *whole* pattern!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 729 bytes --]

On Fri, Aug 13, 2021 at 10:57:44AM -0500, Rob Herring wrote:
> On Fri, Aug 13, 2021 at 8:47 AM Robin Murphy <robin.murphy@arm.com> wrote:

> > In fact it's the other way round - "optional" in this case is for when
> > the supply may legitimately not exist so the driver may or may not need
> > to handle it, so it can return -ENODEV if a regulator isn't described by
> > firmware. A non-optional regulator is assumed to represent a necessary
> > supply, so if there's nothing described by firmware you get the (valid)
> > dummy regulator back.

> Ah yes, regulators is the oddball. Surely no one else will assume the
> same behavior of _optional() variants across subsystems... ;)

It would be silly to copy the *whole* pattern!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 729 bytes --]

On Fri, Aug 13, 2021 at 10:57:44AM -0500, Rob Herring wrote:
> On Fri, Aug 13, 2021 at 8:47 AM Robin Murphy <robin.murphy@arm.com> wrote:

> > In fact it's the other way round - "optional" in this case is for when
> > the supply may legitimately not exist so the driver may or may not need
> > to handle it, so it can return -ENODEV if a regulator isn't described by
> > firmware. A non-optional regulator is assumed to represent a necessary
> > supply, so if there's nothing described by firmware you get the (valid)
> > dummy regulator back.

> Ah yes, regulators is the oddball. Surely no one else will assume the
> same behavior of _optional() variants across subsystems... ;)

It would be silly to copy the *whole* pattern!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote:
> On 2021-08-13 12:33, Dan Carpenter wrote:
> > If devm_regulator_get_optional() returns an error pointer, then we
> > should return it to the user.  The current code makes an exception
> > for -ENODEV that will result in an error pointer dereference on the
> > next line when it calls regulator_enable().  Remove the exception.
> 
> Doesn't this break the apparent intent of the regulator being optional,
> though?

Argh...  Crap.  My patch is wrong, but the bug is real.

This code should follow the standard kernel idiom of returning error
pointers when there are errors and returning NULL when an optional
feature is disabled.  The problem with returning a Special Error code
to mean "disabled" is that someone will use the Special Error code to
mean an error.

And that has already sort of happened, because _regulator_get() returns
-ENODEV which would will cause the Oops I described in my patch.

Ugh...  The correct thing is to convert it to NULL/error pointers.  I
have not looked at how hard that is though...

regards,
dan carpenter

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote:
> On 2021-08-13 12:33, Dan Carpenter wrote:
> > If devm_regulator_get_optional() returns an error pointer, then we
> > should return it to the user.  The current code makes an exception
> > for -ENODEV that will result in an error pointer dereference on the
> > next line when it calls regulator_enable().  Remove the exception.
> 
> Doesn't this break the apparent intent of the regulator being optional,
> though?

Argh...  Crap.  My patch is wrong, but the bug is real.

This code should follow the standard kernel idiom of returning error
pointers when there are errors and returning NULL when an optional
feature is disabled.  The problem with returning a Special Error code
to mean "disabled" is that someone will use the Special Error code to
mean an error.

And that has already sort of happened, because _regulator_get() returns
-ENODEV which would will cause the Oops I described in my patch.

Ugh...  The correct thing is to convert it to NULL/error pointers.  I
have not looked at how hard that is though...

regards,
dan carpenter



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote:
> On 2021-08-13 12:33, Dan Carpenter wrote:
> > If devm_regulator_get_optional() returns an error pointer, then we
> > should return it to the user.  The current code makes an exception
> > for -ENODEV that will result in an error pointer dereference on the
> > next line when it calls regulator_enable().  Remove the exception.
> 
> Doesn't this break the apparent intent of the regulator being optional,
> though?

Argh...  Crap.  My patch is wrong, but the bug is real.

This code should follow the standard kernel idiom of returning error
pointers when there are errors and returning NULL when an optional
feature is disabled.  The problem with returning a Special Error code
to mean "disabled" is that someone will use the Special Error code to
mean an error.

And that has already sort of happened, because _regulator_get() returns
-ENODEV which would will cause the Oops I described in my patch.

Ugh...  The correct thing is to convert it to NULL/error pointers.  I
have not looked at how hard that is though...

regards,
dan carpenter



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 14:54, Dan Carpenter wrote:
> On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote:
>> On 2021-08-13 12:33, Dan Carpenter wrote:
>>> If devm_regulator_get_optional() returns an error pointer, then we
>>> should return it to the user.  The current code makes an exception
>>> for -ENODEV that will result in an error pointer dereference on the
>>> next line when it calls regulator_enable().  Remove the exception.
>>
>> Doesn't this break the apparent intent of the regulator being optional,
>> though?
> 
> Argh...  Crap.  My patch is wrong, but the bug is real.

Yeah, I guess this probably wants to follow the same pattern as 
rockchip_pcie_set_vpcie() in the older driver, where it's the regulator 
API calls which get wrapped in checks.

> This code should follow the standard kernel idiom of returning error
> pointers when there are errors and returning NULL when an optional
> feature is disabled.  The problem with returning a Special Error code
> to mean "disabled" is that someone will use the Special Error code to
> mean an error.
> 
> And that has already sort of happened, because _regulator_get() returns
> -ENODEV which would will cause the Oops I described in my patch.
> 
> Ugh...  The correct thing is to convert it to NULL/error pointers.  I
> have not looked at how hard that is though...

Indeed I've thought before that it would be nice if regulators worked 
like GPIOs, where the absence of an optional one does give you NULL, and 
most of the API is also NULL-safe. Probably a pretty big job though...

Thanks,
Robin.

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 14:54, Dan Carpenter wrote:
> On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote:
>> On 2021-08-13 12:33, Dan Carpenter wrote:
>>> If devm_regulator_get_optional() returns an error pointer, then we
>>> should return it to the user.  The current code makes an exception
>>> for -ENODEV that will result in an error pointer dereference on the
>>> next line when it calls regulator_enable().  Remove the exception.
>>
>> Doesn't this break the apparent intent of the regulator being optional,
>> though?
> 
> Argh...  Crap.  My patch is wrong, but the bug is real.

Yeah, I guess this probably wants to follow the same pattern as 
rockchip_pcie_set_vpcie() in the older driver, where it's the regulator 
API calls which get wrapped in checks.

> This code should follow the standard kernel idiom of returning error
> pointers when there are errors and returning NULL when an optional
> feature is disabled.  The problem with returning a Special Error code
> to mean "disabled" is that someone will use the Special Error code to
> mean an error.
> 
> And that has already sort of happened, because _regulator_get() returns
> -ENODEV which would will cause the Oops I described in my patch.
> 
> Ugh...  The correct thing is to convert it to NULL/error pointers.  I
> have not looked at how hard that is though...

Indeed I've thought before that it would be nice if regulators worked 
like GPIOs, where the absence of an optional one does give you NULL, and 
most of the API is also NULL-safe. Probably a pretty big job though...

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 14:54, Dan Carpenter wrote:
> On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote:
>> On 2021-08-13 12:33, Dan Carpenter wrote:
>>> If devm_regulator_get_optional() returns an error pointer, then we
>>> should return it to the user.  The current code makes an exception
>>> for -ENODEV that will result in an error pointer dereference on the
>>> next line when it calls regulator_enable().  Remove the exception.
>>
>> Doesn't this break the apparent intent of the regulator being optional,
>> though?
> 
> Argh...  Crap.  My patch is wrong, but the bug is real.

Yeah, I guess this probably wants to follow the same pattern as 
rockchip_pcie_set_vpcie() in the older driver, where it's the regulator 
API calls which get wrapped in checks.

> This code should follow the standard kernel idiom of returning error
> pointers when there are errors and returning NULL when an optional
> feature is disabled.  The problem with returning a Special Error code
> to mean "disabled" is that someone will use the Special Error code to
> mean an error.
> 
> And that has already sort of happened, because _regulator_get() returns
> -ENODEV which would will cause the Oops I described in my patch.
> 
> Ugh...  The correct thing is to convert it to NULL/error pointers.  I
> have not looked at how hard that is though...

Indeed I've thought before that it would be nice if regulators worked 
like GPIOs, where the absence of an optional one does give you NULL, and 
most of the API is also NULL-safe. Probably a pretty big job though...

Thanks,
Robin.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:
> Indeed I've thought before that it would be nice if regulators worked like
> GPIOs, where the absence of an optional one does give you NULL, and most of
> the API is also NULL-safe. Probably a pretty big job though...

Yeah.  You have to write a NULL-safe API with optional feature.  And,
yeah, it's a lot of work to do it in retroactively...  Too much.  I'll
just redo my patch.

regards,
dan carpenter

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:
> Indeed I've thought before that it would be nice if regulators worked like
> GPIOs, where the absence of an optional one does give you NULL, and most of
> the API is also NULL-safe. Probably a pretty big job though...

Yeah.  You have to write a NULL-safe API with optional feature.  And,
yeah, it's a lot of work to do it in retroactively...  Too much.  I'll
just redo my patch.

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:
> Indeed I've thought before that it would be nice if regulators worked like
> GPIOs, where the absence of an optional one does give you NULL, and most of
> the API is also NULL-safe. Probably a pretty big job though...

Yeah.  You have to write a NULL-safe API with optional feature.  And,
yeah, it's a lot of work to do it in retroactively...  Too much.  I'll
just redo my patch.

regards,
dan carpenter

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

If devm_regulator_get_optional() returns -ENODEV then it would lead
to an error pointer dereference on the next line and in the error
handling code.

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: The -ENODEV from devm_regulator_get_optional() has to be handled
    specially.

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..c9b341e55cbb 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -224,15 +224,17 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 
 	/* DON'T MOVE ME: must be enable before PHY init */
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
-	if (IS_ERR(rockchip->vpcie3v3))
+	if (IS_ERR(rockchip->vpcie3v3)) {
 		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
 			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
 					"failed to get vpcie3v3 regulator\n");
-
-	ret = regulator_enable(rockchip->vpcie3v3);
-	if (ret) {
-		dev_err(dev, "failed to enable vpcie3v3 regulator\n");
-		return ret;
+		rockchip->vpcie3v3 = NULL;
+	} else {
+		ret = regulator_enable(rockchip->vpcie3v3);
+		if (ret) {
+			dev_err(dev, "failed to enable vpcie3v3 regulator\n");
+			return ret;
+		}
 	}
 
 	ret = rockchip_pcie_phy_init(rockchip);
@@ -255,7 +257,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 deinit_phy:
 	rockchip_pcie_phy_deinit(rockchip);
 disable_regulator:
-	regulator_disable(rockchip->vpcie3v3);
+	if (rockchip->vpcie3v3)
+		regulator_disable(rockchip->vpcie3v3);
 
 	return ret;
 }
-- 
2.20.1

[PATCH v2] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

If devm_regulator_get_optional() returns -ENODEV then it would lead
to an error pointer dereference on the next line and in the error
handling code.

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: The -ENODEV from devm_regulator_get_optional() has to be handled
    specially.

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..c9b341e55cbb 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -224,15 +224,17 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 
 	/* DON'T MOVE ME: must be enable before PHY init */
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
-	if (IS_ERR(rockchip->vpcie3v3))
+	if (IS_ERR(rockchip->vpcie3v3)) {
 		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
 			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
 					"failed to get vpcie3v3 regulator\n");
-
-	ret = regulator_enable(rockchip->vpcie3v3);
-	if (ret) {
-		dev_err(dev, "failed to enable vpcie3v3 regulator\n");
-		return ret;
+		rockchip->vpcie3v3 = NULL;
+	} else {
+		ret = regulator_enable(rockchip->vpcie3v3);
+		if (ret) {
+			dev_err(dev, "failed to enable vpcie3v3 regulator\n");
+			return ret;
+		}
 	}
 
 	ret = rockchip_pcie_phy_init(rockchip);
@@ -255,7 +257,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 deinit_phy:
 	rockchip_pcie_phy_deinit(rockchip);
 disable_regulator:
-	regulator_disable(rockchip->vpcie3v3);
+	if (rockchip->vpcie3v3)
+		regulator_disable(rockchip->vpcie3v3);
 
 	return ret;
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

If devm_regulator_get_optional() returns -ENODEV then it would lead
to an error pointer dereference on the next line and in the error
handling code.

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: The -ENODEV from devm_regulator_get_optional() has to be handled
    specially.

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..c9b341e55cbb 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -224,15 +224,17 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 
 	/* DON'T MOVE ME: must be enable before PHY init */
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
-	if (IS_ERR(rockchip->vpcie3v3))
+	if (IS_ERR(rockchip->vpcie3v3)) {
 		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
 			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
 					"failed to get vpcie3v3 regulator\n");
-
-	ret = regulator_enable(rockchip->vpcie3v3);
-	if (ret) {
-		dev_err(dev, "failed to enable vpcie3v3 regulator\n");
-		return ret;
+		rockchip->vpcie3v3 = NULL;
+	} else {
+		ret = regulator_enable(rockchip->vpcie3v3);
+		if (ret) {
+			dev_err(dev, "failed to enable vpcie3v3 regulator\n");
+			return ret;
+		}
 	}
 
 	ret = rockchip_pcie_phy_init(rockchip);
@@ -255,7 +257,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 deinit_phy:
 	rockchip_pcie_phy_deinit(rockchip);
 disable_regulator:
-	regulator_disable(rockchip->vpcie3v3);
+	if (rockchip->vpcie3v3)
+		regulator_disable(rockchip->vpcie3v3);
 
 	return ret;
 }
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Lorenzo Pieralisi]

On Fri, Aug 13, 2021 at 05:26:48PM +0300, Dan Carpenter wrote:
> If devm_regulator_get_optional() returns -ENODEV then it would lead
> to an error pointer dereference on the next line and in the error
> handling code.
> 
> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: The -ENODEV from devm_regulator_get_optional() has to be handled
>     specially.
> 
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

I squashed it in with the patch that is fixing, thanks a lot.

Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 20cef2e06f66..c9b341e55cbb 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -224,15 +224,17 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  
>  	/* DON'T MOVE ME: must be enable before PHY init */
>  	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> -	if (IS_ERR(rockchip->vpcie3v3))
> +	if (IS_ERR(rockchip->vpcie3v3)) {
>  		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
>  			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
>  					"failed to get vpcie3v3 regulator\n");
> -
> -	ret = regulator_enable(rockchip->vpcie3v3);
> -	if (ret) {
> -		dev_err(dev, "failed to enable vpcie3v3 regulator\n");
> -		return ret;
> +		rockchip->vpcie3v3 = NULL;
> +	} else {
> +		ret = regulator_enable(rockchip->vpcie3v3);
> +		if (ret) {
> +			dev_err(dev, "failed to enable vpcie3v3 regulator\n");
> +			return ret;
> +		}
>  	}
>  
>  	ret = rockchip_pcie_phy_init(rockchip);
> @@ -255,7 +257,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  deinit_phy:
>  	rockchip_pcie_phy_deinit(rockchip);
>  disable_regulator:
> -	regulator_disable(rockchip->vpcie3v3);
> +	if (rockchip->vpcie3v3)
> +		regulator_disable(rockchip->vpcie3v3);
>  
>  	return ret;
>  }
> -- 
> 2.20.1
> 

Re: [PATCH v2] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Lorenzo Pieralisi]

On Fri, Aug 13, 2021 at 05:26:48PM +0300, Dan Carpenter wrote:
> If devm_regulator_get_optional() returns -ENODEV then it would lead
> to an error pointer dereference on the next line and in the error
> handling code.
> 
> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: The -ENODEV from devm_regulator_get_optional() has to be handled
>     specially.
> 
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

I squashed it in with the patch that is fixing, thanks a lot.

Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 20cef2e06f66..c9b341e55cbb 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -224,15 +224,17 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  
>  	/* DON'T MOVE ME: must be enable before PHY init */
>  	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> -	if (IS_ERR(rockchip->vpcie3v3))
> +	if (IS_ERR(rockchip->vpcie3v3)) {
>  		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
>  			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
>  					"failed to get vpcie3v3 regulator\n");
> -
> -	ret = regulator_enable(rockchip->vpcie3v3);
> -	if (ret) {
> -		dev_err(dev, "failed to enable vpcie3v3 regulator\n");
> -		return ret;
> +		rockchip->vpcie3v3 = NULL;
> +	} else {
> +		ret = regulator_enable(rockchip->vpcie3v3);
> +		if (ret) {
> +			dev_err(dev, "failed to enable vpcie3v3 regulator\n");
> +			return ret;
> +		}
>  	}
>  
>  	ret = rockchip_pcie_phy_init(rockchip);
> @@ -255,7 +257,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  deinit_phy:
>  	rockchip_pcie_phy_deinit(rockchip);
>  disable_regulator:
> -	regulator_disable(rockchip->vpcie3v3);
> +	if (rockchip->vpcie3v3)
> +		regulator_disable(rockchip->vpcie3v3);
>  
>  	return ret;
>  }
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Lorenzo Pieralisi]

On Fri, Aug 13, 2021 at 05:26:48PM +0300, Dan Carpenter wrote:
> If devm_regulator_get_optional() returns -ENODEV then it would lead
> to an error pointer dereference on the next line and in the error
> handling code.
> 
> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: The -ENODEV from devm_regulator_get_optional() has to be handled
>     specially.
> 
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

I squashed it in with the patch that is fixing, thanks a lot.

Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 20cef2e06f66..c9b341e55cbb 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -224,15 +224,17 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  
>  	/* DON'T MOVE ME: must be enable before PHY init */
>  	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> -	if (IS_ERR(rockchip->vpcie3v3))
> +	if (IS_ERR(rockchip->vpcie3v3)) {
>  		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
>  			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
>  					"failed to get vpcie3v3 regulator\n");
> -
> -	ret = regulator_enable(rockchip->vpcie3v3);
> -	if (ret) {
> -		dev_err(dev, "failed to enable vpcie3v3 regulator\n");
> -		return ret;
> +		rockchip->vpcie3v3 = NULL;
> +	} else {
> +		ret = regulator_enable(rockchip->vpcie3v3);
> +		if (ret) {
> +			dev_err(dev, "failed to enable vpcie3v3 regulator\n");
> +			return ret;
> +		}
>  	}
>  
>  	ret = rockchip_pcie_phy_init(rockchip);
> @@ -255,7 +257,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  deinit_phy:
>  	rockchip_pcie_phy_deinit(rockchip);
>  disable_regulator:
> -	regulator_disable(rockchip->vpcie3v3);
> +	if (rockchip->vpcie3v3)
> +		regulator_disable(rockchip->vpcie3v3);
>  
>  	return ret;
>  }
> -- 
> 2.20.1
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

> Indeed I've thought before that it would be nice if regulators worked like
> GPIOs, where the absence of an optional one does give you NULL, and most of
> the API is also NULL-safe. Probably a pretty big job though...

It also encourages *really* bad practice with error handling, and in
general there are few use cases for optional regulators where there's
not some other actions that need to be taken in the case where the
supply isn't there (elimintating some operating points or features,
reconfiguring power internally and so on).  If we genuninely don't need
to do anything special one wonders why we're trying to turn the power on
in the first place.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 723 bytes --]

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

> Indeed I've thought before that it would be nice if regulators worked like
> GPIOs, where the absence of an optional one does give you NULL, and most of
> the API is also NULL-safe. Probably a pretty big job though...

It also encourages *really* bad practice with error handling, and in
general there are few use cases for optional regulators where there's
not some other actions that need to be taken in the case where the
supply isn't there (elimintating some operating points or features,
reconfiguring power internally and so on).  If we genuninely don't need
to do anything special one wonders why we're trying to turn the power on
in the first place.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 723 bytes --]

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

> Indeed I've thought before that it would be nice if regulators worked like
> GPIOs, where the absence of an optional one does give you NULL, and most of
> the API is also NULL-safe. Probably a pretty big job though...

It also encourages *really* bad practice with error handling, and in
general there are few use cases for optional regulators where there's
not some other actions that need to be taken in the case where the
supply isn't there (elimintating some operating points or features,
reconfiguring power internally and so on).  If we genuninely don't need
to do anything special one wonders why we're trying to turn the power on
in the first place.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 15:32, Mark Brown wrote:
> On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:
> 
>> Indeed I've thought before that it would be nice if regulators worked like
>> GPIOs, where the absence of an optional one does give you NULL, and most of
>> the API is also NULL-safe. Probably a pretty big job though...
> 
> It also encourages *really* bad practice with error handling, and in
> general there are few use cases for optional regulators where there's
> not some other actions that need to be taken in the case where the
> supply isn't there (elimintating some operating points or features,
> reconfiguring power internally and so on).  If we genuninely don't need
> to do anything special one wonders why we're trying to turn the power on
> in the first place.

Sure, once you get into it, regulators are arguably a rather deeper area 
than GPIOs, so in terms of the NULL-safe aspect anything beyond 
enable/disable - for the sake of keeping trivial usage simple - would be 
pretty questionable for sure.

A lot of the usage of regulator_get_optional() seems to be just making 
sure some external thing is powered between probe() and remove() if it's 
not hard-wired already, so maybe something like a 
devm_regulator_get_optional_enabled() could be an answer to that 
argument without even touching the underlying API.

Robin.

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 15:32, Mark Brown wrote:
> On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:
> 
>> Indeed I've thought before that it would be nice if regulators worked like
>> GPIOs, where the absence of an optional one does give you NULL, and most of
>> the API is also NULL-safe. Probably a pretty big job though...
> 
> It also encourages *really* bad practice with error handling, and in
> general there are few use cases for optional regulators where there's
> not some other actions that need to be taken in the case where the
> supply isn't there (elimintating some operating points or features,
> reconfiguring power internally and so on).  If we genuninely don't need
> to do anything special one wonders why we're trying to turn the power on
> in the first place.

Sure, once you get into it, regulators are arguably a rather deeper area 
than GPIOs, so in terms of the NULL-safe aspect anything beyond 
enable/disable - for the sake of keeping trivial usage simple - would be 
pretty questionable for sure.

A lot of the usage of regulator_get_optional() seems to be just making 
sure some external thing is powered between probe() and remove() if it's 
not hard-wired already, so maybe something like a 
devm_regulator_get_optional_enabled() could be an answer to that 
argument without even touching the underlying API.

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Robin Murphy]

On 2021-08-13 15:32, Mark Brown wrote:
> On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:
> 
>> Indeed I've thought before that it would be nice if regulators worked like
>> GPIOs, where the absence of an optional one does give you NULL, and most of
>> the API is also NULL-safe. Probably a pretty big job though...
> 
> It also encourages *really* bad practice with error handling, and in
> general there are few use cases for optional regulators where there's
> not some other actions that need to be taken in the case where the
> supply isn't there (elimintating some operating points or features,
> reconfiguring power internally and so on).  If we genuninely don't need
> to do anything special one wonders why we're trying to turn the power on
> in the first place.

Sure, once you get into it, regulators are arguably a rather deeper area 
than GPIOs, so in terms of the NULL-safe aspect anything beyond 
enable/disable - for the sake of keeping trivial usage simple - would be 
pretty questionable for sure.

A lot of the usage of regulator_get_optional() seems to be just making 
sure some external thing is powered between probe() and remove() if it's 
not hard-wired already, so maybe something like a 
devm_regulator_get_optional_enabled() could be an answer to that 
argument without even touching the underlying API.

Robin.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1667 bytes --]

On Fri, Aug 13, 2021 at 04:00:45PM +0100, Robin Murphy wrote:
> On 2021-08-13 15:32, Mark Brown wrote:

> > It also encourages *really* bad practice with error handling, and in
> > general there are few use cases for optional regulators where there's
> > not some other actions that need to be taken in the case where the
> > supply isn't there (elimintating some operating points or features,
> > reconfiguring power internally and so on).  If we genuninely don't need
> > to do anything special one wonders why we're trying to turn the power on
> > in the first place.

> Sure, once you get into it, regulators are arguably a rather deeper area
> than GPIOs, so in terms of the NULL-safe aspect anything beyond
> enable/disable - for the sake of keeping trivial usage simple - would be
> pretty questionable for sure.

enable and disable are among my biggest worries frankly, if the device
was supposed to have power and that didn't work then there's probably
some kind of issue.

> A lot of the usage of regulator_get_optional() seems to be just making sure
> some external thing is powered between probe() and remove() if it's not
> hard-wired already, so maybe something like a
> devm_regulator_get_optional_enabled() could be an answer to that argument
> without even touching the underlying API.

I'm fairly sure a lot of the regulator_get_optional() usage is just
abuse of the API, every so often I get fed up enough to send patches
converting users that look like you describe to normal regulator API
calls.  People really don't get the dummy regulator stuff and seem to
think that _get_optional() is what you use to avoid writing any error
handling code :(

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1667 bytes --]

On Fri, Aug 13, 2021 at 04:00:45PM +0100, Robin Murphy wrote:
> On 2021-08-13 15:32, Mark Brown wrote:

> > It also encourages *really* bad practice with error handling, and in
> > general there are few use cases for optional regulators where there's
> > not some other actions that need to be taken in the case where the
> > supply isn't there (elimintating some operating points or features,
> > reconfiguring power internally and so on).  If we genuninely don't need
> > to do anything special one wonders why we're trying to turn the power on
> > in the first place.

> Sure, once you get into it, regulators are arguably a rather deeper area
> than GPIOs, so in terms of the NULL-safe aspect anything beyond
> enable/disable - for the sake of keeping trivial usage simple - would be
> pretty questionable for sure.

enable and disable are among my biggest worries frankly, if the device
was supposed to have power and that didn't work then there's probably
some kind of issue.

> A lot of the usage of regulator_get_optional() seems to be just making sure
> some external thing is powered between probe() and remove() if it's not
> hard-wired already, so maybe something like a
> devm_regulator_get_optional_enabled() could be an answer to that argument
> without even touching the underlying API.

I'm fairly sure a lot of the regulator_get_optional() usage is just
abuse of the API, every so often I get fed up enough to send patches
converting users that look like you describe to normal regulator API
calls.  People really don't get the dummy regulator stuff and seem to
think that _get_optional() is what you use to avoid writing any error
handling code :(

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1667 bytes --]

On Fri, Aug 13, 2021 at 04:00:45PM +0100, Robin Murphy wrote:
> On 2021-08-13 15:32, Mark Brown wrote:

> > It also encourages *really* bad practice with error handling, and in
> > general there are few use cases for optional regulators where there's
> > not some other actions that need to be taken in the case where the
> > supply isn't there (elimintating some operating points or features,
> > reconfiguring power internally and so on).  If we genuninely don't need
> > to do anything special one wonders why we're trying to turn the power on
> > in the first place.

> Sure, once you get into it, regulators are arguably a rather deeper area
> than GPIOs, so in terms of the NULL-safe aspect anything beyond
> enable/disable - for the sake of keeping trivial usage simple - would be
> pretty questionable for sure.

enable and disable are among my biggest worries frankly, if the device
was supposed to have power and that didn't work then there's probably
some kind of issue.

> A lot of the usage of regulator_get_optional() seems to be just making sure
> some external thing is powered between probe() and remove() if it's not
> hard-wired already, so maybe something like a
> devm_regulator_get_optional_enabled() could be an answer to that argument
> without even touching the underlying API.

I'm fairly sure a lot of the regulator_get_optional() usage is just
abuse of the API, every so often I get fed up enough to send patches
converting users that look like you describe to normal regulator API
calls.  People really don't get the dummy regulator stuff and seem to
think that _get_optional() is what you use to avoid writing any error
handling code :(

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

On Fri, Aug 13, 2021 at 03:32:50PM +0100, Mark Brown wrote:
> On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:
> 
> > Indeed I've thought before that it would be nice if regulators worked like
> > GPIOs, where the absence of an optional one does give you NULL, and most of
> > the API is also NULL-safe. Probably a pretty big job though...
> 
> It also encourages *really* bad practice with error handling

I'm not necessarily 100% positive what you mean by this.  I think you
mean you don't like when people pass invalid pointers to free functions?
But making regulator code NULL-safe wouldn't affect error handling
because NULL wouldn't be an error.

	p = get_optional();
	if (IS_ERR(p))
		return PTR_ERR(p);
	enable(p);
	...
	disable(p);

It all works nicely.

regards,
dan carpenter

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

On Fri, Aug 13, 2021 at 03:32:50PM +0100, Mark Brown wrote:
> On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:
> 
> > Indeed I've thought before that it would be nice if regulators worked like
> > GPIOs, where the absence of an optional one does give you NULL, and most of
> > the API is also NULL-safe. Probably a pretty big job though...
> 
> It also encourages *really* bad practice with error handling

I'm not necessarily 100% positive what you mean by this.  I think you
mean you don't like when people pass invalid pointers to free functions?
But making regulator code NULL-safe wouldn't affect error handling
because NULL wouldn't be an error.

	p = get_optional();
	if (IS_ERR(p))
		return PTR_ERR(p);
	enable(p);
	...
	disable(p);

It all works nicely.

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Dan Carpenter]

On Fri, Aug 13, 2021 at 03:32:50PM +0100, Mark Brown wrote:
> On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:
> 
> > Indeed I've thought before that it would be nice if regulators worked like
> > GPIOs, where the absence of an optional one does give you NULL, and most of
> > the API is also NULL-safe. Probably a pretty big job though...
> 
> It also encourages *really* bad practice with error handling

I'm not necessarily 100% positive what you mean by this.  I think you
mean you don't like when people pass invalid pointers to free functions?
But making regulator code NULL-safe wouldn't affect error handling
because NULL wouldn't be an error.

	p = get_optional();
	if (IS_ERR(p))
		return PTR_ERR(p);
	enable(p);
	...
	disable(p);

It all works nicely.

regards,
dan carpenter


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

On Fri, Aug 13, 2021 at 06:45:05PM +0300, Dan Carpenter wrote:
> On Fri, Aug 13, 2021 at 03:32:50PM +0100, Mark Brown wrote:
> > On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

> > > Indeed I've thought before that it would be nice if regulators worked like
> > > GPIOs, where the absence of an optional one does give you NULL, and most of
> > > the API is also NULL-safe. Probably a pretty big job though...

> > It also encourages *really* bad practice with error handling

> I'm not necessarily 100% positive what you mean by this.  I think you
> mean you don't like when people pass invalid pointers to free functions?

No, it's the case where people don't bother checking if they got the
regulator in the first place, don't bother checking if when they tried
to enable the regulator that actually worked, and don't do whatever
extra handling they need to do to configure the system for the fact that
one of the power supplies is missing.  It really only helps in the case
where you can just ignore the regulator completely.

> But making regulator code NULL-safe wouldn't affect error handling
> because NULL wouldn't be an error.
> 
> 	p = get_optional();
> 	if (IS_ERR(p))
> 		return PTR_ERR(p);
> 	enable(p);

Your example already misses the error handling on enable...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1322 bytes --]

On Fri, Aug 13, 2021 at 06:45:05PM +0300, Dan Carpenter wrote:
> On Fri, Aug 13, 2021 at 03:32:50PM +0100, Mark Brown wrote:
> > On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

> > > Indeed I've thought before that it would be nice if regulators worked like
> > > GPIOs, where the absence of an optional one does give you NULL, and most of
> > > the API is also NULL-safe. Probably a pretty big job though...

> > It also encourages *really* bad practice with error handling

> I'm not necessarily 100% positive what you mean by this.  I think you
> mean you don't like when people pass invalid pointers to free functions?

No, it's the case where people don't bother checking if they got the
regulator in the first place, don't bother checking if when they tried
to enable the regulator that actually worked, and don't do whatever
extra handling they need to do to configure the system for the fact that
one of the power supplies is missing.  It really only helps in the case
where you can just ignore the regulator completely.

> But making regulator code NULL-safe wouldn't affect error handling
> because NULL wouldn't be an error.
> 
> 	p = get_optional();
> 	if (IS_ERR(p))
> 		return PTR_ERR(p);
> 	enable(p);

Your example already misses the error handling on enable...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1322 bytes --]

On Fri, Aug 13, 2021 at 06:45:05PM +0300, Dan Carpenter wrote:
> On Fri, Aug 13, 2021 at 03:32:50PM +0100, Mark Brown wrote:
> > On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

> > > Indeed I've thought before that it would be nice if regulators worked like
> > > GPIOs, where the absence of an optional one does give you NULL, and most of
> > > the API is also NULL-safe. Probably a pretty big job though...

> > It also encourages *really* bad practice with error handling

> I'm not necessarily 100% positive what you mean by this.  I think you
> mean you don't like when people pass invalid pointers to free functions?

No, it's the case where people don't bother checking if they got the
regulator in the first place, don't bother checking if when they tried
to enable the regulator that actually worked, and don't do whatever
extra handling they need to do to configure the system for the fact that
one of the power supplies is missing.  It really only helps in the case
where you can just ignore the regulator completely.

> But making regulator code NULL-safe wouldn't affect error handling
> because NULL wouldn't be an error.
> 
> 	p = get_optional();
> 	if (IS_ERR(p))
> 		return PTR_ERR(p);
> 	enable(p);

Your example already misses the error handling on enable...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip