[PATCH v3 0/3] scsi: libsas: Fix disk not being scanned in after being removed

[PATCH v3 0/3] scsi: libsas: Fix disk not being scanned in after being removed

[Xingui Yang]

This patch series fixes an issue when do discovery on an empty PHY to
update PHY info after device unregister could cause newly connected device
to not be scanned.

Changes since v2:
- Based on John's suggestion, allow smp_execute_task() arguments to be on
the stack.
- Based on John's suggestion, add a helper sas_get_sas_addr_and_dev_type.
- Updated comments.

Changes since v1:
- Use sas_get_phy_discover() instead of sas_get_phy_attached_dev() in
sas_rediscover_dev() and use disc_resp to update phy info.

Xingui Yang (3):
  scsi: libsas: Allow smp_execute_task() arguments to be on the stack
  scsi: libsas: Add a helper sas_get_sas_addr_and_dev_type()
  scsi: libsas: Fix disk not being scanned in after being removed

 drivers/scsi/libsas/sas_expander.c | 64 ++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] scsi: libsas: Allow smp_execute_task() arguments to be on the stack

[Xingui Yang]

We need to use alloc_smp_resp() and alloc_smp_req() before call
smp_execute_task() as we can't allocate these memories on the stack for
calling sg_init_one(). But if we changed smp_execute_task() to memcpy
from/to data on the stack, it might make callers simpler.

Suggested-by: John Garry <john.g.garry@oracle.com>
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 32 ++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index a2204674b680..1eeb69cba8da 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -120,17 +120,6 @@ static int smp_execute_task_sg(struct domain_device *dev,
 	return res;
 }
 
-static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
-			    void *resp, int resp_size)
-{
-	struct scatterlist req_sg;
-	struct scatterlist resp_sg;
-
-	sg_init_one(&req_sg, req, req_size);
-	sg_init_one(&resp_sg, resp, resp_size);
-	return smp_execute_task_sg(dev, &req_sg, &resp_sg);
-}
-
 /* ---------- Allocations ---------- */
 
 static inline void *alloc_smp_req(int size)
@@ -146,6 +135,27 @@ static inline void *alloc_smp_resp(int size)
 	return kzalloc(size, GFP_KERNEL);
 }
 
+static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
+			    void *resp, int resp_size)
+{
+	struct scatterlist req_sg;
+	struct scatterlist resp_sg;
+	void *_req = kmemdup(req, req_size, GFP_KERNEL);
+	void *_resp = alloc_smp_resp(resp_size);
+	int ret;
+
+	if (!_req || !resp)
+		return -ENOMEM;
+
+	sg_init_one(&req_sg, _req, req_size);
+	sg_init_one(&resp_sg, _resp, resp_size);
+	ret = smp_execute_task_sg(dev, &req_sg, &resp_sg);
+	memcpy(resp, _resp, resp_size);
+	kfree(_req);
+	kfree(_resp);
+	return ret;
+}
+
 static char sas_route_char(struct domain_device *dev, struct ex_phy *phy)
 {
 	switch (phy->routing_attr) {
-- 
2.17.1

Re: [PATCH v3 1/3] scsi: libsas: Allow smp_execute_task() arguments to be on the stack

[Dan Carpenter]

Hi Xingui,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Xingui-Yang/scsi-libsas-Allow-smp_execute_task-arguments-to-be-on-the-stack/20240307-174215
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20240307093733.41222-2-yangxingui%40huawei.com
patch subject: [PATCH v3 1/3] scsi: libsas: Allow smp_execute_task() arguments to be on the stack
config: i386-randconfig-141-20240308 (https://download.01.org/0day-ci/archive/20240310/202403102353.jUPi6fOP-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202403102353.jUPi6fOP-lkp@intel.com/

New smatch warnings:
drivers/scsi/libsas/sas_expander.c:148 smp_execute_task() warn: possible memory leak of '_req'

vim +/_req +148 drivers/scsi/libsas/sas_expander.c

adfd2325dfc5cf6 Xingui Yang     2024-03-07  138  static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
adfd2325dfc5cf6 Xingui Yang     2024-03-07  139  			    void *resp, int resp_size)
adfd2325dfc5cf6 Xingui Yang     2024-03-07  140  {
adfd2325dfc5cf6 Xingui Yang     2024-03-07  141  	struct scatterlist req_sg;
adfd2325dfc5cf6 Xingui Yang     2024-03-07  142  	struct scatterlist resp_sg;
adfd2325dfc5cf6 Xingui Yang     2024-03-07  143  	void *_req = kmemdup(req, req_size, GFP_KERNEL);
adfd2325dfc5cf6 Xingui Yang     2024-03-07  144  	void *_resp = alloc_smp_resp(resp_size);
adfd2325dfc5cf6 Xingui Yang     2024-03-07  145  	int ret;
adfd2325dfc5cf6 Xingui Yang     2024-03-07  146  
adfd2325dfc5cf6 Xingui Yang     2024-03-07  147  	if (!_req || !resp)
adfd2325dfc5cf6 Xingui Yang     2024-03-07 @148  		return -ENOMEM;

I haven't looked at the callers so I don't know how likely it is for one
of the allocations to fail and the other succeed...  But it seems
possible.

adfd2325dfc5cf6 Xingui Yang     2024-03-07  149  
adfd2325dfc5cf6 Xingui Yang     2024-03-07  150  	sg_init_one(&req_sg, _req, req_size);
adfd2325dfc5cf6 Xingui Yang     2024-03-07  151  	sg_init_one(&resp_sg, _resp, resp_size);
adfd2325dfc5cf6 Xingui Yang     2024-03-07  152  	ret = smp_execute_task_sg(dev, &req_sg, &resp_sg);
adfd2325dfc5cf6 Xingui Yang     2024-03-07  153  	memcpy(resp, _resp, resp_size);
adfd2325dfc5cf6 Xingui Yang     2024-03-07  154  	kfree(_req);
adfd2325dfc5cf6 Xingui Yang     2024-03-07  155  	kfree(_resp);
adfd2325dfc5cf6 Xingui Yang     2024-03-07  156  	return ret;
adfd2325dfc5cf6 Xingui Yang     2024-03-07  157  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 1/3] scsi: libsas: Allow smp_execute_task() arguments to be on the stack

[yangxingui]

On 2024/3/11 13:42, Dan Carpenter wrote:
> Hi Xingui,
> 
> kernel test robot noticed the following build warnings:
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Xingui-Yang/scsi-libsas-Allow-smp_execute_task-arguments-to-be-on-the-stack/20240307-174215
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
> patch link:    https://lore.kernel.org/r/20240307093733.41222-2-yangxingui%40huawei.com
> patch subject: [PATCH v3 1/3] scsi: libsas: Allow smp_execute_task() arguments to be on the stack
> config: i386-randconfig-141-20240308 (https://download.01.org/0day-ci/archive/20240310/202403102353.jUPi6fOP-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202403102353.jUPi6fOP-lkp@intel.com/
> 
> New smatch warnings:
> drivers/scsi/libsas/sas_expander.c:148 smp_execute_task() warn: possible memory leak of '_req'
> 
> vim +/_req +148 drivers/scsi/libsas/sas_expander.c
> 
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  138  static int smp_execute_task(struct domain_device *dev, void *req, int req_size,
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  139  			    void *resp, int resp_size)
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  140  {
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  141  	struct scatterlist req_sg;
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  142  	struct scatterlist resp_sg;
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  143  	void *_req = kmemdup(req, req_size, GFP_KERNEL);
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  144  	void *_resp = alloc_smp_resp(resp_size);
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  145  	int ret;
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  146
> adfd2325dfc5cf6 Xingui Yang     2024-03-07  147  	if (!_req || !resp)
> adfd2325dfc5cf6 Xingui Yang     2024-03-07 @148  		return -ENOMEM;
> 
> I haven't looked at the callers so I don't know how likely it is for one
> of the allocations to fail and the other succeed...  But it seems
> possible.

Yes, it's possible. This patch has been canceled in v4. Based on John's 
suggestion, if there are plans to resubmit modifications , we will pay 
attention to this, thank you.

Thanks,
Xingui

[PATCH v3 2/3] scsi: libsas: Add a helper sas_get_sas_addr_and_dev_type()

[Xingui Yang]

Add a helper to get attached_sas_addr and device type from disc_resp.

Suggested-by: John Garry <john.g.garry@oracle.com>
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 1eeb69cba8da..d6147616339f 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1631,6 +1631,16 @@ int sas_discover_root_expander(struct domain_device *dev)
 
 /* ---------- Domain revalidation ---------- */
 
+static void sas_get_sas_addr_and_dev_type(struct smp_disc_resp *disc_resp,
+					  u8 *sas_addr,
+					  enum sas_device_type *type)
+{
+	memcpy(sas_addr, disc_resp->disc.attached_sas_addr, SAS_ADDR_SIZE);
+	*type = to_dev_type(&disc_resp->disc);
+	if (*type == SAS_PHY_UNUSED)
+		memset(sas_addr, 0, SAS_ADDR_SIZE);
+}
+
 static int sas_get_phy_discover(struct domain_device *dev,
 				int phy_id, struct smp_disc_resp *disc_resp)
 {
@@ -1684,13 +1694,8 @@ int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
 		return -ENOMEM;
 
 	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(&disc_resp->disc);
-		if (*type == 0)
-			memset(sas_addr, 0, SAS_ADDR_SIZE);
-	}
+	if (res == 0)
+		sas_get_sas_addr_and_dev_type(disc_resp, sas_addr, type);
 	kfree(disc_resp);
 	return res;
 }
-- 
2.17.1

Re: [PATCH v3 2/3] scsi: libsas: Add a helper sas_get_sas_addr_and_dev_type()

[John Garry]

On 07/03/2024 09:37, Xingui Yang wrote:
> Add a helper to get attached_sas_addr and device type from disc_resp.
> 
> Suggested-by: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>

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

> ---
>   drivers/scsi/libsas/sas_expander.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 1eeb69cba8da..d6147616339f 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1631,6 +1631,16 @@ int sas_discover_root_expander(struct domain_device *dev)
>   
>   /* ---------- Domain revalidation ---------- */
>   
> +static void sas_get_sas_addr_and_dev_type(struct smp_disc_resp *disc_resp,
> +					  u8 *sas_addr,
> +					  enum sas_device_type *type)
> +{
> +	memcpy(sas_addr, disc_resp->disc.attached_sas_addr, SAS_ADDR_SIZE);
> +	*type = to_dev_type(&disc_resp->disc);
> +	if (*type == SAS_PHY_UNUSED)
> +		memset(sas_addr, 0, SAS_ADDR_SIZE);
> +}
> +
>   static int sas_get_phy_discover(struct domain_device *dev,
>   				int phy_id, struct smp_disc_resp *disc_resp)
>   {
> @@ -1684,13 +1694,8 @@ int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
>   		return -ENOMEM;
>   
>   	res = sas_get_phy_discover(dev, phy_id, disc_resp);

At some stage - I am not saying necessarily now! - it would be good to 
stop this function returning both a linux error code and a SAS protocol 
response code

> -	if (res == 0) {
> -		memcpy(sas_addr, disc_resp->disc.attached_sas_addr,
> -		       SAS_ADDR_SIZE);
> -		*type = to_dev_type(&disc_resp->disc);
> -		if (*type == 0)
> -			memset(sas_addr, 0, SAS_ADDR_SIZE);
> -	}
> +	if (res == 0)
> +		sas_get_sas_addr_and_dev_type(disc_resp, sas_addr, type);
>   	kfree(disc_resp);
>   	return res;
>   }

[PATCH v3 3/3] scsi: libsas: Fix disk not being scanned in after being removed

[Xingui Yang]

As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
update PHY info"), do discovery will send a new SMP_DISCOVER and update
phy->phy_change_count. We found that if the disk is reconnected and phy
change_count changes at this time, the disk scanning process will not be
triggered.

Therefore, call sas_set_ex_phy() to update the PHY info with the results of
the last query. And because the previous phy info will be used when calling
sas_unregister_devs_sas_addr(), sas_unregister_devs_sas_addr() should be
called before sas_set_ex_phy().

Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to update PHY info")
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index d6147616339f..5ef77a7d235e 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1955,6 +1955,7 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
 	struct expander_device *ex = &dev->ex_dev;
 	struct ex_phy *phy = &ex->ex_phy[phy_id];
 	enum sas_device_type type = SAS_PHY_UNUSED;
+	struct smp_disc_resp disc_resp;
 	u8 sas_addr[SAS_ADDR_SIZE];
 	char msg[80] = "";
 	int res;
@@ -1966,7 +1967,7 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
 		 SAS_ADDR(dev->sas_addr), phy_id, msg);
 
 	memset(sas_addr, 0, SAS_ADDR_SIZE);
-	res = sas_get_phy_attached_dev(dev, phy_id, sas_addr, &type);
+	res = sas_get_phy_discover(dev, phy_id, &disc_resp);
 	switch (res) {
 	case SMP_RESP_NO_PHY:
 		phy->phy_state = PHY_NOT_PRESENT;
@@ -1984,14 +1985,18 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
 		return res;
 	}
 
+	if (res == 0)
+		sas_get_sas_addr_and_dev_type(&disc_resp, sas_addr, &type);
+
 	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
 		phy->phy_state = PHY_EMPTY;
 		sas_unregister_devs_sas_addr(dev, phy_id, last);
 		/*
-		 * Even though the PHY is empty, for convenience we discover
-		 * the PHY to update the PHY info, like negotiated linkrate.
+		 * Even though the PHY is empty, for convenience we update
+		 * the PHY info, like negotiated linkrate.
 		 */
-		sas_ex_phy_discover(dev, phy_id);
+		if (res == 0)
+			sas_set_ex_phy(dev, phy_id, &disc_resp);
 		return res;
 	} else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
 		   dev_type_flutter(type, phy->attached_dev_type)) {
-- 
2.17.1

Re: [PATCH v3 3/3] scsi: libsas: Fix disk not being scanned in after being removed

[John Garry]

On 07/03/2024 09:37, Xingui Yang wrote:
> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
> update PHY info"), do discovery will send a new SMP_DISCOVER and update
> phy->phy_change_count. We found that if the disk is reconnected and phy
> change_count changes at this time, the disk scanning process will not be
> triggered.
> 
> Therefore, call sas_set_ex_phy() to update the PHY info with the results of
> the last query. And because the previous phy info will be used when calling
> sas_unregister_devs_sas_addr(), sas_unregister_devs_sas_addr() should be
> called before sas_set_ex_phy().
> 
> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to update PHY info")
> Signed-off-by: Xingui Yang<yangxingui@huawei.com>

I am also ok with a change to revert to allocating the resp memory with 
alloc_smp_resp(), but make the changes neat please:
Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH v3 3/3] scsi: libsas: Fix disk not being scanned in after being removed

[yangxingui]

Hi John,
On 2024/3/7 18:08, John Garry wrote:
> On 07/03/2024 09:37, Xingui Yang wrote:
>> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
>> update PHY info"), do discovery will send a new SMP_DISCOVER and update
>> phy->phy_change_count. We found that if the disk is reconnected and phy
>> change_count changes at this time, the disk scanning process will not be
>> triggered.
>>
>> Therefore, call sas_set_ex_phy() to update the PHY info with the 
>> results of
>> the last query. And because the previous phy info will be used when 
>> calling
>> sas_unregister_devs_sas_addr(), sas_unregister_devs_sas_addr() should be
>> called before sas_set_ex_phy().
>>
>> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to 
>> update PHY info")
>> Signed-off-by: Xingui Yang<yangxingui@huawei.com>
> 
> I am also ok with a change to revert to allocating the resp memory with 
> alloc_smp_resp(), but make the changes neat please:
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> .

Thanks for your review, I have updated the version.

Thanks,
Xingui

Re: [PATCH v3 0/3] scsi: libsas: Fix disk not being scanned in after being removed

[John Garry]

On 07/03/2024 09:37, Xingui Yang wrote:
> This patch series fixes an issue when do discovery on an empty PHY to
> update PHY info after device unregister could cause newly connected device
> to not be scanned.
> 
> Changes since v2:
> - Based on John's suggestion, allow smp_execute_task() arguments to be on
> the stack.

You can't just change one particular smp_execute_task() callsite to put 
the disc memories on the stack - they should all be changed.

Better yet, we should see how the changes look and then decide to change 
at all.

Anyway, since you are simplifying the code, let's revisit the 
smp_execute_task() change later.

Thanks,
John