[PATCH 1/4] sata: ahsata: Fix resource leak

[PATCH 1/4] sata: ahsata: Fix resource leak

[Peng Fan]

From: Ye Li <ye.li@nxp.com>

Fix coverity issue CID 3606684: Resource leak (RESOURCE_LEAK)
leaked_storage: Variable uc_priv going out of scope leaks the storage it points to

Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/ata/dwc_ahsata.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c
index c2e28fe518..a775214792 100644
--- a/drivers/ata/dwc_ahsata.c
+++ b/drivers/ata/dwc_ahsata.c
@@ -847,6 +847,9 @@ static int ahci_init_one(int pdev)
 	struct ahci_uc_priv *uc_priv = NULL;
 
 	uc_priv = malloc(sizeof(struct ahci_uc_priv));
+	if (!uc_priv)
+		return -ENOMEM;
+
 	memset(uc_priv, 0, sizeof(struct ahci_uc_priv));
 	uc_priv->dev = pdev;
 
@@ -871,6 +874,8 @@ static int ahci_init_one(int pdev)
 	return 0;
 
 err_out:
+	if (uc_priv)
+		free(uc_priv);
 	return rc;
 }
 
-- 
2.16.4

[PATCH 2/4] sata: ahsata: Fix wrong operand for checking SERR DIAG_X

[Peng Fan]

From: Ye Li <ye.li@nxp.com>

Fix coverity issue CID 3261683: Wrong operator used
(CONSTANT_EXPRESSION_RESULT) operator_confusion:
({...; __v;}) | 67108864 is always 1/true regardless of the values
of its operand. This occurs as the logical operand of !

When DIAG_X is set, the PHY COMINIT signal is detected, so
should use '&' to check whether it is set.

Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/ata/dwc_ahsata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c
index a775214792..4b37a02338 100644
--- a/drivers/ata/dwc_ahsata.c
+++ b/drivers/ata/dwc_ahsata.c
@@ -223,7 +223,7 @@ static int ahci_host_init(struct ahci_uc_priv *uc_priv)
 
 		/* Wait for COMINIT bit 26 (DIAG_X) in SERR */
 		timeout = 1000;
-		while (!(readl(&port_mmio->serr) | SATA_PORT_SERR_DIAG_X)
+		while (!(readl(&port_mmio->serr) & SATA_PORT_SERR_DIAG_X)
 			&& --timeout)
 			;
 		if (timeout <= 0) {
-- 
2.16.4

[PATCH 2/4] sata: ahsata: Fix wrong operand for checking SERR DIAG_X

[Simon Glass]

On Sun, 3 May 2020 at 08:04, Peng Fan <peng.fan@nxp.com> wrote:
>
> From: Ye Li <ye.li@nxp.com>
>
> Fix coverity issue CID 3261683: Wrong operator used
> (CONSTANT_EXPRESSION_RESULT) operator_confusion:
> ({...; __v;}) | 67108864 is always 1/true regardless of the values
> of its operand. This occurs as the logical operand of !
>
> When DIAG_X is set, the PHY COMINIT signal is detected, so
> should use '&' to check whether it is set.
>
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/ata/dwc_ahsata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 2/4] sata: ahsata: Fix wrong operand for checking SERR DIAG_X

[Tom Rini]

On Sun, May 03, 2020 at 10:27:01PM +0800, Peng Fan wrote:

> From: Ye Li <ye.li@nxp.com>
> 
> Fix coverity issue CID 3261683: Wrong operator used
> (CONSTANT_EXPRESSION_RESULT) operator_confusion:
> ({...; __v;}) | 67108864 is always 1/true regardless of the values
> of its operand. This occurs as the logical operand of !
> 
> When DIAG_X is set, the PHY COMINIT signal is detected, so
> should use '&' to check whether it is set.
> 
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200508/631fdc45/attachment.sig>

[PATCH 3/4] sata: dwc_ahsata: Fix incorrect free

[Peng Fan]

From: Ye Li <ye.li@nxp.com>

Fix coverity issue CID 43665: Free of address-of expression (BAD_FREE)
incorrect_free: free frees incorrect pointer pp.

pp points the port array field of struct ahci_uc_priv, should not free it.

Acked-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/ata/dwc_ahsata.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c
index 4b37a02338..82fbb50da6 100644
--- a/drivers/ata/dwc_ahsata.c
+++ b/drivers/ata/dwc_ahsata.c
@@ -450,7 +450,6 @@ static int ahci_port_start(struct ahci_uc_priv *uc_priv, u8 port)
 
 	mem = (u32)malloc(AHCI_PORT_PRIV_DMA_SZ + 1024);
 	if (!mem) {
-		free(pp);
 		printf("No mem for table!\n");
 		return -ENOMEM;
 	}
-- 
2.16.4

[PATCH 3/4] sata: dwc_ahsata: Fix incorrect free

[Simon Glass]

On Sun, 3 May 2020 at 08:04, Peng Fan <peng.fan@nxp.com> wrote:
>
> From: Ye Li <ye.li@nxp.com>
>
> Fix coverity issue CID 43665: Free of address-of expression (BAD_FREE)
> incorrect_free: free frees incorrect pointer pp.
>
> pp points the port array field of struct ahci_uc_priv, should not free it.
>
> Acked-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/ata/dwc_ahsata.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/4] sata: dwc_ahsata: Fix incorrect free

[Tom Rini]

On Sun, May 03, 2020 at 10:27:02PM +0800, Peng Fan wrote:

> From: Ye Li <ye.li@nxp.com>
> 
> Fix coverity issue CID 43665: Free of address-of expression (BAD_FREE)
> incorrect_free: free frees incorrect pointer pp.
> 
> pp points the port array field of struct ahci_uc_priv, should not free it.
> 
> Acked-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200508/b6f3ddba/attachment.sig>

[PATCH 4/4] sata: dwc_ahsata: Fix memory issue in reset_sata

[Peng Fan]

From: Ye Li <ye.li@nxp.com>

The reset_sata should reset the sata device info and free the
probe_ent memory. Otherwise, it will cause memory leak if we
init the sata again.

Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/ata/dwc_ahsata.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c
index 82fbb50da6..2bc1de8b98 100644
--- a/drivers/ata/dwc_ahsata.c
+++ b/drivers/ata/dwc_ahsata.c
@@ -918,6 +918,9 @@ int reset_sata(int dev)
 	while (readl(&host_mmio->ghc) & SATA_HOST_GHC_HR)
 		udelay(100);
 
+	free(uc_priv);
+	memset(&sata_dev_desc[dev], 0, sizeof(struct blk_desc));
+
 	return 0;
 }
 
-- 
2.16.4

[PATCH 4/4] sata: dwc_ahsata: Fix memory issue in reset_sata

[Tom Rini]

On Sun, May 03, 2020 at 10:27:03PM +0800, Peng Fan wrote:

> From: Ye Li <ye.li@nxp.com>
> 
> The reset_sata should reset the sata device info and free the
> probe_ent memory. Otherwise, it will cause memory leak if we
> init the sata again.
> 
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200508/2fb8e862/attachment.sig>

[PATCH 1/4] sata: ahsata: Fix resource leak

[Simon Glass]

On Sun, 3 May 2020 at 08:04, Peng Fan <peng.fan@nxp.com> wrote:
>
> From: Ye Li <ye.li@nxp.com>
>
> Fix coverity issue CID 3606684: Resource leak (RESOURCE_LEAK)
> leaked_storage: Variable uc_priv going out of scope leaks the storage it points to
>
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/ata/dwc_ahsata.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c
> index c2e28fe518..a775214792 100644
> --- a/drivers/ata/dwc_ahsata.c
> +++ b/drivers/ata/dwc_ahsata.c
> @@ -847,6 +847,9 @@ static int ahci_init_one(int pdev)
>         struct ahci_uc_priv *uc_priv = NULL;
>
>         uc_priv = malloc(sizeof(struct ahci_uc_priv));
> +       if (!uc_priv)
> +               return -ENOMEM;
> +
>         memset(uc_priv, 0, sizeof(struct ahci_uc_priv));
>         uc_priv->dev = pdev;
>
> @@ -871,6 +874,8 @@ static int ahci_init_one(int pdev)
>         return 0;
>
>  err_out:
> +       if (uc_priv)
> +               free(uc_priv);

Seems like you can avoid the if() since it is always allocated at this point?

The migration date for SATA was over 6 months ago so really this code
should not be used at this point.

Regards,
Simon

[PATCH 1/4] sata: ahsata: Fix resource leak

[Peng Fan]

Hi Simon,

> Subject: Re: [PATCH 1/4] sata: ahsata: Fix resource leak
> 
> On Sun, 3 May 2020 at 08:04, Peng Fan <peng.fan@nxp.com> wrote:
> >
> > From: Ye Li <ye.li@nxp.com>
> >
> > Fix coverity issue CID 3606684: Resource leak (RESOURCE_LEAK)
> > leaked_storage: Variable uc_priv going out of scope leaks the storage
> > it points to
> >
> > Signed-off-by: Ye Li <ye.li@nxp.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/ata/dwc_ahsata.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> >
> > diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c index
> > c2e28fe518..a775214792 100644
> > --- a/drivers/ata/dwc_ahsata.c
> > +++ b/drivers/ata/dwc_ahsata.c
> > @@ -847,6 +847,9 @@ static int ahci_init_one(int pdev)
> >         struct ahci_uc_priv *uc_priv = NULL;
> >
> >         uc_priv = malloc(sizeof(struct ahci_uc_priv));
> > +       if (!uc_priv)
> > +               return -ENOMEM;
> > +
> >         memset(uc_priv, 0, sizeof(struct ahci_uc_priv));
> >         uc_priv->dev = pdev;
> >
> > @@ -871,6 +874,8 @@ static int ahci_init_one(int pdev)
> >         return 0;
> >
> >  err_out:
> > +       if (uc_priv)
> > +               free(uc_priv);
> 
> Seems like you can avoid the if() since it is always allocated at this point?
> 
> The migration date for SATA was over 6 months ago so really this code should
> not be used at this point.

This is to upstream nxp downstream patch. Since this is bug fix, I hope it is ok to
be in until the driver is removed.

I'll also try to convert to use DM sata.

Thanks,
Peng.

> 
> Regards,
> Simon

[PATCH 1/4] sata: ahsata: Fix resource leak

[Tom Rini]

On Sun, May 03, 2020 at 10:27:00PM +0800, Peng Fan wrote:

> From: Ye Li <ye.li@nxp.com>
> 
> Fix coverity issue CID 3606684: Resource leak (RESOURCE_LEAK)
> leaked_storage: Variable uc_priv going out of scope leaks the storage it points to
> 
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200508/9860c3bc/attachment.sig>