diff mbox series

[v2,18/25] hw/sd: ssi-sd: Bump up version ids of VMStateDescription

Message ID 20210123104016.17485-19-bmeng.cn@gmail.com (mailing list archive)
State New, archived
Headers show
Series hw/riscv: sifive_u: Add missing SPI support | expand

Commit Message

Bin Meng Jan. 23, 2021, 10:40 a.m. UTC 
From: Bin Meng <bin.meng@windriver.com>

With all these fixes and improvements, there is no way for the
VMStateDescription to keep backward compatibility. We will have
to bump up version ids.

The s->mode check in the post_load() hook is also updated.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v2:
- new patch: bump up version ids of VMStateDescription

 hw/sd/ssi-sd.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 24, 2021, 4:59 p.m. UTC | #1 
On 1/23/21 11:40 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> With all these fixes and improvements, there is no way for the
> VMStateDescription to keep backward compatibility. We will have
> to bump up version ids.

Unfortunately this breaks bisectability (think about downstream
distributions cherry-picking patches individually).

I don't think there is a problem increasing 2 -> 3 -> 4 -> 5
(Cc'ed David in case). Could you respin increasing the version
on each VMState change?

> 
> The s->mode check in the post_load() hook is also updated.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - new patch: bump up version ids of VMStateDescription
> 
>  hw/sd/ssi-sd.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index ee4fbc3dfe..0c507f3ec5 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -4,6 +4,11 @@
>   * Copyright (c) 2007-2009 CodeSourcery.
>   * Written by Paul Brook
>   *
> + * Copyright (c) 2021 Wind River Systems, Inc.
> + * Improved by Bin Meng <bin.meng@windriver.com>
> + *
> + * Validated with U-Boot v2021.01 and Linux v5.10 mmc_spi driver
> + *
>   * This code is licensed under the GNU GPL v2.
>   *
>   * Contributions after 2012-01-13 are licensed under the terms of the
> @@ -319,7 +324,7 @@ static int ssi_sd_post_load(void *opaque, int version_id)
>  {
>      ssi_sd_state *s = (ssi_sd_state *)opaque;
>  
> -    if (s->mode > SSI_SD_DATA_READ) {
> +    if (s->mode > SSI_SD_SKIP_CRC16) {
>          return -EINVAL;
>      }
>      if (s->mode == SSI_SD_CMDARG &&
> @@ -337,8 +342,8 @@ static int ssi_sd_post_load(void *opaque, int version_id)
>  
>  static const VMStateDescription vmstate_ssi_sd = {
>      .name = "ssi_sd",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>      .post_load = ssi_sd_post_load,
>      .fields = (VMStateField []) {
>          VMSTATE_UINT32(mode, ssi_sd_state),
>
Philippe Mathieu-Daudé Jan. 24, 2021, 5:07 p.m. UTC | #2 
On 1/24/21 5:59 PM, Philippe Mathieu-Daudé wrote:
> On 1/23/21 11:40 AM, Bin Meng wrote:
>> From: Bin Meng <bin.meng@windriver.com>
>>
>> With all these fixes and improvements, there is no way for the
>> VMStateDescription to keep backward compatibility. We will have
>> to bump up version ids.
> 
> Unfortunately this breaks bisectability (think about downstream
> distributions cherry-picking patches individually).
> 
> I don't think there is a problem increasing 2 -> 3 -> 4 -> 5
> (Cc'ed David in case). Could you respin increasing the version
> on each VMState change?
> 
>>
>> The s->mode check in the post_load() hook is also updated.
>>
>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>>
>> ---
>>
>> Changes in v2:
>> - new patch: bump up version ids of VMStateDescription
>>
>>  hw/sd/ssi-sd.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
>> index ee4fbc3dfe..0c507f3ec5 100644
>> --- a/hw/sd/ssi-sd.c
>> +++ b/hw/sd/ssi-sd.c
>> @@ -4,6 +4,11 @@
>>   * Copyright (c) 2007-2009 CodeSourcery.
>>   * Written by Paul Brook
>>   *
>> + * Copyright (c) 2021 Wind River Systems, Inc.
>> + * Improved by Bin Meng <bin.meng@windriver.com>
>> + *
>> + * Validated with U-Boot v2021.01 and Linux v5.10 mmc_spi driver
>> + *
>>   * This code is licensed under the GNU GPL v2.
>>   *
>>   * Contributions after 2012-01-13 are licensed under the terms of the
>> @@ -319,7 +324,7 @@ static int ssi_sd_post_load(void *opaque, int version_id)
>>  {
>>      ssi_sd_state *s = (ssi_sd_state *)opaque;
>>  
>> -    if (s->mode > SSI_SD_DATA_READ) {
>> +    if (s->mode > SSI_SD_SKIP_CRC16) {

Doesn't this belong to patch #16 "Support single block write"?

>>          return -EINVAL;
>>      }
>>      if (s->mode == SSI_SD_CMDARG &&
>> @@ -337,8 +342,8 @@ static int ssi_sd_post_load(void *opaque, int version_id)
>>  
>>  static const VMStateDescription vmstate_ssi_sd = {
>>      .name = "ssi_sd",
>> -    .version_id = 2,
>> -    .minimum_version_id = 2,
>> +    .version_id = 3,
>> +    .minimum_version_id = 3,
>>      .post_load = ssi_sd_post_load,
>>      .fields = (VMStateField []) {
>>          VMSTATE_UINT32(mode, ssi_sd_state),
>>
>
Bin Meng Jan. 25, 2021, 1:20 a.m. UTC | #3 
On Mon, Jan 25, 2021 at 12:59 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/23/21 11:40 AM, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > With all these fixes and improvements, there is no way for the
> > VMStateDescription to keep backward compatibility. We will have
> > to bump up version ids.
>
> Unfortunately this breaks bisectability (think about downstream
> distributions cherry-picking patches individually).
>
> I don't think there is a problem increasing 2 -> 3 -> 4 -> 5
> (Cc'ed David in case). Could you respin increasing the version
> on each VMState change?
>

I definitely could be wrong, the reason I posted a single patch to
upreve the version is that, I was under an impression that in each big
release (like here 5.2.0 -> 6.0.0), the incompatibility version id
should be bumped up once.
It does not look correct to me that in a big release we bump up the
version id for 10 times.

Since this is a series to fix issues in the ssi-sd, I don't think it's
practical for downstream to just cherry-pick some commits while
leaving some other commits there.

Regards,
Bin
Dr. David Alan Gilbert Jan. 25, 2021, 10:41 a.m. UTC | #4 
* Bin Meng (bmeng.cn@gmail.com) wrote:
> On Mon, Jan 25, 2021 at 12:59 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 1/23/21 11:40 AM, Bin Meng wrote:
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > With all these fixes and improvements, there is no way for the
> > > VMStateDescription to keep backward compatibility. We will have
> > > to bump up version ids.
> >
> > Unfortunately this breaks bisectability (think about downstream
> > distributions cherry-picking patches individually).
> >
> > I don't think there is a problem increasing 2 -> 3 -> 4 -> 5
> > (Cc'ed David in case). Could you respin increasing the version
> > on each VMState change?
> >
> 
> I definitely could be wrong, the reason I posted a single patch to
> upreve the version is that, I was under an impression that in each big
> release (like here 5.2.0 -> 6.0.0), the incompatibility version id
> should be bumped up once.
> It does not look correct to me that in a big release we bump up the
> version id for 10 times.

I think I agree; I don't think we've ever done it incrementally like
that before.

It would only break bisectability if you were cross-version migrating
during the bisect which is rare.

> Since this is a series to fix issues in the ssi-sd, I don't think it's
> practical for downstream to just cherry-pick some commits while
> leaving some other commits there.

Never underestimate downstream :-)
However, please add a comment when you're doing incrimentals like this -
e.g. a TODO or something showing that it's unfinished and you need the
remaining patches so people don't do it accidentally.

Dave

> Regards,
> Bin
>
Bin Meng Jan. 25, 2021, 10:48 a.m. UTC | #5 
On Mon, Jan 25, 2021 at 6:41 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Bin Meng (bmeng.cn@gmail.com) wrote:
> > On Mon, Jan 25, 2021 at 12:59 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > >
> > > On 1/23/21 11:40 AM, Bin Meng wrote:
> > > > From: Bin Meng <bin.meng@windriver.com>
> > > >
> > > > With all these fixes and improvements, there is no way for the
> > > > VMStateDescription to keep backward compatibility. We will have
> > > > to bump up version ids.
> > >
> > > Unfortunately this breaks bisectability (think about downstream
> > > distributions cherry-picking patches individually).
> > >
> > > I don't think there is a problem increasing 2 -> 3 -> 4 -> 5
> > > (Cc'ed David in case). Could you respin increasing the version
> > > on each VMState change?
> > >
> >
> > I definitely could be wrong, the reason I posted a single patch to
> > upreve the version is that, I was under an impression that in each big
> > release (like here 5.2.0 -> 6.0.0), the incompatibility version id
> > should be bumped up once.
> > It does not look correct to me that in a big release we bump up the
> > version id for 10 times.
>
> I think I agree; I don't think we've ever done it incrementally like
> that before.
>

Thanks David.

> It would only break bisectability if you were cross-version migrating
> during the bisect which is rare.
>
> > Since this is a series to fix issues in the ssi-sd, I don't think it's
> > practical for downstream to just cherry-pick some commits while
> > leaving some other commits there.
>
> Never underestimate downstream :-)
> However, please add a comment when you're doing incrimentals like this -
> e.g. a TODO or something showing that it's unfinished and you need the
> remaining patches so people don't do it accidentally.
>

Sure, next time :)

Philippe, I guess we will need to hold on your PR?
http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226135

Regards,
Bin
diff mbox series

Patch

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index ee4fbc3dfe..0c507f3ec5 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -4,6 +4,11 @@ 
  * Copyright (c) 2007-2009 CodeSourcery.
  * Written by Paul Brook
  *
+ * Copyright (c) 2021 Wind River Systems, Inc.
+ * Improved by Bin Meng <bin.meng@windriver.com>
+ *
+ * Validated with U-Boot v2021.01 and Linux v5.10 mmc_spi driver
+ *
  * This code is licensed under the GNU GPL v2.
  *
  * Contributions after 2012-01-13 are licensed under the terms of the
@@ -319,7 +324,7 @@  static int ssi_sd_post_load(void *opaque, int version_id)
 {
     ssi_sd_state *s = (ssi_sd_state *)opaque;
 
-    if (s->mode > SSI_SD_DATA_READ) {
+    if (s->mode > SSI_SD_SKIP_CRC16) {
         return -EINVAL;
     }
     if (s->mode == SSI_SD_CMDARG &&
@@ -337,8 +342,8 @@  static int ssi_sd_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_ssi_sd = {
     .name = "ssi_sd",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .post_load = ssi_sd_post_load,
     .fields = (VMStateField []) {
         VMSTATE_UINT32(mode, ssi_sd_state),