[v3] hw/sd/sdcard: Accumulation of cleanups and fixes

[PATCH v3 15/17] tests/qtest: Disable npcm7xx_sdhci tests using hardcoded RCA

Posted by Philippe Mathieu-Daudé 5 months ago

Disable tests using 0x4567 hardcoded RCA otherwise when
using random RCA we get:

  ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread: assertion failed: (ret == len)
  not ok /arm/npcm7xx_sdhci/read_sd - ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread: assertion failed: (ret == len)
  Bail out!

See https://lore.kernel.org/qemu-devel/37f83be9-deb5-42a1-b704-14984351d803@linaro.org/

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Cc: Hao Wu <wuhaotsh@google.com>
Cc: Shengtan Mao <stmao@google.com>
Cc: Tyrone Ting <kfting@nuvoton.com>
---
 tests/qtest/npcm7xx_sdhci-test.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/qtest/npcm7xx_sdhci-test.c b/tests/qtest/npcm7xx_sdhci-test.c
index 5d68540e52..6a42b142ad 100644
--- a/tests/qtest/npcm7xx_sdhci-test.c
+++ b/tests/qtest/npcm7xx_sdhci-test.c
@@ -44,6 +44,7 @@ static QTestState *setup_sd_card(void)
     sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x41200000, 0, (41 << 8));
     sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_ALL_SEND_CID);
     sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_SEND_RELATIVE_ADDR);
+    g_test_skip("hardcoded 0x4567 card address");
     sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0,
                    SDHC_SELECT_DESELECT_CARD);
 
@@ -76,6 +77,9 @@ static void test_read_sd(void)
 {
     QTestState *qts = setup_sd_card();
 
+    g_test_skip("hardcoded 0x4567 card address used in setup_sd_card()");
+    return;
+
     write_sdread(qts, "hello world");
     write_sdread(qts, "goodbye");
 
@@ -108,6 +112,9 @@ static void test_write_sd(void)
 {
     QTestState *qts = setup_sd_card();
 
+    g_test_skip("hardcoded 0x4567 card address used in setup_sd_card()");
+    return;
+
     sdwrite_read(qts, "hello world");
     sdwrite_read(qts, "goodbye");
 
-- 
2.41.0

Re: [PATCH v3 15/17] tests/qtest: Disable npcm7xx_sdhci tests using hardcoded RCA

Posted by Thomas Huth 5 months ago

On 27/06/2024 18.22, Philippe Mathieu-Daudé wrote:
> Disable tests using 0x4567 hardcoded RCA otherwise when
> using random RCA we get:
> 
>    ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread: assertion failed: (ret == len)
>    not ok /arm/npcm7xx_sdhci/read_sd - ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread: assertion failed: (ret == len)
>    Bail out!
> 
> See https://lore.kernel.org/qemu-devel/37f83be9-deb5-42a1-b704-14984351d803@linaro.org/
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Cc: Hao Wu <wuhaotsh@google.com>
> Cc: Shengtan Mao <stmao@google.com>
> Cc: Tyrone Ting <kfting@nuvoton.com>
> ---
>   tests/qtest/npcm7xx_sdhci-test.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/tests/qtest/npcm7xx_sdhci-test.c b/tests/qtest/npcm7xx_sdhci-test.c
> index 5d68540e52..6a42b142ad 100644
> --- a/tests/qtest/npcm7xx_sdhci-test.c
> +++ b/tests/qtest/npcm7xx_sdhci-test.c
> @@ -44,6 +44,7 @@ static QTestState *setup_sd_card(void)
>       sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x41200000, 0, (41 << 8));
>       sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_ALL_SEND_CID);
>       sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_SEND_RELATIVE_ADDR);
> +    g_test_skip("hardcoded 0x4567 card address");

This g_test_skip here does not make too much sense (since you're doing it in 
the caller site, too) ... could you please replace it with a proper comment 
why this code needs to be reworked? Thanks!

  Thomas


>       sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0,
>                      SDHC_SELECT_DESELECT_CARD);
>   
> @@ -76,6 +77,9 @@ static void test_read_sd(void)
>   {
>       QTestState *qts = setup_sd_card();
>   
> +    g_test_skip("hardcoded 0x4567 card address used in setup_sd_card()");
> +    return;
> +
>       write_sdread(qts, "hello world");
>       write_sdread(qts, "goodbye");
>   
> @@ -108,6 +112,9 @@ static void test_write_sd(void)
>   {
>       QTestState *qts = setup_sd_card();
>   
> +    g_test_skip("hardcoded 0x4567 card address used in setup_sd_card()");
> +    return;
> +
>       sdwrite_read(qts, "hello world");
>       sdwrite_read(qts, "goodbye");
>

Re: [PATCH v3 15/17] tests/qtest: Disable npcm7xx_sdhci tests using hardcoded RCA

Posted by Philippe Mathieu-Daudé 5 months ago

On 27/6/24 18:47, Thomas Huth wrote:
> On 27/06/2024 18.22, Philippe Mathieu-Daudé wrote:
>> Disable tests using 0x4567 hardcoded RCA otherwise when
>> using random RCA we get:
>>
>>    ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread: 
>> assertion failed: (ret == len)
>>    not ok /arm/npcm7xx_sdhci/read_sd - 
>> ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread: 
>> assertion failed: (ret == len)
>>    Bail out!
>>
>> See 
>> https://lore.kernel.org/qemu-devel/37f83be9-deb5-42a1-b704-14984351d803@linaro.org/
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> Cc: Hao Wu <wuhaotsh@google.com>
>> Cc: Shengtan Mao <stmao@google.com>
>> Cc: Tyrone Ting <kfting@nuvoton.com>
>> ---
>>   tests/qtest/npcm7xx_sdhci-test.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/tests/qtest/npcm7xx_sdhci-test.c 
>> b/tests/qtest/npcm7xx_sdhci-test.c
>> index 5d68540e52..6a42b142ad 100644
>> --- a/tests/qtest/npcm7xx_sdhci-test.c
>> +++ b/tests/qtest/npcm7xx_sdhci-test.c
>> @@ -44,6 +44,7 @@ static QTestState *setup_sd_card(void)
>>       sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x41200000, 0, (41 << 
>> 8));
>>       sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_ALL_SEND_CID);
>>       sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, 
>> SDHC_SEND_RELATIVE_ADDR);
>> +    g_test_skip("hardcoded 0x4567 card address");
> 
> This g_test_skip here does not make too much sense (since you're doing 
> it in the caller site, too) ... could you please replace it with a 
> proper comment why this code needs to be reworked? Thanks!

Sorry I forgot to tag "NOTFORMERGE". Ideally I'd wait Google
maintainers to fix the test so we don't need this patch. If
they don't I'll update as you suggested.
(An alternative I haven't investigated is to run the test
using the -seed argument to force deterministic mode).

Thanks!

> 
>   Thomas
> 
> 
>>       sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0,
>>                      SDHC_SELECT_DESELECT_CARD);
>> @@ -76,6 +77,9 @@ static void test_read_sd(void)
>>   {
>>       QTestState *qts = setup_sd_card();
>> +    g_test_skip("hardcoded 0x4567 card address used in 
>> setup_sd_card()");
>> +    return;