ata: ahci_st: fixup layering violations / drvdata errors
Subsystems (2)
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.
---
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
@@ -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
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_datavariable 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 itdrivers/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:
Thanks,
Maxime
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.
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
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.
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_datavariable 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 itdrivers/ata/ahci_st.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)drivers/ata/ahci_st.c +0
-0
[...]
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
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_datavariable 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 itdrivers/ata/ahci_st.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)drivers/ata/ahci_st.c +0
-0[...]
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