On 6/9/22 21:19, John Garry wrote:
> On 09/06/2022 03:24, Damien Le Moal wrote:
>> Similarly to sas report general and discovery responses, define the
>> structure struct smp_rps_resp to handle SATA PHY report responses
>
> nit: name smp_rps_resp is a bit cryptic to me. Is
> smp_report_phy_sata_resp just too long? I always come up with verbatim
> names ....
Yes, I think so too. I went for the minimal changes here, reusing the name
of the main field. All of the resp structs have bad names I thing. the
"rg" one is cryptic too and the "disc" one makes me think "disk" all the
time...
Luckily, the function names were these are used are not bad so the code is
still easy to follow, I think.
Together with the req side rework, I will rename all this.
>
>> using a structure with a size that is exactly equal to the sas defined
>> response size.
>>
>> With this change, struct smp_resp becomes unused and is removed.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: John Garry <john.garry@xxxxxxxxxx>
Thanks.
>
>> ---
>> drivers/scsi/aic94xx/aic94xx_dev.c | 2 +-
>> drivers/scsi/libsas/sas_expander.c | 4 ++--
>> drivers/scsi/libsas/sas_internal.h | 2 +-
>> include/scsi/libsas.h | 2 +-
>> include/scsi/sas.h | 8 ++------
>> 5 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/scsi/aic94xx/aic94xx_dev.c b/drivers/scsi/aic94xx/aic94xx_dev.c
>> index 73506a459bf8..91d196f26b76 100644
>> --- a/drivers/scsi/aic94xx/aic94xx_dev.c
>> +++ b/drivers/scsi/aic94xx/aic94xx_dev.c
>> @@ -159,7 +159,7 @@ static int asd_init_target_ddb(struct domain_device *dev)
>> flags |= OPEN_REQUIRED;
>> if ((dev->dev_type == SAS_SATA_DEV) ||
>> (dev->tproto & SAS_PROTOCOL_STP)) {
>> - struct smp_resp *rps_resp = &dev->sata_dev.rps_resp;
>> + struct smp_rps_resp *rps_resp = &dev->sata_dev.rps_resp;
>> if (rps_resp->frame_type == SMP_RESPONSE &&
>> rps_resp->function == SMP_REPORT_PHY_SATA &&
>> rps_resp->result == SMP_RESP_FUNC_ACC) {
>> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
>> index 78a38980636e..fa2209080cc2 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -676,10 +676,10 @@ int sas_smp_get_phy_events(struct sas_phy *phy)
>> #ifdef CONFIG_SCSI_SAS_ATA
>>
>> #define RPS_REQ_SIZE 16
>> -#define RPS_RESP_SIZE 60
>> +#define RPS_RESP_SIZE sizeof(struct smp_rps_resp)
>>
>> int sas_get_report_phy_sata(struct domain_device *dev, int phy_id,
>> - struct smp_resp *rps_resp)
>> + struct smp_rps_resp *rps_resp)
>> {
>> int res;
>> u8 *rps_req = alloc_smp_req(RPS_REQ_SIZE);
>> diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
>> index 13d0ffaada93..8d0ad3abc7b5 100644
>> --- a/drivers/scsi/libsas/sas_internal.h
>> +++ b/drivers/scsi/libsas/sas_internal.h
>> @@ -83,7 +83,7 @@ struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
>> struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id);
>> int sas_ex_phy_discover(struct domain_device *dev, int single);
>> int sas_get_report_phy_sata(struct domain_device *dev, int phy_id,
>> - struct smp_resp *rps_resp);
>> + struct smp_rps_resp *rps_resp);
>> int sas_try_ata_reset(struct asd_sas_phy *phy);
>> void sas_hae_reset(struct work_struct *work);
>>
>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>> index ff04eb6d250b..2dbead74a2af 100644
>> --- a/include/scsi/libsas.h
>> +++ b/include/scsi/libsas.h
>> @@ -145,7 +145,7 @@ struct sata_device {
>>
>> struct ata_port *ap;
>> struct ata_host *ata_host;
>> - struct smp_resp rps_resp ____cacheline_aligned; /* report_phy_sata_resp */
>> + struct smp_rps_resp rps_resp ____cacheline_aligned; /* report_phy_sata_resp */
>> u8 fis[ATA_RESP_FIS_SIZE];
>> };
>>
>> diff --git a/include/scsi/sas.h b/include/scsi/sas.h
>> index a8f9743ed6fc..71b749bed3b0 100644
>> --- a/include/scsi/sas.h
>> +++ b/include/scsi/sas.h
>> @@ -712,16 +712,12 @@ struct smp_disc_resp {
>> struct discover_resp disc;
>> } __attribute__ ((packed));
>>
>> -struct smp_resp {
>> +struct smp_rps_resp {
>> u8 frame_type;
>> u8 function;
>> u8 result;
>> u8 reserved;
>> - union {
>> - struct report_general_resp rg;
>> - struct discover_resp disc;
>> - struct report_phy_sata_resp rps;
>> - };
>> + struct report_phy_sata_resp rps;
>> } __attribute__ ((packed));
>>
>> #endif /* _SAS_H_ */
>
--
Damien Le Moal
Western Digital Research
