Re: [PATCH v2 4/5] scsi: libsas: factor out sas_ata_add_dev()

[Jason Yan <yanaijie@huawei.com>]

On 2022/12/14 0:22, John Garry wrote:
> On 13/12/2022 15:09, Jason Yan wrote:
>> Factor out sas_ata_add_dev() and put it in sas_ata.c since it is a sata
>> related interface. Also follow the standard coding style to define an
>> inline empty function when CONFIG_SCSI_SAS_ATA is not enabled.
>>
>> Cc: John Garry <john.g.garry@oracle.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> 
> Apart from comment, below:
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 
>> ---
>>   drivers/scsi/libsas/sas_ata.c      | 62 ++++++++++++++++++++++++++++++
>>   drivers/scsi/libsas/sas_expander.c | 54 +-------------------------
>>   include/scsi/sas_ata.h             |  9 +++++
>>   3 files changed, 73 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_ata.c 
>> b/drivers/scsi/libsas/sas_ata.c
> 
>> @@ -109,6 +111,13 @@ static inline int sas_discover_sata(struct 
>> domain_device *dev)
>>       pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot 
>> attach\n");
>>       return -ENXIO;
>>   }
>> +static inline int sas_ata_add_dev(struct domain_device *parent, 
>> struct ex_phy *phy,
>> +                  struct domain_device *child, int phy_id)
>> +{
>> +    pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot 
>> add device, target proto 0x%x at %016llx:0x%x\n",
>> +          phy->attached_tproto, SAS_ADDR(parent->sas_addr), phy_id);
> 
> Do you really think that we need to add all this info, like 
> parent->sas_addr?
> 

I did not want to change the functionality at the first time so I kept 
these info printing.

> Indeed, I think that we could make all these prints for 
> CONFIG_SCSI_SAS_ATA=N into a single global pr_notice_once(). That's just 
> my thoughts.
> 

Yeah, makes sense. Let me have a try.

Thanks

>> +    return -ENODEV;
>> +}
>>   #endif
>>   #endif /* _SAS_ATA_H_ */
> 
> .