| Message ID | 20200305145314.32579-5-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9e6c382f5a6161eb55115fb56614b9827f2e7da3 |
| Headers | show |
| Series | ASoC: Intel: Skylake: Fix HDaudio and Dmic | expand |
> - intel_nhlt_free(skl->nhlt); > + if (skl->nhlt) > + intel_nhlt_free(skl->nhlt); we could alternatively move the test in intel_nhlt_free, which seems like a more robust thing to do?
On 2020-03-06 22:03, Pierre-Louis Bossart wrote: > > >> - intel_nhlt_free(skl->nhlt); >> + if (skl->nhlt) >> + intel_nhlt_free(skl->nhlt); > > we could alternatively move the test in intel_nhlt_free, which seems > like a more robust thing to do? Depends. In general kernel-internal API trusts its caller and appending 'ifs' everywhere would unnecessarily slow entire kernel down. While intel_nhlt_free is called rarely, I'd still argue caller should be sane about its invocation. 'if' in skl_probe could be avoided had the function's structure been better. 'if' in skl_remove is just fine, though. Let's leave it as is. Czarek
On 3/9/20 8:03 AM, Cezary Rojewski wrote: > On 2020-03-06 22:03, Pierre-Louis Bossart wrote: >> >> >>> - intel_nhlt_free(skl->nhlt); >>> + if (skl->nhlt) >>> + intel_nhlt_free(skl->nhlt); >> >> we could alternatively move the test in intel_nhlt_free, which seems >> like a more robust thing to do? > > Depends. In general kernel-internal API trusts its caller and appending > 'ifs' everywhere would unnecessarily slow entire kernel down. While > intel_nhlt_free is called rarely, I'd still argue caller should be sane > about its invocation. > > 'if' in skl_probe could be avoided had the function's structure been > better. 'if' in skl_remove is just fine, though. > > Let's leave it as is. it's also used in SOF: sound/soc/sof/intel/hda.c: intel_nhlt_free(nhlt); that's why I suggested to factor the test so that both users don't need to add the if.
On 2020-03-09 18:01, Pierre-Louis Bossart wrote: > On 3/9/20 8:03 AM, Cezary Rojewski wrote: >> On 2020-03-06 22:03, Pierre-Louis Bossart wrote: >>> >>> >>>> - intel_nhlt_free(skl->nhlt); >>>> + if (skl->nhlt) >>>> + intel_nhlt_free(skl->nhlt); >>> >>> we could alternatively move the test in intel_nhlt_free, which seems >>> like a more robust thing to do? >> >> Depends. In general kernel-internal API trusts its caller and >> appending 'ifs' everywhere would unnecessarily slow entire kernel >> down. While intel_nhlt_free is called rarely, I'd still argue caller >> should be sane about its invocation. >> >> 'if' in skl_probe could be avoided had the function's structure been >> better. 'if' in skl_remove is just fine, though. >> >> Let's leave it as is. > > it's also used in SOF: > > sound/soc/sof/intel/hda.c: intel_nhlt_free(nhlt); > > that's why I suggested to factor the test so that both users don't need > to add the if. I understand, and my explanation still applies. SOF's intel_nhlt_free usage is great example actually. Caller is sane about its doings as it should be. Internal API needs not to suffer from callers irresponsibility. PCM does not call ::free() if ::open() fails, same as device-driver model does not care about ::remove() if ::probe() fails. Czarek
On 3/9/20 12:38 PM, Cezary Rojewski wrote: > On 2020-03-09 18:01, Pierre-Louis Bossart wrote: >> On 3/9/20 8:03 AM, Cezary Rojewski wrote: >>> On 2020-03-06 22:03, Pierre-Louis Bossart wrote: >>>> >>>> >>>>> - intel_nhlt_free(skl->nhlt); >>>>> + if (skl->nhlt) >>>>> + intel_nhlt_free(skl->nhlt); >>>> >>>> we could alternatively move the test in intel_nhlt_free, which seems >>>> like a more robust thing to do? >>> >>> Depends. In general kernel-internal API trusts its caller and >>> appending 'ifs' everywhere would unnecessarily slow entire kernel >>> down. While intel_nhlt_free is called rarely, I'd still argue caller >>> should be sane about its invocation. >>> >>> 'if' in skl_probe could be avoided had the function's structure been >>> better. 'if' in skl_remove is just fine, though. >>> >>> Let's leave it as is. >> >> it's also used in SOF: >> >> sound/soc/sof/intel/hda.c: intel_nhlt_free(nhlt); >> >> that's why I suggested to factor the test so that both users don't >> need to add the if. > > I understand, and my explanation still applies. > > SOF's intel_nhlt_free usage is great example actually. Caller is sane > about its doings as it should be. Internal API needs not to suffer from > callers irresponsibility. ok, indeed looking at the code there's no need for the test.
diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c index 19f328d71f24..d9c8f5cb389e 100644 --- a/sound/soc/intel/skylake/skl-nhlt.c +++ b/sound/soc/intel/skylake/skl-nhlt.c @@ -182,7 +182,8 @@ void skl_nhlt_remove_sysfs(struct skl_dev *skl) { struct device *dev = &skl->pci->dev; - sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr); + if (skl->nhlt) + sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr); } /* diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index e2e531c96dd1..7ad8a75759bd 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -633,6 +633,9 @@ static int skl_clock_device_register(struct skl_dev *skl) struct platform_device_info pdevinfo = {NULL}; struct skl_clk_pdata *clk_pdata; + if (!skl->nhlt) + return 0; + clk_pdata = devm_kzalloc(&skl->pci->dev, sizeof(*clk_pdata), GFP_KERNEL); if (!clk_pdata) @@ -1074,7 +1077,8 @@ static int skl_probe(struct pci_dev *pci, out_clk_free: skl_clock_device_unregister(skl); out_nhlt_free: - intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt); out_free: skl_free(bus); @@ -1123,7 +1127,8 @@ static void skl_remove(struct pci_dev *pci) skl_dmic_device_unregister(skl); skl_clock_device_unregister(skl); skl_nhlt_remove_sysfs(skl); - intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt); skl_free(bus); dev_set_drvdata(&pci->dev, NULL); }
Some configurations expose no NHLT table at all within their /sys/firmware/acpi/tables. To prevent NULL-dereference errors from occurring, adjust probe flow and append additional safety checks in functions involved in NHLT lifecycle. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/skylake/skl-nhlt.c | 3 ++- sound/soc/intel/skylake/skl.c | 9 +++++++-- 2 files changed, 9 insertions(+), 3 deletions(-)