mbox series

[v7,0/1] audio jack software injection

Message ID 20210125032118.13269-1-hui.wang@canonical.com
Headers show
Series audio jack software injection | expand

Message

Hui Wang Jan. 25, 2021, 3:21 a.m. UTC 
the changes in the v7:
 - change the format of the last part in jack-injection.rst
 - add dependence SND_DEBUG in the Kconfig
 - create debugfs_mount_dir/sound and debugfs_mount_dir/sound/cardN only SND_DEBUG is enabled
 - change simple_write_to_buffer(buf, count, ppos, from, count) to
   simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, from, count)
 - rewrite the function parse_mask_bits() according to v6's comment

the changes in the v6:
 - use the sizeof(buf) to replace the digits in the scnprintf
 - squash the document patch into the 0001-xxx.patch.


the changes in the v5:
 - add a Kconfig to enable/disable the jack injection feature
 - replace all kzalloc with local char[] in the debugfs fops
 - replace the chars of !isalnum() to '_' for the jack folder's name
 - squash all .c files patches into one patch
 - add a document to explain jack injection, for easier review, put
   the document in a standalone patch. If needed, will squash this
   patch to the previous one.
 

the changes in the v4:
 - change the sound-core to sound and change the sound_core_debugfs_root
   to sound_debugfs_root in the 0001-xxx.patch
 - change kzalloc/kfree to kvzalloc/kvfree in the debugfs fops for
   0001-xxx.patch and 0003-xxx.patch
 - And if needed, I could squash 4 patches into 1 patch before merging.

the changes in the v3 (for easy to review, divide change into 4 patches):
 - address the comment about the snd_jack_report() and _snd_jack_report(),
   the v2 design is hard to understand and is hard to review, in the v3,
   separate the jack_report to snd_jack_report() and snd_jack_inject_report(),
   hw jack events call snd_jack_report() as before, if a jack contains multi
   jack_kctl and the jack_kctl's sw_inject is enabled, the status and the
   related input-dev's events will not be reproted. The injection events call
   snd_jack_inject_report(). This change is squashed to 0001-xxx.patch,  the
   rest part of 0001-xxx.patch is same as v2.

 - address the comment about folders'name in the 0002-xxx.patch, so far, drop
   the '/', ',', '=' and ' ' from the folders' name.

 - address the comment about adding more debugfs nodes in the 0003-xxx.patch,
   it adds kctl_id, mask_bits, status and type.

 - address the comment about save-n-restore jack's hw status in the
   0004-xxx.patch, adding a hw_status_cache and save the last reported jack
   hw event, once the sw_inject is disabled, will restore all jack_kctl's
   state under the same snd_jack with hw_status_cache.
[snip]


the changes in the V2:
 - using debugfs instead of sysfs
 - using jack_ctrl to create a folder instead of snd_jack, since ASoC drivers
   could create multi jack_ctrls within a snd_jack
 - create a folder for each jack_ctrl instead for all jack_ctrls
[ snip ]

Hui Wang (1):
  alsa: jack: implement software jack injection via debugfs

 Documentation/sound/designs/index.rst         |   1 +
 .../sound/designs/jack-injection.rst          | 166 ++++++++++
 include/sound/core.h                          |   2 +
 include/sound/jack.h                          |   1 +
 sound/core/Kconfig                            |   9 +
 sound/core/init.c                             |  16 +
 sound/core/jack.c                             | 299 +++++++++++++++++-
 sound/core/sound.c                            |  11 +
 8 files changed, 501 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/sound/designs/jack-injection.rst

Comments

Takashi Iwai Jan. 25, 2021, 2:32 p.m. UTC | #1 
On Mon, 25 Jan 2021 04:21:18 +0100,
Hui Wang wrote:
> 
> This change adds audio jack injection feature through debugfs, with
> this feature, we could validate alsa userspace changes by injecting
> plugin or plugout events to the non-phantom audio jacks.
> 
> With this change, the sound core will build the folders
> $debugfs_mount_dir/sound/cardN if SND_DEBUG and DEBUG_FS are enabled.
> And if users also enable the SND_JACK_INJECTION_DEBUG, the jack
> injection nodes will be built in the folder cardN like below:
> 
> $tree $debugfs_mount_dir/sound
> $debugfs_mount_dir/sound
> ├── card0
> │   ├── HDMI_DP_pcm_10_Jack
> │   │   ├── jackin_inject
> │   │   ├── kctl_id
> │   │   ├── mask_bits
> │   │   ├── status
> │   │   ├── sw_inject_enable
> │   │   └── type
> ...
> │   └── HDMI_DP_pcm_9_Jack
> │       ├── jackin_inject
> │       ├── kctl_id
> │       ├── mask_bits
> │       ├── status
> │       ├── sw_inject_enable
> │       └── type
> └── card1
>     ├── HDMI_DP_pcm_5_Jack
>     │   ├── jackin_inject
>     │   ├── kctl_id
>     │   ├── mask_bits
>     │   ├── status
>     │   ├── sw_inject_enable
>     │   └── type
>     ...
>     ├── Headphone_Jack
>     │   ├── jackin_inject
>     │   ├── kctl_id
>     │   ├── mask_bits
>     │   ├── status
>     │   ├── sw_inject_enable
>     │   └── type
>     └── Headset_Mic_Jack
>         ├── jackin_inject
>         ├── kctl_id
>         ├── mask_bits
>         ├── status
>         ├── sw_inject_enable
>         └── type
> 
> The nodes kctl_id, mask_bits, status and type are read-only, users
> could check jack or jack_kctl's information through them.
> 
> The nodes sw_inject_enable and jackin_inject are directly used for
> injection. The sw_inject_enable is read-write, users could check if
> software injection is enabled or not on this jack, and users could
> echo 1 or 0 to enable or disable software injection on this jack. Once
> the injection is enabled, the jack will not change by hardware events
> anymore, once the injection is disabled, the jack will restore the
> last reported hardware events to the jack. The jackin_inject is
> write-only, if the injection is enabled, users could echo 1 or 0 to
> this node to inject plugin or plugout events to this jack.
> 
> For the detailed usage information on these nodes, please refer to
> Documentation/sound/designs/jack-injection.rst.
> 
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
> Reviewed-by: Jaroslav Kysela <perex@perex.cz>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>

Some minor nitpicking:

> +static int snd_jack_debugfs_add_inject_node(struct snd_jack *jack,
> +					    struct snd_jack_kctl *jack_kctl)
> +{
> +	char *tname;
> +	int i;
> +
> +	tname = kstrdup(jack_kctl->kctl->id.name, GFP_KERNEL);
> +	if (!tname)
> +		return -ENOMEM;
> +
> +	/* replace the chars which are not suitable for folder's name with _ */
> +	for (i = 0; i < strlen(tname); i++)

No need to use strlen(), just check the NUL character on tname[i].

> --- a/sound/core/sound.c
> +++ b/sound/core/sound.c
> @@ -9,6 +9,7 @@
>  #include <linux/time.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/debugfs.h>
>  #include <sound/core.h>
>  #include <sound/minors.h>
>  #include <sound/info.h>
> @@ -39,6 +40,9 @@ MODULE_ALIAS_CHARDEV_MAJOR(CONFIG_SND_MAJOR);
>  int snd_ecards_limit;
>  EXPORT_SYMBOL(snd_ecards_limit);
>  
> +struct dentry *sound_debugfs_root;
> +EXPORT_SYMBOL_GPL(sound_debugfs_root);

I guess this should be wrapped with CONFIG_SND_DEBUG ifdef?


thanks,

Takashi
Hui Wang Jan. 26, 2021, 6:55 a.m. UTC | #2 
On 1/25/21 10:32 PM, Takashi Iwai wrote:
> On Mon, 25 Jan 2021 04:21:18 +0100,
> Hui Wang wrote:
>> This change adds audio jack injection feature through debugfs, with
>> this feature, we could validate alsa userspace changes by injecting
>> plugin or plugout events to the non-phantom audio jacks.
>>
>> With this change, the sound core will build the folders
>> $debugfs_mount_dir/sound/cardN if SND_DEBUG and DEBUG_FS are enabled.
<snip>
>> +		return -ENOMEM;
>> +
>> +	/* replace the chars which are not suitable for folder's name with _ */
>> +	for (i = 0; i < strlen(tname); i++)
> No need to use strlen(), just check the NUL character on tname[i].
OK, will change it to:     for (i = 0; tname[i] != '\0'; i++)
>
>> --- a/sound/core/sound.c
>> +++ b/sound/core/sound.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/time.h>
>>   #include <linux/device.h>
>>   #include <linux/module.h>
>> +#include <linux/debugfs.h>
>>   #include <sound/core.h>
>>   #include <sound/minors.h>
>>   #include <sound/info.h>
>> @@ -39,6 +40,9 @@ MODULE_ALIAS_CHARDEV_MAJOR(CONFIG_SND_MAJOR);
>>   int snd_ecards_limit;
>>   EXPORT_SYMBOL(snd_ecards_limit);
>>   
>> +struct dentry *sound_debugfs_root;
>> +EXPORT_SYMBOL_GPL(sound_debugfs_root);
> I guess this should be wrapped with CONFIG_SND_DEBUG ifdef?

OK, will add the wrap.

thanks,

Hui.

>
>
> thanks,
>
> Takashi
Takashi Iwai Jan. 26, 2021, 6:58 a.m. UTC | #3 
On Tue, 26 Jan 2021 07:55:29 +0100,
Hui Wang wrote:
> 
> 
> On 1/25/21 10:32 PM, Takashi Iwai wrote:
> > On Mon, 25 Jan 2021 04:21:18 +0100,
> > Hui Wang wrote:
> >> This change adds audio jack injection feature through debugfs, with
> >> this feature, we could validate alsa userspace changes by injecting
> >> plugin or plugout events to the non-phantom audio jacks.
> >>
> >> With this change, the sound core will build the folders
> >> $debugfs_mount_dir/sound/cardN if SND_DEBUG and DEBUG_FS are enabled.
> <snip>
> >> +		return -ENOMEM;
> >> +
> >> +	/* replace the chars which are not suitable for folder's name with _ */
> >> +	for (i = 0; i < strlen(tname); i++)
> > No need to use strlen(), just check the NUL character on tname[i].
> OK, will change it to:     for (i = 0; tname[i] != '\0'; i++)

Even the "!= '\0" part can be dropped, too :)

  for (i = 0; tname[i]; i++)
    ....


Takashi
Hui Wang Jan. 26, 2021, 7:03 a.m. UTC | #4 
On 1/26/21 2:58 PM, Takashi Iwai wrote:
> On Tue, 26 Jan 2021 07:55:29 +0100,
> Hui Wang wrote:
>>
>> On 1/25/21 10:32 PM, Takashi Iwai wrote:
>>> On Mon, 25 Jan 2021 04:21:18 +0100,
>>> Hui Wang wrote:
>>>> This change adds audio jack injection feature through debugfs, with
>>>> this feature, we could validate alsa userspace changes by injecting
>>>> plugin or plugout events to the non-phantom audio jacks.
>>>>
>>>> With this change, the sound core will build the folders
>>>> $debugfs_mount_dir/sound/cardN if SND_DEBUG and DEBUG_FS are enabled.
>> <snip>
>>>> +		return -ENOMEM;
>>>> +
>>>> +	/* replace the chars which are not suitable for folder's name with _ */
>>>> +	for (i = 0; i < strlen(tname); i++)
>>> No need to use strlen(), just check the NUL character on tname[i].
>> OK, will change it to:     for (i = 0; tname[i] != '\0'; i++)
> Even the "!= '\0" part can be dropped, too :)
>
>    for (i = 0; tname[i]; i++)
>      ....

OK, got it.

thx.

>
>
> Takashi