Re: [PATCH] drm/amdgpu/acp: Fix slab-out-of-bounds in mfd_add_device in acp_hw_init

[Agrawal, Akshu]

 Was this patch ever picked up?  I can't find it in agd5f/linux.
>>>
>>>
>>> It wasn't applied.  I don't see 51f7415039d4 ("drm/amd/amdgpu:
>>> creating two I2S instances for stoney/cz") upstream yet either.
>>> Daniel, Vijendar, which ones do you want applied?  Can you send me the
>>> patches?
>>>
>>> Alex
>>
>>
>> Hi Alex,
>>
>> "drm/amd/amdgpu: creating two I2S instances for stoney/cz" patch exists in
>> drm-next branch. Please pick the patch .
> 
> So just that one?  I seem to recall there being later revisions of
> that patch that you reworked after applying the original version.
> Also that patch was originally part of a larger series.  Are those
> changes required too?
> 
> Alex
> 

Hi Alex,

In agd5f/linux, branch "amd-staging-drm-next",
506f7d1 drm/amd/amdgpu: creating two I2S instances for stoney/cz
patch is present.
This patch is the correct version and there aren't any other changes
required with it.

Only Dan's, this mail's patch is currently missing form the tree.

Thanks,
Akshu
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amdgpu/acp: Fix slab-out-of-bounds in mfd_add_device in acp_hw_init

[Agrawal, Akshu]

On 4/13/2018 9:45 PM, Daniel Kurtz wrote:

Commit 51f7415039d4 ("drm/amd/amdgpu: creating two I2S instances for
stoney/cz") added support for the "BT_I2S" ACP i2s channel.  As part of
this change, one additional acp resource was added, but the "num_resource"
count was accidentally incremented by 2.

This incorrect count eventually causes mfd_add_device() to try to access
an invalid memory address (the location of non-existent resource 5.

This fault was detected by running a KASAN enabled kernel, which produced
the following splat at boot:

[6.612987] 
==
[6.613509] BUG: KASAN: slab-out-of-bounds in mfd_add_device+0x4bc/0x7a7
[6.613509] Read of size 8 at addr 880107d4dc58 by task swapper/0/1
[6.613509]
[6.613509] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.14.33 #349
[6.613509] Hardware name: Google Grunt/Grunt, BIOS 
Google_Grunt.10543.0.2018_04_03_1812 04/02/2018
[6.613509] Call Trace:
[6.613509]  dump_stack+0x4d/0x63
[6.613509]  print_address_description+0x80/0x2d6
[6.613509]  ? mfd_add_device+0x4bc/0x7a7
[6.613509]  kasan_report+0x255/0x295
[6.613509]  mfd_add_device+0x4bc/0x7a7
[6.613509]  ? kasan_kmalloc+0x99/0xa8
[6.613509]  ? mfd_add_devices+0x58/0xe4
[6.613509]  ? __kmalloc+0x154/0x178
[6.613509]  mfd_add_devices+0xa5/0xe4
[6.613509]  acp_hw_init+0x92e/0xc4a
[6.613509]  amdgpu_device_init+0x1dfb/0x22a2
[6.613509]  ? kmalloc_order+0x53/0x5d
[6.613509]  ? kmalloc_order_trace+0x23/0xb3
[6.613509]  amdgpu_driver_load_kms+0xce/0x267
[6.613509]  drm_dev_register+0x169/0x2fb
[6.613509]  amdgpu_pci_probe+0x217/0x242
[6.613509]  pci_device_probe+0x101/0x18e
[6.613509]  driver_probe_device+0x1dd/0x419
[6.613509]  ? ___might_sleep+0x80/0x1b6
[6.613509]  __driver_attach+0x9f/0xc9
[6.613509]  ? driver_probe_device+0x419/0x419
[6.613509]  bus_for_each_dev+0xbc/0xe1
[6.613509]  bus_add_driver+0x189/0x2c0
[6.613509]  driver_register+0x108/0x156
[6.613509]  ? ttm_init+0x67/0x67
[6.613509]  do_one_initcall+0xb2/0x161
[6.613509]  kernel_init_freeable+0x25a/0x308
[6.613509]  ? rest_init+0xcc/0xcc
[6.613509]  kernel_init+0x11/0x10d
[6.613509]  ? rest_init+0xcc/0xcc
[6.613509]  ret_from_fork+0x22/0x40
[6.613509]
[6.613509] Allocated by task 1:
[6.613509]  save_stack+0x46/0xce
[6.613509]  kasan_kmalloc+0x99/0xa8
[6.613509]  kmem_cache_alloc_trace+0x11a/0x13e
[6.613509]  acp_hw_init+0x210/0xc4a
[6.613509]  amdgpu_device_init+0x1dfb/0x22a2
[6.613509]  amdgpu_driver_load_kms+0xce/0x267
[6.613509]  drm_dev_register+0x169/0x2fb
[6.613509]  amdgpu_pci_probe+0x217/0x242
[6.613509]  pci_device_probe+0x101/0x18e
[6.613509]  driver_probe_device+0x1dd/0x419
[6.613509]  __driver_attach+0x9f/0xc9
[6.613509]  bus_for_each_dev+0xbc/0xe1
[6.613509]  bus_add_driver+0x189/0x2c0
[6.613509]  driver_register+0x108/0x156
[6.613509]  do_one_initcall+0xb2/0x161
[6.613509]  kernel_init_freeable+0x25a/0x308
[6.613509]  kernel_init+0x11/0x10d
[6.613509]  ret_from_fork+0x22/0x40
[6.613509]
[6.613509] Freed by task 0:
[6.613509] (stack is not available)
[6.613509]
[6.613509] The buggy address belongs to the object at 880107d4db08
[6.613509]  which belongs to the cache kmalloc-512 of size 512
[6.613509] The buggy address is located 336 bytes inside of
[6.613509]  512-byte region [880107d4db08, 880107d4dd08)
[6.613509] The buggy address belongs to the page:
[6.613509] page:ea00041f5300 count:1 mapcount:0 mapping:  
(null) index:0x0 compound_mapcount: 0
[6.613509] flags: 0x80008100(slab|head)
[6.613509] raw: 80008100   
000100120012
[6.613509] raw: ea0004208520 88010b001680 88010b002cc0 

[6.613509] page dumped because: kasan: bad access detected
[6.613509]
[6.613509] Memory state around the buggy address:
[6.613509]  880107d4db00: fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[6.613509]  880107d4db80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00
[6.613509] >880107d4dc00: 00 00 00 00 00 00 00 00 00 fc fc fc fc fc fc 
fc
[6.613509] ^
[6.613509]  880107d4dc80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[6.613509]  880107d4dd00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc
[6.613509] 
==

Fixes: 51f7415039d4 ("drm/amd/amdgpu: creating two I2S instances for stoney/cz")
Signed-off-by: Daniel Kurtz 

Acked-by: Akshu Agrawal 


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git 

Re: [PATCH v2 2/3] ASoC: rt5645: Wait for 400msec before concluding on value of RT5645_VENDOR_ID2

[Agrawal, Akshu]

On 11/8/2017 11:39 PM, Mark Brown wrote:

On Wed, Nov 08, 2017 at 12:24:03PM -0500, Alex Deucher wrote:


regmap_read(regmap, RT5645_VENDOR_ID2, );
  
+	/*

+* Read after 400msec, as it is the interval required between
+* read and power On.
+*/
+   msleep(TIME_TO_POWER_MS);
+   regmap_read(regmap, RT5645_VENDOR_ID2, );
+


This leaves the original read in there so we've both got the early read
(which might upset things potentially) and the delayed read.  Shouldn't
we just be adding a msleep() before the existing read?



My bad, I should have removed the addition of register read from the patch.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] ASoC: amd: Report accurate hw_ptr during dma

[Agrawal, Akshu]

On 11/7/2017 5:07 PM, Mark Brown wrote:

On Tue, Nov 07, 2017 at 07:26:03PM +0530, Mukunda,Vijendar wrote:

Removing URL links and commit-ready description in v2.


This doesn't really answer my question:


These two URLs are different, what was being reviewed here?  What is
Commit-Ready supposed to mean?


Same patch is reviewed, once on 4.4 kernel (659699) and then on 4.12 
kernel (672267).

Commit-ready is to get it merged on tree after receiving a +2.



Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] ASoC: rt5645: Wait for 400msec before concluding on value of RT5645_VENDOR_ID2

[Agrawal, Akshu]

On 11/6/2017 9:54 PM, Mark Brown wrote:

On Fri, Nov 03, 2017 at 04:35:45PM -0400, Alex Deucher wrote:


Minimum time required between power On of codec and read
of RT5645_VENDOR_ID2 is 400msec. We should wait and attempt
before erroring out.


So the description says we have to wait 400ms before attempting a
read...


Yes, that's true.




BUG=b:66978383


What does this mean?


A bug ID. Removing it in V2.




@@ -3786,6 +3789,15 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
}
regmap_read(regmap, RT5645_VENDOR_ID2, );
  
+	/*

+* Read for 400msec, as it is the interval required between
+* read and power On.
+*/
+   while (val != RT5645_DEVICE_ID && val != RT5650_DEVICE_ID && --timeout) 
{
+   msleep(1);
+   regmap_read(regmap, RT5645_VENDOR_ID2, );
+   }
+


...but what we actually do is try to read up to 400 times starting well
before that 400ms is up.  This directly contradicts what the commit
message said we needed, may take a lot longer if the chip misbehaves on
the I2C bus while it's not ready (which wouldn't be that much of a
surprise), might lead to us reporting before the chip is really stable
(if the read happens to work while the chip isn't yet stable) and could
cause lots of noise on the console if the I2C controller gets upset.
What are we actually waiting for here?



In my understanding if we get RT5645 or RT5650 DEVICE ID from register 
RT5645_VENDOR_ID2 then that means the chip is stable.



If we really need 400ms of dead reckoning time (which is a lot for a
modern chip, that feels more like a VMID ramp) then it's going to be
safer to just do that.



Agreed, will just sleep for 400ms before reading the register value.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [radeon-alex:sound-for-next-stoney 1/3] acp-pcm-dma.c:undefined reference to `__umoddi3'

[Agrawal, Akshu]

Hi Vijendar,

Below is the patch from Guenter Roeck  to fix build 
errors on 32 bit.


Regards,
Akshu

From 3aa3a8549e986d860300e0623d048ec93482da19 Mon Sep 17 00:00:00 2001
From: Guenter Roeck 
Date: Fri, 22 Sep 2017 09:43:27 -0700
Subject: [PATCH] FIXUP: FROMLIST: ASoC: amd: Report accurate hw_ptr 
during dma


ERROR: "__aeabi_uldivmod" [sound/soc/amd/snd-soc-acp-pcm.ko] undefined!

64-bit divides require special operations to avoid build errors on 32-bit
systems.

BUG=b:63121716
TEST="Build i386:allmodconfig"

Change-Id: I684934d1d16d3f7f72eb1067f23dcb2acf15b645
Signed-off-by: Guenter Roeck 
Reviewed-on: https://chromium-review.googlesource.com/678919
Reviewed-by: Jason Clinton 
(cherry picked from commit 7ca726e80f21abdbaed9a5a70def1c33a26f8533)
Reviewed-on: https://chromium-review.googlesource.com/681618
---
 sound/soc/amd/acp-pcm-dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 13d040a4d26f..ef7e98ad960c 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -856,12 +856,11 @@ static snd_pcm_uframes_t acp_dma_pointer(struct 
snd_pcm_substream *substream)

if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
if (bytescount > rtd->renderbytescount)
bytescount = bytescount - rtd->renderbytescount;
-   pos =  bytescount % buffersize;
} else {
if (bytescount > rtd->capturebytescount)
bytescount = bytescount - rtd->capturebytescount;
-   pos = bytescount % buffersize;
}
+   pos = do_div(bytescount, buffersize);
return bytes_to_frames(runtime, pos);
 }


On 11/4/2017 7:26 AM, kbuild test robot wrote:

tree:   git://people.freedesktop.org/~agd5f/linux.git sound-for-next-stoney
head:   d01a736c0b1595c5e65b2f2b16fe2da87ec3bc4c
commit: 41f9cb7a4d2aaf31aad6a87278f028add9d2d1a6 [1/3] ASoC: amd: Report 
accurate hw_ptr during dma
config: i386-allyesconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
 git checkout 41f9cb7a4d2aaf31aad6a87278f028add9d2d1a6
 # save the attached .config to linux build tree
 make ARCH=i386

All errors (new ones prefixed by >>):

sound/soc/amd/acp-pcm-dma.o: In function `acp_dma_pointer':

acp-pcm-dma.c:(.text+0x3d7): undefined reference to `__umoddi3'

acp-pcm-dma.c:(.text+0x3fc): undefined reference to `__umoddi3'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6 v3] ASoC: AMD: Add machine driver for cz rt5650

[Agrawal, Akshu]

On 8/31/2017 5:08 PM, Mark Brown wrote:
> On Fri, Aug 18, 2017 at 02:10:30PM -0400, Alex Deucher wrote:
> 
>> +++ b/sound/soc/amd/Kconfig
>> @@ -2,3 +2,10 @@ config SND_SOC_AMD_ACP
>>  tristate "AMD Audio Coprocessor support"
>>  help
>>   This option enables ACP DMA support on AMD platform.
>> +config SND_SOC_AMD_CZ_RT5645_MACH
>> +tristate "AMD CZ support for RT5645"
> 
> Missing blank line between the stanzas.

Done. Will push the change in next revision.

> 
>> +select SND_SOC_RT5645
>> +select SND_SOC_AMD_ACP
>> +depends on I2C_DESIGNWARE_PLATFORM
> 
> No system dependencies of any kind?  Looking at this I'd expect at least
> CONFIG_ACPI || COMPILE_TEST.  It's also unclear to me how the DesignWare
> device is going to be instantiated here or if that should be a direct
> depenency at all here.
>

Added I2C for system dependency and removed 
I2C_DESIGNWARE_PLATFORM as dependency

>> +ret = snd_soc_register_card(card);
> 
> devm_snd_soc_register_card() and then you don't need the remove
> function.
>

Done.

>> +static const struct acpi_device_id cz_audio_acpi_match[] = {
>> +{ "AMDI1002", 0 },
>> +{},
>> +};
>> +
> 
> Missing MODULE_DEVICE_TABLE().
>

Done.


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel