diff mbox

pata_legacy: Allow disabling of legacy PATA device probes on non-PCI systems

Message ID 20170208194330.GB25826@htj.duckdns.org
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Tejun Heo Feb. 8, 2017, 7:43 p.m. UTC
Hello,

On Tue, Feb 07, 2017 at 12:21:37PM -0800, Gwendal Grignou wrote:
> I am wondering if we don't have a circular dependency:
> We do the final put_device (in scsi_host_put) on ap->scsi_host in
> ata_host_release(), but it is not called because
> [scsi_host]->shost_gendev.parent is &ap->tdev which hold the put on
> its parent, ap.
> 
> If my understanding is correct, as Tejun pointed out, removing the put
> on ap in ata_tport_release and the get_device(parent) in ata_tport_add
> should unlock the situation.

Heh, I'm not quite sure I follow but something like the following, right?

Matthew, can you please give this a try?

Thanks.

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

Comments

Gwendal Grignou Feb. 9, 2017, 11:49 p.m. UTC | #1
On Wed, Feb 8, 2017 at 11:43 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Tue, Feb 07, 2017 at 12:21:37PM -0800, Gwendal Grignou wrote:
>> I am wondering if we don't have a circular dependency:
>> We do the final put_device (in scsi_host_put) on ap->scsi_host in
>> ata_host_release(), but it is not called because
>> [scsi_host]->shost_gendev.parent is &ap->tdev which hold the put on
>> its parent, ap.
>>
>> If my understanding is correct, as Tejun pointed out, removing the put
>> on ap in ata_tport_release and the get_device(parent) in ata_tport_add
>> should unlock the situation.
>
> Heh, I'm not quite sure I follow but something like the following, right?
That's correct.
>
> Matthew, can you please give this a try?
>
> Thanks.
>
> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
> index 7ef16c0..20e2b7a 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -224,7 +224,6 @@ static DECLARE_TRANSPORT_CLASS(ata_port_class,
>
>  static void ata_tport_release(struct device *dev)
>  {
> -       put_device(dev->parent);
>  }
>
>  /**
> @@ -284,7 +283,7 @@ int ata_tport_add(struct device *parent,
>         device_initialize(dev);
>         dev->type = &ata_port_type;
>
> -       dev->parent = get_device(parent);
> +       dev->parent = parent;
>         dev->release = ata_tport_release;
>         dev_set_name(dev, "ata%d", ap->print_id);
>         transport_setup_device(dev);
> @@ -348,7 +347,6 @@ static DECLARE_TRANSPORT_CLASS(ata_link_class,
>
>  static void ata_tlink_release(struct device *dev)
>  {
> -       put_device(dev->parent);
>  }
>
>  /**
> @@ -410,7 +408,7 @@ int ata_tlink_add(struct ata_link *link)
>         int error;
>
>         device_initialize(dev);
> -       dev->parent = get_device(&ap->tdev);
> +       dev->parent = &ap->tdev;
>         dev->release = ata_tlink_release;
>         if (ata_is_host_link(link))
>                 dev_set_name(dev, "link%d", ap->print_id);
> @@ -589,7 +587,6 @@ static DECLARE_TRANSPORT_CLASS(ata_dev_class,
>
>  static void ata_tdev_release(struct device *dev)
>  {
> -       put_device(dev->parent);
>  }
>
>  /**
> @@ -662,7 +659,7 @@ static int ata_tdev_add(struct ata_device *ata_dev)
>         int error;
>
>         device_initialize(dev);
> -       dev->parent = get_device(&link->tdev);
> +       dev->parent = &link->tdev;
>         dev->release = ata_tdev_release;
>         if (ata_is_host_link(link))
>                 dev_set_name(dev, "dev%d.%d", ap->print_id,ata_dev->devno);
--
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
tedheadster Feb. 10, 2017, 12:36 a.m. UTC | #2
I just tested this patch and it is working for me.

- Matthew
--
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
Tejun Heo Feb. 10, 2017, 1:19 p.m. UTC | #3
On Thu, Feb 09, 2017 at 07:36:59PM -0500, tedheadster wrote:
> I just tested this patch and it is working for me.

Awesome, Gwendal, care to write up the path description?

Thanks.
tedheadster Feb. 23, 2017, 7:26 p.m. UTC | #4
Gwendal,
  could you submit a formal patch so we can get it in the next kernel?

- Matthew
--
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
diff mbox

Patch

diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 7ef16c0..20e2b7a 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -224,7 +224,6 @@  static DECLARE_TRANSPORT_CLASS(ata_port_class,
 
 static void ata_tport_release(struct device *dev)
 {
-	put_device(dev->parent);
 }
 
 /**
@@ -284,7 +283,7 @@  int ata_tport_add(struct device *parent,
 	device_initialize(dev);
 	dev->type = &ata_port_type;
 
-	dev->parent = get_device(parent);
+	dev->parent = parent;
 	dev->release = ata_tport_release;
 	dev_set_name(dev, "ata%d", ap->print_id);
 	transport_setup_device(dev);
@@ -348,7 +347,6 @@  static DECLARE_TRANSPORT_CLASS(ata_link_class,
 
 static void ata_tlink_release(struct device *dev)
 {
-	put_device(dev->parent);
 }
 
 /**
@@ -410,7 +408,7 @@  int ata_tlink_add(struct ata_link *link)
 	int error;
 
 	device_initialize(dev);
-	dev->parent = get_device(&ap->tdev);
+	dev->parent = &ap->tdev;
 	dev->release = ata_tlink_release;
 	if (ata_is_host_link(link))
 		dev_set_name(dev, "link%d", ap->print_id);
@@ -589,7 +587,6 @@  static DECLARE_TRANSPORT_CLASS(ata_dev_class,
 
 static void ata_tdev_release(struct device *dev)
 {
-	put_device(dev->parent);
 }
 
 /**
@@ -662,7 +659,7 @@  static int ata_tdev_add(struct ata_device *ata_dev)
 	int error;
 
 	device_initialize(dev);
-	dev->parent = get_device(&link->tdev);
+	dev->parent = &link->tdev;
 	dev->release = ata_tdev_release;
 	if (ata_is_host_link(link))
 		dev_set_name(dev, "dev%d.%d", ap->print_id,ata_dev->devno);