All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer
@ 2017-03-17 15:44 Charles Keepax
  2017-03-17 20:07 ` Austin, Brian
  2017-03-17 21:54 ` Applied "ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer" to the asoc tree Mark Brown
  0 siblings, 2 replies; 3+ messages in thread
From: Charles Keepax @ 2017-03-17 15:44 UTC (permalink / raw)
  To: broonie; +Cc: brian.austin, alsa-devel, lgirdwood, Paul.Handrigan, patches

The driver stashes a CODEC pointer in the cs35l35_private structure,
which is used to obtain a struct device pointer for error messages in the
interrupt handler.

However, doing so is not very safe as the interrupt is registered, as it
should be in bus probe, but the CODEC pointer can't be safely stored until
the ASoC level probe. This leaves a window between the two probes where if
any interrupts are received a NULL pointer will be deferenced in the IRQ
handler.

Fix this issue by saving a pointer to the device directly and passing that
to the error messages in the interrupt handler rather than using the CODEC
pointer to access the device pointer.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/cs35l35.c | 29 ++++++++++++++---------------
 sound/soc/codecs/cs35l35.h |  2 +-
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/sound/soc/codecs/cs35l35.c b/sound/soc/codecs/cs35l35.c
index 05117fc..1d9f332 100644
--- a/sound/soc/codecs/cs35l35.c
+++ b/sound/soc/codecs/cs35l35.c
@@ -741,8 +741,6 @@ static int cs35l35_codec_probe(struct snd_soc_codec *codec)
 	struct monitor_cfg *monitor_config = &cs35l35->pdata.mon_cfg;
 	int ret;
 
-	cs35l35->codec = codec;
-
 	/* Set Platform Data */
 	if (cs35l35->pdata.bst_vctl)
 		regmap_update_bits(cs35l35->regmap, CS35L35_BST_CVTR_V_CTL,
@@ -1004,7 +1002,6 @@ static struct regmap_config cs35l35_regmap = {
 static irqreturn_t cs35l35_irq(int irq, void *data)
 {
 	struct cs35l35_private *cs35l35 = data;
-	struct snd_soc_codec *codec = cs35l35->codec;
 	unsigned int sticky1, sticky2, sticky3, sticky4;
 	unsigned int mask1, mask2, mask3, mask4, current1;
 
@@ -1032,7 +1029,7 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 
 	/* handle the interrupts */
 	if (sticky1 & CS35L35_CAL_ERR) {
-		dev_crit(codec->dev, "Calibration Error\n");
+		dev_crit(cs35l35->dev, "Calibration Error\n");
 
 		/* error is no longer asserted; safe to reset */
 		if (!(current1 & CS35L35_CAL_ERR)) {
@@ -1051,10 +1048,10 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky1 & CS35L35_AMP_SHORT) {
-		dev_crit(codec->dev, "AMP Short Error\n");
+		dev_crit(cs35l35->dev, "AMP Short Error\n");
 		/* error is no longer asserted; safe to reset */
 		if (!(current1 & CS35L35_AMP_SHORT)) {
-			dev_dbg(codec->dev, "Amp short error release\n");
+			dev_dbg(cs35l35->dev, "Amp short error release\n");
 			regmap_update_bits(cs35l35->regmap,
 					CS35L35_PROT_RELEASE_CTL,
 					CS35L35_SHORT_RLS, 0);
@@ -1069,11 +1066,11 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky1 & CS35L35_OTW) {
-		dev_warn(codec->dev, "Over temperature warning\n");
+		dev_warn(cs35l35->dev, "Over temperature warning\n");
 
 		/* error is no longer asserted; safe to reset */
 		if (!(current1 & CS35L35_OTW)) {
-			dev_dbg(codec->dev, "Over temperature warn release\n");
+			dev_dbg(cs35l35->dev, "Over temperature warn release\n");
 			regmap_update_bits(cs35l35->regmap,
 					CS35L35_PROT_RELEASE_CTL,
 					CS35L35_OTW_RLS, 0);
@@ -1088,10 +1085,10 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky1 & CS35L35_OTE) {
-		dev_crit(codec->dev, "Over temperature error\n");
+		dev_crit(cs35l35->dev, "Over temperature error\n");
 		/* error is no longer asserted; safe to reset */
 		if (!(current1 & CS35L35_OTE)) {
-			dev_dbg(codec->dev, "Over temperature error release\n");
+			dev_dbg(cs35l35->dev, "Over temperature error release\n");
 			regmap_update_bits(cs35l35->regmap,
 					CS35L35_PROT_RELEASE_CTL,
 					CS35L35_OTE_RLS, 0);
@@ -1106,7 +1103,7 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky3 & CS35L35_BST_HIGH) {
-		dev_crit(codec->dev, "VBST error: powering off!\n");
+		dev_crit(cs35l35->dev, "VBST error: powering off!\n");
 		regmap_update_bits(cs35l35->regmap, CS35L35_PWRCTL2,
 			CS35L35_PDN_AMP, CS35L35_PDN_AMP);
 		regmap_update_bits(cs35l35->regmap, CS35L35_PWRCTL1,
@@ -1114,7 +1111,7 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky3 & CS35L35_LBST_SHORT) {
-		dev_crit(codec->dev, "LBST error: powering off!\n");
+		dev_crit(cs35l35->dev, "LBST error: powering off!\n");
 		regmap_update_bits(cs35l35->regmap, CS35L35_PWRCTL2,
 			CS35L35_PDN_AMP, CS35L35_PDN_AMP);
 		regmap_update_bits(cs35l35->regmap, CS35L35_PWRCTL1,
@@ -1122,13 +1119,13 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky2 & CS35L35_VPBR_ERR)
-		dev_dbg(codec->dev, "Error: Reactive Brownout\n");
+		dev_dbg(cs35l35->dev, "Error: Reactive Brownout\n");
 
 	if (sticky4 & CS35L35_VMON_OVFL)
-		dev_dbg(codec->dev, "Error: VMON overflow\n");
+		dev_dbg(cs35l35->dev, "Error: VMON overflow\n");
 
 	if (sticky4 & CS35L35_IMON_OVFL)
-		dev_dbg(codec->dev, "Error: IMON overflow\n");
+		dev_dbg(cs35l35->dev, "Error: IMON overflow\n");
 
 	return IRQ_HANDLED;
 }
@@ -1365,6 +1362,8 @@ static int cs35l35_i2c_probe(struct i2c_client *i2c_client,
 	if (!cs35l35)
 		return -ENOMEM;
 
+	cs35l35->dev = dev;
+
 	i2c_set_clientdata(i2c_client, cs35l35);
 	cs35l35->regmap = devm_regmap_init_i2c(i2c_client, &cs35l35_regmap);
 	if (IS_ERR(cs35l35->regmap)) {
diff --git a/sound/soc/codecs/cs35l35.h b/sound/soc/codecs/cs35l35.h
index c203081..156d2f0 100644
--- a/sound/soc/codecs/cs35l35.h
+++ b/sound/soc/codecs/cs35l35.h
@@ -265,7 +265,7 @@
 			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
 
 struct  cs35l35_private {
-	struct snd_soc_codec *codec;
+	struct device *dev;
 	struct cs35l35_platform_data pdata;
 	struct regmap *regmap;
 	struct regulator_bulk_data supplies[2];
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer
  2017-03-17 15:44 [PATCH] ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer Charles Keepax
@ 2017-03-17 20:07 ` Austin, Brian
  2017-03-17 21:54 ` Applied "ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer" to the asoc tree Mark Brown
  1 sibling, 0 replies; 3+ messages in thread
From: Austin, Brian @ 2017-03-17 20:07 UTC (permalink / raw)
  To: Charles Keepax, broonie; +Cc: alsa-devel, patches, lgirdwood, Handrigan, Paul



On 3/17/17, 10:44 AM, "Charles Keepax" <ckeepax@opensource.wolfsonmicro.com> wrote:

    The driver stashes a CODEC pointer in the cs35l35_private structure,
    which is used to obtain a struct device pointer for error messages in the
    interrupt handler.
    
    However, doing so is not very safe as the interrupt is registered, as it
    should be in bus probe, but the CODEC pointer can't be safely stored until
    the ASoC level probe. This leaves a window between the two probes where if
    any interrupts are received a NULL pointer will be deferenced in the IRQ
    handler.
    
    Fix this issue by saving a pointer to the device directly and passing that
    to the error messages in the interrupt handler rather than using the CODEC
    pointer to access the device pointer.
    
    Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
    ---
Acked-by: Brian Austin <brian.austin@cirrus.com> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Applied "ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer" to the asoc tree
  2017-03-17 15:44 [PATCH] ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer Charles Keepax
  2017-03-17 20:07 ` Austin, Brian
@ 2017-03-17 21:54 ` Mark Brown
  1 sibling, 0 replies; 3+ messages in thread
From: Mark Brown @ 2017-03-17 21:54 UTC (permalink / raw)
  To: Charles Keepax
  Cc: brian.austin, patches, alsa-devel, Paul.Handrigan, lgirdwood, broonie

The patch

   ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 5d3d0ad688eacf9567d7d67a5eec3c436cc1064c Mon Sep 17 00:00:00 2001
From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Date: Fri, 17 Mar 2017 15:44:55 +0000
Subject: [PATCH] ASoC: cs35l35: Stash dev pointer directly rather than CODEC
 pointer

The driver stashes a CODEC pointer in the cs35l35_private structure,
which is used to obtain a struct device pointer for error messages in the
interrupt handler.

However, doing so is not very safe as the interrupt is registered, as it
should be in bus probe, but the CODEC pointer can't be safely stored until
the ASoC level probe. This leaves a window between the two probes where if
any interrupts are received a NULL pointer will be deferenced in the IRQ
handler.

Fix this issue by saving a pointer to the device directly and passing that
to the error messages in the interrupt handler rather than using the CODEC
pointer to access the device pointer.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/cs35l35.c | 29 ++++++++++++++---------------
 sound/soc/codecs/cs35l35.h |  2 +-
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/sound/soc/codecs/cs35l35.c b/sound/soc/codecs/cs35l35.c
index 05117fca7e3c..1d9f332b2eec 100644
--- a/sound/soc/codecs/cs35l35.c
+++ b/sound/soc/codecs/cs35l35.c
@@ -741,8 +741,6 @@ static int cs35l35_codec_probe(struct snd_soc_codec *codec)
 	struct monitor_cfg *monitor_config = &cs35l35->pdata.mon_cfg;
 	int ret;
 
-	cs35l35->codec = codec;
-
 	/* Set Platform Data */
 	if (cs35l35->pdata.bst_vctl)
 		regmap_update_bits(cs35l35->regmap, CS35L35_BST_CVTR_V_CTL,
@@ -1004,7 +1002,6 @@ static struct regmap_config cs35l35_regmap = {
 static irqreturn_t cs35l35_irq(int irq, void *data)
 {
 	struct cs35l35_private *cs35l35 = data;
-	struct snd_soc_codec *codec = cs35l35->codec;
 	unsigned int sticky1, sticky2, sticky3, sticky4;
 	unsigned int mask1, mask2, mask3, mask4, current1;
 
@@ -1032,7 +1029,7 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 
 	/* handle the interrupts */
 	if (sticky1 & CS35L35_CAL_ERR) {
-		dev_crit(codec->dev, "Calibration Error\n");
+		dev_crit(cs35l35->dev, "Calibration Error\n");
 
 		/* error is no longer asserted; safe to reset */
 		if (!(current1 & CS35L35_CAL_ERR)) {
@@ -1051,10 +1048,10 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky1 & CS35L35_AMP_SHORT) {
-		dev_crit(codec->dev, "AMP Short Error\n");
+		dev_crit(cs35l35->dev, "AMP Short Error\n");
 		/* error is no longer asserted; safe to reset */
 		if (!(current1 & CS35L35_AMP_SHORT)) {
-			dev_dbg(codec->dev, "Amp short error release\n");
+			dev_dbg(cs35l35->dev, "Amp short error release\n");
 			regmap_update_bits(cs35l35->regmap,
 					CS35L35_PROT_RELEASE_CTL,
 					CS35L35_SHORT_RLS, 0);
@@ -1069,11 +1066,11 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky1 & CS35L35_OTW) {
-		dev_warn(codec->dev, "Over temperature warning\n");
+		dev_warn(cs35l35->dev, "Over temperature warning\n");
 
 		/* error is no longer asserted; safe to reset */
 		if (!(current1 & CS35L35_OTW)) {
-			dev_dbg(codec->dev, "Over temperature warn release\n");
+			dev_dbg(cs35l35->dev, "Over temperature warn release\n");
 			regmap_update_bits(cs35l35->regmap,
 					CS35L35_PROT_RELEASE_CTL,
 					CS35L35_OTW_RLS, 0);
@@ -1088,10 +1085,10 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky1 & CS35L35_OTE) {
-		dev_crit(codec->dev, "Over temperature error\n");
+		dev_crit(cs35l35->dev, "Over temperature error\n");
 		/* error is no longer asserted; safe to reset */
 		if (!(current1 & CS35L35_OTE)) {
-			dev_dbg(codec->dev, "Over temperature error release\n");
+			dev_dbg(cs35l35->dev, "Over temperature error release\n");
 			regmap_update_bits(cs35l35->regmap,
 					CS35L35_PROT_RELEASE_CTL,
 					CS35L35_OTE_RLS, 0);
@@ -1106,7 +1103,7 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky3 & CS35L35_BST_HIGH) {
-		dev_crit(codec->dev, "VBST error: powering off!\n");
+		dev_crit(cs35l35->dev, "VBST error: powering off!\n");
 		regmap_update_bits(cs35l35->regmap, CS35L35_PWRCTL2,
 			CS35L35_PDN_AMP, CS35L35_PDN_AMP);
 		regmap_update_bits(cs35l35->regmap, CS35L35_PWRCTL1,
@@ -1114,7 +1111,7 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky3 & CS35L35_LBST_SHORT) {
-		dev_crit(codec->dev, "LBST error: powering off!\n");
+		dev_crit(cs35l35->dev, "LBST error: powering off!\n");
 		regmap_update_bits(cs35l35->regmap, CS35L35_PWRCTL2,
 			CS35L35_PDN_AMP, CS35L35_PDN_AMP);
 		regmap_update_bits(cs35l35->regmap, CS35L35_PWRCTL1,
@@ -1122,13 +1119,13 @@ static irqreturn_t cs35l35_irq(int irq, void *data)
 	}
 
 	if (sticky2 & CS35L35_VPBR_ERR)
-		dev_dbg(codec->dev, "Error: Reactive Brownout\n");
+		dev_dbg(cs35l35->dev, "Error: Reactive Brownout\n");
 
 	if (sticky4 & CS35L35_VMON_OVFL)
-		dev_dbg(codec->dev, "Error: VMON overflow\n");
+		dev_dbg(cs35l35->dev, "Error: VMON overflow\n");
 
 	if (sticky4 & CS35L35_IMON_OVFL)
-		dev_dbg(codec->dev, "Error: IMON overflow\n");
+		dev_dbg(cs35l35->dev, "Error: IMON overflow\n");
 
 	return IRQ_HANDLED;
 }
@@ -1365,6 +1362,8 @@ static int cs35l35_i2c_probe(struct i2c_client *i2c_client,
 	if (!cs35l35)
 		return -ENOMEM;
 
+	cs35l35->dev = dev;
+
 	i2c_set_clientdata(i2c_client, cs35l35);
 	cs35l35->regmap = devm_regmap_init_i2c(i2c_client, &cs35l35_regmap);
 	if (IS_ERR(cs35l35->regmap)) {
diff --git a/sound/soc/codecs/cs35l35.h b/sound/soc/codecs/cs35l35.h
index c203081fc94c..156d2f0e6fd8 100644
--- a/sound/soc/codecs/cs35l35.h
+++ b/sound/soc/codecs/cs35l35.h
@@ -265,7 +265,7 @@
 			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
 
 struct  cs35l35_private {
-	struct snd_soc_codec *codec;
+	struct device *dev;
 	struct cs35l35_platform_data pdata;
 	struct regmap *regmap;
 	struct regulator_bulk_data supplies[2];
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-03-17 21:54 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-17 15:44 [PATCH] ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer Charles Keepax
2017-03-17 20:07 ` Austin, Brian
2017-03-17 21:54 ` Applied "ASoC: cs35l35: Stash dev pointer directly rather than CODEC pointer" to the asoc tree Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.