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);