Re: [Qemu-devel] [PATCH v7 06/16] sdhci: add init_readonly_registers() to initialize the CAPAB register

[Philippe Mathieu-Daudé]

On 01/18/2018 12:47 PM, Marcel Apfelbaum wrote:
> On 18/01/2018 17:44, Philippe Mathieu-Daudé wrote:
>> On 01/18/2018 09:32 AM, Philippe Mathieu-Daudé wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>> ---
>>>   hw/sd/sdhci.c | 17 ++++++++++++++---
>>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index f9264d3be5..08b85558f1 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -1174,12 +1174,19 @@ static inline unsigned int
>>> sdhci_get_fifolen(SDHCIState *s)
>>>       }
>>>   }
>>>   +static void sdhci_init_readonly_registers(SDHCIState *s, Error
>>> **errp)
>>> +{
>>> +    if (s->capareg == UINT64_MAX) {
>>> +        s->capareg = SDHC_CAPAB_REG_DEFAULT;
>>> +    }
>>> +}
>>> +
>>>   /* --- qdev common --- */
>>>     #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>>> -    /* Capabilities registers provide information on supported features
>>> -     * of this specific host controller implementation */ \
>>> -    DEFINE_PROP_UINT64("capareg", _state, capareg,
>>> SDHC_CAPAB_REG_DEFAULT), \
>>> +    /* deprecated: Capabilities registers provide information on
>>> supported
>>> +     * features of this specific host controller implementation */ \
>>> +    DEFINE_PROP_UINT64("capareg", _state, capareg, UINT64_MAX), \
>>>       DEFINE_PROP_UINT64("maxcurr", _state, maxcurr, 0)
>>>     static void sdhci_initfn(SDHCIState *s)
>>> @@ -1204,6 +1211,10 @@ static void sdhci_uninitfn(SDHCIState *s)
>>>     static void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>   {
>>> +    sdhci_init_readonly_registers(s, errp);
>>> +    if (errp && *errp) {
>>
> 
> Hi Phillipe,
> 
>> Paolo said this is wrong (as in bad pattern?).
>>
>> I will respin probably using 'bool sdhci_init_readonly_registers()'
>> instead.
>>
> 
> I always use the explanations in include/qapi/error.h
> as guidelines.

I'll read them more carefully ;)

Just checking with this cocci script,

@@
Error **errp;
@@
*if (errp && *errp) {
    ...
 }

we get:

+++ ./hw/sd/sdhci.c
@@ -1303,7 +1303,6 @@ static void sdhci_pci_realize(PCIDevice

     sdhci_initfn(s);
     sdhci_common_realize(s, errp);
-    if (errp && *errp) {
         return;
     }

@@ -1383,7 +1382,6 @@ static void sdhci_sysbus_realize(DeviceS
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);

     sdhci_common_realize(s, errp);
-    if (errp && *errp) {
         return;
     }

+++ ./hw/mem/pc-dimm.c
@@ -138,7 +138,6 @@ static int pc_existing_dimms_capacity_in
                 cap->errp);
         }

-        if (cap->errp && *cap->errp) {
             return 1;
         }
     }
@@ -320,7 +319,6 @@ uint64_t pc_dimm_get_free_addr(uint64_t
         uint64_t dimm_size = object_property_get_uint(OBJECT(dimm),
                                                       PC_DIMM_SIZE_PROP,
                                                       errp);
-        if (errp && *errp) {
             goto out;
         }

+++ ./exec.c
@@ -1656,7 +1656,6 @@ static void *file_ram_alloc(RAMBlock *bl

     if (mem_prealloc) {
         os_mem_prealloc(fd, area, memory, smp_cpus, errp);
-        if (errp && *errp) {
             qemu_ram_munmap(area, memory);
             return NULL;
         }

not that bad.