On 22 August 2017 at 09:44, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 21 August 2017 at 22:13, Matt Parker <mtpa...@gmail.com> wrote:
>> intel-hda is still using the old_mmio accessors for io.
>> This updates the device to use .read and .write accessors instead.
>
> Hi; thanks for this patch.
>
> It looks like you forgot to provide your Signed-off-by: line;
> we can't accept patches without one, I'm afraid.
>
>> ---
>> hw/audio/intel-hda.c | 56
>> +++++++++-------------------------------------------
>> 1 file changed, 9 insertions(+), 47 deletions(-)
>>
>> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
>> index 06acc98f7b..c0f002f744 100644
>> --- a/hw/audio/intel-hda.c
>> +++ b/hw/audio/intel-hda.c
>> @@ -1043,66 +1043,28 @@ static void intel_hda_regs_reset(IntelHDAState *d)
>>
>> /* --------------------------------------------------------------------- */
>>
>> -static void intel_hda_mmio_writeb(void *opaque, hwaddr addr, uint32_t val)
>> +static void intel_hda_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>> unsigned size)
>> {
>> IntelHDAState *d = opaque;
>> const IntelHDAReg *reg = intel_hda_reg_find(d, addr);
>>
>> - intel_hda_reg_write(d, reg, val, 0xff);
>> + intel_hda_reg_write(d, reg, val, (1UL << (size * 8)) - 1);
>
> If size is 4 and 'unsigned long' is 32 bits (which it usually
> is) then this will shift off the end of the value, which is
> undefined behaviour.
>
> You could fix that by using '1ULL' instead of '1UL', but
> I'd suggest using MAKE_64BIT_MASK(0, size * 8) for this.
> (that macro is in "qemu/bitops.h", which you'll probably need
> to add a #include for.) Using the macro makes the intent a bit
> clearer and means nobody has to think about whether the bit
> shifting is doing what it's supposed to.
>
> I recommend putting a newline in the prototype to keep
> it below 80 lines and please the checkpatch script, too.
>
Thanks for the suggestions. I'll have a look into bitops.h and make
sure to run the checkpatch script before submitting any future
patches.
Matt
