| Message ID | 20221204081643.3835966-5-yanaijie@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: libsas: Some coding style fixes and cleanups | expand |
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:
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
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
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 --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:
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(-)