Re: [PATCHSET] libata: implement new initialization model w/ iomap support, take 2

[Tejun Heo <htejun@gmail.com>]

Hello, Brian.

Brian King wrote:
>> 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.

I see.

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

You're right.  I was confused that ->slave_destroy() is called on host 
release.  SCSI doesn't have host release callback.  What do you think 
about adding a host release callback?  That should make things easier 
and it is generally useful.

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

Or we can just keep ata_host_init() and let sas allocate ata_host as 
part of SCSI host and free all dynamic stuff when all ports are 
detached.  It's a bit hacky but should work for the time being.

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

For SAS, libata isn't controlling SCSI host, so I think it's more 
logical to leave SCSI devices to SAS too.  Would that make much difference?

Thanks.

-- 
tejun