diff mbox series

[4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev()

Message ID 20221204081643.3835966-5-yanaijie@huawei.com (mailing list archive)
State Superseded
Headers show
Series scsi: libsas: Some coding style fixes and cleanups | expand

Commit Message

Jason Yan Dec. 4, 2022, 8:16 a.m. UTC
The domain device 'child' is allocated in sas_ex_discover_end_dev() and
never been added to dev_list. So remove the useless list_del() and
related locks.

Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

John Garry Dec. 5, 2022, 9:14 a.m. UTC | #1
On 04/12/2022 08:16, Jason Yan wrote:
> The domain device 'child' is allocated in sas_ex_discover_end_dev() and
> never been added to dev_list. So remove the useless list_del() and
> related locks.
> 
> Cc: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>   drivers/scsi/libsas/sas_expander.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index a8af723fab3c..82ea7560a888 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -875,9 +875,6 @@ static struct domain_device *sas_ex_discover_end_dev(
>    out_list_del:
>   	sas_rphy_free(child->rphy);
>   	list_del(&child->disco_list_node);
> -	spin_lock_irq(&parent->port->dev_list_lock);
> -	list_del(&child->dev_list_node);
> -	spin_unlock_irq(&parent->port->dev_list_lock);

Since we have the spin lock'ing, this seems to be have been 
intentionally added (and not some simple typo or similar) - any idea of 
the origin?

Thanks,
John

>    out_free:
>   	sas_port_delete(phy->port);
>    out_err:
Jason Yan Dec. 8, 2022, 7:21 a.m. UTC | #2
On 2022/12/5 17:14, John Garry wrote:
> On 04/12/2022 08:16, Jason Yan wrote:
>> The domain device 'child' is allocated in sas_ex_discover_end_dev() and
>> never been added to dev_list. So remove the useless list_del() and
>> related locks.
>>
>> Cc: John Garry <john.g.garry@oracle.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> ---
>>   drivers/scsi/libsas/sas_expander.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>> b/drivers/scsi/libsas/sas_expander.c
>> index a8af723fab3c..82ea7560a888 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -875,9 +875,6 @@ static struct domain_device *sas_ex_discover_end_dev(
>>    out_list_del:
>>       sas_rphy_free(child->rphy);
>>       list_del(&child->disco_list_node);
>> -    spin_lock_irq(&parent->port->dev_list_lock);
>> -    list_del(&child->dev_list_node);
>> -    spin_unlock_irq(&parent->port->dev_list_lock);
> 
> Since we have the spin lock'ing, this seems to be have been 
> intentionally added (and not some simple typo or similar) - any idea of 
> the origin?

The new device used to be added to the dev_list in this function. But 
after 92625f9bff38 ("[SCSI] libsas: restore scan order") and 
87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery competing with 
ata error handling") it is added to the disco_list instead. But the 
list_del() and locking is forgot to be removed.

Thanks,
Jason
John Garry Dec. 8, 2022, 10:40 a.m. UTC | #3
On 08/12/2022 07:21, Jason Yan wrote:
>>> t_list_del:
>>>       sas_rphy_free(child->rphy);
>>>       list_del(&child->disco_list_node);
>>> -    spin_lock_irq(&parent->port->dev_list_lock);
>>> -    list_del(&child->dev_list_node);
>>> -    spin_unlock_irq(&parent->port->dev_list_lock);
>>
>> Since we have the spin lock'ing, this seems to be have been 
>> intentionally added (and not some simple typo or similar) - any idea 
>> of the origin?
> 
> The new device used to be added to the dev_list in this function. But 
> after 92625f9bff38 ("[SCSI] libsas: restore scan order") and 
> 87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery competing with 
> ata error handling") it is added to the disco_list instead. But the 
> list_del() and locking is forgot to be removed.

OK, so can we have a fixes tag then? That even helps review, as I can 
then quickly see where we went wrong.

Thanks,
John
Jason Yan Dec. 8, 2022, 11:11 a.m. UTC | #4
On 2022/12/8 18:40, John Garry wrote:
> On 08/12/2022 07:21, Jason Yan wrote:
>>>> t_list_del:
>>>>       sas_rphy_free(child->rphy);
>>>>       list_del(&child->disco_list_node);
>>>> -    spin_lock_irq(&parent->port->dev_list_lock);
>>>> -    list_del(&child->dev_list_node);
>>>> -    spin_unlock_irq(&parent->port->dev_list_lock);
>>>
>>> Since we have the spin lock'ing, this seems to be have been 
>>> intentionally added (and not some simple typo or similar) - any idea 
>>> of the origin?
>>
>> The new device used to be added to the dev_list in this function. But 
>> after 92625f9bff38 ("[SCSI] libsas: restore scan order") and 
>> 87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery competing 
>> with ata error handling") it is added to the disco_list instead. But 
>> the list_del() and locking is forgot to be removed.
> 
> OK, so can we have a fixes tag then? That even helps review, as I can 
> then quickly see where we went wrong.
> 

Sure.

Thanks,
Jason
diff mbox series

Patch

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index a8af723fab3c..82ea7560a888 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -875,9 +875,6 @@  static struct domain_device *sas_ex_discover_end_dev(
  out_list_del:
 	sas_rphy_free(child->rphy);
 	list_del(&child->disco_list_node);
-	spin_lock_irq(&parent->port->dev_list_lock);
-	list_del(&child->dev_list_node);
-	spin_unlock_irq(&parent->port->dev_list_lock);
  out_free:
 	sas_port_delete(phy->port);
  out_err: