Re: [Qemu-devel] [PATCH 5/5] hw/block/pflash_cfi02: Add the DeviceReset() handler

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/6/19 5:17 PM, Laszlo Ersek wrote:
> On 05/05/19 22:06, Philippe Mathieu-Daudé wrote:
>> The pflash device is a child of TYPE_DEVICE, so it can implement
>> the DeviceReset handler. Actually it has to implement it, else
>> on machine reset it might stay in an incoherent state, as it has
>> been reported in the buglink listed below.
>>
>> Add the DeviceReset handler and remove its call from the realize()
>> function.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>> Reported-by: Laszlo Ersek <lersek@redhat.com>
> 
> - IMO, the above two tags should be dropped from the commit message, as
> they are specific to CFI01.

OK.

> - Additionally, the commit message references the realize() function
> (correctly), but the patch doesn't change that function. That is, the
> patch doesn't remove the pflash_reset() call from pflash_cfi02_realize()
> that was introduced in the last patch.

You found a bug, having your reviewing CFI02 is useful :)
I had it correct in a staged branch, then messed while rebasing.

Thanks for noticing this,

Phil.

> 
> Thanks
> Laszlo
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/block/pflash_cfi02.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index f321b74433c..5af367d1563 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -674,6 +674,11 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>>      pfl->cfi_table[0x3c] = 0x00;
>>  }
>>  
>> +static void pflash_cfi02_reset(DeviceState *dev)
>> +{
>> +    pflash_reset(PFLASH_CFI02(dev));
>> +}
>> +
>>  static Property pflash_cfi02_properties[] = {
>>      DEFINE_PROP_DRIVE("drive", PFlashCFI02, blk),
>>      DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, nb_blocs, 0),
>> @@ -701,6 +706,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>  
>> +    dc->reset = pflash_cfi02_reset;
>>      dc->realize = pflash_cfi02_realize;
>>      dc->unrealize = pflash_cfi02_unrealize;
>>      dc->props = pflash_cfi02_properties;
>>
>