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

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(+)

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,
>

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html