Tejun Heo wrote: > Brian King wrote: >> 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? This could be made to work with the addition of a new API. Rather than having just ata_sas_port_destroy, we would need ata_sas_port_delete and ata_sas_port_free. ata_sas_port_delete would remove the port from the ata_host and do everything except free the ata port since there could still be references to it. Then ata_sas_port_free would get called once all the references were deleted. >> 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. Right. I saw that. > 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. The current design for SAS is to have a single ata_host per scsi host. This means the ata_host is really tied to the object lifetime rules of the scsi host. In the current SAS code, ata_host_set does not get allocated as a separate piece of memory, but rather as part of the scsi_host object. This means the memory for the ata_host doesn't get freed until the last reference to the scsi host is released. Making ata_host a separate allocation changes these rules and forces the caller to know when to free the ata_host memory, which they currently do not know. In order to do what you are proposing, I think we would need to add some refcounting to the ata_host object. Each allocated ata_port would get a reference to its parent ata_host and would put a reference when the port is freed. Otherwise I am concerned that we would end up in the situation where the ata_host is freed before all the ata ports and the scsi_host is freed. It might be appropriate to create an ata_host class device and an ata_port class device... >>> 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? Possibly. Are you proposing that ata_dev_attach would then end up calling scsi_add_device after doing the ATA initialization stuff? Brian -- Brian King eServer Storage I/O IBM Linux Technology Center - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
