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

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

[Matt Parker]

intel-hda is still using the old_mmio accessors for io.
This updates the device to use .read and .write accessors instead.
---
 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);
 }
 
-static void intel_hda_mmio_writew(void *opaque, hwaddr addr, uint32_t val)
+static uint64_t intel_hda_mmio_read(void *opaque, hwaddr addr, unsigned size)
 {
     IntelHDAState *d = opaque;
     const IntelHDAReg *reg = intel_hda_reg_find(d, addr);
 
-    intel_hda_reg_write(d, reg, val, 0xffff);
-}
-
-static void intel_hda_mmio_writel(void *opaque, hwaddr addr, uint32_t val)
-{
-    IntelHDAState *d = opaque;
-    const IntelHDAReg *reg = intel_hda_reg_find(d, addr);
-
-    intel_hda_reg_write(d, reg, val, 0xffffffff);
-}
-
-static uint32_t intel_hda_mmio_readb(void *opaque, hwaddr addr)
-{
-    IntelHDAState *d = opaque;
-    const IntelHDAReg *reg = intel_hda_reg_find(d, addr);
-
-    return intel_hda_reg_read(d, reg, 0xff);
-}
-
-static uint32_t intel_hda_mmio_readw(void *opaque, hwaddr addr)
-{
-    IntelHDAState *d = opaque;
-    const IntelHDAReg *reg = intel_hda_reg_find(d, addr);
-
-    return intel_hda_reg_read(d, reg, 0xffff);
-}
-
-static uint32_t intel_hda_mmio_readl(void *opaque, hwaddr addr)
-{
-    IntelHDAState *d = opaque;
-    const IntelHDAReg *reg = intel_hda_reg_find(d, addr);
-
-    return intel_hda_reg_read(d, reg, 0xffffffff);
+    return intel_hda_reg_read(d, reg, (1UL << (size * 8)) - 1);
 }
 
 static const MemoryRegionOps intel_hda_mmio_ops = {
-    .old_mmio = {
-        .read = {
-            intel_hda_mmio_readb,
-            intel_hda_mmio_readw,
-            intel_hda_mmio_readl,
-        },
-        .write = {
-            intel_hda_mmio_writeb,
-            intel_hda_mmio_writew,
-            intel_hda_mmio_writel,
-        },
+    .read = intel_hda_mmio_read,
+    .write = intel_hda_mmio_write,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 4,
     },
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
-- 
2.13.2

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170821211320.7026-1-mtparkr@gmail.com
Subject: [Qemu-devel] [PATCH] audio: intel-hda: do not use old_mmio accesses

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
87ea375b1e audio: intel-hda: do not use old_mmio accesses

=== OUTPUT BEGIN ===
Checking PATCH 1/1: audio: intel-hda: do not use old_mmio accesses...
WARNING: line over 80 characters
#19: FILE: hw/audio/intel-hda.c:1046:
+static void intel_hda_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 75 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Peter Maydell]

On 21 August 2017 at 22:13, Matt Parker <mtparkr@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
-- PMM

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

[Matt Parker]

On 22 August 2017 at 09:44, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 21 August 2017 at 22:13, Matt Parker <mtparkr@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