qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] audio: intel-hda: do not use old_mmio accesses


From: Matt Parker
Subject: Re: [Qemu-devel] [PATCH] audio: intel-hda: do not use old_mmio accesses
Date: Wed, 23 Aug 2017 21:45:52 +0100

On 22 August 2017 at 09:44, Peter Maydell <address@hidden> wrote:
> On 21 August 2017 at 22:13, Matt Parker <address@hidden> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]