Re: [PATCH 1/4] ASoC: Intel: Haswell: Adjust machine device private context

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2019-08-22 16:07, Pierre-Louis Bossart wrote:
> On 8/22/19 6:36 AM, Cezary Rojewski wrote:
>> Apart from Haswell machines, all other devices have their private data
>> set to snd_soc_acpi_mach instance.
>>
>> Changes for HSW/ BDW boards introduced with series:
>> https://patchwork.kernel.org/cover/10782035/
>>
>> added support for dai_link platform_name adjustments within card probe
>> routines. These take for granted private_data points to
>> snd_soc_acpi_mach whereas for Haswell, it's sst_pdata instead. Change
>> private context of platform_device - representing machine board - to
>> address this.
> 
> Cezary, see the comments of the initial series:
> 
> "Note that byt-max98080, byt-rt5640 were not modified since they are
> deprecated. bytcht-nocodec and the Skylake/Kabylake machine drivers
> changes were not changed since SOF does not support them. There may be
> additional changes if and when Skylake/Kabylake are supported by SOF
> (largely a firmware authentication issue, not technical difficulty)."
> 
> I intentionally did not touch the Haswell and Baytrail legacy since both 
> drivers do not update the platform name, this is only done for cases 
> where SOF is used.
> 
> So while I don't mind a change, it's got to come with tests for each 
> variant, and if you do the changes for Haswell then you want to change 
> Baytrail legacy machine drivers as well. And are we going to change the 
> SKL/KBL machine drivers to allow for this platform name rewrite?
> 
> Also the information below is misleading: nothing is broken in the 
> current solution and -stable kernels do not need to pick this patchset. 
> This is a code alignment and the behavior is identical.
> 
> Or as an alternative we leave the code as is...
> 

Guess I wasn't clear enough:
- this code fixes panic generated by series found under link above.

Following code added within machine probe for broadwell.c:
	/* override plaform name, if required */
	mach = (&pdev->dev)->platform_data;
	pdata = (&pdev->dev)->platform_data;
	if (mach) /* extra check since legacy does not pass parameters */ {
		platform_name = mach->mach_params.platform;
		dev_warn(&pdev->dev, "Broadwell platform_name: %s, %s, %s, %s\n", 
mach->id, mach->drv_name, mach->fw_filename, platform_name);
		dev_warn(&pdev->dev, "Broadwell id and res_idx: %x, %d\n", pdata->id, 
pdata->resindex_dma_base);
	}


Generates:

[   25.982151] broadwell-audio broadwell-audio: Broadwell platform_name: 
, (null), (efault), (null)
[   25.982157] broadwell-audio broadwell-audio: Broadwell id and 
res_idx: 3438, 1040384


Conslusion:
0x3438 == BDW_ID
1040384 -> 0x0FE000 -> WPT_DSP_DMA_ADDR_OFFSET
confirms the claim.

As stated, during cleanups and moving stuff around, code you've added 
generates panics. Right now it works only because of offsets of 
miscasted object pointing to uninitialized variable (luckily).
platform_name is initialized as NULL for all SKL+ and legacy platforms 
and thus the snd_soc_fixup_dai_links_platform_name returns immediately. 
So by all means, change is not only save, but required.

Code is being tested on 2x BDW-Y which live now a happy live in our lab 
with the rest of the platforms.

Czarek


>> Fixes: e87055d732e3 ("ASoC: Intel: haswell: platform name fixup support")
>> Fixes: 7e40ddcf974a ("ASoC: Intel: bdw-rt5677: platform name fixup 
>> support")
>> Fixes: 2d067b2807f9 ("ASoC: Intel: broadwell: platform name fixup 
>> support")
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/soc/intel/common/sst-acpi.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/intel/common/sst-acpi.c 
>> b/sound/soc/intel/common/sst-acpi.c
>> index 15f2b27e643f..c34f628c7987 100644
>> --- a/sound/soc/intel/common/sst-acpi.c
>> +++ b/sound/soc/intel/common/sst-acpi.c
>> @@ -109,11 +109,12 @@ int sst_acpi_probe(struct platform_device *pdev)
>>       }
>>       platform_set_drvdata(pdev, sst_acpi);
>> +    mach->pdata = sst_pdata;
>>       /* register machine driver */
>>       sst_acpi->pdev_mach =
>>           platform_device_register_data(dev, mach->drv_name, -1,
>> -                          sst_pdata, sizeof(*sst_pdata));
>> +                          mach, sizeof(*mach));
>>       if (IS_ERR(sst_acpi->pdev_mach))
>>           return PTR_ERR(sst_acpi->pdev_mach);
>>
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel