ata: ahci_st: fixup layering violations / drvdata errors

PATCH In review Brian Norris 5 participants 7 messages Apr 8, 2015 – Apr 20, 2015

Subsystems (2)

libata Arm/STI architecture

lore

Share & tools

lore

Brian Norris

author

2015-04-08 18:59 UTC #0 lore raw

PATCH 1/1 ata: ahci_st: fixup layering violations / drvdata errors

When working on another SATA driver that uses libahci_platform, I noticed an error in this driver; it tries to the the driver data for its device, while libata also thinks it can set the driver data. See:

ahci_platform_init_host()
-> ata_host_alloc_pinfo()

     -> ata_host_alloc()
        -> dev_set_drvdata()

So instead of sticking the IP-specific platform data into drvdata, let's use the plat_data variable that is reserved for this use.

Signed-off-by Brian Norris <[email protected]>

Cc Srinivas Kandagatla <[email protected]>

Cc Maxime Coquelin <[email protected]>

Cc Patrice Chotard <[email protected]>

---
This is ONLY compile tested; I don't have hardware to run. This looks like it could have ramifications on suspend/resume support, and hot device removal (e.g., sysfs unbind), so it might qualify as -stable, if someone can test it

 drivers/ata/ahci_st.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

drivers/ata/ahci_st.c +14 ~~-11~~

index bc971af262e7..2bd2375c2ab1 100644
@@ -37,7 +37,6 @@ struct st_ahci_drv_data {
 	struct reset_control *pwr;
 	struct reset_control *sw_rst;
 	struct reset_control *pwr_rst;
-	struct ahci_host_priv *hpriv;
 };
 
 static void st_ahci_configure_oob(void __iomem *mmio)

@@ -57,7 +56,9 @@ static void st_ahci_configure_oob(void __iomem *mmio)
 
 static int st_ahci_deassert_resets(struct device *dev)
 {
-	struct st_ahci_drv_data *drv_data = dev_get_drvdata(dev);
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct st_ahci_drv_data *drv_data = hpriv->plat_data;
 	int err;
 
 	if (drv_data->pwr) {

@@ -68,7 +69,7 @@ static int st_ahci_deassert_resets(struct device *dev)
 		}
 	}
 
-	st_ahci_configure_oob(drv_data->hpriv->mmio);
+	st_ahci_configure_oob(hpriv->mmio);
 
 	if (drv_data->sw_rst) {
 		err = reset_control_deassert(drv_data->sw_rst);

@@ -92,8 +93,8 @@ static int st_ahci_deassert_resets(struct device *dev)
 static void st_ahci_host_stop(struct ata_host *host)
 {
 	struct ahci_host_priv *hpriv = host->private_data;
+	struct st_ahci_drv_data *drv_data = hpriv->plat_data;
 	struct device *dev = host->dev;
-	struct st_ahci_drv_data *drv_data = dev_get_drvdata(dev);
 	int err;
 
 	if (drv_data->pwr) {

@@ -107,7 +108,9 @@ static void st_ahci_host_stop(struct ata_host *host)
 
 static int st_ahci_probe_resets(struct platform_device *pdev)
 {
-	struct st_ahci_drv_data *drv_data = platform_get_drvdata(pdev);
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct st_ahci_drv_data *drv_data = hpriv->plat_data;
 
 	drv_data->pwr = devm_reset_control_get(&pdev->dev, "pwr-dwn");
 	if (IS_ERR(drv_data->pwr)) {

@@ -161,8 +164,7 @@ static int st_ahci_probe(struct platform_device *pdev)
 	hpriv = ahci_platform_get_resources(pdev);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
-
-	drv_data->hpriv = hpriv;
+	hpriv->plat_data = drv_data;
 
 	err = st_ahci_probe_resets(pdev);
 	if (err)

@@ -185,8 +187,9 @@ static int st_ahci_probe(struct platform_device *pdev)
 #ifdef CONFIG_PM_SLEEP
 static int st_ahci_suspend(struct device *dev)
 {
-	struct st_ahci_drv_data *drv_data = dev_get_drvdata(dev);
-	struct ahci_host_priv *hpriv = drv_data->hpriv;
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct st_ahci_drv_data *drv_data = hpriv->plat_data;
 	int err;
 
 	err = ahci_platform_suspend_host(dev);

@@ -208,8 +211,8 @@ static int st_ahci_suspend(struct device *dev)
 
 static int st_ahci_resume(struct device *dev)
 {
-	struct st_ahci_drv_data *drv_data = dev_get_drvdata(dev);
-	struct ahci_host_priv *hpriv = drv_data->hpriv;
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
 	int err;
 
 	err = ahci_platform_enable_resources(hpriv);

Signature · 1 line

1.9.1

Maxime Coquelin

Acked-by on v1 1/1

2015-04-09 07:18 UTC #1 lore raw

Hello Brian,

Quoted 16 lines · Brian Norris

When working on another SATA driver that uses libahci_platform, I noticed an error in this driver; it tries to the the driver data for its device, while libata also thinks it can set the driver data. See:
   ahci_platform_init_host()
   -> ata_host_alloc_pinfo()
      -> ata_host_alloc()
         -> dev_set_drvdata()
So instead of sticking the IP-specific platform data into drvdata, let's use the plat_data variable that is reserved for this use.
Signed-off-by Brian Norris <[email protected]>
Cc Srinivas Kandagatla <[email protected]>
Cc Maxime Coquelin <[email protected]>
Cc Patrice Chotard <[email protected]>

Quoted 9 lines

---
This is ONLY compile tested; I don't have hardware to run. This looks like it could have ramifications on suspend/resume support, and hot device removal (e.g., sysfs unbind), so it might qualify as -stable, if someone can test it
  drivers/ata/ahci_st.c | 25 ++++++++++++++-----------
  1 file changed, 14 insertions(+), 11 deletions(-)

Your patch looks sensible, thanks for spotting this issue.

Peter, do you have the SATA setup in place to test Brian's patch?

Once tested, you can add my:

Acked-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Peter Griffin

2015-04-09 09:43 UTC #2 lore raw

Hi Maxime,

Quoted 2 lines · Maxime Coquelin

Hello Brian,

<snip>

Quoted 8 lines

Quoted 4 lines
 drivers/ata/ahci_st.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
Your patch looks sensible, thanks for spotting this issue.
Peter, do you have the SATA setup in place to test Brian's patch?

Yes I can test, although not until Saturday as I only have remote access to the board today & tomorrow and the hard drive isn't plugged in.

regards,

Peter.

Tejun Heo

2015-04-17 14:55 UTC #3 lore raw

Quoted 3 lines · Peter Griffin

Yes I can test, although not until Saturday as I only have remote access to the board today & tomorrow and the hard drive isn't plugged in.

What's the status on this patch? Should I push it through for-4.1-fixes?

Thanks.

Signature · 1 line

tejun

Peter Griffin

2015-04-20 13:04 UTC #4 lore raw

Hi Tejun,

Quoted 7 lines · Tejun Heo

Quoted 3 lines · Peter Griffin
Yes I can test, although not until Saturday as I only have remote access to the board today & tomorrow and the hard drive isn't plugged in.
What's the status on this patch? Should I push it through for-4.1-fixes?

The layering violation which Brian identified does need fixing. However please don't take this patch as it currently is, as it causes a NULL ptr dereference.

I can send an updated version in a moment which: -
1) Fixes the conflict you will get when applying it for v4.1 (due to my "st_configure_oob must be called after IP is clocked" patch).

2) Fixes the crash which this patch introduces.

regards,

Peter.

Sergei Shtylyov

2015-04-09 09:34 UTC #5 lore raw

In reply to Brian Norris

Hello.

Quoted 28 lines · Brian Norris

When working on another SATA driver that uses libahci_platform, I noticed an error in this driver; it tries to the the driver data for its device, while libata also thinks it can set the driver data. See:
   ahci_platform_init_host()
   -> ata_host_alloc_pinfo()
      -> ata_host_alloc()
         -> dev_set_drvdata()
So instead of sticking the IP-specific platform data into drvdata, let's use the plat_data variable that is reserved for this use.
Signed-off-by Brian Norris <[email protected]>
Cc Srinivas Kandagatla <[email protected]>
Cc Maxime Coquelin <[email protected]>
Cc Patrice Chotard <[email protected]>
---
This is ONLY compile tested; I don't have hardware to run. This looks like it could have ramifications on suspend/resume support, and hot device removal (e.g., sysfs unbind), so it might qualify as -stable, if someone can test it
  drivers/ata/ahci_st.c | 25 ++++++++++++++-----------
  1 file changed, 14 insertions(+), 11 deletions(-)
drivers/ata/ahci_st.c +0 -0
index bc971af262e7..2bd2375c2ab1 100644

[...]

drivers/ata/ahci_st.c static void st_ahci_host_stop(struct ata_host *host) · line 111

Quoted 6 lines

drivers/ata/ahci_st.c +1 -1

@@ -107,7 +108,9 @@ static void st_ahci_host_stop(struct ata_host *host)
 
  static int st_ahci_probe_resets(struct platform_device *pdev)
  {
-	struct st_ahci_drv_data *drv_data = platform_get_drvdata(pdev);
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);

Why not just platform_get_drvdata()?

[...]

MBR, Sergei

Brian Norris

author

2015-04-09 17:27 UTC #6 lore raw

Quoted 33 lines · Sergei Shtylyov

Hello.
Quoted 28 lines · Brian Norris
When working on another SATA driver that uses libahci_platform, I noticed an error in this driver; it tries to the the driver data for its device, while libata also thinks it can set the driver data. See:
ahci_platform_init_host()
-> ata_host_alloc_pinfo()
     -> ata_host_alloc()
        -> dev_set_drvdata()
So instead of sticking the IP-specific platform data into drvdata, let's use the plat_data variable that is reserved for this use.
Signed-off-by Brian Norris <[email protected]>
Cc Srinivas Kandagatla <[email protected]>
Cc Maxime Coquelin <[email protected]>
Cc Patrice Chotard <[email protected]>
---
This is ONLY compile tested; I don't have hardware to run. This looks like it could have ramifications on suspend/resume support, and hot device removal (e.g., sysfs unbind), so it might qualify as -stable, if someone can test it
 drivers/ata/ahci_st.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
drivers/ata/ahci_st.c +0 -0
index bc971af262e7..2bd2375c2ab1 100644
[...]

Quoted 8 lines

drivers/ata/ahci_st.c static void st_ahci_host_stop(struct ata_host *host) · line 111
Quoted 6 lines
drivers/ata/ahci_st.c +1 -1
@@ -107,7 +108,9 @@ static void st_ahci_host_stop(struct ata_host *host)
 
 static int st_ahci_probe_resets(struct platform_device *pdev)
 {
-	struct st_ahci_drv_data *drv_data = platform_get_drvdata(pdev);
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
Why not just platform_get_drvdata()?

Just being consistent, as that's what the rest of the driver uses. It seems like it'd be easy to misconstrue dev_get_drvrdata(&pdev->dev) and platform_get_drvdata(pdev) as two different things (e.g., when grepping), when in fact they are the same. Seems like a needless source of potential confusion.

Why should I use platform_get_drvdata()?

(NB: I didn't notice at first that the driver already was using platform_get_drvdata(). I pretty much ignored anything that was already there, since it was all wrong anyway.)

Anyway, I don't care either way, and in fact, I don't care about this driver much at all as I never expect to use it; I just noticed the defect when reading.

Brian