[PATCH-for-9.1 v2 0/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[PATCH-for-9.1 v2 0/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Philippe Mathieu-Daudé]

v2:
- Cover PowerNV SSI in MAINTAINERS
- Use GLib API in pnv_spi_xfer_buffer_free()
- Simplify returning early

Supersedes: <20240806134829.351703-3-chalapathi.v@linux.ibm.com>

Chalapathi V (1):
  hw/ssi/pnv_spi: Fixes Coverity CID 1558831

Philippe Mathieu-Daudé (3):
  MAINTAINERS: Cover PowerPC SPI model in PowerNV section
  hw/ssi/pnv_spi: Match _xfer_buffer_free() with _xfer_buffer_new()
  hw/ssi/pnv_spi: Return early in transfer()

 MAINTAINERS      |  2 ++
 hw/ssi/pnv_spi.c | 12 +++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

-- 
2.45.2

[PATCH-for-9.1 v2 1/4] MAINTAINERS: Cover PowerPC SPI model in PowerNV section

[Philippe Mathieu-Daudé]

It is unfair to let the PowerNV SPI model to the SSI
maintainers. Also include the PowerNV ones.

Fixes: 29318db133 ("hw/ssi: Add SPI model")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 74a85360fd..52843876ab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1542,8 +1542,10 @@ F: hw/ppc/pnv*
 F: hw/intc/pnv*
 F: hw/intc/xics_pnv.c
 F: hw/pci-host/pnv*
+F: hw/ssi/pnv_spi.c
 F: include/hw/ppc/pnv*
 F: include/hw/pci-host/pnv*
+F: hw/ssi/pnv_spi*
 F: pc-bios/skiboot.lid
 F: tests/qtest/pnv*
 
-- 
2.45.2

[PATCH-for-9.1 v2 2/4] hw/ssi/pnv_spi: Match _xfer_buffer_free() with _xfer_buffer_new()

[Philippe Mathieu-Daudé]

pnv_spi_xfer_buffer_new() allocates %payload using g_malloc0(),
and pnv_spi_xfer_buffer_write_ptr() allocates %payload->data
using g_realloc(). Use the API equivalent g_free() to release
the buffers.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ssi/pnv_spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index c1297ab733..13a47f07e7 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -53,8 +53,8 @@ static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
 
 static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
 {
-    free(payload->data);
-    free(payload);
+    g_free(payload->data);
+    g_free(payload);
 }
 
 static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer *payload,
-- 
2.45.2

[PATCH-for-9.1 v2 3/4] hw/ssi/pnv_spi: Return early in transfer()

[Philippe Mathieu-Daudé]

Return early to simplify next commit.
No logical change intended.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ssi/pnv_spi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 13a47f07e7..05e6afc11e 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -217,6 +217,9 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
     PnvXferBuffer *rsp_payload = NULL;
 
     rsp_payload = pnv_spi_xfer_buffer_new();
+    if (!rsp_payload) {
+        return;
+    }
     for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
         tx = 0;
         for (int i = 0; i < s->transfer_len; i++) {
@@ -235,9 +238,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
                     (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
         }
     }
-    if (rsp_payload != NULL) {
-        spi_response(s, s->N1_bits, rsp_payload);
-    }
+    spi_response(s, s->N1_bits, rsp_payload);
 }
 
 static inline uint8_t get_seq_index(PnvSpi *s)
-- 
2.45.2

[PATCH-for-9.1 v2 4/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Philippe Mathieu-Daudé]

From: Chalapathi V <chalapathi.v@linux.ibm.com>

In this commit the following coverity scan defect has been fixed
CID 1558831:  Resource leaks  (RESOURCE_LEAK)
  Variable "rsp_payload" going out of scope leaks the storage it
  points to.

Fixes: Coverity CID 1558831
Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
Fixes: b4cb930e40 ("hw/ssi: Extend SPI model")
[PMD: Rebased on previous commit (returning earlier)]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ssi/pnv_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 05e6afc11e..eadc36fa86 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -239,6 +239,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
         }
     }
     spi_response(s, s->N1_bits, rsp_payload);
+    pnv_spi_xfer_buffer_free(rsp_payload);
 }
 
 static inline uint8_t get_seq_index(PnvSpi *s)
-- 
2.45.2

Re: [PATCH-for-9.1 v2 4/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Philippe Mathieu-Daudé]

On 7/8/24 22:28, Philippe Mathieu-Daudé wrote:
> From: Chalapathi V <chalapathi.v@linux.ibm.com>
> 
> In this commit the following coverity scan defect has been fixed
> CID 1558831:  Resource leaks  (RESOURCE_LEAK)
>    Variable "rsp_payload" going out of scope leaks the storage it
>    points to.
> 
> Fixes: Coverity CID 1558831
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> Fixes: b4cb930e40 ("hw/ssi: Extend SPI model")

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> [PMD: Rebased on previous commit (returning earlier)]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ssi/pnv_spi.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 05e6afc11e..eadc36fa86 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -239,6 +239,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
>           }
>       }
>       spi_response(s, s->N1_bits, rsp_payload);
> +    pnv_spi_xfer_buffer_free(rsp_payload);
>   }
>   
>   static inline uint8_t get_seq_index(PnvSpi *s)

Re: [PATCH-for-9.1 v2 3/4] hw/ssi/pnv_spi: Return early in transfer()

[Richard Henderson]

On 8/8/24 06:28, Philippe Mathieu-Daudé wrote:
> Return early to simplify next commit.
> No logical change intended.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ssi/pnv_spi.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 13a47f07e7..05e6afc11e 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -217,6 +217,9 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
>       PnvXferBuffer *rsp_payload = NULL;
>   
>       rsp_payload = pnv_spi_xfer_buffer_new();

this uses g_malloc0

> +    if (!rsp_payload) {
> +        return;
> +    }

so this will never fail.

>       for (int offset = 0; offset < payload->len; offset += s->transfer_len) {
>           tx = 0;
>           for (int i = 0; i < s->transfer_len; i++) {
> @@ -235,9 +238,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload)
>                       (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
>           }
>       }
> -    if (rsp_payload != NULL) {
> -        spi_response(s, s->N1_bits, rsp_payload);
> -    }
> +    spi_response(s, s->N1_bits, rsp_payload);
>   }

The cleanup could simply be this hunk.


r~

Re: [PATCH-for-9.1 v2 2/4] hw/ssi/pnv_spi: Match _xfer_buffer_free() with _xfer_buffer_new()

[Richard Henderson]

On 8/8/24 06:28, Philippe Mathieu-Daudé wrote:
> pnv_spi_xfer_buffer_new() allocates %payload using g_malloc0(),
> and pnv_spi_xfer_buffer_write_ptr() allocates %payload->data
> using g_realloc(). Use the API equivalent g_free() to release
> the buffers.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ssi/pnv_spi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index c1297ab733..13a47f07e7 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -53,8 +53,8 @@ static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
>   
>   static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
>   {
> -    free(payload->data);
> -    free(payload);
> +    g_free(payload->data);
> +    g_free(payload);
>   }
>   
>   static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer *payload,

g_free/free are supposed to be interchangeable in this epoch.

But
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-9.1 v2 4/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Richard Henderson]

On 8/8/24 06:28, Philippe Mathieu-Daudé wrote:
> From: Chalapathi V<chalapathi.v@linux.ibm.com>
> 
> In this commit the following coverity scan defect has been fixed
> CID 1558831:  Resource leaks  (RESOURCE_LEAK)
>    Variable "rsp_payload" going out of scope leaks the storage it
>    points to.
> 
> Fixes: Coverity CID 1558831
> Signed-off-by: Chalapathi V<chalapathi.v@linux.ibm.com>
> Fixes: b4cb930e40 ("hw/ssi: Extend SPI model")
> [PMD: Rebased on previous commit (returning earlier)]
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/ssi/pnv_spi.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-9.1 v2 0/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Cédric Le Goater]

Chalapthi,

On 8/7/24 22:28, Philippe Mathieu-Daudé wrote:
> v2:
> - Cover PowerNV SSI in MAINTAINERS
> - Use GLib API in pnv_spi_xfer_buffer_free()
> - Simplify returning early
> 
> Supersedes: <20240806134829.351703-3-chalapathi.v@linux.ibm.com>

I was wondering where we were on this series. I see there were comments
on the initial one that would need some response at least. Do you have
plans for a respin ?

Thanks,

C.


> Chalapathi V (1):
>    hw/ssi/pnv_spi: Fixes Coverity CID 1558831
> 
> Philippe Mathieu-Daudé (3):
>    MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>    hw/ssi/pnv_spi: Match _xfer_buffer_free() with _xfer_buffer_new()
>    hw/ssi/pnv_spi: Return early in transfer()
> 
>   MAINTAINERS      |  2 ++
>   hw/ssi/pnv_spi.c | 12 +++++++-----
>   2 files changed, 9 insertions(+), 5 deletions(-)
> 

Re: [PATCH-for-9.1 v2 0/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Chalapathi V]

On 12-09-2024 22:25, Cédric Le Goater wrote:
> Chalapthi,
>
> On 8/7/24 22:28, Philippe Mathieu-Daudé wrote:
>> v2:
>> - Cover PowerNV SSI in MAINTAINERS
>> - Use GLib API in pnv_spi_xfer_buffer_free()
>> - Simplify returning early
>>
>> Supersedes: <20240806134829.351703-3-chalapathi.v@linux.ibm.com>
>
> I was wondering where we were on this series. I see there were comments
> on the initial one that would need some response at least. Do you have
> plans for a respin ?
>
> Thanks,
>
> C.
>
Hello Cedric,

Thank You so much for reminding me. I apologize for not getting back on 
this sooner. I am working on the review comments from initial v1 
patchset and send the v2 patchset ASAP.

Thank You,

Chalapathi
>
>> Chalapathi V (1):
>>    hw/ssi/pnv_spi: Fixes Coverity CID 1558831
>>
>> Philippe Mathieu-Daudé (3):
>>    MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>>    hw/ssi/pnv_spi: Match _xfer_buffer_free() with _xfer_buffer_new()
>>    hw/ssi/pnv_spi: Return early in transfer()
>>
>>   MAINTAINERS      |  2 ++
>>   hw/ssi/pnv_spi.c | 12 +++++++-----
>>   2 files changed, 9 insertions(+), 5 deletions(-)
>>
>
>

Re: [PATCH-for-9.1 v2 0/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Cédric Le Goater]

Hello,

On 9/13/24 15:24, Chalapathi V wrote:
> 
> On 12-09-2024 22:25, Cédric Le Goater wrote:
>> Chalapthi,
>>
>> On 8/7/24 22:28, Philippe Mathieu-Daudé wrote:
>>> v2:
>>> - Cover PowerNV SSI in MAINTAINERS
>>> - Use GLib API in pnv_spi_xfer_buffer_free()
>>> - Simplify returning early
>>>
>>> Supersedes: <20240806134829.351703-3-chalapathi.v@linux.ibm.com>
>>
>> I was wondering where we were on this series. I see there were comments
>> on the initial one that would need some response at least. Do you have
>> plans for a respin ?
>>
>> Thanks,
>>
>> C.
>>
> Hello Cedric,
> 
> Thank You so much for reminding me. I apologize for not getting back on this sooner. 

That's fine. We have some spare time before the QEMU 9.2 cycle
closes. I'd say ~2 months. Still, it would be good to address
these issues before adding more models (Dan's TPM device model)
relying on it.

> I am working on the review comments from initial v1 patchset and send the v2 patchset ASAP.

That would be a v3 ? Since Philippe sent a v2.

Thanks,

C.

Re: [PATCH-for-9.1 v2 0/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Chalapathi V]

On 13-09-2024 19:07, Cédric Le Goater wrote:
> Hello,
>
> On 9/13/24 15:24, Chalapathi V wrote:
>>
>> On 12-09-2024 22:25, Cédric Le Goater wrote:
>>> Chalapthi,
>>>
>>> On 8/7/24 22:28, Philippe Mathieu-Daudé wrote:
>>>> v2:
>>>> - Cover PowerNV SSI in MAINTAINERS
>>>> - Use GLib API in pnv_spi_xfer_buffer_free()
>>>> - Simplify returning early
>>>>
>>>> Supersedes: <20240806134829.351703-3-chalapathi.v@linux.ibm.com>
>>>
>>> I was wondering where we were on this series. I see there were comments
>>> on the initial one that would need some response at least. Do you have
>>> plans for a respin ?
>>>
>>> Thanks,
>>>
>>> C.
>>>
>> Hello Cedric,
>>
>> Thank You so much for reminding me. I apologize for not getting back 
>> on this sooner. 
>
> That's fine. We have some spare time before the QEMU 9.2 cycle
> closes. I'd say ~2 months. Still, it would be good to address
> these issues before adding more models (Dan's TPM device model)
> relying on it.
Sure. Thank You.
>
>> I am working on the review comments from initial v1 patchset and send 
>> the v2 patchset ASAP.
>
> That would be a v3 ? Since Philippe sent a v2.
>
> Thanks,
>
> C.
Sure. Will send v3 for the series: hw/ssi/pnv_spi: Fixes Coverity CID 
1558831 and

a separate series for resolving Coverity CID 1558827.

Thank You,

Chalapathi

Re: [PATCH-for-9.1 v2 0/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Nicholas Piggin]

On Thu Aug 8, 2024 at 6:28 AM AEST, Philippe Mathieu-Daudé wrote:
> v2:
> - Cover PowerNV SSI in MAINTAINERS
> - Use GLib API in pnv_spi_xfer_buffer_free()
> - Simplify returning early
>
> Supersedes: <20240806134829.351703-3-chalapathi.v@linux.ibm.com>
>
> Chalapathi V (1):
>   hw/ssi/pnv_spi: Fixes Coverity CID 1558831
>
> Philippe Mathieu-Daudé (3):
>   MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>   hw/ssi/pnv_spi: Match _xfer_buffer_free() with _xfer_buffer_new()
>   hw/ssi/pnv_spi: Return early in transfer()

Note that I included this series in the ppc 9.2 PR, because Chalapathi
has not found time to finish the better rework. There were a couple
of comments about style / unnecessary code for these, but that will
all get replaced by the rework so I prefer to leave unchanged. Thanks
all for the help with this.

Thanks,
Nick

Re: [PATCH-for-9.1 v2 0/4] hw/ssi/pnv_spi: Fixes Coverity CID 1558831

[Philippe Mathieu-Daudé]

On 4/11/24 02:51, Nicholas Piggin wrote:
> On Thu Aug 8, 2024 at 6:28 AM AEST, Philippe Mathieu-Daudé wrote:
>> v2:
>> - Cover PowerNV SSI in MAINTAINERS
>> - Use GLib API in pnv_spi_xfer_buffer_free()
>> - Simplify returning early
>>
>> Supersedes: <20240806134829.351703-3-chalapathi.v@linux.ibm.com>
>>
>> Chalapathi V (1):
>>    hw/ssi/pnv_spi: Fixes Coverity CID 1558831
>>
>> Philippe Mathieu-Daudé (3):
>>    MAINTAINERS: Cover PowerPC SPI model in PowerNV section
>>    hw/ssi/pnv_spi: Match _xfer_buffer_free() with _xfer_buffer_new()
>>    hw/ssi/pnv_spi: Return early in transfer()
> 
> Note that I included this series in the ppc 9.2 PR, because Chalapathi
> has not found time to finish the better rework. There were a couple
> of comments about style / unnecessary code for these, but that will
> all get replaced by the rework so I prefer to leave unchanged. Thanks
> all for the help with this.

Thanks Nick!