Re: [PATCH 1/6] scsi: libsas: move sas_get_ata_command_set() up to save the declaration

[Jason Yan <yanaijie@huawei.com>]

On 2022/12/5 16:45, John Garry wrote:
> On 04/12/2022 08:16, Jason Yan wrote:
>> There is a sas_get_ata_command_set() declaration above sas_get_ata_info()
>> to make it compile ok. However this function is defined in the same file
>> below. So move it up to save the declaration.
>>
>> Cc: John Garry <john.g.garry@oracle.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> 
> Apart from comments, below:
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 

Thank you John.

>> ---
>>   drivers/scsi/libsas/sas_ata.c | 28 +++++++++++++---------------
>>   1 file changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_ata.c 
>> b/drivers/scsi/libsas/sas_ata.c
>> index f7439bf9cdc6..34009c330eb2 100644
>> --- a/drivers/scsi/libsas/sas_ata.c
>> +++ b/drivers/scsi/libsas/sas_ata.c
>> @@ -239,7 +239,19 @@ static struct sas_internal 
>> *dev_to_sas_internal(struct domain_device *dev)
>>       return to_sas_internal(dev->port->ha->core.shost->transportt);
>>   }
>> -static int sas_get_ata_command_set(struct domain_device *dev);
>> +static int sas_get_ata_command_set(struct domain_device *dev)
>> +{
>> +    struct dev_to_host_fis *fis =
>> +        (struct dev_to_host_fis *) dev->frame_rcvd;
> 
> nit: I did not think that we add a whitespace before casting. And I 
> think that it would be neater if fis was assigned separately, avoiding 
> overflowing the line in this way.
> 
> Having said that, it does seem odd to cast from u8 [] to struct 
> dev_to_host_fis * and then back to (u8 *) in the ata_tf_from_fis() call, 
> below.

Yeah, odd. I'd prefer remove 'fis' and then do:

ata_tf_from_fis(dev->frame_rcvd, &tf).

Thanks,
Jason