sound: hda_intel: add card number to irq description

PATCH In review Heiner Kallweit 3 participants 7 messages Dec 22, 2015 – Jan 12, 2016 Sound

Share & tools

Heiner Kallweit

author

2015-12-22 18:09 UTC #0 lore raw

PATCH v2 1/1 sound: hda_intel: add card number to irq description

Currently the info in /proc/interrupts doesn't allow to figure out which interrupt belongs to which card (HDMI, PCH, ..).
Therefore add card details to the interrupt description. With the patch the info in /proc/interrupts looks like this:

PCI-MSI 442368-edge snd_hda_intel:card1
PCI-MSI 49152-edge snd_hda_intel:card0

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
Make extension more generic and implement it in snd_card. This way every driver using struct snd_card can easily be switched to using the more descriptive irq describer.

---
 include/sound/core.h      | 1 +
 sound/core/init.c         | 3 +++
 sound/pci/hda/hda_intel.c | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

include/sound/core.h +1 -0

index cdfecaf..31079ea 100644
@@ -99,6 +99,7 @@ struct snd_card {
 	char driver[16];		/* driver name */
 	char shortname[32];		/* short name of this soundcard */
 	char longname[80];		/* name of this soundcard */
+	char irq_descr[32];		/* Interrupt description */
 	char mixername[80];		/* mixer name */
 	char components[128];		/* card components delimited with
 								space */

sound/core/init.c +3 -0

index 20f37fb..6bda843 100644
@@ -268,6 +268,9 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 	if (err < 0)
 		goto __error;
 
+	snprintf(card->irq_descr, sizeof(card->irq_descr), "%s:%s",
+		 dev_driver_string(card->dev), dev_name(&card->card_dev));
+
 	/* the control interface cannot be accessed from the user space until */
 	/* snd_cards_bitmask and snd_cards are set with snd_card_register */
 	err = snd_ctl_create(card);

sound/pci/hda/hda_intel.c +1 -1

index 83800ac..c0bef11 100644
@@ -725,7 +725,7 @@ static int azx_acquire_irq(struct azx *chip, int do_disconnect)
 
 	if (request_irq(chip->pci->irq, azx_interrupt,
 			chip->msi ? 0 : IRQF_SHARED,
-			KBUILD_MODNAME, chip)) {
+			chip->card->irq_descr, chip)) {
 		dev_err(chip->card->dev,
 			"unable to grab IRQ %d, disabling device\n",
 			chip->pci->irq);

Signature · 1 line

2.6.4

Takashi Iwai

2015-12-23 07:40 UTC #1 lore raw

Quoted 9 lines · Heiner Kallweit

Currently the info in /proc/interrupts doesn't allow to figure out which interrupt belongs to which card (HDMI, PCH, ..).
Therefore add card details to the interrupt description. With the patch the info in /proc/interrupts looks like this:
PCI-MSI 442368-edge snd_hda_intel:card1
PCI-MSI 49152-edge snd_hda_intel:card0

Quoted 6 lines

Signed-off-by Heiner Kallweit <[email protected]>
---
v2:
Make extension more generic and implement it in snd_card. This way every driver using struct snd_card can easily be switched to using the more descriptive irq describer.

Thanks, the patch looks good to me, but I have a small hesitation to
add such a new filed blindly to snd_card struct. Although it's
relatively small (32 bytes), it's not zero, after all.

Maybe we'll take this approach in the end, I guess, but let's see whether this rings bell to others.

Takashi

Quoted 50 lines

---
 include/sound/core.h      | 1 +
 sound/core/init.c         | 3 +++
 sound/pci/hda/hda_intel.c | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

include/sound/core.h +1 -0

index cdfecaf..31079ea 100644
@@ -99,6 +99,7 @@ struct snd_card {
 	char driver[16];		/* driver name */
 	char shortname[32];		/* short name of this soundcard */
 	char longname[80];		/* name of this soundcard */
+	char irq_descr[32];		/* Interrupt description */
 	char mixername[80];		/* mixer name */
 	char components[128];		/* card components delimited with
 								space */

sound/core/init.c +3 -0

index 20f37fb..6bda843 100644
@@ -268,6 +268,9 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 	if (err < 0)
 		goto __error;
 
+	snprintf(card->irq_descr, sizeof(card->irq_descr), "%s:%s",
+		 dev_driver_string(card->dev), dev_name(&card->card_dev));
+
 	/* the control interface cannot be accessed from the user space until */
 	/* snd_cards_bitmask and snd_cards are set with snd_card_register */
 	err = snd_ctl_create(card);

sound/pci/hda/hda_intel.c +1 -1

index 83800ac..c0bef11 100644
@@ -725,7 +725,7 @@ static int azx_acquire_irq(struct azx *chip, int do_disconnect)
 
 	if (request_irq(chip->pci->irq, azx_interrupt,
 			chip->msi ? 0 : IRQF_SHARED,
-			KBUILD_MODNAME, chip)) {
+			chip->card->irq_descr, chip)) {
 		dev_err(chip->card->dev,
 			"unable to grab IRQ %d, disabling device\n",
 			chip->pci->irq);

Signature · 1 line

2.6.4

Heiner Kallweit

author

2015-12-23 07:57 UTC #2 lore raw

Quoted 18 lines · Takashi Iwai

Quoted 9 lines · Heiner Kallweit
Currently the info in /proc/interrupts doesn't allow to figure out which interrupt belongs to which card (HDMI, PCH, ..).
Therefore add card details to the interrupt description. With the patch the info in /proc/interrupts looks like this:
PCI-MSI 442368-edge snd_hda_intel:card1
PCI-MSI 49152-edge snd_hda_intel:card0
Quoted 6 lines
Signed-off-by Heiner Kallweit <[email protected]>
---
v2:
Make extension more generic and implement it in snd_card. This way every driver using struct snd_card can easily be switched to using the more descriptive irq describer.

Quoted 7 lines

Thanks, the patch looks good to me, but I have a small hesitation to
add such a new filed blindly to snd_card struct. Although it's
relatively small (32 bytes), it's not zero, after all.
Maybe we'll take this approach in the end, I guess, but let's see whether this rings bell to others.

We could also allocate the irq describer dynamically with kasprintf or devm_kasprintf and just add a pointer to snd_card struct. This would save us a few bytes, question is whether it's worth it. I'm open to any suggestion.

Heiner

Quoted 54 lines

Takashi

Quoted 50 lines

---
 include/sound/core.h      | 1 +
 sound/core/init.c         | 3 +++
 sound/pci/hda/hda_intel.c | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

include/sound/core.h +1 -0

index cdfecaf..31079ea 100644
@@ -99,6 +99,7 @@ struct snd_card {
      char driver[16];                /* driver name */
      char shortname[32];             /* short name of this soundcard */
      char longname[80];              /* name of this soundcard */
+     char irq_descr[32];             /* Interrupt description */
      char mixername[80];             /* mixer name */
      char components[128];           /* card components delimited with
                                                              space */

sound/core/init.c +3 -0

index 20f37fb..6bda843 100644
@@ -268,6 +268,9 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
      if (err < 0)
              goto __error;
 
+     snprintf(card->irq_descr, sizeof(card->irq_descr), "%s:%s",
+              dev_driver_string(card->dev), dev_name(&card->card_dev));
+
      /* the control interface cannot be accessed from the user space until */
      /* snd_cards_bitmask and snd_cards are set with snd_card_register */
      err = snd_ctl_create(card);

sound/pci/hda/hda_intel.c +1 -2

index 83800ac..c0bef11 100644
@@ -725,7 +725,7 @@ static int azx_acquire_irq(struct azx *chip, int do_disconnect)
 
      if (request_irq(chip->pci->irq, azx_interrupt,
                      chip->msi ? 0 : IRQF_SHARED,
-                     KBUILD_MODNAME, chip)) {
+                     chip->card->irq_descr, chip)) {
              dev_err(chip->card->dev,
                      "unable to grab IRQ %d, disabling device\n",
                      chip->pci->irq);
--

2.6.4

Takashi Iwai

2015-12-23 08:32 UTC #3 lore raw

Quoted 24 lines · Heiner Kallweit

Quoted 22 lines · Takashi Iwai
Quoted 9 lines · Heiner Kallweit
Currently the info in /proc/interrupts doesn't allow to figure out which interrupt belongs to which card (HDMI, PCH, ..).
Therefore add card details to the interrupt description. With the patch the info in /proc/interrupts looks like this:
PCI-MSI 442368-edge snd_hda_intel:card1
PCI-MSI 49152-edge snd_hda_intel:card0
Quoted 6 lines
Signed-off-by Heiner Kallweit <[email protected]>
---
v2:
Make extension more generic and implement it in snd_card. This way every driver using struct snd_card can easily be switched to using the more descriptive irq describer.
Thanks, the patch looks good to me, but I have a small hesitation to
add such a new filed blindly to snd_card struct. Although it's
relatively small (32 bytes), it's not zero, after all.

Quoted 6 lines

Quoted 3 lines
Maybe we'll take this approach in the end, I guess, but let's see whether this rings bell to others.
We could also allocate the irq describer dynamically with kasprintf or devm_kasprintf and just add a pointer to snd_card struct. This would save us a few bytes, question is whether it's worth it.

Yes, that's my first idea, too. But this will end up eating more bytes totally.

Takashi

Quoted 58 lines

I'm open to any suggestion.

Heiner

Quoted 54 lines

Takashi

Quoted 50 lines

---
 include/sound/core.h      | 1 +
 sound/core/init.c         | 3 +++
 sound/pci/hda/hda_intel.c | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

include/sound/core.h +1 -0

index cdfecaf..31079ea 100644
@@ -99,6 +99,7 @@ struct snd_card {
      char driver[16];                /* driver name */
      char shortname[32];             /* short name of this soundcard */
      char longname[80];              /* name of this soundcard */
+     char irq_descr[32];             /* Interrupt description */
      char mixername[80];             /* mixer name */
      char components[128];           /* card components delimited with
                                                              space */

sound/core/init.c +3 -0

index 20f37fb..6bda843 100644
@@ -268,6 +268,9 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
      if (err < 0)
              goto __error;
 
+     snprintf(card->irq_descr, sizeof(card->irq_descr), "%s:%s",
+              dev_driver_string(card->dev), dev_name(&card->card_dev));
+
      /* the control interface cannot be accessed from the user space until */
      /* snd_cards_bitmask and snd_cards are set with snd_card_register */
      err = snd_ctl_create(card);

sound/pci/hda/hda_intel.c +1 -2

index 83800ac..c0bef11 100644
@@ -725,7 +725,7 @@ static int azx_acquire_irq(struct azx *chip, int do_disconnect)
 
      if (request_irq(chip->pci->irq, azx_interrupt,
                      chip->msi ? 0 : IRQF_SHARED,
-                     KBUILD_MODNAME, chip)) {
+                     chip->card->irq_descr, chip)) {
              dev_err(chip->card->dev,
                      "unable to grab IRQ %d, disabling device\n",
                      chip->pci->irq);
--

2.6.4

Clemens Ladisch

2015-12-23 21:58 UTC #4 lore raw

In reply to Takashi Iwai

Quoted 3 lines · Takashi Iwai

I have a small hesitation to
add such a new filed blindly to snd_card struct. Although it's
relatively small (32 bytes), it's not zero, after all.

There is only one struct per card.

And it is doubtful that other solutions would save space when the code size is taken into account.

Regards,
Clemens

Heiner Kallweit

author

2016-01-12 19:27 UTC #5 lore raw

Am 23.12.2015 um 22:58 schrieb Clemens Ladisch:

Quoted 5 lines

Quoted 3 lines · Takashi Iwai
I have a small hesitation to
add such a new filed blindly to snd_card struct. Although it's
relatively small (32 bytes), it's not zero, after all.

Quoted 9 lines

There is only one struct per card.
And it is doubtful that other solutions would save space when the code size is taken into account.
Regards,
Clemens

Any final opinion regarding this patch?

Regards, Heiner

Takashi Iwai

2016-01-12 20:11 UTC #6 lore raw

Quoted 9 lines · Heiner Kallweit

Am 23.12.2015 um 22:58 schrieb Clemens Ladisch:
Quoted 7 lines
Quoted 3 lines · Takashi Iwai
I have a small hesitation to
add such a new filed blindly to snd_card struct. Although it's
relatively small (32 bytes), it's not zero, after all.
There is only one struct per card.

Quoted 8 lines

Quoted 7 lines
And it is doubtful that other solutions would save space when the code size is taken into account.
Regards,
Clemens
Any final opinion regarding this patch?

Sorry, I forgot this after vacation.
Since there has been no better idea, I took your patch as is now.

Thanks!

Takashi