| Message ID | 1522229918-4748-9-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mar 28 2018 18:38, Vinod Koul wrote: > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index c0edac80df34..690e56a35237 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -2882,6 +2882,26 @@ int snd_soc_dai_set_tdm_slot(struct snd_soc_dai *dai, > EXPORT_SYMBOL_GPL(snd_soc_dai_set_tdm_slot); > > /** > + * snd_soc_dai_set_sdw_stream() - Configures a DAI for SDW stream operation > + * @dai: DAI > + * @stream: STREAM > + * @direction: Stream direction(Playback/Capture) > + * SoundWire subsystem doesn't have a notion of direction and we reuse > + * the ASoC stream direction to configure sink/source ports. > + * > + * Returns 0 on success, a negative error code otherwise. > + */ > +int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai, > + void *stream, int direction) > +{ > + if (dai->driver->ops->set_sdw_stream) > + return dai->driver->ops->set_sdw_stream(dai, stream, direction); > + else > + return -ENOTSUPP; > +} > +EXPORT_SYMBOL_GPL(snd_soc_dai_set_sdw_stream); It's better for this kind of code to be incline function in any header. In general, new symbols increase maintenance cost of binary of kernel-related stuffs. It's preferable to avoid the addition if possible, IMO. Regards Takashi Sakamoto
On Fri, Mar 30, 2018 at 12:05:00PM +0900, Takashi Sakamoto wrote: > Hi, > > On Mar 28 2018 18:38, Vinod Koul wrote: > > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > > index c0edac80df34..690e56a35237 100644 > > --- a/sound/soc/soc-core.c > > +++ b/sound/soc/soc-core.c > > @@ -2882,6 +2882,26 @@ int snd_soc_dai_set_tdm_slot(struct snd_soc_dai *dai, > > EXPORT_SYMBOL_GPL(snd_soc_dai_set_tdm_slot); > > /** > > + * snd_soc_dai_set_sdw_stream() - Configures a DAI for SDW stream operation > > + * @dai: DAI > > + * @stream: STREAM > > + * @direction: Stream direction(Playback/Capture) > > + * SoundWire subsystem doesn't have a notion of direction and we reuse > > + * the ASoC stream direction to configure sink/source ports. > > + * > > + * Returns 0 on success, a negative error code otherwise. > > + */ > > +int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai, > > + void *stream, int direction) > > +{ > > + if (dai->driver->ops->set_sdw_stream) > > + return dai->driver->ops->set_sdw_stream(dai, stream, direction); > > + else > > + return -ENOTSUPP; > > +} > > +EXPORT_SYMBOL_GPL(snd_soc_dai_set_sdw_stream); > > It's better for this kind of code to be incline function in any header. In > general, new symbols increase maintenance cost of binary of kernel-related > stuffs. It's preferable to avoid the addition if possible, IMO. I don't understand, functionally it's the same, there should not be any increased maintenance either way. Please explain how this makes things "harder"? thanks, greg k-h
Hi Greg, I'm sorry for delay but I had a short trip. On Mar 30 2018 15:03, Greg KH wrote: > On Fri, Mar 30, 2018 at 12:05:00PM +0900, Takashi Sakamoto wrote: >> Hi, >> >> On Mar 28 2018 18:38, Vinod Koul wrote: >>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c >>> index c0edac80df34..690e56a35237 100644 >>> --- a/sound/soc/soc-core.c >>> +++ b/sound/soc/soc-core.c >>> @@ -2882,6 +2882,26 @@ int snd_soc_dai_set_tdm_slot(struct snd_soc_dai *dai, >>> EXPORT_SYMBOL_GPL(snd_soc_dai_set_tdm_slot); >>> /** >>> + * snd_soc_dai_set_sdw_stream() - Configures a DAI for SDW stream operation >>> + * @dai: DAI >>> + * @stream: STREAM >>> + * @direction: Stream direction(Playback/Capture) >>> + * SoundWire subsystem doesn't have a notion of direction and we reuse >>> + * the ASoC stream direction to configure sink/source ports. >>> + * >>> + * Returns 0 on success, a negative error code otherwise. >>> + */ >>> +int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai, >>> + void *stream, int direction) >>> +{ >>> + if (dai->driver->ops->set_sdw_stream) >>> + return dai->driver->ops->set_sdw_stream(dai, stream, direction); >>> + else >>> + return -ENOTSUPP; >>> +} >>> +EXPORT_SYMBOL_GPL(snd_soc_dai_set_sdw_stream); >> >> It's better for this kind of code to be incline function in any header. In >> general, new symbols increase maintenance cost of binary of kernel-related >> stuffs. It's preferable to avoid the addition if possible, IMO. > > I don't understand, functionally it's the same, there should not be any > increased maintenance either way. Please explain how this makes things > "harder"? Hm, if so it might be my misunderstanding to reasons for typical usage of inline functions in kernel source, sorry. In my understanding, exported symbols are put into some sections of ELF binary. Addition of new symbols increases size of the section. Additionally, after linking vmlinux, kbuild scans built-in symbols and make a file with entries of them. The addition increases time of this step. Furthermore, at the end of building kernel, kmod is called to generate some map files for exported symbols in loadable module. In a view of distributors, these files are maintained by binary packages of any type carefully because some incompatibilities can be delivered such as version mismatch. For these reasons, I think thing goes harder when people carelessly add new symbols for functions with a few codes; e.g. accessing to a member of structure, then simply check an return it. Actually I can see some examples in upstreamed headers. Thanks Takashi Sakamoto
On Mon, 02 Apr 2018 08:26:35 +0200, Takashi Sakamoto wrote: > > Hi Greg, > > I'm sorry for delay but I had a short trip. > > On Mar 30 2018 15:03, Greg KH wrote: > > On Fri, Mar 30, 2018 at 12:05:00PM +0900, Takashi Sakamoto wrote: > >> Hi, > >> > >> On Mar 28 2018 18:38, Vinod Koul wrote: > >>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > >>> index c0edac80df34..690e56a35237 100644 > >>> --- a/sound/soc/soc-core.c > >>> +++ b/sound/soc/soc-core.c > >>> @@ -2882,6 +2882,26 @@ int snd_soc_dai_set_tdm_slot(struct snd_soc_dai *dai, > >>> EXPORT_SYMBOL_GPL(snd_soc_dai_set_tdm_slot); > >>> /** > >>> + * snd_soc_dai_set_sdw_stream() - Configures a DAI for SDW stream operation > >>> + * @dai: DAI > >>> + * @stream: STREAM > >>> + * @direction: Stream direction(Playback/Capture) > >>> + * SoundWire subsystem doesn't have a notion of direction and we reuse > >>> + * the ASoC stream direction to configure sink/source ports. > >>> + * > >>> + * Returns 0 on success, a negative error code otherwise. > >>> + */ > >>> +int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai, > >>> + void *stream, int direction) > >>> +{ > >>> + if (dai->driver->ops->set_sdw_stream) > >>> + return dai->driver->ops->set_sdw_stream(dai, stream, direction); > >>> + else > >>> + return -ENOTSUPP; > >>> +} > >>> +EXPORT_SYMBOL_GPL(snd_soc_dai_set_sdw_stream); > >> > >> It's better for this kind of code to be incline function in any header. In > >> general, new symbols increase maintenance cost of binary of kernel-related > >> stuffs. It's preferable to avoid the addition if possible, IMO. > > > > I don't understand, functionally it's the same, there should not be any > > increased maintenance either way. Please explain how this makes things > > "harder"? > > Hm, if so it might be my misunderstanding to reasons for typical usage > of inline functions in kernel source, sorry. > > In my understanding, exported symbols are put into some sections of > ELF binary. Addition of new symbols increases size of the > section. Additionally, after linking vmlinux, kbuild scans built-in > symbols and make a file with entries of them. The addition increases > time of this step. Furthermore, at the end of building kernel, kmod is > called to generate some map files for exported symbols in loadable > module. In a view of distributors, these files are maintained by > binary packages of any type carefully because some incompatibilities > can be delivered such as version mismatch. > > For these reasons, I think thing goes harder when people carelessly > add new symbols for functions with a few codes; e.g. accessing to a > member of structure, then simply check an return it. Actually I can > see some examples in upstreamed headers. The advantage of inline function isn't about the maintenance cost. It's mostly for performance, as well as the binary size reduction. Actually, when a kernel live-patching comes into play, an inline function is worse from the maintenance POV. Then we'd have to patch every place that is expanded instead of a single place. However, this doesn't discourage the use of inline function, either. Overall, the performance is still the most important factor for major cases. So I agree with that this particular case can be well inlined, supposing that no complex code is planned to be added in future. thanks, Takashi
Hi, On Apr 3 2018 21:03, Takashi Iwai wrote: >>>> It's better for this kind of code to be incline function in any header. In >>>> general, new symbols increase maintenance cost of binary of kernel-related >>>> stuffs. It's preferable to avoid the addition if possible, IMO. >>> >>> I don't understand, functionally it's the same, there should not be any >>> increased maintenance either way. Please explain how this makes things >>> "harder"? >> >> Hm, if so it might be my misunderstanding to reasons for typical usage >> of inline functions in kernel source, sorry. >> >> In my understanding, exported symbols are put into some sections of >> ELF binary. Addition of new symbols increases size of the >> section. Additionally, after linking vmlinux, kbuild scans built-in >> symbols and make a file with entries of them. The addition increases >> time of this step. Furthermore, at the end of building kernel, kmod is >> called to generate some map files for exported symbols in loadable >> module. In a view of distributors, these files are maintained by >> binary packages of any type carefully because some incompatibilities >> can be delivered such as version mismatch. >> >> For these reasons, I think thing goes harder when people carelessly >> add new symbols for functions with a few codes; e.g. accessing to a >> member of structure, then simply check an return it. Actually I can >> see some examples in upstreamed headers. > > The advantage of inline function isn't about the maintenance cost. > It's mostly for performance, as well as the binary size reduction. > > Actually, when a kernel live-patching comes into play, an inline > function is worse from the maintenance POV. Then we'd have to patch > every place that is expanded instead of a single place. > > However, this doesn't discourage the use of inline function, either. I'm OK for this view, and let me add it to my criteria for my daily reviewing work. Thanks for sharing the view. > Overall, the performance is still the most important factor for major > cases. So I agree with that this particular case can be well inlined, > supposing that no complex code is planned to be added in future. I agree with it as well. When developers add more complexity to content of the inline function, then let them convert it to exported symbols. Thanks Takashi Sakamoto
On Thu, Apr 05, 2018 at 02:03:31PM +0900, Takashi Sakamoto wrote: > Hi, > > On Apr 3 2018 21:03, Takashi Iwai wrote: > >>>>It's better for this kind of code to be incline function in any header. In > >>>>general, new symbols increase maintenance cost of binary of kernel-related > >>>>stuffs. It's preferable to avoid the addition if possible, IMO. > >>> > >>>I don't understand, functionally it's the same, there should not be any > >>>increased maintenance either way. Please explain how this makes things > >>>"harder"? > >> > >>Hm, if so it might be my misunderstanding to reasons for typical usage > >>of inline functions in kernel source, sorry. > >> > >>In my understanding, exported symbols are put into some sections of > >>ELF binary. Addition of new symbols increases size of the > >>section. Additionally, after linking vmlinux, kbuild scans built-in > >>symbols and make a file with entries of them. The addition increases > >>time of this step. Furthermore, at the end of building kernel, kmod is > >>called to generate some map files for exported symbols in loadable > >>module. In a view of distributors, these files are maintained by > >>binary packages of any type carefully because some incompatibilities > >>can be delivered such as version mismatch. > >> > >>For these reasons, I think thing goes harder when people carelessly > >>add new symbols for functions with a few codes; e.g. accessing to a > >>member of structure, then simply check an return it. Actually I can > >>see some examples in upstreamed headers. > > > >The advantage of inline function isn't about the maintenance cost. > >It's mostly for performance, as well as the binary size reduction. > > > >Actually, when a kernel live-patching comes into play, an inline > >function is worse from the maintenance POV. Then we'd have to patch > >every place that is expanded instead of a single place. > > > >However, this doesn't discourage the use of inline function, either. > > I'm OK for this view, and let me add it to my criteria for my daily > reviewing work. Thanks for sharing the view. For us the motivation to keep as proposed was prior art. Currently all of the snd_soc_dai_set_* APIs are doing similar functionally of setting something on DAIs and not inlined. Said that I agree this can be inlined so we shall do so. > >Overall, the performance is still the most important factor for major > >cases. So I agree with that this particular case can be well inlined, > >supposing that no complex code is planned to be added in future. > > I agree with it as well. When developers add more complexity to content > of the inline function, then let them convert it to exported symbols. > > > Thanks > > Takashi Sakamoto
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 58acd00cae19..78fa068560cf 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -129,6 +129,9 @@ int snd_soc_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt); int snd_soc_dai_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask, unsigned int rx_mask, int slots, int slot_width); +int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai, + void *stream, int direction); + int snd_soc_dai_set_channel_map(struct snd_soc_dai *dai, unsigned int tx_num, unsigned int *tx_slot, unsigned int rx_num, unsigned int *rx_slot); @@ -168,6 +171,8 @@ struct snd_soc_dai_ops { unsigned int rx_num, unsigned int *rx_slot); int (*set_tristate)(struct snd_soc_dai *dai, int tristate); + int (*set_sdw_stream)(struct snd_soc_dai *dai, + void *stream, int direction); /* * DAI digital mute - optional. * Called by soc-core to minimise any pops. diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index c0edac80df34..690e56a35237 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2882,6 +2882,26 @@ int snd_soc_dai_set_tdm_slot(struct snd_soc_dai *dai, EXPORT_SYMBOL_GPL(snd_soc_dai_set_tdm_slot); /** + * snd_soc_dai_set_sdw_stream() - Configures a DAI for SDW stream operation + * @dai: DAI + * @stream: STREAM + * @direction: Stream direction(Playback/Capture) + * SoundWire subsystem doesn't have a notion of direction and we reuse + * the ASoC stream direction to configure sink/source ports. + * + * Returns 0 on success, a negative error code otherwise. + */ +int snd_soc_dai_set_sdw_stream(struct snd_soc_dai *dai, + void *stream, int direction) +{ + if (dai->driver->ops->set_sdw_stream) + return dai->driver->ops->set_sdw_stream(dai, stream, direction); + else + return -ENOTSUPP; +} +EXPORT_SYMBOL_GPL(snd_soc_dai_set_sdw_stream); + +/** * snd_soc_dai_set_channel_map - configure DAI audio channel map * @dai: DAI * @tx_num: how many TX channels