[PATCH v3 1/4] sata_highbank: utilize common ata_platform_remove_one()

[PATCH v3 1/4] sata_highbank: utilize common ata_platform_remove_one()

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
v2: fix whitespace
v3: Resend to fix a trivial conflict

(Note: This patch is on its 3rd version, where others are v2.)

 drivers/ata/sata_highbank.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index 75c0f3c..dc7d78e 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -368,16 +368,6 @@ err0:
 	return rc;
 }
 
-static int __devexit ahci_highbank_remove(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct ata_host *host = dev_get_drvdata(dev);
-
-	ata_host_detach(host);
-
-	return 0;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int ahci_highbank_suspend(struct device *dev)
 {
@@ -432,7 +422,7 @@ SIMPLE_DEV_PM_OPS(ahci_highbank_pm_ops,
 		  ahci_highbank_suspend, ahci_highbank_resume);
 
 static struct platform_driver ahci_highbank_driver = {
-        .remove = __devexit_p(ahci_highbank_remove),
+	.remove = ata_platform_remove_one,
         .driver = {
                 .name = "highbank-ahci",
                 .owner = THIS_MODULE,
-- 
1.8.0

[PATCH v2 2/4] pata_octeon_cf: perform host detach, removal on exit

[Brian Norris]

This driver does not detach and remove its ata_host properly on device
removal. Add the common .remove helper.

Note: I do not know this driver well enough to ensure this is the right
thing to do. Merge this patch with caution.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Acked-by: Tejun Heo <tj@kernel.org>
Cc: David Daney <david.daney@cavium.com>
---
v2: no change (rebased along with previous patch)

 drivers/ata/pata_octeon_cf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ata/pata_octeon_cf.c b/drivers/ata/pata_octeon_cf.c
index 1d61d5d..d8df93b 100644
--- a/drivers/ata/pata_octeon_cf.c
+++ b/drivers/ata/pata_octeon_cf.c
@@ -921,6 +921,7 @@ free_cf_port:
 
 static struct platform_driver octeon_cf_driver = {
 	.probe		= octeon_cf_probe,
+	.remove		= ata_platform_remove_one,
 	.driver		= {
 		.name	= DRV_NAME,
 		.owner	= THIS_MODULE,
-- 
1.8.0

Re: [PATCH v2 2/4] pata_octeon_cf: perform host detach, removal on exit

[David Daney]

On 12/03/2012 10:34 AM, Brian Norris wrote:
> This driver does not detach and remove its ata_host properly on device
> removal. Add the common .remove helper.
>
> Note: I do not know this driver well enough to ensure this is the right
> thing to do. Merge this patch with caution.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Acked-by: Tejun Heo <tj@kernel.org>
> Cc: David Daney <david.daney@cavium.com>
> ---
> v2: no change (rebased along with previous patch)
>
>   drivers/ata/pata_octeon_cf.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/ata/pata_octeon_cf.c b/drivers/ata/pata_octeon_cf.c
> index 1d61d5d..d8df93b 100644
> --- a/drivers/ata/pata_octeon_cf.c
> +++ b/drivers/ata/pata_octeon_cf.c
> @@ -921,6 +921,7 @@ free_cf_port:
>
>   static struct platform_driver octeon_cf_driver = {
>   	.probe		= octeon_cf_probe,
> +	.remove		= ata_platform_remove_one,

Can you point me at the definition of ata_platform_remove_one()?

I can seem to find it.  Without knowing what that does, I would be 
inclined to NACK the whole thing.

How did you test the patch?

This patch is likely to be incomplete as the driver is also missing the 
module_exit() things.

It might be simpler to just make the driver "bool" instead of "tristate" 
in the Kconfig.

>   	.driver		= {
>   		.name	= DRV_NAME,
>   		.owner	= THIS_MODULE,
>

Re: [PATCH v2 2/4] pata_octeon_cf: perform host detach, removal on exit

[Brian Norris]

Jeff,

Per David's comments, I recommend that this patch be dropped from inclusion.

Hi David,

On Mon, Dec 3, 2012 at 4:26 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 12/03/2012 10:34 AM, Brian Norris wrote:
>> This driver does not detach and remove its ata_host properly on device
>> removal. Add the common .remove helper.
>>
>> Note: I do not know this driver well enough to ensure this is the right
>> thing to do. Merge this patch with caution.
>>
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>> Acked-by: Tejun Heo <tj@kernel.org>
>> Cc: David Daney <david.daney@cavium.com>
>> ---
>> v2: no change (rebased along with previous patch)
>>
>>   drivers/ata/pata_octeon_cf.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/ata/pata_octeon_cf.c b/drivers/ata/pata_octeon_cf.c
>> index 1d61d5d..d8df93b 100644
>> --- a/drivers/ata/pata_octeon_cf.c
>> +++ b/drivers/ata/pata_octeon_cf.c
>> @@ -921,6 +921,7 @@ free_cf_port:
>>
>>   static struct platform_driver octeon_cf_driver = {
>>         .probe          = octeon_cf_probe,
>> +       .remove         = ata_platform_remove_one,
>
>
> Can you point me at the definition of ata_platform_remove_one()?

It's a recent addition, and this series (v2/v3) is just part of the
already-accepted series. Jeff already queued it all up in
libata-dev.git:
https://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=shortlog;h=refs/heads/ALL

> I can seem to find it.  Without knowing what that does, I would be inclined
> to NACK the whole thing.

A NACK is probably the right thing. I was mostly converting a few
other drivers which used some simple, common patterns to use my new
common code, but this driver was missing it altogether. It looks like
there may be bigger issues, though, as you point out.

> How did you test the patch?

I didn't; I don't have a cavium-octeon system.

> This patch is likely to be incomplete as the driver is also missing the
> module_exit() things.
>
> It might be simpler to just make the driver "bool" instead of "tristate" in
> the Kconfig.

As noted earlier, I don't have much interest in this driver. I agree
that there are some other issues with the driver; I think it leaks
memory if it is ever allowed to unload, for one. Feel free to submit
an alternative patch to prevent this driver from being built as a
module.

Brian

Re: [PATCH v2 2/4] pata_octeon_cf: perform host detach, removal on exit

[Jeff Garzik]

On 12/04/2012 01:10 AM, Brian Norris wrote:
>> >I can seem to find it.  Without knowing what that does, I would be inclined
>> >to NACK the whole thing.
> A NACK is probably the right thing. I was mostly converting a few
> other drivers which used some simple, common patterns to use my new
> common code, but this driver was missing it altogether. It looks like
> there may be bigger issues, though, as you point out.
>
>> >How did you test the patch?
> I didn't; I don't have a cavium-octeon system.
>
>> >This patch is likely to be incomplete as the driver is also missing the
>> >module_exit() things.
>> >
>> >It might be simpler to just make the driver "bool" instead of "tristate" in
>> >the Kconfig.
> As noted earlier, I don't have much interest in this driver. I agree
> that there are some other issues with the driver; I think it leaks
> memory if it is ever allowed to unload, for one. Feel free to submit
> an alternative patch to prevent this driver from being built as a
> module.

reverted

[PATCH v2 3/4] libata: use pci_get_drvdata() helper

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
v2: no change (rebased along with previous patches)

 drivers/ata/libata-core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8e3f4a9..47d5961 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6287,8 +6287,7 @@ void ata_host_detach(struct ata_host *host)
  */
 void ata_pci_remove_one(struct pci_dev *pdev)
 {
-	struct device *dev = &pdev->dev;
-	struct ata_host *host = dev_get_drvdata(dev);
+	struct ata_host *host = pci_get_drvdata(pdev);
 
 	ata_host_detach(host);
 }
@@ -6357,7 +6356,7 @@ int ata_pci_device_do_resume(struct pci_dev *pdev)
 
 int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
-	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct ata_host *host = pci_get_drvdata(pdev);
 	int rc = 0;
 
 	rc = ata_host_suspend(host, mesg);
@@ -6371,7 +6370,7 @@ int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
 
 int ata_pci_device_resume(struct pci_dev *pdev)
 {
-	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct ata_host *host = pci_get_drvdata(pdev);
 	int rc;
 
 	rc = ata_pci_device_do_resume(pdev);
-- 
1.8.0

[PATCH v2 4/4] pata_of_platform: fix compile error

[Brian Norris]

I failed to include <linux/libata.h>, causing this error:

  drivers/ata/pata_of_platform.c:93: error: 'ata_platform_remove_one' undeclared here (not in a function)

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/ata/pata_of_platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index cd0f9da..e5b234c 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -14,6 +14,7 @@
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/ata_platform.h>
+#include <linux/libata.h>
 
 static int __devinit pata_of_platform_probe(struct platform_device *ofdev)
 {
-- 
1.8.0

Re: [PATCH v3 1/4] sata_highbank: utilize common ata_platform_remove_one()

[Jeff Garzik]

On 12/03/2012 01:34 PM, Brian Norris wrote:
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Acked-by: Tejun Heo <tj@kernel.org>
> ---
> v2: fix whitespace
> v3: Resend to fix a trivial conflict
>
> (Note: This patch is on its 3rd version, where others are v2.)
>
>   drivers/ata/sata_highbank.c | 12 +-----------
>   1 file changed, 1 insertion(+), 11 deletions(-)

applied 1-4