| Message ID | 20220609022456.409087-1-damien.lemoal@opensource.wdc.com |
|---|---|
| Headers | show |
| Series | Fix compilation warnings with gcc12 | expand |
> #define DISCOVER_REQ_SIZE 16 > -#define DISCOVER_RESP_SIZE 56 > +#define DISCOVER_RESP_SIZE sizeof(struct smp_disc_resp) I feel that it would be nice to get rid of these. Maybe something like: #define smp_execute_task_wrap(dev, req, resp)\ smp_execute_task(dev, req, sizeof(*req), resp, DISCOVER_REQ_SIZE) static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req, struct smp_disc_resp *disc_resp, int single) { res = smp_execute_task_wrap(dev, disc_req, disc_resp); We could even introduce a smp req struct to get rid of DISCOVER_REQ_SIZE - the (current) code would be a bit less cryptic then. But no big deal. Looks ok apart from that. Thanks, John
On 6/9/22 17:43, John Garry wrote: > >> #define DISCOVER_REQ_SIZE 16 >> -#define DISCOVER_RESP_SIZE 56 >> +#define DISCOVER_RESP_SIZE sizeof(struct smp_disc_resp) > > I feel that it would be nice to get rid of these. > > Maybe something like: > > #define smp_execute_task_wrap(dev, req, resp)\ > smp_execute_task(dev, req, sizeof(*req), resp, DISCOVER_REQ_SIZE) > > > static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 > *disc_req, struct smp_disc_resp *disc_resp, int single) > { > res = smp_execute_task_wrap(dev, disc_req, disc_resp); > > We could even introduce a smp req struct to get rid of DISCOVER_REQ_SIZE > - the (current) code would be a bit less cryptic then. Yes, I think the req size needs a similar treatment too, and we can remove all these REQ/RESP_SIZE macros. But for now, the req side does not bother gcc and I do not see any warning, so I left it. This series is really about getting rid of the warnings so that we can use CONFIG_WERROR. There are some xfs warnings that needs to be taken care of too to be able to use that one again though. > > But no big deal. Looks ok apart from that. If you agree to do the req cleanup as a followup series, can you send a formal review please ? > > Thanks, > John
On 6/9/22 21:02, John Garry wrote: > On 09/06/2022 12:59, Damien Le Moal wrote: >> On 6/9/22 17:43, John Garry wrote: >>>> #define DISCOVER_REQ_SIZE 16 >>>> -#define DISCOVER_RESP_SIZE 56 >>>> +#define DISCOVER_RESP_SIZE sizeof(struct smp_disc_resp) >>> I feel that it would be nice to get rid of these. >>> >>> Maybe something like: >>> >>> #define smp_execute_task_wrap(dev, req, resp)\ >>> smp_execute_task(dev, req, sizeof(*req), resp, DISCOVER_REQ_SIZE) >>> >>> >>> static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 >>> *disc_req, struct smp_disc_resp *disc_resp, int single) >>> { >>> res = smp_execute_task_wrap(dev, disc_req, disc_resp); >>> >>> We could even introduce a smp req struct to get rid of DISCOVER_REQ_SIZE >>> - the (current) code would be a bit less cryptic then. >> Yes, I think the req size needs a similar treatment too, and we can remove >> all these REQ/RESP_SIZE macros. But for now, the req side does not bother >> gcc and I do not see any warning, so I left it. This series is really >> about getting rid of the warnings so that we can use CONFIG_WERROR. >> There are some xfs warnings that needs to be taken care of too to be able >> to use that one again though. >> >>> But no big deal. Looks ok apart from that. >> If you agree to do the req cleanup as a followup series, > > ok, I'll assume that you can do it when you get a chance.. Yep, one more pile of patches added to the to-do list :) > > can you send a >> formal review please ? >> > Reviewed-by: John Garry <john.garry@huawei.com> Thanks !
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@opensource.wdc.com> > Reviewed-by: John Garry <john.garry@huawei.com> 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, > Patch 1 and 2 fix compilation warnings with gcc 12, leading to kernel > compilation failures if CONFIG_WERROR is enabled. Patch 3 complement > these fixes to have a consistent code with regard to sas responses. Applied to 5.20/scsi-staging, thanks!
On Thu, 9 Jun 2022 11:24:53 +0900, Damien Le Moal wrote: > Patch 1 and 2 fix compilation warnings with gcc 12, leading to kernel > compilation failures if CONFIG_WERROR is enabled. Patch 3 complement > these fixes to have a consistent code with regard to sas responses. > > Damien Le Moal (3): > scsi: libsas: introduce struct smp_disc_resp > scsi: libsas: introduce struct smp_rg_resp > scsi: libsas: introduce struct smp_rps_resp > > [...] Applied to 5.20/scsi-queue, thanks! [1/3] scsi: libsas: introduce struct smp_disc_resp https://git.kernel.org/mkp/scsi/c/c3752f44604f [2/3] scsi: libsas: introduce struct smp_rg_resp https://git.kernel.org/mkp/scsi/c/44f2bfe9ef08 [3/3] scsi: libsas: introduce struct smp_rps_resp https://git.kernel.org/mkp/scsi/c/3dafe0648ddd