| Message ID | 20220609022456.409087-2-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Accepted |
| 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 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.. can you send a > formal review please ? > Reviewed-by: John Garry <john.garry@huawei.com>
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 !
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c index 260e735d06fa..fb998a8a7d3b 100644 --- a/drivers/scsi/libsas/sas_expander.c +++ b/drivers/scsi/libsas/sas_expander.c @@ -175,13 +175,13 @@ static enum sas_device_type to_dev_type(struct discover_resp *dr) return dr->attached_dev_type; } -static void sas_set_ex_phy(struct domain_device *dev, int phy_id, void *rsp) +static void sas_set_ex_phy(struct domain_device *dev, int phy_id, + struct smp_disc_resp *disc_resp) { enum sas_device_type dev_type; enum sas_linkrate linkrate; u8 sas_addr[SAS_ADDR_SIZE]; - struct smp_resp *resp = rsp; - struct discover_resp *dr = &resp->disc; + struct discover_resp *dr = &disc_resp->disc; struct sas_ha_struct *ha = dev->port->ha; struct expander_device *ex = &dev->ex_dev; struct ex_phy *phy = &ex->ex_phy[phy_id]; @@ -198,7 +198,7 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id, void *rsp) BUG_ON(!phy->phy); } - switch (resp->result) { + switch (disc_resp->result) { case SMP_RESP_PHY_VACANT: phy->phy_state = PHY_VACANT; break; @@ -347,12 +347,13 @@ struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id) } #define DISCOVER_REQ_SIZE 16 -#define DISCOVER_RESP_SIZE 56 +#define DISCOVER_RESP_SIZE sizeof(struct smp_disc_resp) static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req, - u8 *disc_resp, int single) + struct smp_disc_resp *disc_resp, + int single) { - struct discover_resp *dr; + struct discover_resp *dr = &disc_resp->disc; int res; disc_req[9] = single; @@ -361,7 +362,6 @@ static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req, disc_resp, DISCOVER_RESP_SIZE); if (res) return res; - dr = &((struct smp_resp *)disc_resp)->disc; if (memcmp(dev->sas_addr, dr->attached_sas_addr, SAS_ADDR_SIZE) == 0) { pr_notice("Found loopback topology, just ignore it!\n"); return 0; @@ -375,7 +375,7 @@ int sas_ex_phy_discover(struct domain_device *dev, int single) struct expander_device *ex = &dev->ex_dev; int res = 0; u8 *disc_req; - u8 *disc_resp; + struct smp_disc_resp *disc_resp; disc_req = alloc_smp_req(DISCOVER_REQ_SIZE); if (!disc_req) @@ -1657,7 +1657,7 @@ int sas_discover_root_expander(struct domain_device *dev) /* ---------- Domain revalidation ---------- */ static int sas_get_phy_discover(struct domain_device *dev, - int phy_id, struct smp_resp *disc_resp) + int phy_id, struct smp_disc_resp *disc_resp) { int res; u8 *disc_req; @@ -1673,10 +1673,8 @@ static int sas_get_phy_discover(struct domain_device *dev, disc_resp, DISCOVER_RESP_SIZE); if (res) goto out; - else if (disc_resp->result != SMP_RESP_FUNC_ACC) { + if (disc_resp->result != SMP_RESP_FUNC_ACC) res = disc_resp->result; - goto out; - } out: kfree(disc_req); return res; @@ -1686,7 +1684,7 @@ static int sas_get_phy_change_count(struct domain_device *dev, int phy_id, int *pcc) { int res; - struct smp_resp *disc_resp; + struct smp_disc_resp *disc_resp; disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE); if (!disc_resp) @@ -1704,19 +1702,17 @@ static int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id, u8 *sas_addr, enum sas_device_type *type) { int res; - struct smp_resp *disc_resp; - struct discover_resp *dr; + struct smp_disc_resp *disc_resp; disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE); if (!disc_resp) return -ENOMEM; - dr = &disc_resp->disc; res = sas_get_phy_discover(dev, phy_id, disc_resp); if (res == 0) { memcpy(sas_addr, disc_resp->disc.attached_sas_addr, SAS_ADDR_SIZE); - *type = to_dev_type(dr); + *type = to_dev_type(&disc_resp->disc); if (*type == 0) memset(sas_addr, 0, SAS_ADDR_SIZE); } diff --git a/include/scsi/sas.h b/include/scsi/sas.h index acfc69fd72d0..b3ee9bd63277 100644 --- a/include/scsi/sas.h +++ b/include/scsi/sas.h @@ -471,18 +471,6 @@ struct report_phy_sata_resp { __be32 crc; } __attribute__ ((packed)); -struct smp_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; - }; -} __attribute__ ((packed)); - #elif defined(__BIG_ENDIAN_BITFIELD) struct sas_identify_frame { /* Byte 0 */ @@ -704,6 +692,18 @@ struct report_phy_sata_resp { __be32 crc; } __attribute__ ((packed)); +#else +#error "Bitfield order not defined!" +#endif + +struct smp_disc_resp { + u8 frame_type; + u8 function; + u8 result; + u8 reserved; + struct discover_resp disc; +} __attribute__ ((packed)); + struct smp_resp { u8 frame_type; u8 function; @@ -716,8 +716,4 @@ struct smp_resp { }; } __attribute__ ((packed)); -#else -#error "Bitfield order not defined!" -#endif - #endif /* _SAS_H_ */
When compiling with gcc 12, several warnings are thrown by gcc when compiling drivers/scsi/libsas/sas_expander.c, e.g.: In function ‘sas_get_phy_change_count’, inlined from ‘sas_find_bcast_phy.constprop’ at drivers/scsi/libsas/sas_expander.c:1737:9: drivers/scsi/libsas/sas_expander.c:1697:39: warning: array subscript ‘struct smp_resp[0]’ is partly outside array bounds of ‘unsigned char[56]’ [-Warray-bounds] 1697 | *pcc = disc_resp->disc.change_count; | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~ This is due to the use of the struct smp_resp to aggregate all possible response types using a union but allocating a response buffer with a size exactly equal to the size of the response type needed. This leads to access to fields of struct smp_resp from an allocated memory area that is smaller than the size of struct smp_resp. Fix this by defining struct smp_disc_resp for sas discovery operations. Since this structure and the generic struct smp_resp are identical for the little endian and big endian archs, move the definition of these structures at the end of include/scsi/sas.h to avoid repeating their definition. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/scsi/libsas/sas_expander.c | 32 +++++++++++++----------------- include/scsi/sas.h | 28 +++++++++++--------------- 2 files changed, 26 insertions(+), 34 deletions(-)