Change in sysfs topology for libata

Change in sysfs topology for libata

[Gwendal Grignou]

Lin,

 Commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a introduces a change
in sysfs directory for ata controller:

Before:
           /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
           |-- ata1                                (ata port)
           |-- host0                               (scsi host)
              |-- target0:0:0                      (scsi target)
                  |-- 0:0:0:0                      (disk)

After:
           /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
           |-- ata1                                (ata port)
               |-- host0                           (scsi host)
                   |-- target0:0:0                 (scsi target)
                       |-- 0:0:0:0                 (disk)

The problem is an ata controller, managed by libata, is still
considered by the kernel as a SCSI controller: diverging even more
from other SCSI controller sysfs layout is not a good idea.
When I wrote libata-transport.c, my plan was to be consistent with SAS
objects: for instance, for a SAS controller with a simple SAS topology
we have:


/sys/bus/pci/devices/0000\:0c\:00.0/
|-- host7/
    |-- phy-7:0
    |-- phy-7:1
..
    |-- phy-7:7
    |-- port-7:0
        |-- end_device-7\:0
            |-- target7:0:0/
                |-- 7:0:0:0
    |-- port-7:1
..

Similarly, I wanted to represent an ata_port [equivalent to SAS phy]
and ata_link [equivalent to SAS port] under scsi_host hostX object,
but as far as i remember, when I wrote the code that was not possible,
I could not find a clean way to share object references between libata
and scsi layer. That's why ata_port object was sitting alongside to
the scsi_host object. I have to see if it is possible now.

In the meantime, what would be the impact to revert that commit?
Do you think melting libata sysfs object into scsi sysfs objects would
work with your changes related to pm?

Thanks,

Gwendal.

Re: Change in sysfs topology for libata

[Gwendal Grignou]

[-Lin, +Lin]

On Wed, Sep 12, 2012 at 1:02 PM, Gwendal Grignou <gwendal@google.com> wrote:
> Lin,
>
>  Commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a introduces a change
> in sysfs directory for ata controller:
>
> Before:
>            /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
>            |-- ata1                                (ata port)
>            |-- host0                               (scsi host)
>               |-- target0:0:0                      (scsi target)
>                   |-- 0:0:0:0                      (disk)
>
> After:
>            /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
>            |-- ata1                                (ata port)
>                |-- host0                           (scsi host)
>                    |-- target0:0:0                 (scsi target)
>                        |-- 0:0:0:0                 (disk)
>
> The problem is an ata controller, managed by libata, is still
> considered by the kernel as a SCSI controller: diverging even more
> from other SCSI controller sysfs layout is not a good idea.
> When I wrote libata-transport.c, my plan was to be consistent with SAS
> objects: for instance, for a SAS controller with a simple SAS topology
> we have:
>
>
> /sys/bus/pci/devices/0000\:0c\:00.0/
> |-- host7/
>     |-- phy-7:0
>     |-- phy-7:1
> ..
>     |-- phy-7:7
>     |-- port-7:0
>         |-- end_device-7\:0
>             |-- target7:0:0/
>                 |-- 7:0:0:0
>     |-- port-7:1
> ..
>
> Similarly, I wanted to represent an ata_port [equivalent to SAS phy]
> and ata_link [equivalent to SAS port] under scsi_host hostX object,
> but as far as i remember, when I wrote the code that was not possible,
> I could not find a clean way to share object references between libata
> and scsi layer. That's why ata_port object was sitting alongside to
> the scsi_host object. I have to see if it is possible now.
>
> In the meantime, what would be the impact to revert that commit?
> Do you think melting libata sysfs object into scsi sysfs objects would
> work with your changes related to pm?
>
> Thanks,
>
> Gwendal.

Re: Change in sysfs topology for libata

[Lin Ming]

On Thu, Sep 13, 2012 at 4:07 AM, Gwendal Grignou <gwendal@google.com> wrote:
> [-Lin, +Lin]

CC Aaron@Intel

>
> On Wed, Sep 12, 2012 at 1:02 PM, Gwendal Grignou <gwendal@google.com> wrote:
>> Lin,
>>
>>  Commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a introduces a change
>> in sysfs directory for ata controller:
>>
>> Before:
>>            /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
>>            |-- ata1                                (ata port)
>>            |-- host0                               (scsi host)
>>               |-- target0:0:0                      (scsi target)
>>                   |-- 0:0:0:0                      (disk)
>>
>> After:
>>            /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
>>            |-- ata1                                (ata port)
>>                |-- host0                           (scsi host)
>>                    |-- target0:0:0                 (scsi target)
>>                        |-- 0:0:0:0                 (disk)
>>
>> The problem is an ata controller, managed by libata, is still
>> considered by the kernel as a SCSI controller: diverging even more
>> from other SCSI controller sysfs layout is not a good idea.
>> When I wrote libata-transport.c, my plan was to be consistent with SAS
>> objects: for instance, for a SAS controller with a simple SAS topology
>> we have:
>>
>>
>> /sys/bus/pci/devices/0000\:0c\:00.0/
>> |-- host7/
>>     |-- phy-7:0
>>     |-- phy-7:1
>> ..
>>     |-- phy-7:7
>>     |-- port-7:0
>>         |-- end_device-7\:0
>>             |-- target7:0:0/
>>                 |-- 7:0:0:0
>>     |-- port-7:1
>> ..
>>
>> Similarly, I wanted to represent an ata_port [equivalent to SAS phy]
>> and ata_link [equivalent to SAS port] under scsi_host hostX object,
>> but as far as i remember, when I wrote the code that was not possible,
>> I could not find a clean way to share object references between libata
>> and scsi layer. That's why ata_port object was sitting alongside to
>> the scsi_host object. I have to see if it is possible now.
>>
>> In the meantime, what would be the impact to revert that commit?
>> Do you think melting libata sysfs object into scsi sysfs objects would
>> work with your changes related to pm?

The SATA disk runtime pm and ZPODD(Zero Power ODD) relies on the new
sysfs directory I introduced in commit 9a6d6a2.

Could you show me what's the sysfs directory structure after libata
sysfs object melted into scsi sysfs objects?

Thanks,
Lin Ming

>>
>> Thanks,
>>
>> Gwendal.

Re: Change in sysfs topology for libata

[Gwendal Grignou]

On Thu, Sep 13, 2012 at 8:38 AM, Lin Ming <minggr@gmail.com> wrote:
> On Thu, Sep 13, 2012 at 4:07 AM, Gwendal Grignou <gwendal@google.com> wrote:
>> [-Lin, +Lin]
>
> CC Aaron@Intel
>
>>
>> On Wed, Sep 12, 2012 at 1:02 PM, Gwendal Grignou <gwendal@google.com> wrote:
>>> Lin,
>>>
>>>  Commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a introduces a change
>>> in sysfs directory for ata controller:
>>>
>>> Before:
>>>            /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
>>>            |-- ata1                                (ata port)
>>>            |-- host0                               (scsi host)
>>>               |-- target0:0:0                      (scsi target)
>>>                   |-- 0:0:0:0                      (disk)
>>>
>>> After:
>>>            /sys/devices/pci0000:00/0000:00:1f.2    (ahci controller)
>>>            |-- ata1                                (ata port)
>>>                |-- host0                           (scsi host)
>>>                    |-- target0:0:0                 (scsi target)
>>>                        |-- 0:0:0:0                 (disk)
>>>
>>> The problem is an ata controller, managed by libata, is still
>>> considered by the kernel as a SCSI controller: diverging even more
>>> from other SCSI controller sysfs layout is not a good idea.
>>> When I wrote libata-transport.c, my plan was to be consistent with SAS
>>> objects: for instance, for a SAS controller with a simple SAS topology
>>> we have:
>>>
>>>
>>> /sys/bus/pci/devices/0000\:0c\:00.0/
>>> |-- host7/
>>>     |-- phy-7:0
>>>     |-- phy-7:1
>>> ..
>>>     |-- phy-7:7
>>>     |-- port-7:0
>>>         |-- end_device-7\:0
>>>             |-- target7:0:0/
>>>                 |-- 7:0:0:0
>>>     |-- port-7:1
>>> ..
>>>
>>> Similarly, I wanted to represent an ata_port [equivalent to SAS phy]
>>> and ata_link [equivalent to SAS port] under scsi_host hostX object,
>>> but as far as i remember, when I wrote the code that was not possible,
>>> I could not find a clean way to share object references between libata
>>> and scsi layer. That's why ata_port object was sitting alongside to
>>> the scsi_host object. I have to see if it is possible now.
>>>
>>> In the meantime, what would be the impact to revert that commit?
>>> Do you think melting libata sysfs object into scsi sysfs objects would
>>> work with your changes related to pm?
>
> The SATA disk runtime pm and ZPODD(Zero Power ODD) relies on the new
> sysfs directory I introduced in commit 9a6d6a2.
>
> Could you show me what's the sysfs directory structure after libata
> sysfs object melted into scsi sysfs objects?

I am testing my patches, but the path to a block device will be as follow:
/sys/device/pci0000:00/0000:00:06.0/0000:09:00.0/host0/port1/link1/dev1.0/target0:0:0/0:0:0:0/block/sda

If you have a port multiplier, a disk behind it - port 4 of the pmp
for instance:
/sys/device/pci0000:00/0000:00:06.0/0000:09:00.0/host5/port6/link6.4/dev6.4.0/target5:4:0/5:4:0:0/block/sde
while the port multiplier itself would be at
/sys/device/pci0000:00/0000:00:06.0/0000:09:00.0/host5/port6/link6/dev6.0

For reference, in a simple SAS topology, block device paths are like:
/sys/device/pci0000:00/0000:00:08.0/0000:0b:00.0/host6/port-6:5/end_device-6:5/target6:0:5/6:0:5:0/block/sdg

Does it make sense to you?

Gwendal.



>
> Thanks,
> Lin Ming
>
>>>
>>> Thanks,
>>>
>>> Gwendal.

Re: Change in sysfs topology for libata

[Aaron Lu]

On Wed, Sep 19, 2012 at 05:48:50PM -0700, Gwendal Grignou wrote:
> On Thu, Sep 13, 2012 at 8:38 AM, Lin Ming <minggr@gmail.com> wrote:
> > On Thu, Sep 13, 2012 at 4:07 AM, Gwendal Grignou <gwendal@google.com> wrote:
> >> [-Lin, +Lin]
> >
> > CC Aaron@Intel

Thanks Lin.

> > Could you show me what's the sysfs directory structure after libata
> > sysfs object melted into scsi sysfs objects?
> 
> I am testing my patches, but the path to a block device will be as follow:
> /sys/device/pci0000:00/0000:00:06.0/0000:09:00.0/host0/port1/link1/dev1.0/target0:0:0/0:0:0:0/block/sda
> 
> If you have a port multiplier, a disk behind it - port 4 of the pmp
> for instance:
> /sys/device/pci0000:00/0000:00:06.0/0000:09:00.0/host5/port6/link6.4/dev6.4.0/target5:4:0/5:4:0:0/block/sde
> while the port multiplier itself would be at
> /sys/device/pci0000:00/0000:00:06.0/0000:09:00.0/host5/port6/link6/dev6.0
> 
> For reference, in a simple SAS topology, block device paths are like:
> /sys/device/pci0000:00/0000:00:08.0/0000:0b:00.0/host6/port-6:5/end_device-6:5/target6:0:5/6:0:5:0/block/sdg
> 
> Does it make sense to you?

Looks good to me. I will test the code once you sent out, thanks.

And I think the following commit will no longer be needed once this
change is done:
commit ae0751ffc77e7f21629970fdab5528c573e637f8
[SCSI] add flag to skip the runtime PM calls on the host

-Aaron

[PATCH 0/3] Insert ATA transport objects in SCSI syfs topology.

[Gwendal Grignou]

This set of patches improve ATA transport classes integration with SCSI
objects.

Before [2.6.x]

Ata and scsi transport class where separated:
`--0000:09:00.0
|  `--ata1
|  |  `--port_port
|  |  `--link1
|  |  |  `--dev1.0
|  |  |  `--dev1.1
|  `--ata2
|  ...
|  `--host0
|  |  `--scsi_host
|  |  |  `--host0
|  |  `--target0:0:0
|  |  |  `--0:0:0:0
|  |  |  |  `--block
|  |  |  |  |  `--sda
|  |  |  |  |  |  `--sda1

In 3.2, Lin - in commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a - addressed
the issue of linking the ata port with the scsi host object by placing the
scsi_host object under ata port objects.

However to be more consistent with other transport, this patch does the opposite:

For instance, with SAS transport, We have
`--0000:0b:00.0
|  `--host6
|  |  `--phy-6:0
|  |  `--phy-6:1
...
|  |  `--port-6:0
|  |  |  `--end_device-6:0
|  |  |  |  `--sas_device
|  |  |  |  |  `--end_device-6:0
|  |  |  |  `--sas_end_device
|  |  |  |  |  `--end_device-6:0
|  |  |  |  `--target6:0:0
|  |  |  |  |  `--6:0:0:0
|  |  |  |  |  |  `--block
|  |  |  |  |  |  |  `--sdb
...
|  |  `--port-6:1
|  |  |  `--end_device-6:1
...
phy and port have to be separated, sas_port are created dynamically.

For ata, all objects are created at initialization time, so the layout is:
`--0000:09:00.0
|  `--host0
|  |  `--port1
|  |  |  `--link1
|  |  |  |  `--dev1.0
|  |  |  |  |  `--target0:0:0
|  |  |  |  |  |  `--0:0:0:0
|  |  |  |  |  |  |  `--block
|  |  |  |  |  |  |  |  `--sda

If we have a port multiplier, more links are created.
`--0000:09:00.0
...
|  `--host4
|  |  `--port5
|  |  |  `--link5
|  |  |  |  `--dev5.0
[device for the port multiplier]
|  |  |  `--link5.0
|  |  |  |  `--dev5.0.0
|  |  |  |  |  `--target4:0:0
|  |  |  |  |  |  `--4:0:0:0
|  |  |  |  |  |  |  `--block
|  |  |  |  |  |  |  |  `--sdc
[disk in port 0 of the port multiplier]
...
|  |  |  `--link5.2
|  |  |  |  `--dev5.2.0
|  |  |  |  |  `--target4:2:0
|  |  |  |  |  |  `--4:2:0:0
|  |  |  |  |  |  |  `--block
|  |  |  |  |  |  |  |  `--sde
[disk in port 2 of the port multiplier]

In consequence, the path of a scsi device becomes:
.../0000:00:1f.2/host0/ata1/link1/dev1.0/target0:0:0/0:0:0:0
dev1.0 indicates the master device [0] in ata port 1.
ata1 being under host0, we know the reliationships between the scsi_host id and
ata port id.

or when a port multiplier is present: for instance the device in port 4 of the
port multiplier:
.../0000:00:06.0/0000:09:00.0/host5/ata6/link6.4/dev6.4.0/target5:4:0/5:4:0:0

Gwendal Grignou (3):
  Revert "ata: make ata port as parent device of scsi host"
  scsi: Allow devices to have arbitrary parent
  libata: Change transport topology layout

 drivers/ata/libata-core.c      |   13 ++++++-------
 drivers/ata/libata-scsi.c      |    4 ++--
 drivers/ata/libata-transport.c |    2 +-
 drivers/firewire/sbp2.c        |    3 ++-
 drivers/message/i2o/i2o_scsi.c |    4 ++--
 drivers/scsi/scsi_scan.c       |    9 +++++----
 include/scsi/scsi_device.h     |    2 +-
 7 files changed, 19 insertions(+), 18 deletions(-)

-- 
1.7.7.3

[PATCH 1/3] Revert "ata: make ata port as parent device of scsi host"

[Gwendal Grignou]

This reverts commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a.

Instead, melt libata transport sysfs object in scsi objects.

Change-Id: I8c709f63ddf7ba97b9e6f449d5c0b8b85e44e818

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-scsi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e3bda07..be38930 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3609,7 +3609,7 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 		shost->max_host_blocked = 1;
 
 		rc = scsi_add_host_with_dma(ap->scsi_host,
-						&ap->tdev, ap->host->dev);
+					    host->dev, host->dev);
 		if (rc)
 			goto err_add;
 	}
-- 
1.7.7.3

[PATCH 2/3] scsi: Allow devices to have arbitrary parent

[Gwendal Grignou]

Allow driver who calls __scsi_add_device directly to create the scsi
device on any parent, not just scsi_host directly.
This is alreay done for transport with their own class [SAS, iSCSI, FC, ...]

Change-Id: Ibcf132a8959fbf732dcd0b34a7f4a570d7cf394d

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-scsi.c      |    4 ++--
 drivers/firewire/sbp2.c        |    3 ++-
 drivers/message/i2o/i2o_scsi.c |    4 ++--
 drivers/scsi/scsi_scan.c       |    9 +++++----
 include/scsi/scsi_device.h     |    2 +-
 5 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index be38930..bfda61f 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3649,8 +3649,8 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 			else
 				channel = link->pmp;
 
-			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
-						 NULL);
+			sdev = __scsi_add_device(&ap->scsi_host->shost_gendev,
+						 channel, id, 0, NULL);
 			if (!IS_ERR(sdev)) {
 				dev->sdev = sdev;
 				scsi_device_put(sdev);
diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 1162d6b..839afa5 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -879,7 +879,8 @@ static void sbp2_login(struct work_struct *work)
 		ssleep(SBP2_INQUIRY_DELAY);
 
 	shost = container_of((void *)tgt, struct Scsi_Host, hostdata[0]);
-	sdev = __scsi_add_device(shost, 0, 0, sbp2_lun2int(lu->lun), lu);
+	sdev = __scsi_add_device(&shost->shost_gendev, 0, 0,
+				 sbp2_lun2int(lu->lun), lu);
 	/*
 	 * FIXME:  We are unable to perform reconnects while in sbp2_login().
 	 * Therefore __scsi_add_device() will get into trouble if a bus reset
diff --git a/drivers/message/i2o/i2o_scsi.c b/drivers/message/i2o/i2o_scsi.c
index 1d31d72..ee1353c 100644
--- a/drivers/message/i2o/i2o_scsi.c
+++ b/drivers/message/i2o/i2o_scsi.c
@@ -294,8 +294,8 @@ static int i2o_scsi_probe(struct device *dev)
 	}
 
 	scsi_dev =
-	    __scsi_add_device(i2o_shost->scsi_host, channel, le32_to_cpu(id),
-			      le64_to_cpu(lun), i2o_dev);
+	    __scsi_add_device(&i2o_shost->scsi_host->shost_gendev, channel,
+			      le32_to_cpu(id), le64_to_cpu(lun), i2o_dev);
 
 	if (IS_ERR(scsi_dev)) {
 		osm_warn("can not add SCSI device %03x\n",
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 56a9379..105123c 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1489,11 +1489,11 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	return ret;
 }
 
-struct scsi_device *__scsi_add_device(struct Scsi_Host *shost, uint channel,
+struct scsi_device *__scsi_add_device(struct device *parent, uint channel,
 				      uint id, uint lun, void *hostdata)
 {
+	struct Scsi_Host *shost = dev_to_shost(parent);
 	struct scsi_device *sdev = ERR_PTR(-ENODEV);
-	struct device *parent = &shost->shost_gendev;
 	struct scsi_target *starget;
 
 	if (strncmp(scsi_scan_type, "none", 4) == 0)
@@ -1524,8 +1524,9 @@ EXPORT_SYMBOL(__scsi_add_device);
 int scsi_add_device(struct Scsi_Host *host, uint channel,
 		    uint target, uint lun)
 {
-	struct scsi_device *sdev = 
-		__scsi_add_device(host, channel, target, lun, NULL);
+	struct scsi_device *sdev =
+		__scsi_add_device(&host->shost_gendev, channel, target,
+				  lun, NULL);
 	if (IS_ERR(sdev))
 		return PTR_ERR(sdev);
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9895f69..9646a1d 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -285,7 +285,7 @@ static inline struct scsi_target *scsi_target(struct scsi_device *sdev)
 #define starget_printk(prefix, starget, fmt, a...)	\
 	dev_printk(prefix, &(starget)->dev, fmt, ##a)
 
-extern struct scsi_device *__scsi_add_device(struct Scsi_Host *,
+extern struct scsi_device *__scsi_add_device(struct device *,
 		uint, uint, uint, void *hostdata);
 extern int scsi_add_device(struct Scsi_Host *host, uint channel,
 			   uint target, uint lun);
-- 
1.7.7.3

[PATCH 3/3] libata: Change transport topology layout

[Gwendal Grignou]

Integrate ata objects [port, link, device] with scsi objects.


The path of a scsi device is:
.../0000:00:1f.2/host0/ata1/link1/dev1.0/target0:0:0/0:0:0:0

or when a port multiplier is present: for instance the device in port 4 of the
port multiplier:
.../0000:00:06.0/0000:09:00.0/host5/ata6/link6.4/dev6.4.0/target5:4:0/5:4:0:0


Change-Id: I202e089208e8746ccdaf2053d43da831a0c0976d

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-core.c      |   13 ++++++-------
 drivers/ata/libata-scsi.c      |    4 ++--
 drivers/ata/libata-transport.c |    2 +-
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 611050d..c83590b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6063,19 +6063,18 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 	for (i = 0; i < host->n_ports; i++)
 		host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
 
+	rc = ata_scsi_add_hosts(host, sht);
+	if (rc)
+		return rc;
 
 	/* Create associated sysfs transport objects  */
 	for (i = 0; i < host->n_ports; i++) {
-		rc = ata_tport_add(host->dev,host->ports[i]);
-		if (rc) {
+		struct ata_port *ap = host->ports[i];
+		rc = ata_tport_add(&ap->scsi_host->shost_gendev, ap);
+		if (rc)
 			goto err_tadd;
-		}
 	}
 
-	rc = ata_scsi_add_hosts(host, sht);
-	if (rc)
-		goto err_tadd;
-
 	/* set cable, sata_spd_limit and report */
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bfda61f..9023bb1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3649,8 +3649,8 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 			else
 				channel = link->pmp;
 
-			sdev = __scsi_add_device(&ap->scsi_host->shost_gendev,
-						 channel, id, 0, NULL);
+			sdev = __scsi_add_device(&dev->tdev, channel, id, 0,
+						 NULL);
 			if (!IS_ERR(sdev)) {
 				dev->sdev = sdev;
 				scsi_device_put(sdev);
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index c04d393..6829be6 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -284,7 +284,7 @@ int ata_tport_add(struct device *parent,
 
 	dev->parent = get_device(parent);
 	dev->release = ata_tport_release;
-	dev_set_name(dev, "ata%d", ap->print_id);
+	dev_set_name(dev, "port%d", ap->print_id);
 	transport_setup_device(dev);
 	error = device_add(dev);
 	if (error) {
-- 
1.7.7.3

Re: [PATCH 0/3] Insert ATA transport objects in SCSI syfs topology.

[Aaron Lu]

On Thu, Sep 27, 2012 at 12:04:01PM -0700, Gwendal Grignou wrote:
> This set of patches improve ATA transport classes integration with SCSI
> objects.
> 
> Before [2.6.x]
> 
> Ata and scsi transport class where separated:
> `--0000:09:00.0
> |  `--ata1
> |  |  `--port_port
> |  |  `--link1
> |  |  |  `--dev1.0
> |  |  |  `--dev1.1
> |  `--ata2
> |  ...
> |  `--host0
> |  |  `--scsi_host
> |  |  |  `--host0
> |  |  `--target0:0:0
> |  |  |  `--0:0:0:0
> |  |  |  |  `--block
> |  |  |  |  |  `--sda
> |  |  |  |  |  |  `--sda1
> 
> In 3.2, Lin - in commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a - addressed
> the issue of linking the ata port with the scsi host object by placing the
> scsi_host object under ata port objects.
> 
> However to be more consistent with other transport, this patch does the opposite:
> 
> For instance, with SAS transport, We have
> `--0000:0b:00.0
> |  `--host6
> |  |  `--phy-6:0
> |  |  `--phy-6:1
> ...
> |  |  `--port-6:0
> |  |  |  `--end_device-6:0
> |  |  |  |  `--sas_device
> |  |  |  |  |  `--end_device-6:0
> |  |  |  |  `--sas_end_device
> |  |  |  |  |  `--end_device-6:0
> |  |  |  |  `--target6:0:0
> |  |  |  |  |  `--6:0:0:0
> |  |  |  |  |  |  `--block
> |  |  |  |  |  |  |  `--sdb
> ...
> |  |  `--port-6:1
> |  |  |  `--end_device-6:1
> ...
> phy and port have to be separated, sas_port are created dynamically.
> 
> For ata, all objects are created at initialization time, so the layout is:
> `--0000:09:00.0
> |  `--host0
> |  |  `--port1
> |  |  |  `--link1
> |  |  |  |  `--dev1.0
> |  |  |  |  |  `--target0:0:0
> |  |  |  |  |  |  `--0:0:0:0
> |  |  |  |  |  |  |  `--block
> |  |  |  |  |  |  |  |  `--sda
> 
> If we have a port multiplier, more links are created.
> `--0000:09:00.0
> ...
> |  `--host4
> |  |  `--port5
> |  |  |  `--link5
> |  |  |  |  `--dev5.0
> [device for the port multiplier]
> |  |  |  `--link5.0
> |  |  |  |  `--dev5.0.0
> |  |  |  |  |  `--target4:0:0
> |  |  |  |  |  |  `--4:0:0:0
> |  |  |  |  |  |  |  `--block
> |  |  |  |  |  |  |  |  `--sdc
> [disk in port 0 of the port multiplier]
> ...
> |  |  |  `--link5.2
> |  |  |  |  `--dev5.2.0
> |  |  |  |  |  `--target4:2:0
> |  |  |  |  |  |  `--4:2:0:0
> |  |  |  |  |  |  |  `--block
> |  |  |  |  |  |  |  |  `--sde
> [disk in port 2 of the port multiplier]
> 
> In consequence, the path of a scsi device becomes:
> .../0000:00:1f.2/host0/ata1/link1/dev1.0/target0:0:0/0:0:0:0
                         ~~~~
Should be port1 :-)

> dev1.0 indicates the master device [0] in ata port 1.
> ata1 being under host0, we know the reliationships between the scsi_host id and
> ata port id.
> 
> or when a port multiplier is present: for instance the device in port 4 of the
> port multiplier:
> .../0000:00:06.0/0000:09:00.0/host5/ata6/link6.4/dev6.4.0/target5:4:0/5:4:0:0

Same here.

Thanks,
Aaron

> 
> Gwendal Grignou (3):
>   Revert "ata: make ata port as parent device of scsi host"
>   scsi: Allow devices to have arbitrary parent
>   libata: Change transport topology layout
> 
>  drivers/ata/libata-core.c      |   13 ++++++-------
>  drivers/ata/libata-scsi.c      |    4 ++--
>  drivers/ata/libata-transport.c |    2 +-
>  drivers/firewire/sbp2.c        |    3 ++-
>  drivers/message/i2o/i2o_scsi.c |    4 ++--
>  drivers/scsi/scsi_scan.c       |    9 +++++----
>  include/scsi/scsi_device.h     |    2 +-
>  7 files changed, 19 insertions(+), 18 deletions(-)
> 
> -- 
> 1.7.7.3
> 

Re: [PATCH 3/3] libata: Change transport topology layout

[Aaron Lu]

On Thu, Sep 27, 2012 at 12:04:04PM -0700, Gwendal Grignou wrote:
> Integrate ata objects [port, link, device] with scsi objects.
> 
> 
> The path of a scsi device is:
> .../0000:00:1f.2/host0/ata1/link1/dev1.0/target0:0:0/0:0:0:0

After test, I noticed that this will break the current ata acpi binding
code, but can be fixed with the following changes:

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 30eb7177..459c5b4 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -978,7 +978,7 @@ void ata_acpi_on_disable(struct ata_device *dev)
 
 static int compat_pci_ata(struct ata_port *ap)
 {
-	struct device *dev = ap->tdev.parent;
+	struct device *dev = ap->host->dev;
 	struct pci_dev *pdev;
 
 	if (!is_pci_dev(dev))
@@ -998,7 +998,7 @@ static int ata_acpi_bind_host(struct ata_port *ap, acpi_handle *handle)
 	if (ap->flags & ATA_FLAG_ACPI_SATA)
 		return -ENODEV;
 
-	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(ap->tdev.parent),
+	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(ap->host->dev),
 			ap->port_no);
 
 	if (!*handle)
@@ -1061,7 +1061,12 @@ static struct ata_port *dev_to_ata_port(struct device *dev)
 
 static int ata_acpi_find_device(struct device *dev, acpi_handle *handle)
 {
-	struct ata_port *ap = dev_to_ata_port(dev);
+	struct ata_port *ap;
+
+	if (scsi_is_host_device(dev))
+		ap = ata_shost_to_port(dev_to_shost(dev));
+	else
+		ap = dev_to_ata_port(dev);
 
 	if (!ap)
 		return -ENODEV;


And to make zero power ODD function, the following changes to enable
runtime pm with no callbacks for the ata_link/ata_device transport
devices are necessary.

diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index c04d393..ce91bd2 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -421,6 +421,10 @@ int ata_tlink_add(struct ata_link *link)
 		goto tlink_err;
 	}
 
+	pm_runtime_no_callbacks(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	transport_add_device(dev);
 	transport_configure_device(dev);
 
@@ -649,6 +653,10 @@ static int ata_tdev_add(struct ata_device *ata_dev)
 		return error;
 	}
 
+	pm_runtime_no_callbacks(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	transport_add_device(dev);
 	transport_configure_device(dev);
 	return 0;

There is no other problems I can see.
Should I prepare a patch to addess the 2 issues?

Thanks,
Aaron

> 
> or when a port multiplier is present: for instance the device in port 4 of the
> port multiplier:
> .../0000:00:06.0/0000:09:00.0/host5/ata6/link6.4/dev6.4.0/target5:4:0/5:4:0:0
> 
> 
> Change-Id: I202e089208e8746ccdaf2053d43da831a0c0976d
> 
> Signed-off-by: Gwendal Grignou <gwendal@google.com>
> ---
>  drivers/ata/libata-core.c      |   13 ++++++-------
>  drivers/ata/libata-scsi.c      |    4 ++--
>  drivers/ata/libata-transport.c |    2 +-
>  3 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 611050d..c83590b 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -6063,19 +6063,18 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
>  	for (i = 0; i < host->n_ports; i++)
>  		host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
>  
> +	rc = ata_scsi_add_hosts(host, sht);
> +	if (rc)
> +		return rc;
>  
>  	/* Create associated sysfs transport objects  */
>  	for (i = 0; i < host->n_ports; i++) {
> -		rc = ata_tport_add(host->dev,host->ports[i]);
> -		if (rc) {
> +		struct ata_port *ap = host->ports[i];
> +		rc = ata_tport_add(&ap->scsi_host->shost_gendev, ap);
> +		if (rc)
>  			goto err_tadd;
> -		}
>  	}
>  
> -	rc = ata_scsi_add_hosts(host, sht);
> -	if (rc)
> -		goto err_tadd;
> -
>  	/* set cable, sata_spd_limit and report */
>  	for (i = 0; i < host->n_ports; i++) {
>  		struct ata_port *ap = host->ports[i];
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index bfda61f..9023bb1 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3649,8 +3649,8 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>  			else
>  				channel = link->pmp;
>  
> -			sdev = __scsi_add_device(&ap->scsi_host->shost_gendev,
> -						 channel, id, 0, NULL);
> +			sdev = __scsi_add_device(&dev->tdev, channel, id, 0,
> +						 NULL);
>  			if (!IS_ERR(sdev)) {
>  				dev->sdev = sdev;
>  				scsi_device_put(sdev);
> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
> index c04d393..6829be6 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -284,7 +284,7 @@ int ata_tport_add(struct device *parent,
>  
>  	dev->parent = get_device(parent);
>  	dev->release = ata_tport_release;
> -	dev_set_name(dev, "ata%d", ap->print_id);
> +	dev_set_name(dev, "port%d", ap->print_id);
>  	transport_setup_device(dev);
>  	error = device_add(dev);
>  	if (error) {
> -- 
> 1.7.7.3
> 

Re: [PATCH 1/3] Revert "ata: make ata port as parent device of scsi host"

[Sergei Shtylyov]

Hello.

On 27-09-2012 21:04, Gwendal Grignou wrote:

> This reverts commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a.
>
> Instead, melt libata transport sysfs object in scsi objects.
>
> Change-Id: I8c709f63ddf7ba97b9e6f449d5c0b8b85e44e818

    Remove this line please, it has no place in the upstream commit.

> Signed-off-by: Gwendal Grignou<gwendal@google.com>
>

MBR, Sergei

[PATCH 0/3] Insert ATA transport objects in SCSI syfs topology.

[Gwendal Grignou]

This set of patches improve ATA transport classes integration with SCSI
objects.

Before [2.6.x]

Ata and scsi transport class where separated:
`--0000:09:00.0
|  `--ata1
|  |  `--port_port
|  |  `--link1
|  |  |  `--dev1.0
|  |  |  `--dev1.1
|  `--ata2
|  ...
|  `--host0
|  |  `--scsi_host
|  |  |  `--host0
|  |  `--target0:0:0
|  |  |  `--0:0:0:0
|  |  |  |  `--block
|  |  |  |  |  `--sda
|  |  |  |  |  |  `--sda1

In 3.2, Lin - in commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a - addressed
the issue of linking the ata port with the scsi host object by placing the
scsi_host object under ata port objects.

However to be more consistent with other transport, this patch does the opposite:

For instance, with SAS transport, We have
`--0000:0b:00.0
|  `--host6
|  |  `--phy-6:0
|  |  `--phy-6:1
...
|  |  `--port-6:0
|  |  |  `--end_device-6:0
|  |  |  |  `--sas_device
|  |  |  |  |  `--end_device-6:0
|  |  |  |  `--sas_end_device
|  |  |  |  |  `--end_device-6:0
|  |  |  |  `--target6:0:0
|  |  |  |  |  `--6:0:0:0
|  |  |  |  |  |  `--block
|  |  |  |  |  |  |  `--sdb
...
|  |  `--port-6:1
|  |  |  `--end_device-6:1
...
phy and port have to be separated, sas_port are created dynamically.

For ata, all objects are created at initialization time, so the layout is:
`--0000:09:00.0
|  `--host0
|  |  `--port1
|  |  |  `--link1
|  |  |  |  `--dev1.0
|  |  |  |  |  `--target0:0:0
|  |  |  |  |  |  `--0:0:0:0
|  |  |  |  |  |  |  `--block
|  |  |  |  |  |  |  |  `--sda

If we have a port multiplier, more links are created.
`--0000:09:00.0
...
|  `--host4
|  |  `--port5
|  |  |  `--link5
|  |  |  |  `--dev5.0
[device for the port multiplier]
|  |  |  `--link5.0
|  |  |  |  `--dev5.0.0
|  |  |  |  |  `--target4:0:0
|  |  |  |  |  |  `--4:0:0:0
|  |  |  |  |  |  |  `--block
|  |  |  |  |  |  |  |  `--sdc
[disk in port 0 of the port multiplier]
...
|  |  |  `--link5.2
|  |  |  |  `--dev5.2.0
|  |  |  |  |  `--target4:2:0
|  |  |  |  |  |  `--4:2:0:0
|  |  |  |  |  |  |  `--block
|  |  |  |  |  |  |  |  `--sde
[disk in port 2 of the port multiplier]

In consequence, the path of a scsi device becomes:
.../0000:00:1f.2/host0/port1/link1/dev1.0/target0:0:0/0:0:0:0
dev1.0 indicates the master device [0] in ata port 1.
ata1 being under host0, we know the reliationships between the scsi_host id and
ata port id.

or when a port multiplier is present: for instance the device in port 4 of the
port multiplier:
.../0000:00:06.0/0000:09:00.0/host5/port6/link6.4/dev6.4.0/target5:4:0/5:4:0:0

Gwendal Grignou (3):
  Revert "ata: make ata port as parent device of scsi host"
  scsi: Allow devices to have arbitrary parent
  libata: Change transport topology layout

 drivers/ata/libata-core.c      |   13 ++++++-------
 drivers/ata/libata-scsi.c      |    4 ++--
 drivers/ata/libata-transport.c |    2 +-
 drivers/firewire/sbp2.c        |    3 ++-
 drivers/message/i2o/i2o_scsi.c |    4 ++--
 drivers/scsi/scsi_scan.c       |    9 +++++----
 include/scsi/scsi_device.h     |    2 +-
 7 files changed, 19 insertions(+), 18 deletions(-)

-- 
1.7.7.3

[PATCH 1/3] Revert "ata: make ata port as parent device of scsi host"

[Gwendal Grignou]

This reverts commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a.

Instead, melt libata transport sysfs object in scsi objects.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-scsi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e3bda07..be38930 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3609,7 +3609,7 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 		shost->max_host_blocked = 1;
 
 		rc = scsi_add_host_with_dma(ap->scsi_host,
-						&ap->tdev, ap->host->dev);
+					    host->dev, host->dev);
 		if (rc)
 			goto err_add;
 	}
-- 
1.7.7.3

[PATCH 2/3] scsi: Allow devices to have arbitrary parent

[Gwendal Grignou]

Allow driver who calls __scsi_add_device directly to create the scsi
device on any parent, not just scsi_host directly.
This is alreay done for transport with their own class [SAS, iSCSI, FC, ...]

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-scsi.c      |    4 ++--
 drivers/firewire/sbp2.c        |    3 ++-
 drivers/message/i2o/i2o_scsi.c |    4 ++--
 drivers/scsi/scsi_scan.c       |    9 +++++----
 include/scsi/scsi_device.h     |    2 +-
 5 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index be38930..bfda61f 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3649,8 +3649,8 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 			else
 				channel = link->pmp;
 
-			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
-						 NULL);
+			sdev = __scsi_add_device(&ap->scsi_host->shost_gendev,
+						 channel, id, 0, NULL);
 			if (!IS_ERR(sdev)) {
 				dev->sdev = sdev;
 				scsi_device_put(sdev);
diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 1162d6b..839afa5 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -879,7 +879,8 @@ static void sbp2_login(struct work_struct *work)
 		ssleep(SBP2_INQUIRY_DELAY);
 
 	shost = container_of((void *)tgt, struct Scsi_Host, hostdata[0]);
-	sdev = __scsi_add_device(shost, 0, 0, sbp2_lun2int(lu->lun), lu);
+	sdev = __scsi_add_device(&shost->shost_gendev, 0, 0,
+				 sbp2_lun2int(lu->lun), lu);
 	/*
 	 * FIXME:  We are unable to perform reconnects while in sbp2_login().
 	 * Therefore __scsi_add_device() will get into trouble if a bus reset
diff --git a/drivers/message/i2o/i2o_scsi.c b/drivers/message/i2o/i2o_scsi.c
index 1d31d72..ee1353c 100644
--- a/drivers/message/i2o/i2o_scsi.c
+++ b/drivers/message/i2o/i2o_scsi.c
@@ -294,8 +294,8 @@ static int i2o_scsi_probe(struct device *dev)
 	}
 
 	scsi_dev =
-	    __scsi_add_device(i2o_shost->scsi_host, channel, le32_to_cpu(id),
-			      le64_to_cpu(lun), i2o_dev);
+	    __scsi_add_device(&i2o_shost->scsi_host->shost_gendev, channel,
+			      le32_to_cpu(id), le64_to_cpu(lun), i2o_dev);
 
 	if (IS_ERR(scsi_dev)) {
 		osm_warn("can not add SCSI device %03x\n",
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 56a9379..105123c 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1489,11 +1489,11 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	return ret;
 }
 
-struct scsi_device *__scsi_add_device(struct Scsi_Host *shost, uint channel,
+struct scsi_device *__scsi_add_device(struct device *parent, uint channel,
 				      uint id, uint lun, void *hostdata)
 {
+	struct Scsi_Host *shost = dev_to_shost(parent);
 	struct scsi_device *sdev = ERR_PTR(-ENODEV);
-	struct device *parent = &shost->shost_gendev;
 	struct scsi_target *starget;
 
 	if (strncmp(scsi_scan_type, "none", 4) == 0)
@@ -1524,8 +1524,9 @@ EXPORT_SYMBOL(__scsi_add_device);
 int scsi_add_device(struct Scsi_Host *host, uint channel,
 		    uint target, uint lun)
 {
-	struct scsi_device *sdev = 
-		__scsi_add_device(host, channel, target, lun, NULL);
+	struct scsi_device *sdev =
+		__scsi_add_device(&host->shost_gendev, channel, target,
+				  lun, NULL);
 	if (IS_ERR(sdev))
 		return PTR_ERR(sdev);
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9895f69..9646a1d 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -285,7 +285,7 @@ static inline struct scsi_target *scsi_target(struct scsi_device *sdev)
 #define starget_printk(prefix, starget, fmt, a...)	\
 	dev_printk(prefix, &(starget)->dev, fmt, ##a)
 
-extern struct scsi_device *__scsi_add_device(struct Scsi_Host *,
+extern struct scsi_device *__scsi_add_device(struct device *,
 		uint, uint, uint, void *hostdata);
 extern int scsi_add_device(struct Scsi_Host *host, uint channel,
 			   uint target, uint lun);
-- 
1.7.7.3

[PATCH 3/3] libata: Change transport topology layout

[Gwendal Grignou]

Integrate ata objects [port, link, device] with scsi objects.


The path of a scsi device is:
.../0000:00:1f.2/host0/port1/link1/dev1.0/target0:0:0/0:0:0:0

or when a port multiplier is present: for instance the device in port 4 of the
port multiplier:
.../0000:00:06.0/0000:09:00.0/host5/port6/link6.4/dev6.4.0/target5:4:0/5:4:0:0

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-core.c      |   13 ++++++-------
 drivers/ata/libata-scsi.c      |    4 ++--
 drivers/ata/libata-transport.c |    2 +-
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 611050d..c83590b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6063,19 +6063,18 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 	for (i = 0; i < host->n_ports; i++)
 		host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
 
+	rc = ata_scsi_add_hosts(host, sht);
+	if (rc)
+		return rc;
 
 	/* Create associated sysfs transport objects  */
 	for (i = 0; i < host->n_ports; i++) {
-		rc = ata_tport_add(host->dev,host->ports[i]);
-		if (rc) {
+		struct ata_port *ap = host->ports[i];
+		rc = ata_tport_add(&ap->scsi_host->shost_gendev, ap);
+		if (rc)
 			goto err_tadd;
-		}
 	}
 
-	rc = ata_scsi_add_hosts(host, sht);
-	if (rc)
-		goto err_tadd;
-
 	/* set cable, sata_spd_limit and report */
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bfda61f..9023bb1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3649,8 +3649,8 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 			else
 				channel = link->pmp;
 
-			sdev = __scsi_add_device(&ap->scsi_host->shost_gendev,
-						 channel, id, 0, NULL);
+			sdev = __scsi_add_device(&dev->tdev, channel, id, 0,
+						 NULL);
 			if (!IS_ERR(sdev)) {
 				dev->sdev = sdev;
 				scsi_device_put(sdev);
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index c04d393..6829be6 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -284,7 +284,7 @@ int ata_tport_add(struct device *parent,
 
 	dev->parent = get_device(parent);
 	dev->release = ata_tport_release;
-	dev_set_name(dev, "ata%d", ap->print_id);
+	dev_set_name(dev, "port%d", ap->print_id);
 	transport_setup_device(dev);
 	error = device_add(dev);
 	if (error) {
-- 
1.7.7.3

Re: [PATCH 0/3] Insert ATA transport objects in SCSI syfs topology.

[Dan Williams]

On Mon, Oct 1, 2012 at 11:22 AM, Gwendal Grignou <gwendal@google.com> wrote:
> This set of patches improve ATA transport classes integration with SCSI
> objects.
>
> Before [2.6.x]
>
> Ata and scsi transport class where separated:
> `--0000:09:00.0
> |  `--ata1
> |  |  `--port_port
> |  |  `--link1
> |  |  |  `--dev1.0
> |  |  |  `--dev1.1
> |  `--ata2
> |  ...
> |  `--host0
> |  |  `--scsi_host
> |  |  |  `--host0
> |  |  `--target0:0:0
> |  |  |  `--0:0:0:0
> |  |  |  |  `--block
> |  |  |  |  |  `--sda
> |  |  |  |  |  |  `--sda1
>
> In 3.2, Lin - in commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a - addressed
> the issue of linking the ata port with the scsi host object by placing the
> scsi_host object under ata port objects.
>
> However to be more consistent with other transport, this patch does the opposite:
>
> For instance, with SAS transport, We have
> `--0000:0b:00.0
> |  `--host6
> |  |  `--phy-6:0
> |  |  `--phy-6:1
> ...
> |  |  `--port-6:0
> |  |  |  `--end_device-6:0
> |  |  |  |  `--sas_device
> |  |  |  |  |  `--end_device-6:0
> |  |  |  |  `--sas_end_device
> |  |  |  |  |  `--end_device-6:0
> |  |  |  |  `--target6:0:0
> |  |  |  |  |  `--6:0:0:0
> |  |  |  |  |  |  `--block
> |  |  |  |  |  |  |  `--sdb
> ...
> |  |  `--port-6:1
> |  |  |  `--end_device-6:1
> ...
> phy and port have to be separated, sas_port are created dynamically.
>
> For ata, all objects are created at initialization time, so the layout is:
> `--0000:09:00.0
> |  `--host0
> |  |  `--port1
> |  |  |  `--link1
> |  |  |  |  `--dev1.0
> |  |  |  |  |  `--target0:0:0
> |  |  |  |  |  |  `--0:0:0:0
> |  |  |  |  |  |  |  `--block
> |  |  |  |  |  |  |  |  `--sda
>
> If we have a port multiplier, more links are created.
> `--0000:09:00.0
> ...
> |  `--host4
> |  |  `--port5
> |  |  |  `--link5
> |  |  |  |  `--dev5.0
> [device for the port multiplier]
> |  |  |  `--link5.0
> |  |  |  |  `--dev5.0.0
> |  |  |  |  |  `--target4:0:0
> |  |  |  |  |  |  `--4:0:0:0
> |  |  |  |  |  |  |  `--block
> |  |  |  |  |  |  |  |  `--sdc
> [disk in port 0 of the port multiplier]
> ...
> |  |  |  `--link5.2
> |  |  |  |  `--dev5.2.0
> |  |  |  |  |  `--target4:2:0
> |  |  |  |  |  |  `--4:2:0:0
> |  |  |  |  |  |  |  `--block
> |  |  |  |  |  |  |  |  `--sde
> [disk in port 2 of the port multiplier]
>
> In consequence, the path of a scsi device becomes:
> .../0000:00:1f.2/host0/port1/link1/dev1.0/target0:0:0/0:0:0:0
> dev1.0 indicates the master device [0] in ata port 1.
> ata1 being under host0, we know the reliationships between the scsi_host id and
> ata port id.
>
> or when a port multiplier is present: for instance the device in port 4 of the
> port multiplier:
> .../0000:00:06.0/0000:09:00.0/host5/port6/link6.4/dev6.4.0/target5:4:0/5:4:0:0

What's the benefit of this?  From a PM perspective now it seems we'll
have the parent hardware port suspended before all the scsi_hosts on
that port, which defeats the original purpose of putting the ata_port
in the PM hierarchy.

--
Dan

[PATCH 3/3] libata: Change transport topology layout

[Gwendal Grignou]

Integrate ata objects [port, link, device] with scsi objects.

Before [2.6.x]

The path of a scsi device is:
.../0000:00:1f.2/host0/port1/link1/dev1.0/target0:0:0/0:0:0:0

or when a port multiplier is present: for instance the device in port 4 of the
port multiplier:
.../0000:00:06.0/0000:09:00.0/host5/port6/link6.4/dev6.4.0/target5:4:0/5:4:0:0

Fix ACPI code that relied on previous topology. 


Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-acpi.c      |   11 ++++++++---
 drivers/ata/libata-core.c      |   13 ++++++-------
 drivers/ata/libata-scsi.c      |    4 ++--
 drivers/ata/libata-transport.c |   10 +++++++++-
 4 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index fd9ecf7..8e7f451 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -1065,7 +1065,7 @@ void ata_acpi_unbind(struct ata_device *dev)
 
 static int compat_pci_ata(struct ata_port *ap)
 {
-	struct device *dev = ap->tdev.parent;
+	struct device *dev = ap->host->dev;
 	struct pci_dev *pdev;
 
 	if (!is_pci_dev(dev))
@@ -1085,7 +1085,7 @@ static int ata_acpi_bind_host(struct ata_port *ap, acpi_handle *handle)
 	if (ap->flags & ATA_FLAG_ACPI_SATA)
 		return -ENODEV;
 
-	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(ap->tdev.parent),
+	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(ap->host->dev),
 			ap->port_no);
 
 	if (!*handle)
@@ -1150,7 +1150,12 @@ static struct ata_port *dev_to_ata_port(struct device *dev)
 
 static int ata_acpi_find_device(struct device *dev, acpi_handle *handle)
 {
-	struct ata_port *ap = dev_to_ata_port(dev);
+	struct ata_port *ap;
+
+	if (scsi_is_host_device(dev))
+		ap = ata_shost_to_port(dev_to_shost(dev));
+	else
+		ap = dev_to_ata_port(dev);
 
 	if (!ap)
 		return -ENODEV;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 611050d..c83590b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6063,19 +6063,18 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 	for (i = 0; i < host->n_ports; i++)
 		host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
 
+	rc = ata_scsi_add_hosts(host, sht);
+	if (rc)
+		return rc;
 
 	/* Create associated sysfs transport objects  */
 	for (i = 0; i < host->n_ports; i++) {
-		rc = ata_tport_add(host->dev,host->ports[i]);
-		if (rc) {
+		struct ata_port *ap = host->ports[i];
+		rc = ata_tport_add(&ap->scsi_host->shost_gendev, ap);
+		if (rc)
 			goto err_tadd;
-		}
 	}
 
-	rc = ata_scsi_add_hosts(host, sht);
-	if (rc)
-		goto err_tadd;
-
 	/* set cable, sata_spd_limit and report */
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bfda61f..9023bb1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3649,8 +3649,8 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 			else
 				channel = link->pmp;
 
-			sdev = __scsi_add_device(&ap->scsi_host->shost_gendev,
-						 channel, id, 0, NULL);
+			sdev = __scsi_add_device(&dev->tdev, channel, id, 0,
+						 NULL);
 			if (!IS_ERR(sdev)) {
 				dev->sdev = sdev;
 				scsi_device_put(sdev);
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index c04d393..ce91bd2 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -284,7 +284,7 @@ int ata_tport_add(struct device *parent,
 
 	dev->parent = get_device(parent);
 	dev->release = ata_tport_release;
-	dev_set_name(dev, "ata%d", ap->print_id);
+	dev_set_name(dev, "port%d", ap->print_id);
 	transport_setup_device(dev);
 	error = device_add(dev);
 	if (error) {
@@ -421,6 +421,10 @@ int ata_tlink_add(struct ata_link *link)
 		goto tlink_err;
 	}
 
+	pm_runtime_no_callbacks(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	transport_add_device(dev);
 	transport_configure_device(dev);
 
@@ -649,6 +653,10 @@ static int ata_tdev_add(struct ata_device *ata_dev)
 		return error;
 	}
 
+	pm_runtime_no_callbacks(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	transport_add_device(dev);
 	transport_configure_device(dev);
 	return 0;
-- 
1.7.7.3

Re: [PATCH 0/3] Insert ATA transport objects in SCSI syfs topology.

[Gwendal Grignou]

On Mon, Oct 1, 2012 at 12:14 PM, Dan Williams <djbw@fb.com> wrote:
> On Mon, Oct 1, 2012 at 11:22 AM, Gwendal Grignou <gwendal@google.com> wrote:
>> This set of patches improve ATA transport classes integration with SCSI
>> objects.
>>
>> Before [2.6.x]
>>
>> Ata and scsi transport class where separated:
>> `--0000:09:00.0
>> |  `--ata1
>> |  |  `--port_port
>> |  |  `--link1
>> |  |  |  `--dev1.0
>> |  |  |  `--dev1.1
>> |  `--ata2
>> |  ...
>> |  `--host0
>> |  |  `--scsi_host
>> |  |  |  `--host0
>> |  |  `--target0:0:0
>> |  |  |  `--0:0:0:0
>> |  |  |  |  `--block
>> |  |  |  |  |  `--sda
>> |  |  |  |  |  |  `--sda1
>>
>> In 3.2, Lin - in commit 9a6d6a2ddabbd32c07f6a38b659e5f3db319fa5a - addressed
>> the issue of linking the ata port with the scsi host object by placing the
>> scsi_host object under ata port objects.
>>
>> However to be more consistent with other transport, this patch does the opposite:
>>
>> For instance, with SAS transport, We have
>> `--0000:0b:00.0
>> |  `--host6
>> |  |  `--phy-6:0
>> |  |  `--phy-6:1
>> ...
>> |  |  `--port-6:0
>> |  |  |  `--end_device-6:0
>> |  |  |  |  `--sas_device
>> |  |  |  |  |  `--end_device-6:0
>> |  |  |  |  `--sas_end_device
>> |  |  |  |  |  `--end_device-6:0
>> |  |  |  |  `--target6:0:0
>> |  |  |  |  |  `--6:0:0:0
>> |  |  |  |  |  |  `--block
>> |  |  |  |  |  |  |  `--sdb
>> ...
>> |  |  `--port-6:1
>> |  |  |  `--end_device-6:1
>> ...
>> phy and port have to be separated, sas_port are created dynamically.
>>
>> For ata, all objects are created at initialization time, so the layout is:
>> `--0000:09:00.0
>> |  `--host0
>> |  |  `--port1
>> |  |  |  `--link1
>> |  |  |  |  `--dev1.0
>> |  |  |  |  |  `--target0:0:0
>> |  |  |  |  |  |  `--0:0:0:0
>> |  |  |  |  |  |  |  `--block
>> |  |  |  |  |  |  |  |  `--sda
>>
>> If we have a port multiplier, more links are created.
>> `--0000:09:00.0
>> ...
>> |  `--host4
>> |  |  `--port5
>> |  |  |  `--link5
>> |  |  |  |  `--dev5.0
>> [device for the port multiplier]
>> |  |  |  `--link5.0
>> |  |  |  |  `--dev5.0.0
>> |  |  |  |  |  `--target4:0:0
>> |  |  |  |  |  |  `--4:0:0:0
>> |  |  |  |  |  |  |  `--block
>> |  |  |  |  |  |  |  |  `--sdc
>> [disk in port 0 of the port multiplier]
>> ...
>> |  |  |  `--link5.2
>> |  |  |  |  `--dev5.2.0
>> |  |  |  |  |  `--target4:2:0
>> |  |  |  |  |  |  `--4:2:0:0
>> |  |  |  |  |  |  |  `--block
>> |  |  |  |  |  |  |  |  `--sde
>> [disk in port 2 of the port multiplier]
>>
>> In consequence, the path of a scsi device becomes:
>> .../0000:00:1f.2/host0/port1/link1/dev1.0/target0:0:0/0:0:0:0
>> dev1.0 indicates the master device [0] in ata port 1.
>> ata1 being under host0, we know the reliationships between the scsi_host id and
>> ata port id.
>>
>> or when a port multiplier is present: for instance the device in port 4 of the
>> port multiplier:
>> .../0000:00:06.0/0000:09:00.0/host5/port6/link6.4/dev6.4.0/target5:4:0/5:4:0:0
>
> What's the benefit of this?
+ To unify ata transport sysfs topology with other scsi transport.
+ To easily map a ata_port with its associated scsi_host structure.

> From a PM perspective now it seems we'll
> have the parent hardware port suspended before all the scsi_hosts on
> that port,
There is a one to one mapping between ata port and scsi-host, so it works.
All ata ports must be suspended before the parent hardware is.

Gwendal.
> which defeats the original purpose of putting the ata_port
> in the PM hierarchy.
>
> --
> Dan

Re: [PATCH 0/3] Insert ATA transport objects in SCSI syfs topology.

[Dan Williams]

On Thu, Oct 4, 2012 at 9:56 AM, Gwendal Grignou <gwendal@google.com> wrote:
>> What's the benefit of this?
> + To unify ata transport sysfs topology with other scsi transport.

My concern is the thrash and breakage to switch the ordering around
given the (minor) growing pains injecting an ata_port into the device
path caused.  Although, it seems like Aaron has caught where this
reversal broke things at the cost of some additional special-casing (4
files changed, 25 insertions(+), 13 deletions(-)).  Patch 1 also
creates a problem for bisections as the code that assumes
<dev>/port/host will break.

I don't know... I'm all for consistency, but if the only justification
is to make the transports look the "same" we'll still have a glaring
transport difference between ata and sas.  In the sas case one
hba/host spanning all possible sas domains vs the ata case of a
guaranteed ata_port per "ata domain" relationship with at least one
host per port.  The "port" does live higher in the topology in the ata
case.

> + To easily map a ata_port with its associated scsi_host structure.

Not sure this is getting any easier.  There would now be three options
based on kernel version: look for the ata_port as a host attribute,
look at the host's parent, or look for the host's child.

--
Dan