Hello,

Brian King wrote:

Tejun Heo wrote:

quoted

Brian, can you please take a look at how LLDs use new init functions
and see if SAS can use the same approach?  If you pass NULL as @sht to
ata_host_alloc(), it won't associate ports as part of Scsi_Host just
as ata_sas_port_alloc() does and ata_host_free() will do the right
thing when freeing a host allocated that way.

I started looking through your patch set and have a few comments.

The idea of a static number of "ata ports" per ata host in SAS doesn't
really work. Since you can have ATA devices under a SAS expander, the
number of possible ATA devices that can be attached to a SAS adapter
can be rather large and can change depending on the SAS fabric. If libata
ever needed to iterate over the ata_port's for a SAS ata_host, then we would
probably need to convert the static array of ata_ports to a linked
list, allowing it to be more dynamic.

Making the array dynamically-sized isn't difficult at all; however, the 
current libata code assumes that ata_host->ports[] array is packed - ie. 
no intervening empty entry.  Can SAS keep this restriction or does it 
need more flexibility?

Object lifetime rules also have me concerned. Currently, for SAS,
there are a couple objects that libata is concerned with. The first is
an ata_host_set, which I am allocating as part of the scsi_host struct,
so it inherits the object lifetime rules of that. The second is the
ata_port, which I allocate and free in target_alloc/target_destroy,
so I get refcounting for free there as well. Your patch set introduces
an ata_host struct, which is kmalloc'ed and doesn't inherit any of the
above refcounting. 

Actually, ata_host is ata_host_set.  It's just renamed recently.

cca3974e48607c3775dc73b544a5700b2e37c21a in libata-dev#upstream

Hmmm.... I was kind of hoping SAS could use ata_host_alloc() and store 
its pointer and then later release it w/ ata_host_free(), hmmm.. maybe 
you can call ata_host_free from ->slave_destroy?.  That gives libata 
more control over the host structure (e.g. if we implement dynamic ports 
array, it needs to be freed too).  Port lifetime rules aren't changed by 
these updates and host free does need some changes but IMHO that 
shouldn't be difficult.

quoted

sata_sil24.c is a pretty straight-forward example.  If you can't
determine the number of ports when allocating host, please take a look
at how ahci.c initializes its host.

The intention was to allow SAS to use all the regular init/deinit
functions just as other LLDs.  If something doesn't seem to be right,
please let me know.

I think it can use bits of it, but I think the actual device discovery
process is better initiated by the SAS layer. The SAS layer knows what
devices are out there when it does discovery and can tell libata about
them.

Hmmm.... I see.  Something like ata_dev_attach(adev) after initialized 
by SAS maybe?

-- 
tejun