Re: [PATCH v3 03/23] ata: libata-scsi: link ata port and scsi device

Damien Le Moal
2023-09-19T16:27:39Z

On 2023/09/19 6:21, Niklas Cassel wrote:
>> diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c
>> index 17f6ccee53c7..32968b4cf8e4 100644
>> --- a/drivers/ata/pata_macio.c
>> +++ b/drivers/ata/pata_macio.c
>> @@ -918,6 +918,7 @@ static const struct scsi_host_template pata_macio_sht = {
>> * use 64K minus 256
>> */
>> .max_segment_size = MAX_DBDMA_SEG,
>> + .slave_alloc = ata_scsi_slave_alloc,
>> .slave_configure = pata_macio_slave_config,
>> .sdev_groups = ata_common_sdev_groups,
>> .can_queue = ATA_DEF_QUEUE,
>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>> index d105db5c7d81..353ac7b2f14a 100644
>> --- a/drivers/ata/sata_mv.c
>> +++ b/drivers/ata/sata_mv.c
>> @@ -673,6 +673,7 @@ static const struct scsi_host_template mv6_sht = {
>> .sdev_groups = ata_ncq_sdev_groups,
>> .change_queue_depth = ata_scsi_change_queue_depth,
>> .tag_alloc_policy = BLK_TAG_ALLOC_RR,
>> + .slave_alloc = ata_scsi_slave_alloc,
>
> It seems wrong to add .slave_alloc to all different ata_port_operations structs.
> The .slave_configure is added on all different ata_port_operations structs,
> because the callback can be different for different drivers.
>
> However, adding the device link is done in .ata_scsi_slave_alloc,
> removing the device link is done in .ata_scsi_slave_destroy.
>
> Thus, I suggest that we only add the
> .slave_alloc = ata_scsi_slave_alloc,
>
> to __ATA_BASE_SHT() in libata.h, which is currently the only
> place which has .slave_destroy defined.

Good point. I will make this change.

--
Damien Le Moal
Western Digital Research