| Message ID | 1479566911-5580-3-git-send-email-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/19/2016 03:48 PM, Krzysztof Kozlowski wrote: [...] > @@ -206,15 +204,10 @@ static int __init smdk_audio_init(void) > int ret; > char *str; > > - if (machine_is_smdkc100() > - || machine_is_smdkv210() || machine_is_smdkc110()) { > - smdk.num_links = 3; > - } else if (machine_is_smdk6410()) { > - str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; > - str[strlen(str) - 1] = '2'; > - str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; > - str[strlen(str) - 1] = '2'; > - } > + str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; > + str[strlen(str) - 1] = '2'; > + str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; > + str[strlen(str) - 1] = '2'; This could be further simplified by just updating the initial cpu_dai_name string in the dai_link struct. Especially considering that the cpu_dai_name is a string literal and the ARM kernel now has rodata write protection enabled by default, so modifying it will crash the kernel.
On 11/19/2016 04:42 PM, Lars-Peter Clausen wrote: > On 11/19/2016 03:48 PM, Krzysztof Kozlowski wrote: > [...] >> @@ -206,15 +204,10 @@ static int __init smdk_audio_init(void) >> int ret; >> char *str; >> >> - if (machine_is_smdkc100() >> - || machine_is_smdkv210() || machine_is_smdkc110()) { >> - smdk.num_links = 3; >> - } else if (machine_is_smdk6410()) { >> - str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; >> - str[strlen(str) - 1] = '2'; >> - str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; >> - str[strlen(str) - 1] = '2'; >> - } >> + str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; >> + str[strlen(str) - 1] = '2'; >> + str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; >> + str[strlen(str) - 1] = '2'; > > This could be further simplified by just updating the initial cpu_dai_name > string in the dai_link struct. > > Especially considering that the cpu_dai_name is a string literal and the ARM > kernel now has rodata write protection enabled by default, so modifying it > will crash the kernel. Spoke too soon, you fix this up in the next patch. But I'd just squash that change into this patch. I think it is pretty safe to assume that it is correct.
On 11/19/2016 04:45 PM, Lars-Peter Clausen wrote: > On 11/19/2016 04:42 PM, Lars-Peter Clausen wrote: >> On 11/19/2016 03:48 PM, Krzysztof Kozlowski wrote: >> [...] >>> @@ -206,15 +204,10 @@ static int __init smdk_audio_init(void) >>> int ret; >>> char *str; >>> >>> - if (machine_is_smdkc100() >>> - || machine_is_smdkv210() || machine_is_smdkc110()) { >>> - smdk.num_links = 3; >>> - } else if (machine_is_smdk6410()) { >>> - str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; >>> - str[strlen(str) - 1] = '2'; >>> - str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; >>> - str[strlen(str) - 1] = '2'; >>> - } >>> + str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; >>> + str[strlen(str) - 1] = '2'; >>> + str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; >>> + str[strlen(str) - 1] = '2'; >> >> This could be further simplified by just updating the initial cpu_dai_name >> string in the dai_link struct. >> >> Especially considering that the cpu_dai_name is a string literal and the ARM >> kernel now has rodata write protection enabled by default, so modifying it >> will crash the kernel. > > Spoke too soon, you fix this up in the next patch. But I'd just squash that > change into this patch. I think it is pretty safe to assume that it is correct. > And another thing. Since num_links is always 2 now the last entry from the smdk_dai array can be removed and num_links can be initialized using ARRAY_SIZE().
On Sat, Nov 19, 2016 at 04:48:26PM +0100, Lars-Peter Clausen wrote: > On 11/19/2016 04:45 PM, Lars-Peter Clausen wrote: > > On 11/19/2016 04:42 PM, Lars-Peter Clausen wrote: > >> On 11/19/2016 03:48 PM, Krzysztof Kozlowski wrote: > >> [...] > >>> @@ -206,15 +204,10 @@ static int __init smdk_audio_init(void) > >>> int ret; > >>> char *str; > >>> > >>> - if (machine_is_smdkc100() > >>> - || machine_is_smdkv210() || machine_is_smdkc110()) { > >>> - smdk.num_links = 3; > >>> - } else if (machine_is_smdk6410()) { > >>> - str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; > >>> - str[strlen(str) - 1] = '2'; > >>> - str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; > >>> - str[strlen(str) - 1] = '2'; > >>> - } > >>> + str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; > >>> + str[strlen(str) - 1] = '2'; > >>> + str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; > >>> + str[strlen(str) - 1] = '2'; > >> > >> This could be further simplified by just updating the initial cpu_dai_name > >> string in the dai_link struct. > >> > >> Especially considering that the cpu_dai_name is a string literal and the ARM > >> kernel now has rodata write protection enabled by default, so modifying it > >> will crash the kernel. > > > > Spoke too soon, you fix this up in the next patch. But I'd just squash that > > change into this patch. I think it is pretty safe to assume that it is correct. Yes, I wanted to split trivial change from something which would be nice to test (I did not test it). However you're right that logically this is the same change. > And another thing. Since num_links is always 2 now the last entry from the > smdk_dai array can be removed and num_links can be initialized using > ARRAY_SIZE(). Ahh, indeed. The third DAI link (SEC_PLAYBACK) could be removed now. Thanks for feedback, Krzysztof
diff --git a/sound/soc/samsung/smdk_wm8580.c b/sound/soc/samsung/smdk_wm8580.c index 548bfd993788..59fd3b8fd414 100644 --- a/sound/soc/samsung/smdk_wm8580.c +++ b/sound/soc/samsung/smdk_wm8580.c @@ -14,8 +14,6 @@ #include <sound/soc.h> #include <sound/pcm_params.h> -#include <asm/mach-types.h> - #include "../codecs/wm8580.h" #include "i2s.h" @@ -206,15 +204,10 @@ static int __init smdk_audio_init(void) int ret; char *str; - if (machine_is_smdkc100() - || machine_is_smdkv210() || machine_is_smdkc110()) { - smdk.num_links = 3; - } else if (machine_is_smdk6410()) { - str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; - str[strlen(str) - 1] = '2'; - str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; - str[strlen(str) - 1] = '2'; - } + str = (char *)smdk_dai[PRI_PLAYBACK].cpu_dai_name; + str[strlen(str) - 1] = '2'; + str = (char *)smdk_dai[PRI_CAPTURE].cpu_dai_name; + str[strlen(str) - 1] = '2'; smdk_snd_device = platform_device_alloc("soc-audio", -1); if (!smdk_snd_device)
MACH_SMDKC100, MACH_SMDKV210 and MACH_SMDKC110 are no longer supported so drop the dead code. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- sound/soc/samsung/smdk_wm8580.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)