Re: [PATCH 4/20] libata: separate out ata_host_alloc() and ata_host_attach()

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> ACK in general, but minor nits are found in comments below.
> 
> Also, to make reviewing and git bisect easier, it would be nice to split 
> ata_host_attach() separation into another patch.

Okay, will try.

>>  drivers/ata/libata-core.c |  451 
>> ++++++++++++++++++++++++++++++---------------
>>  include/linux/libata.h    |    7 +
>>  2 files changed, 312 insertions(+), 146 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index d7bf4a1..eeb9dc6 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -5243,11 +5243,15 @@ void ata_dev_init(struct ata_device *dev
>>   *    ata_port_init - Initialize an ata_port structure
>>   *    @ap: Structure to initialize
>>   *    @host: Collection of hosts to which @ap belongs
>> - *    @ent: Probe information provided by low-level driver
>> - *    @port_no: Port number associated with this ata_port
>> + *    @ent: Probe information provided by low-level driver (optional)
>> + *    @port_no: Port number associated with this ata_port (optional)
> 
> I would perhaps note that port_no is require iff ent != NULL

Will do.

>> +int ata_host_add_ports(struct ata_host *host,
>> +               struct scsi_host_template *sht, int n_ports)
>> +{
>> +    int i;
>> +
>> +    if (host->n_ports || n_ports > ATA_MAX_PORTS)
>> +        return -EINVAL;
> 
> For what code path would host->n_ports be non-zero?

It's just an error check as this part of code is not very flexible (yet).

>> @@ -5755,8 +5909,10 @@ void ata_host_free(struct ata_host *host
>>      /* free */
>>      for (i = 0; i < host->n_ports; i++) {
>>          struct ata_port *ap = host->ports[i];
>> -        if (ap)
>> +        if (ap->pflags & ATA_PFLAG_SHOST_OWNER)
>>              scsi_host_put(ap->scsi_host);
>> +        else
>> +            kfree(ap);
> 
> Do we ever have a reason to care about ap->scsi_host in the SAS case?
> 
> It seems like we could kill ATA_PFLAG_SHOST_OWNER, and just test 
> scsi_host for NULL.

I thought SAS might want to use the field to back link ATA port to its 
SCSI host.  If that's not necessary, I'll drop the flag.

Thanks.

-- 
tejun