[PATCH 0/3] Fix compilation warnings with gcc12

[PATCH 0/3] Fix compilation warnings with gcc12

[Damien Le Moal]

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

 drivers/scsi/aic94xx/aic94xx_dev.c |  2 +-
 drivers/scsi/libsas/sas_expander.c | 67 +++++++++++++-----------------
 drivers/scsi/libsas/sas_internal.h |  2 +-
 include/scsi/libsas.h              |  2 +-
 include/scsi/sas.h                 | 42 +++++++++----------
 5 files changed, 53 insertions(+), 62 deletions(-)

-- 
2.36.1

[PATCH 1/3] scsi: libsas: introduce struct smp_disc_resp

[Damien Le Moal]

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(-)

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_ */
-- 
2.36.1

[PATCH 2/3] scsi: libsas: introduce struct smp_rg_resp

[Damien Le Moal]

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_ex_change_count’,
    inlined from ‘sas_find_bcast_dev’ at
    drivers/scsi/libsas/sas_expander.c:1816:8:
drivers/scsi/libsas/sas_expander.c:1781:20: warning: array subscript
‘struct smp_resp[0]’ is partly outside array bounds of ‘unsigned
char[32]’ [-Warray-bounds]
 1781 |         if (rg_resp->result != SMP_RESP_FUNC_ACC) {
      |             ~~~~~~~^~~~~~~~

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_rg_resp for sas report general
responses.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/libsas/sas_expander.c | 31 +++++++++++++-----------------
 include/scsi/sas.h                 |  8 ++++++++
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index fb998a8a7d3b..78a38980636e 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -429,27 +429,14 @@ static int sas_expander_discover(struct domain_device *dev)
 
 #define MAX_EXPANDER_PHYS 128
 
-static void ex_assign_report_general(struct domain_device *dev,
-					    struct smp_resp *resp)
-{
-	struct report_general_resp *rg = &resp->rg;
-
-	dev->ex_dev.ex_change_count = be16_to_cpu(rg->change_count);
-	dev->ex_dev.max_route_indexes = be16_to_cpu(rg->route_indexes);
-	dev->ex_dev.num_phys = min(rg->num_phys, (u8)MAX_EXPANDER_PHYS);
-	dev->ex_dev.t2t_supp = rg->t2t_supp;
-	dev->ex_dev.conf_route_table = rg->conf_route_table;
-	dev->ex_dev.configuring = rg->configuring;
-	memcpy(dev->ex_dev.enclosure_logical_id, rg->enclosure_logical_id, 8);
-}
-
 #define RG_REQ_SIZE   8
-#define RG_RESP_SIZE 32
+#define RG_RESP_SIZE  sizeof(struct smp_rg_resp)
 
 static int sas_ex_general(struct domain_device *dev)
 {
 	u8 *rg_req;
-	struct smp_resp *rg_resp;
+	struct smp_rg_resp *rg_resp;
+	struct report_general_resp *rg;
 	int res;
 	int i;
 
@@ -480,7 +467,15 @@ static int sas_ex_general(struct domain_device *dev)
 			goto out;
 		}
 
-		ex_assign_report_general(dev, rg_resp);
+		rg = &rg_resp->rg;
+		dev->ex_dev.ex_change_count = be16_to_cpu(rg->change_count);
+		dev->ex_dev.max_route_indexes = be16_to_cpu(rg->route_indexes);
+		dev->ex_dev.num_phys = min(rg->num_phys, (u8)MAX_EXPANDER_PHYS);
+		dev->ex_dev.t2t_supp = rg->t2t_supp;
+		dev->ex_dev.conf_route_table = rg->conf_route_table;
+		dev->ex_dev.configuring = rg->configuring;
+		memcpy(dev->ex_dev.enclosure_logical_id,
+		       rg->enclosure_logical_id, 8);
 
 		if (dev->ex_dev.configuring) {
 			pr_debug("RG: ex %016llx self-configuring...\n",
@@ -1756,7 +1751,7 @@ static int sas_get_ex_change_count(struct domain_device *dev, int *ecc)
 {
 	int res;
 	u8  *rg_req;
-	struct smp_resp  *rg_resp;
+	struct smp_rg_resp  *rg_resp;
 
 	rg_req = alloc_smp_req(RG_REQ_SIZE);
 	if (!rg_req)
diff --git a/include/scsi/sas.h b/include/scsi/sas.h
index b3ee9bd63277..a8f9743ed6fc 100644
--- a/include/scsi/sas.h
+++ b/include/scsi/sas.h
@@ -696,6 +696,14 @@ struct report_phy_sata_resp {
 #error "Bitfield order not defined!"
 #endif
 
+struct smp_rg_resp {
+	u8    frame_type;
+	u8    function;
+	u8    result;
+	u8    reserved;
+	struct report_general_resp rg;
+} __attribute__ ((packed));
+
 struct smp_disc_resp {
 	u8    frame_type;
 	u8    function;
-- 
2.36.1

[PATCH 3/3] scsi: libsas: introduce struct smp_rps_resp

[Damien Le Moal]

Similarly to sas report general and discovery responses, define the
structure struct smp_rps_resp to handle SATA PHY report responses
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>
---
 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_ */
-- 
2.36.1

Re: [PATCH 1/3] scsi: libsas: introduce struct smp_disc_resp

[John Garry]

>   #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

Re: [PATCH 1/3] scsi: libsas: introduce struct smp_disc_resp

[Damien Le Moal]

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


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] scsi: libsas: introduce struct smp_disc_resp

[John Garry]

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>

Re: [PATCH 1/3] scsi: libsas: introduce struct smp_disc_resp

[Damien Le Moal]

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 !


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] scsi: libsas: introduce struct smp_rg_resp

[John Garry]

On 09/06/2022 03:24, Damien Le Moal wrote:
> 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_ex_change_count’,
>      inlined from ‘sas_find_bcast_dev’ at
>      drivers/scsi/libsas/sas_expander.c:1816:8:
> drivers/scsi/libsas/sas_expander.c:1781:20: warning: array subscript
> ‘struct smp_resp[0]’ is partly outside array bounds of ‘unsigned
> char[32]’ [-Warray-bounds]
>   1781 |         if (rg_resp->result != SMP_RESP_FUNC_ACC) {
>        |             ~~~~~~~^~~~~~~~
> 
> 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_rg_resp for sas report general
> responses.
> 
> Signed-off-by: Damien Le Moal<damien.lemoal@opensource.wdc.com>

Reviewed-by: John Garry <john.garry@huawei.com>

Re: [PATCH 3/3] scsi: libsas: introduce struct smp_rps_resp

[John Garry]

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 ....

> 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>

> ---
>   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_ */

Re: [PATCH 3/3] scsi: libsas: introduce struct smp_rps_resp

[Damien Le Moal]

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 Le Moal
Western Digital Research

Re: [PATCH 0/3] Fix compilation warnings with gcc12

[Martin K. Petersen]

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!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/3] Fix compilation warnings with gcc12

[Martin K. Petersen]

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

-- 
Martin K. Petersen	Oracle Linux Engineering