| Message ID | 20240918165045.21298-1-chalapathi.v@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() | expand |
On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote: > Hello, > > v3: > 1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi* > 2. Use of PnvXferBuffer results in a additonal process overhead due to > frequent dynamic allocations and hence use an existing Fifo8 buffer. > 3. Use a local variable seq_index and use it with in while loop instead > of repeatedly calling get_seq_index() and make sure s->seq_op doesn't > overrun when seq_index is incremented. > > Tested: > passed make check and make check-avocado > > Supersedes: <20240807202804.56038-1-philmd@linaro.org> Hi Chalapathi, To be clear, this fixes Coverity CID 1558831? A Resolves: tag for the CID should be there, I guess it's patch 2? I like patch 2, but since it is quite a significant change, should we take the v2 series first which is much smaller, then add this conversion on top of it? If it was long-standing code that would be important (because you don't want to introduce regressions or conflicts when backporting fixes). Since this is a new model I guess there is leeway to just take v3 as is. Thanks, Nick > > Philippe Mathieu-Daudé (1): > MAINTAINERS: Cover PowerPC SPI model in PowerNV section > > Chalapathi V (2): > hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure > hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index(). > > MAINTAINERS | 2 + > include/hw/ssi/pnv_spi.h | 3 + > hw/ssi/pnv_spi.c | 228 +++++++++++++++------------------------ > 3 files changed, 89 insertions(+), 144 deletions(-)
On 10/8/24 09:43, Nicholas Piggin wrote: > On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote: >> Hello, >> >> v3: >> 1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi* >> 2. Use of PnvXferBuffer results in a additonal process overhead due to >> frequent dynamic allocations and hence use an existing Fifo8 buffer. >> 3. Use a local variable seq_index and use it with in while loop instead >> of repeatedly calling get_seq_index() and make sure s->seq_op doesn't >> overrun when seq_index is incremented. >> >> Tested: >> passed make check and make check-avocado >> >> Supersedes: <20240807202804.56038-1-philmd@linaro.org> > > Hi Chalapathi, > > To be clear, this fixes Coverity CID 1558831? A Resolves: > tag for the CID should be there, I guess it's patch 2? Patches should have a tag : Resolves: Coverity CID XYZZY Thanks, C. > > I like patch 2, but since it is quite a significant change, > should we take the v2 series first which is much smaller, > then add this conversion on top of it? > > If it was long-standing code that would be important (because > you don't want to introduce regressions or conflicts when > backporting fixes). Since this is a new model I guess there > is leeway to just take v3 as is. > > Thanks, > Nick > >> >> Philippe Mathieu-Daudé (1): >> MAINTAINERS: Cover PowerPC SPI model in PowerNV section >> >> Chalapathi V (2): >> hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure >> hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index(). >> >> MAINTAINERS | 2 + >> include/hw/ssi/pnv_spi.h | 3 + >> hw/ssi/pnv_spi.c | 228 +++++++++++++++------------------------ >> 3 files changed, 89 insertions(+), 144 deletions(-) >
On 08-10-2024 18:14, Cédric Le Goater wrote: > On 10/8/24 09:43, Nicholas Piggin wrote: >> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote: >>> Hello, >>> >>> v3: >>> 1. Update the PowerNV maintainer section to include hw/ssi/pnv_spi* >>> 2. Use of PnvXferBuffer results in a additonal process overhead due to >>> frequent dynamic allocations and hence use an existing Fifo8 buffer. >>> 3. Use a local variable seq_index and use it with in while loop instead >>> of repeatedly calling get_seq_index() and make sure s->seq_op doesn't >>> overrun when seq_index is incremented. >>> >>> Tested: >>> passed make check and make check-avocado >>> >>> Supersedes: <20240807202804.56038-1-philmd@linaro.org> >> >> Hi Chalapathi, >> >> To be clear, this fixes Coverity CID 1558831? A Resolves: >> tag for the CID should be there, I guess it's patch 2? > > > Patches should have a tag : > > Resolves: Coverity CID XYZZY > > Thanks, > > C. Sure. Will update. Thank You > > >> >> I like patch 2, but since it is quite a significant change, >> should we take the v2 series first which is much smaller, >> then add this conversion on top of it? >> >> If it was long-standing code that would be important (because >> you don't want to introduce regressions or conflicts when >> backporting fixes). Since this is a new model I guess there >> is leeway to just take v3 as is. >> >> Thanks, >> Nick >> >>> >>> Philippe Mathieu-Daudé (1): >>> MAINTAINERS: Cover PowerPC SPI model in PowerNV section >>> >>> Chalapathi V (2): >>> hw/ssi/pnv_spi: Replace PnvXferBuffer with Fifo8 structure >>> hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index(). >>> >>> MAINTAINERS | 2 + >>> include/hw/ssi/pnv_spi.h | 3 + >>> hw/ssi/pnv_spi.c | 228 >>> +++++++++++++++------------------------ >>> 3 files changed, 89 insertions(+), 144 deletions(-) >> >