Re: [PATCH] drm/amdgpu: fix i2s_pdata out of bound array access

[Mukunda,Vijendar]

On 7/27/22 8:25 PM, Alex Deucher wrote:
> On Wed, Jul 27, 2022 at 10:42 AM Vijendar Mukunda
>  wrote:
>>
>> Fixed following Smatch static checker warning:
>>
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:393 acp_hw_init()
>> error: buffer overflow 'i2s_pdata' 3 <= 3
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:396 acp_hw_init()
>> error: buffer overflow 'i2s_pdata' 3 <= 3
>>
>> Reported-by: Dan Carpenter 
>> Signed-off-by: Vijendar Mukunda 
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 8 
>>  1 file changed, 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> index bcc7ee02e0fc..6d72355ac492 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> @@ -390,14 +390,6 @@ static int acp_hw_init(void *handle)
>> i2s_pdata[2].i2s_reg_comp1 = ACP_BT_COMP1_REG_OFFSET;
>> i2s_pdata[2].i2s_reg_comp2 = ACP_BT_COMP2_REG_OFFSET;
>>
>> -   i2s_pdata[3].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET;
>> -   switch (adev->asic_type) {
>> -   case CHIP_STONEY:
>> -   i2s_pdata[3].quirks |= 
>> DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
>> -   break;
>> -   default:
>> -   break;
>> -   }
> 
> Is this actually not used or should we just increase the allocation size?
> 
> Alex
it's my bad. i2s_pdata array size is 3. when we recently included code
changes for JD platform , this piece of code was added mistakenly for
Stoney platform switch case.

--
Vijendar

> 
>> adev->acp.acp_res[0].name = "acp2x_dma";
>> adev->acp.acp_res[0].flags = IORESOURCE_MEM;
>> adev->acp.acp_res[0].start = acp_base;
>> --
>> 2.25.1
>>

Re: [PATCH RESEND V2 1/3] drm/amdgpu: fix checkpatch warnings

[Mukunda,Vijendar]

On 7/5/22 4:51 PM, Christian König wrote:
> 
> 
> Am 04.07.22 um 15:54 schrieb Vijendar Mukunda:
>> From: vijendar 
>>
>> Fixed below checkpatch warnings and errors
>>
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:131: CHECK: Comparison to NULL
>> could be written "apd"
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:150: CHECK: Comparison to NULL
>> could be written "apd"
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:196: CHECK: Prefer kernel type
>> 'u64' over 'uint64_t'
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:224: CHECK: Please don't use
>> multiple blank lines
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:226: CHECK: Comparison to NULL
>> could be written "!adev->acp.acp_genpd"
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:233: CHECK: Please don't use
>> multiple blank lines
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:239: CHECK: Alignment should
>> match open parenthesis
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:241: CHECK: Comparison to NULL
>> could be written "!adev->acp.acp_cell"
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:247: CHECK: Comparison to NULL
>> could be written "!adev->acp.acp_res"
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:253: CHECK: Comparison to NULL
>> could be written "!i2s_pdata"
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:350: CHECK: Alignment should
>> match open parenthesis
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c:550: ERROR: that open brace {
>> should be on the previous line
>>
>> Signed-off-by: Vijendar Mukunda 
>> Reviewed-by: Alex Deucher 
>>
>> changes since v1:
>>  Modified commit label as drm/amdgpu
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 27 +
>>   1 file changed, 10 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> index cc9c9f8b23b2..ba1605ff521f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>> @@ -128,7 +128,7 @@ static int acp_poweroff(struct generic_pm_domain
>> *genpd)
>>   struct amdgpu_device *adev;
>>     apd = container_of(genpd, struct acp_pm_domain, gpd);
>> -    if (apd != NULL) {
>> +    if (apd) {
> 
> Well that's still not correct. The variable apd is compute by an upcast
> of the genpd parameter.
> 
> Since that upcast never returns a NULL value (gpd is not the first
> member of the structure) this whole check is completely nonsense.
> 
> I strongly suggest to just remove most of those NULL checks.

Will remove Null checks and post the new patch.

-
Vijendar
> 
> Only the ones directly after memory allocation really make sense.
> 
> Regards,
> Christian.
> 
>>   adev = apd->adev;
>>   /* call smu to POWER GATE ACP block
>>    * smu will
>> @@ -147,7 +147,7 @@ static int acp_poweron(struct generic_pm_domain
>> *genpd)
>>   struct amdgpu_device *adev;
>>     apd = container_of(genpd, struct acp_pm_domain, gpd);
>> -    if (apd != NULL) {
>> +    if (apd) {
>>   adev = apd->adev;
>>   /* call smu to UNGATE ACP block
>>    * smu will
>> @@ -193,7 +193,7 @@ static int acp_genpd_remove_device(struct device
>> *dev, void *data)
>>   static int acp_hw_init(void *handle)
>>   {
>>   int r;
>> -    uint64_t acp_base;
>> +    u64 acp_base;
>>   u32 val = 0;
>>   u32 count = 0;
>>   struct i2s_platform_data *i2s_pdata = NULL;
>> @@ -220,37 +220,32 @@ static int acp_hw_init(void *handle)
>>   return -EINVAL;
>>     acp_base = adev->rmmio_base;
>> -
>> -
>>   adev->acp.acp_genpd = kzalloc(sizeof(struct acp_pm_domain),
>> GFP_KERNEL);
>> -    if (adev->acp.acp_genpd == NULL)
>> +    if (!adev->acp.acp_genpd)
>>   return -ENOMEM;
>>     adev->acp.acp_genpd->gpd.name = "ACP_AUDIO";
>>   adev->acp.acp_genpd->gpd.power_off = acp_poweroff;
>>   adev->acp.acp_genpd->gpd.power_on = acp_poweron;
>> -
>> -
>>   adev->acp.acp_genpd->adev = adev;
>>     pm_genpd_init(>acp.acp_genpd->gpd, NULL, false);
>>   -    adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell),
>> -    GFP_KERNEL);
>> +    adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell),
>> GFP_KERNEL);
>>   -    if (adev->acp.acp_cell == NULL) {
>> +    if (!adev->acp.acp_cell) {
>>   r = -ENOMEM;
>>   goto failure;
>>   }
>>     adev->acp.acp_res = kcalloc(5, sizeof(struct resource),
>> GFP_KERNEL);
>> -    if (adev->acp.acp_res == NULL) {
>> +    if (!adev->acp.acp_res) {
>>   r = -ENOMEM;
>>   goto failure;
>>   }
>>     i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data),
>> GFP_KERNEL);
>> -    if (i2s_pdata == NULL) {
>> +    if (!i2s_pdata) {
>>   r = -ENOMEM;
>>   goto failure;
>>   }
>> @@ -346,8 +341,7 @@ static int acp_hw_init(void *handle)
>>   adev->acp.acp_cell[3].platform_data = _pdata[2];
>>   adev->acp.acp_cell[3].pdata_size = sizeof(struct
>> i2s_platform_data);
>>   -    r = 

Re: [PATCH V2 0/5] I2S driver changes for Jadeite platform

[Mukunda,Vijendar]

On 7/1/22 5:21 PM, Mark Brown wrote:
> On Fri, Jul 01, 2022 at 05:11:02PM +0530, Vijendar Mukunda wrote:
> 
>> This patch set depends on:
>> --checkpatch warnings patch
>>  
>> --https://patchwork.kernel.org/project/alsa-devel/patch/20220627125834.481731-1-vijendar.muku...@amd.com/
> 
> That's "drm: amd: amdgpu: fix checkpatch warnings", but it looks like
> there's no build time dependencies for patch 3 so I could go ahead with
> it and the DRM changes go through the DRM tree I think?
> 
correct. First two patches including dependent patch should through DRM
tree. There is no build dependencies for AsoC tree patches. From patch 3
on wards, it can be picked for upstream review for ASoC tree.

It's my bad. Patch series should be split in to two. one should go
through ASoC tree and another one through DRM tree.

> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.
I agree. will take care about patch commit descriptions.

Re: [PATCH 4/5] ASoC: amd: add Machine driver for Jadeite platform

[Mukunda,Vijendar]

On 6/30/22 4:40 PM, Mark Brown wrote:
> On Thu, Jun 30, 2022 at 08:47:54AM +0530, Vijendar Mukunda wrote:
> 
>> +static int st_es8336_hw_params(struct snd_pcm_substream *substream,
>> +   struct snd_pcm_hw_params *params)
>> +{
>> +int ret = 0;
>> +struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
>> +struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>> +
>> +ret = snd_soc_dai_set_sysclk(codec_dai, 0, params_rate(params) * 256, 
>> SND_SOC_CLOCK_IN);
>> +if (ret < 0) {
>> +dev_err(rtd->dev, "can't set codec sysclk: %d\n", ret);
>> +return ret;
>> +}
>> +return ret;
>> +}
> 
>> +static const unsigned int st_channels[] = {
>> +DUAL_CHANNEL,
>> +};
>> +
>> +static const unsigned int st_rates[] = {
>> +48000,
>> +};
> 
> If the clock rate is fixed why not just set the sysclk once at startup
> too?
Yes. We can set sysclk once at the startup as clock rate is fixed.
Will modify the code and post the new patch.

Re: [PATCH 5/5] ASoC: amd: enable machine driver build for Jadeite platform

[Mukunda,Vijendar]

On 6/30/22 4:41 PM, Mark Brown wrote:
> On Thu, Jun 30, 2022 at 08:47:55AM +0530, Vijendar Mukunda wrote:
> 
>> +depends on SND_SOC_AMD_ACP && I2C && ACPI
> 
> The code treated ACPI as optional so you could relax the ACPI dependency
> ot be "ACPI || COMPILE_TEST" (I think the same applies to I2C).

Will fix it and push the newer version.

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

[Mukunda,Vijendar]

On Tuesday 03 July 2018 09:50 PM, Alex Deucher wrote:

On Mon, Jul 2, 2018 at 5:48 PM, Daniel Kurtz  wrote:

Hi Alex,

On Sun, Apr 15, 2018 at 9:48 PM Agrawal, Akshu  wrote:




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 



Was this patch 

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

[Mukunda,Vijendar]

Removing URL links and commit-ready description in v2.


On Monday 06 November 2017 09:18 PM, Mark Brown wrote:

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


Signed-off-by: Vijendar Mukunda 
Signed-off-by: Akshu Agrawal 
Reviewed-on: https://chromium-review.googlesource.com/659699
Commit-Ready: Akshu Agrawal 
Tested-by: Akshu Agrawal 
Reviewed-by: Jason Clinton 
Reviewed-on: https://chromium-review.googlesource.com/676627

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


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

RE: [PATCH 4/8] ASoC: AMD: added condition checks for CZ specific code

[Mukunda, Vijendar]

-Original Message-
From: Mark Brown [mailto:broo...@kernel.org] 
Sent: Wednesday, June 28, 2017 11:36 PM
To: Alex Deucher
Cc: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; 
alsa-de...@alsa-project.org; airl...@gmail.com; Mukunda, Vijendar; 
rajeevkumar.li...@gmail.com; lgirdw...@gmail.com; ti...@suse.de; 
pe...@perex.cz; Deucher, Alexander
Subject: Re: [PATCH 4/8] ASoC: AMD: added condition checks for CZ specific code

On Fri, Jun 23, 2017 at 12:35:02PM -0400, Alex Deucher wrote:

> Reviewed-by: Alex Deucher <alexander.deuc...@amd.com>


> index dcbf997..e48ae5d 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -34,6 +34,8 @@
>  
>  #define MAX_BUFFER (PLAYBACK_MAX_PERIOD_SIZE * 
> PLAYBACK_MAX_NUM_PERIODS)  #define MIN_BUFFER MAX_BUFFER
> +#define CHIP_STONEY 14
> +#define CHIP_CARRIZO 13

>These defines are being added in the middle of a file but CHIP_STONEY is also 
>used in another file in the previous patch (and apparently extensively 
>throughout the DRM driver already).  This is obviously not good, >we shouldn't 
>have multiple copies of the definition.

> - } else {
> + if (adata->asic_type == CHIP_CARRIZO) {
> + for (bank = 1; bank <= 4; bank++)
> + acp_set_sram_bank_state(adata->acp_mmio, bank,
> + false);

> I'm not seeing any poweroff cases for other chips being added, and again 
> switch statements please.

We will modify code to use single definition for CHIP_STONEY and CHIP_CARRIZO.
There are only two chip sets based on ACP 2.x design(Carrizo and Stoney).
Our future Chip sets going to use different design based on next ACP IP version.

In the current patch, Condition checks added for Carrizo for setting SRAM BANK 
state.
Memory Gating is disabled in Stoney,i.e SRAM Bank's won't be turned off. The 
default state for SRAM banks is ON.
As Memory Gating is disabled, there is no need to add condition checks for 
Stoney to set SRAM Bank state.

Apart from Memory Gating, there are  few more differences between Stoney and 
Carrizo chip sets.
Stoney specific DMA driver changes submitted in a separate patch.

Vijendar



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