The reset() code is used in various places, refactor it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 6dc04f156a7..073cd14978f 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
     }
 };
 
+static void pflash_reset(PFlashCFI01 *pfl)
+{
+    trace_pflash_reset();
+    pfl->wcycle = 0;
+    pfl->cmd = 0;
+    pfl->status = 0;
+    memory_region_rom_device_set_romd(&pfl->mem, true);
+}
+
 /* Perform a CFI query based on the bank width of the flash.
  * If this code is called we know we have a device_width set for
  * this flash.
@@ -275,8 +284,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
     default:
         /* This should never happen : reset state & treat it as a read */
         DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
-        pfl->wcycle = 0;
-        pfl->cmd = 0;
+        pflash_reset(pfl);
         /* fall through to read code */
     case 0x00:
         /* Flash area read */
@@ -639,10 +647,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                   "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
 
  reset_flash:
-    trace_pflash_reset();
-    memory_region_rom_device_set_romd(&pfl->mem, true);
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
+    pflash_reset(pfl);
 }
 
 
@@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
-    pfl->status = 0;
+    pflash_reset(pfl);
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
-- 
2.20.1

On Sun, May 05, 2019 at 10:05:59PM +0200, Philippe Mathieu-Daudé wrote:
>The reset() code is used in various places, refactor it.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>

>---
> hw/block/pflash_cfi01.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>index 6dc04f156a7..073cd14978f 100644
>--- a/hw/block/pflash_cfi01.c
>+++ b/hw/block/pflash_cfi01.c
>@@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
>     }
> };
> 
>+static void pflash_reset(PFlashCFI01 *pfl)
>+{
>+    trace_pflash_reset();
>+    pfl->wcycle = 0;
>+    pfl->cmd = 0;
>+    pfl->status = 0;
>+    memory_region_rom_device_set_romd(&pfl->mem, true);
>+}
>+
> /* Perform a CFI query based on the bank width of the flash.
>  * If this code is called we know we have a device_width set for
>  * this flash.
>@@ -275,8 +284,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>     default:
>         /* This should never happen : reset state & treat it as a read */
>         DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
>-        pfl->wcycle = 0;
>-        pfl->cmd = 0;
>+        pflash_reset(pfl);
>         /* fall through to read code */
>     case 0x00:
>         /* Flash area read */
>@@ -639,10 +647,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                   "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
> 
>  reset_flash:
>-    trace_pflash_reset();
>-    memory_region_rom_device_set_romd(&pfl->mem, true);
>-    pfl->wcycle = 0;
>-    pfl->cmd = 0;
>+    pflash_reset(pfl);
> }
> 
> 
>@@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>         pfl->max_device_width = pfl->device_width;
>     }
> 
>-    pfl->wcycle = 0;
>-    pfl->cmd = 0;
>-    pfl->status = 0;
>+    pflash_reset(pfl);
>     /* Hardcoded CFI table */
>     /* Standard "QRY" string */
>     pfl->cfi_table[0x10] = 'Q';
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

On 05/05/19 22:05, Philippe Mathieu-Daudé wrote:
> The reset() code is used in various places, refactor it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 6dc04f156a7..073cd14978f 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>  
> +static void pflash_reset(PFlashCFI01 *pfl)
> +{
> +    trace_pflash_reset();
> +    pfl->wcycle = 0;
> +    pfl->cmd = 0;
> +    pfl->status = 0;
> +    memory_region_rom_device_set_romd(&pfl->mem, true);
> +}
> +
>  /* Perform a CFI query based on the bank width of the flash.
>   * If this code is called we know we have a device_width set for
>   * this flash.
> @@ -275,8 +284,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>      default:
>          /* This should never happen : reset state & treat it as a read */
>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
> -        pfl->wcycle = 0;
> -        pfl->cmd = 0;
> +        pflash_reset(pfl);
>          /* fall through to read code */
>      case 0x00:
>          /* Flash area read */

This change introduces two new actions:
- zeroing "status"
- flipping the chip to romd mode (unless it's already in romd mode)

> @@ -639,10 +647,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                    "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>  
>   reset_flash:
> -    trace_pflash_reset();
> -    memory_region_rom_device_set_romd(&pfl->mem, true);
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pflash_reset(pfl);
>  }

This change introduces a new action:
- zeroing "status"

>  
>  
> @@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>  
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> -    pfl->status = 0;
> +    pflash_reset(pfl);

This change introduces a new action:
- flipping the chip to romd mode (unless it's already in romd mode)

>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
> 

Therefore, this patch is not *obviously* a refactoring patch. (IOW, it
may be a pure refactoring patch, but it's not very easy to see.) I suggest:

- either documenting the new actions in the commit message (and why they
are justified),

- or prepending a separate patch that first unifies the behavior in all
the separate reset locations, and then this patch could indeed become an
"obvious" refactoring / code exctraction.

(Clearly, I'm asking for this with an eye towards git-bisect.)

Thanks
Laszlo

On 05/05/19 22:05, Philippe Mathieu-Daudé wrote:
> The reset() code is used in various places, refactor it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)

BTW: you didn't add an explicit "Cc: stable" tag here, so I almost
missed it -- but, mainly, is this really suitable material for stable?
We haven't really noticed (or at least pin-pointed) the lack of the
reset handlers in 12 years, correct?

Thanks
Laszlo

> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 6dc04f156a7..073cd14978f 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>  
> +static void pflash_reset(PFlashCFI01 *pfl)
> +{
> +    trace_pflash_reset();
> +    pfl->wcycle = 0;
> +    pfl->cmd = 0;
> +    pfl->status = 0;
> +    memory_region_rom_device_set_romd(&pfl->mem, true);
> +}
> +
>  /* Perform a CFI query based on the bank width of the flash.
>   * If this code is called we know we have a device_width set for
>   * this flash.
> @@ -275,8 +284,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>      default:
>          /* This should never happen : reset state & treat it as a read */
>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
> -        pfl->wcycle = 0;
> -        pfl->cmd = 0;
> +        pflash_reset(pfl);
>          /* fall through to read code */
>      case 0x00:
>          /* Flash area read */
> @@ -639,10 +647,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                    "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>  
>   reset_flash:
> -    trace_pflash_reset();
> -    memory_region_rom_device_set_romd(&pfl->mem, true);
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pflash_reset(pfl);
>  }
>  
>  
> @@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>  
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> -    pfl->status = 0;
> +    pflash_reset(pfl);
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
>