From: Xiang Chen chenxiang66@hisilicon.com
There are some small fixes related to scsi and libsas: - Use void for sas_discover_event() return code; - Remove duplicatted setting fro task->task_state_flags; - Remove unused parameter for function sas_ata_eh(); - Use spin_lock/unlock instead of spin_lock/unlock_irq or spin_lock_irqsave /spin_unlock_irqrestore for asd_sas_port->dev_list_lock and sas_ha->phy_port_lock; - Remove unused member cmd_pool for structure scsi_host_template; - Remove unnecessary memset for scsilun;
Xiang Chen (7): {topost} scsi: libsas: Use void for sas_discover_event() return code {topost} scsi: libsas: Remove duplicated setting for task->task_state_flags {topost} scsi: libsas: Remove unused parameter for function sas_ata_eh() {topost} scsi: libsas: Use spin_lock/unlock() for asd_sas_port->dev_list_lock {topost} scsi: libsas: Use spin_lock/unlock() for sas_ha->phy_port_lock {topost} scsi: Remove unused member cmd_pool for structure scsi_host_template {topost} scsi: Remove unnecssary memset for scsilun
drivers/scsi/libsas/sas_ata.c | 8 +++----- drivers/scsi/libsas/sas_discover.c | 26 ++++++++++++-------------- drivers/scsi/libsas/sas_expander.c | 12 ++++++------ drivers/scsi/libsas/sas_port.c | 12 +++++------- drivers/scsi/libsas/sas_scsi_host.c | 12 +++++------- drivers/scsi/scsi_common.c | 2 -- include/scsi/libsas.h | 2 +- include/scsi/sas_ata.h | 6 ++---- include/scsi/scsi_host.h | 3 --- 9 files changed, 34 insertions(+), 49 deletions(-)
From: Xiang Chen chenxiang66@hisilicon.com
The callers of function sas_discover_event() don't check the return value of it, and also it only returns 0, so use void for its return code.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com --- drivers/scsi/libsas/sas_discover.c | 6 ++---- include/scsi/libsas.h | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c index 2faa7a372284..eef270a70754 100644 --- a/drivers/scsi/libsas/sas_discover.c +++ b/drivers/scsi/libsas/sas_discover.c @@ -556,19 +556,17 @@ static void sas_chain_event(int event, unsigned long *pending, } }
-int sas_discover_event(struct asd_sas_port *port, enum discover_event ev) +void sas_discover_event(struct asd_sas_port *port, enum discover_event ev) { struct sas_discovery *disc;
if (!port) - return 0; + return; disc = &port->disc;
BUG_ON(ev >= DISC_NUM_EVENTS);
sas_chain_event(ev, &disc->pending, &disc->disc_work[ev].work, port->ha); - - return 0; }
/** diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index e2a219a2f748..2105ea0b2633 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -686,7 +686,7 @@ void sas_ex_revalidate_domain(struct domain_device *port_dev, bool *retry);
void sas_unregister_domain_devices(struct asd_sas_port *port, int gone); void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *); -int sas_discover_event(struct asd_sas_port *, enum discover_event ev); +void sas_discover_event(struct asd_sas_port *, enum discover_event ev);
int sas_discover_sata(struct domain_device *); int sas_discover_end_dev(struct domain_device *);
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chenchenxiang66@hisilicon.com
The callers of function sas_discover_event() don't check the return value of it, and also it only returns 0, so use void for its return code.
"don't check its return value"
Signed-off-by: Xiang Chenchenxiang66@hisilicon.com
Reviewed-by: John Garry john.garry@huawei.com
在 2021/12/24 16:35, John Garry 写道:
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chenchenxiang66@hisilicon.com
The callers of function sas_discover_event() don't check the return value of it, and also it only returns 0, so use void for its return code.
"don't check its return value"
ok
Signed-off-by: Xiang Chenchenxiang66@hisilicon.com
Reviewed-by: John Garry john.garry@huawei.com .
From: Xiang Chen chenxiang66@hisilicon.com
Task->task_state_flags is already set in function sas_alloc_task(), so remove duplicasted setting.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com --- drivers/scsi/libsas/sas_ata.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index 6bd00f3809c0..de18dd988b49 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -209,7 +209,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc) } task->scatter = qc->sg; task->ata_task.retry_count = 1; - task->task_state_flags = SAS_TASK_STATE_PENDING; qc->lldd_task = task;
task->ata_task.use_ncq = ata_is_ncq(qc->tf.protocol);
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
Task->task_state_flags is already set in function sas_alloc_task(), so remove duplicasted setting.
"duplicasted" spelling
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
Reviewed-by: John Garry john.garry@huawei.com
drivers/scsi/libsas/sas_ata.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index 6bd00f3809c0..de18dd988b49 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -209,7 +209,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc) } task->scatter = qc->sg; task->ata_task.retry_count = 1;
task->task_state_flags = SAS_TASK_STATE_PENDING; qc->lldd_task = task;
task->ata_task.use_ncq = ata_is_ncq(qc->tf.protocol);
在 2021/12/24 16:36, John Garry 写道:
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
Task->task_state_flags is already set in function sas_alloc_task(), so remove duplicasted setting.
"duplicasted" spelling
ok
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
Reviewed-by: John Garry john.garry@huawei.com
drivers/scsi/libsas/sas_ata.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index 6bd00f3809c0..de18dd988b49 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -209,7 +209,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc) } task->scatter = qc->sg; task->ata_task.retry_count = 1;
- task->task_state_flags = SAS_TASK_STATE_PENDING; qc->lldd_task = task; task->ata_task.use_ncq = ata_is_ncq(qc->tf.protocol);
.
From: Xiang Chen chenxiang66@hisilicon.com
Input parameter work_q is not unused in function sas_ata_eh(), so remote it.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com --- drivers/scsi/libsas/sas_ata.c | 3 +-- drivers/scsi/libsas/sas_scsi_host.c | 2 +- include/scsi/sas_ata.h | 6 ++---- 3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index de18dd988b49..19827a23784e 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -800,8 +800,7 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost) sas_enable_revalidation(sas_ha); }
-void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q, - struct list_head *done_q) +void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q) { struct scsi_cmnd *cmd, *n; struct domain_device *eh_dev; diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 1a2944b4e6cf..99f4e70e9683 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -757,7 +757,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost) * scsi_unjam_host does, but we skip scsi_eh_abort_cmds because any * command we see here has no sas_task and is thus unknown to the HA. */ - sas_ata_eh(shost, &eh_work_q, &ha->eh_done_q); + sas_ata_eh(shost, &eh_work_q); if (!scsi_eh_get_sense(&eh_work_q, &ha->eh_done_q)) scsi_eh_ready_devs(shost, &eh_work_q, &ha->eh_done_q);
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h index 416c9c47d0e7..21e7c10c6295 100644 --- a/include/scsi/sas_ata.h +++ b/include/scsi/sas_ata.h @@ -25,8 +25,7 @@ int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy); int sas_ata_init(struct domain_device *dev); void sas_ata_task_abort(struct sas_task *task); void sas_ata_strategy_handler(struct Scsi_Host *shost); -void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q, - struct list_head *done_q); +void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q); void sas_ata_schedule_reset(struct domain_device *dev); void sas_ata_wait_eh(struct domain_device *dev); void sas_probe_sata(struct asd_sas_port *port); @@ -52,8 +51,7 @@ static inline void sas_ata_strategy_handler(struct Scsi_Host *shost) { }
-static inline void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q, - struct list_head *done_q) +static inline void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q) { }
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
Input parameter work_q is not unused in function sas_ata_eh(), so remote it.
remote it?
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
drivers/scsi/libsas/sas_ata.c | 3 +-- drivers/scsi/libsas/sas_scsi_host.c | 2 +- include/scsi/sas_ata.h | 6 ++---- 3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index de18dd988b49..19827a23784e 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -800,8 +800,7 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost) sas_enable_revalidation(sas_ha); }
-void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q)+void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q) { struct scsi_cmnd *cmd, *n; struct domain_device *eh_dev; diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 1a2944b4e6cf..99f4e70e9683 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -757,7 +757,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost) * scsi_unjam_host does, but we skip scsi_eh_abort_cmds because any * command we see here has no sas_task and is thus unknown to the HA. */
- sas_ata_eh(shost, &eh_work_q, &ha->eh_done_q);
- sas_ata_eh(shost, &eh_work_q); if (!scsi_eh_get_sense(&eh_work_q, &ha->eh_done_q)) scsi_eh_ready_devs(shost, &eh_work_q, &ha->eh_done_q);
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h index 416c9c47d0e7..21e7c10c6295 100644 --- a/include/scsi/sas_ata.h +++ b/include/scsi/sas_ata.h @@ -25,8 +25,7 @@ int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy); int sas_ata_init(struct domain_device *dev); void sas_ata_task_abort(struct sas_task *task); void sas_ata_strategy_handler(struct Scsi_Host *shost); -void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q);+void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q); void sas_ata_schedule_reset(struct domain_device *dev); void sas_ata_wait_eh(struct domain_device *dev); void sas_probe_sata(struct asd_sas_port *port); @@ -52,8 +51,7 @@ static inline void sas_ata_strategy_handler(struct Scsi_Host *shost) { }
-static inline void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q)+static inline void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q) { }
在 2021/12/24 16:37, John Garry 写道:
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
Input parameter work_q is not unused in function sas_ata_eh(), so remote it.
remote it?
It should be "remove it", typo
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
drivers/scsi/libsas/sas_ata.c | 3 +-- drivers/scsi/libsas/sas_scsi_host.c | 2 +- include/scsi/sas_ata.h | 6 ++---- 3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index de18dd988b49..19827a23784e 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -800,8 +800,7 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost) sas_enable_revalidation(sas_ha); } -void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q)+void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q) { struct scsi_cmnd *cmd, *n; struct domain_device *eh_dev; diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 1a2944b4e6cf..99f4e70e9683 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -757,7 +757,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost) * scsi_unjam_host does, but we skip scsi_eh_abort_cmds because any * command we see here has no sas_task and is thus unknown to the HA. */
- sas_ata_eh(shost, &eh_work_q, &ha->eh_done_q);
- sas_ata_eh(shost, &eh_work_q); if (!scsi_eh_get_sense(&eh_work_q, &ha->eh_done_q)) scsi_eh_ready_devs(shost, &eh_work_q, &ha->eh_done_q); diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 416c9c47d0e7..21e7c10c6295 100644 --- a/include/scsi/sas_ata.h +++ b/include/scsi/sas_ata.h @@ -25,8 +25,7 @@ int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy); int sas_ata_init(struct domain_device *dev); void sas_ata_task_abort(struct sas_task *task); void sas_ata_strategy_handler(struct Scsi_Host *shost); -void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q);+void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q); void sas_ata_schedule_reset(struct domain_device *dev); void sas_ata_wait_eh(struct domain_device *dev); void sas_probe_sata(struct asd_sas_port *port); @@ -52,8 +51,7 @@ static inline void sas_ata_strategy_handler(struct Scsi_Host *shost) { } -static inline void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
struct list_head *done_q)+static inline void sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q) { }
.
From: Xiang Chen chenxiang66@hisilicon.com
Spinlock asd_sas_port->dev_list_lock is used when domain_dev is added or removed. But some places use spin_lock()/spin_unlock() while other places use spin_lock_irq() /spin_unlock_irq(). Actually all those places are called in process context (workqueue) now, so use spin_lock/unlock() instead of spin_lock/unlock_irq().
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com --- drivers/scsi/libsas/sas_discover.c | 16 ++++++++-------- drivers/scsi/libsas/sas_expander.c | 12 ++++++------ 2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c index eef270a70754..e7d328ec609b 100644 --- a/drivers/scsi/libsas/sas_discover.c +++ b/drivers/scsi/libsas/sas_discover.c @@ -152,9 +152,9 @@ static int sas_get_port_device(struct asd_sas_port *port) if (dev_is_sata(dev) || dev->dev_type == SAS_END_DEVICE) list_add_tail(&dev->disco_list_node, &port->disco_list); else { - spin_lock_irq(&port->dev_list_lock); + spin_lock(&port->dev_list_lock); list_add_tail(&dev->dev_list_node, &port->dev_list); - spin_unlock_irq(&port->dev_list_lock); + spin_unlock(&port->dev_list_lock); }
spin_lock(&port->phy_list_lock); @@ -211,9 +211,9 @@ static void sas_probe_devices(struct asd_sas_port *port)
/* devices must be domain members before link recovery and probe */ list_for_each_entry(dev, &port->disco_list, disco_list_node) { - spin_lock_irq(&port->dev_list_lock); + spin_lock(&port->dev_list_lock); list_add_tail(&dev->dev_list_node, &port->dev_list); - spin_unlock_irq(&port->dev_list_lock); + spin_unlock(&port->dev_list_lock); }
sas_probe_sata(port); @@ -321,11 +321,11 @@ static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_d else list_del_init(&dev->siblings);
- spin_lock_irq(&port->dev_list_lock); + spin_lock(&port->dev_list_lock); list_del_init(&dev->dev_list_node); if (dev_is_sata(dev)) sas_ata_end_eh(dev->sata_dev.ap); - spin_unlock_irq(&port->dev_list_lock); + spin_unlock(&port->dev_list_lock);
spin_lock_irq(&ha->lock); if (dev->dev_type == SAS_END_DEVICE && @@ -473,9 +473,9 @@ static void sas_discover_domain(struct work_struct *work) if (error) { sas_rphy_free(dev->rphy); list_del_init(&dev->disco_list_node); - spin_lock_irq(&port->dev_list_lock); + spin_lock(&port->dev_list_lock); list_del_init(&dev->dev_list_node); - spin_unlock_irq(&port->dev_list_lock); + spin_unlock(&port->dev_list_lock);
sas_put_device(dev); port->port_dev = NULL; diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c index 6425b9fd99ee..cfc4c6db7593 100644 --- a/drivers/scsi/libsas/sas_expander.c +++ b/drivers/scsi/libsas/sas_expander.c @@ -902,9 +902,9 @@ 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); + spin_lock(&parent->port->dev_list_lock); list_del(&child->dev_list_node); - spin_unlock_irq(&parent->port->dev_list_lock); + spin_unlock(&parent->port->dev_list_lock); out_free: sas_port_delete(phy->port); out_err: @@ -997,16 +997,16 @@ static struct domain_device *sas_ex_discover_expander( sas_fill_in_rphy(child, rphy); sas_rphy_add(rphy);
- spin_lock_irq(&parent->port->dev_list_lock); + spin_lock(&parent->port->dev_list_lock); list_add_tail(&child->dev_list_node, &parent->port->dev_list); - spin_unlock_irq(&parent->port->dev_list_lock); + spin_unlock(&parent->port->dev_list_lock);
res = sas_discover_expander(child); if (res) { sas_rphy_delete(rphy); - spin_lock_irq(&parent->port->dev_list_lock); + spin_lock(&parent->port->dev_list_lock); list_del(&child->dev_list_node); - spin_unlock_irq(&parent->port->dev_list_lock); + spin_unlock(&parent->port->dev_list_lock); sas_put_device(child); sas_port_delete(phy->port); phy->port = NULL;
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chenchenxiang66@hisilicon.com
Spinlock asd_sas_port->dev_list_lock is used when domain_dev is added or removed. But some places use spin_lock()/spin_unlock() while other places use spin_lock_irq() /spin_unlock_irq(). Actually all those places are called in process context (workqueue) now, so use spin_lock/unlock() instead of
spin_lock/unlock_irq().
Please use form spin_{lock, unlock}_irq(), and similar for other places.
So do you have an idea why spin_lock_irq() was used in the first place? There has to be a reason. Where some interrupt handlers using the locks?
Thanks, John
在 2022/1/7 0:01, John Garry 写道:
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chenchenxiang66@hisilicon.com
Spinlock asd_sas_port->dev_list_lock is used when domain_dev is added or removed. But some places use spin_lock()/spin_unlock() while other places use spin_lock_irq() /spin_unlock_irq(). Actually all those places are called in process context (workqueue) now, so use spin_lock/unlock() instead of
spin_lock/unlock_irq().
Please use form spin_{lock, unlock}_irq(), and similar for other places.
ok
So do you have an idea why spin_lock_irq() was used in the first place? There has to be a reason. Where some interrupt handlers using the locks?
spin_lock_irq() in the first place is only called by sas_discover_domain() which is not in interrupt handlers. I check all those places which call asd_sas_port->dev_list_lock, and don't find any place which is called in interrupt handlers. Actually there are some places which use spin_lock() and other places which use spin_lock_irq() for asd_sas_port->dev_list_lock, if it were used in interrupt handler, i think all the places should use spin_lock_irq() instead.
Thanks, John .
From: Xiang Chen chenxiang66@hisilicon.com
For spinlock sas_ha->phy_port_lock, some places use spin_lock_irq() / spin_unlock_irq() and other places use spin_lock_irqsave() / spin_unlock_irqrestore(). Actually all those places are called in process context (workqueue), so use spin_lock/unlock() instead.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com --- drivers/scsi/libsas/sas_ata.c | 4 ++-- drivers/scsi/libsas/sas_discover.c | 4 ++-- drivers/scsi/libsas/sas_port.c | 12 +++++------- drivers/scsi/libsas/sas_scsi_host.c | 10 ++++------ 4 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index 19827a23784e..85d75fd30b63 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -773,7 +773,7 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost) */ sas_disable_revalidation(sas_ha);
- spin_lock_irq(&sas_ha->phy_port_lock); + spin_lock(&sas_ha->phy_port_lock); for (i = 0; i < sas_ha->num_phys; i++) { struct asd_sas_port *port = sas_ha->sas_port[i]; struct domain_device *dev; @@ -793,7 +793,7 @@ void sas_ata_strategy_handler(struct Scsi_Host *shost) } spin_unlock(&port->dev_list_lock); } - spin_unlock_irq(&sas_ha->phy_port_lock); + spin_unlock(&sas_ha->phy_port_lock);
async_synchronize_full_domain(&async);
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c index e7d328ec609b..b0ae431f766a 100644 --- a/drivers/scsi/libsas/sas_discover.c +++ b/drivers/scsi/libsas/sas_discover.c @@ -407,12 +407,12 @@ void sas_device_set_phy(struct domain_device *dev, struct sas_port *port) new_phy = sas_port_get_phy(port);
/* pin and record last seen phy */ - spin_lock_irq(&ha->phy_port_lock); + spin_lock(&ha->phy_port_lock); if (new_phy) { sas_port_put_phy(dev->phy); dev->phy = new_phy; } - spin_unlock_irq(&ha->phy_port_lock); + spin_unlock(&ha->phy_port_lock); }
/* ---------- Discovery and Revalidation ---------- */ diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c index 67b429dcf1ff..dc17674034a3 100644 --- a/drivers/scsi/libsas/sas_port.c +++ b/drivers/scsi/libsas/sas_port.c @@ -82,7 +82,6 @@ static void sas_form_port(struct asd_sas_phy *phy) struct domain_device *port_dev; struct sas_internal *si = to_sas_internal(sas_ha->core.shost->transportt); - unsigned long flags;
if (port) { if (!phy_is_wideport_member(port, phy)) @@ -103,7 +102,7 @@ static void sas_form_port(struct asd_sas_phy *phy) }
/* see if the phy should be part of a wide port */ - spin_lock_irqsave(&sas_ha->phy_port_lock, flags); + spin_lock(&sas_ha->phy_port_lock); for (i = 0; i < sas_ha->num_phys; i++) { port = sas_ha->sas_port[i]; spin_lock(&port->phy_list_lock); @@ -133,7 +132,7 @@ static void sas_form_port(struct asd_sas_phy *phy)
if (i >= sas_ha->num_phys) { pr_err("%s: couldn't find a free port, bug?\n", __func__); - spin_unlock_irqrestore(&sas_ha->phy_port_lock, flags); + spin_unlock(&sas_ha->phy_port_lock); return; }
@@ -156,7 +155,7 @@ static void sas_form_port(struct asd_sas_phy *phy) } else port->linkrate = max(port->linkrate, phy->linkrate); spin_unlock(&port->phy_list_lock); - spin_unlock_irqrestore(&sas_ha->phy_port_lock, flags); + spin_unlock(&sas_ha->phy_port_lock);
if (!port->port) { port->port = sas_port_alloc(phy->phy->dev.parent, port->id); @@ -203,7 +202,6 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone) struct sas_internal *si = to_sas_internal(sas_ha->core.shost->transportt); struct domain_device *dev; - unsigned long flags;
if (!port) return; /* done by a phy event */ @@ -225,7 +223,7 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone) if (si->dft->lldd_port_deformed) si->dft->lldd_port_deformed(phy);
- spin_lock_irqsave(&sas_ha->phy_port_lock, flags); + spin_lock(&sas_ha->phy_port_lock); spin_lock(&port->phy_list_lock);
list_del_init(&phy->port_phy_el); @@ -245,7 +243,7 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone) port->phy_mask = 0; } spin_unlock(&port->phy_list_lock); - spin_unlock_irqrestore(&sas_ha->phy_port_lock, flags); + spin_unlock(&sas_ha->phy_port_lock);
/* Only insert revalidate event if the port still has members */ if (port->port && dev && dev_is_expander(dev->dev_type)) { diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 99f4e70e9683..dd9311948dff 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -372,17 +372,16 @@ struct sas_phy *sas_get_local_phy(struct domain_device *dev) { struct sas_ha_struct *ha = dev->port->ha; struct sas_phy *phy; - unsigned long flags;
/* a published domain device always has a valid phy, it may be * stale, but it is never NULL */ BUG_ON(!dev->phy);
- spin_lock_irqsave(&ha->phy_port_lock, flags); + spin_lock(&ha->phy_port_lock); phy = dev->phy; get_device(&phy->dev); - spin_unlock_irqrestore(&ha->phy_port_lock, flags); + spin_unlock(&ha->phy_port_lock);
return phy; } @@ -802,9 +801,8 @@ struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy) struct sas_ha_struct *ha = SHOST_TO_SAS_HA(shost); struct domain_device *found_dev = NULL; int i; - unsigned long flags;
- spin_lock_irqsave(&ha->phy_port_lock, flags); + spin_lock(&ha->phy_port_lock); for (i = 0; i < ha->num_phys; i++) { struct asd_sas_port *port = ha->sas_port[i]; struct domain_device *dev; @@ -820,7 +818,7 @@ struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy) spin_unlock(&port->dev_list_lock); } found: - spin_unlock_irqrestore(&ha->phy_port_lock, flags); + spin_unlock(&ha->phy_port_lock);
return found_dev; }
From: Xiang Chen chenxiang66@hisilicon.com
The member cmd_pool in structure scsi_host_template is not used, so remove it.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com --- include/scsi/scsi_host.h | 3 --- 1 file changed, 3 deletions(-)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index ebe059badba0..21eae43701c0 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -16,7 +16,6 @@ struct completion; struct module; struct scsi_cmnd; struct scsi_device; -struct scsi_host_cmd_pool; struct scsi_target; struct Scsi_Host; struct scsi_transport_template; @@ -493,8 +492,6 @@ struct scsi_host_template { */ u64 vendor_id;
- struct scsi_host_cmd_pool *cmd_pool; - /* Delay for runtime autosuspend */ int rpm_autosuspend_delay; };
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
The member cmd_pool in structure scsi_host_template is not used, so remove it.
It's useful to mention when it was last used.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
include/scsi/scsi_host.h | 3 --- 1 file changed, 3 deletions(-)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index ebe059badba0..21eae43701c0 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -16,7 +16,6 @@ struct completion; struct module; struct scsi_cmnd; struct scsi_device; -struct scsi_host_cmd_pool; struct scsi_target; struct Scsi_Host; struct scsi_transport_template; @@ -493,8 +492,6 @@ struct scsi_host_template { */ u64 vendor_id;
- struct scsi_host_cmd_pool *cmd_pool;
- /* Delay for runtime autosuspend */ int rpm_autosuspend_delay; };
在 2021/12/24 16:40, John Garry 写道:
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
The member cmd_pool in structure scsi_host_template is not used, so remove it.
It's useful to mention when it was last used.
ok
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
include/scsi/scsi_host.h | 3 --- 1 file changed, 3 deletions(-)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index ebe059badba0..21eae43701c0 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -16,7 +16,6 @@ struct completion; struct module; struct scsi_cmnd; struct scsi_device; -struct scsi_host_cmd_pool; struct scsi_target; struct Scsi_Host; struct scsi_transport_template; @@ -493,8 +492,6 @@ struct scsi_host_template { */ u64 vendor_id;
- struct scsi_host_cmd_pool *cmd_pool;
/* Delay for runtime autosuspend */ int rpm_autosuspend_delay;.
From: Xiang Chen chenxiang66@hisilicon.com
For scsilun, it will be overwrote and not necessary to be set as 0.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com --- drivers/scsi/scsi_common.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c index 6e50e81a8216..5e7267b15f49 100644 --- a/drivers/scsi/scsi_common.c +++ b/drivers/scsi/scsi_common.c @@ -115,8 +115,6 @@ void int_to_scsilun(u64 lun, struct scsi_lun *scsilun) { int i;
- memset(scsilun->scsi_lun, 0, sizeof(scsilun->scsi_lun)); - for (i = 0; i < sizeof(lun); i += 2) { scsilun->scsi_lun[i] = (lun >> 8) & 0xFF; scsilun->scsi_lun[i+1] = lun & 0xFF;
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
For scsilun, it will be overwrote and not necessary to be set as 0.
overwritten
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
drivers/scsi/scsi_common.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c index 6e50e81a8216..5e7267b15f49 100644 --- a/drivers/scsi/scsi_common.c +++ b/drivers/scsi/scsi_common.c @@ -115,8 +115,6 @@ void int_to_scsilun(u64 lun, struct scsi_lun *scsilun) { int i;
- memset(scsilun->scsi_lun, 0, sizeof(scsilun->scsi_lun));
- for (i = 0; i < sizeof(lun); i += 2) {
What if scsi_lun is not divisible by 2? The last byte would not be zeroed then.
scsilun->scsi_lun[i] = (lun >> 8) & 0xFF; scsilun->scsi_lun[i+1] = lun & 0xFF;
在 2021/12/24 16:43, John Garry 写道:
On 24/12/2021 06:07, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
For scsilun, it will be overwrote and not necessary to be set as 0.
overwritten
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
drivers/scsi/scsi_common.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c index 6e50e81a8216..5e7267b15f49 100644 --- a/drivers/scsi/scsi_common.c +++ b/drivers/scsi/scsi_common.c @@ -115,8 +115,6 @@ void int_to_scsilun(u64 lun, struct scsi_lun *scsilun) { int i;
- memset(scsilun->scsi_lun, 0, sizeof(scsilun->scsi_lun));
for (i = 0; i < sizeof(lun); i += 2) {What if scsi_lun is not divisible by 2? The last byte would not be zeroed then.
From the comment of the function, the scsi lun is always 8-bytes vlaue, and i don't think it will become the value which is not divisible by 2. I think if the length of scsi lun is not divisible by 2, it should overwritten the value one byte by one byte.
scsilun->scsi_lun[i] = (lun >> 8) & 0xFF; scsilun->scsi_lun[i+1] = lun & 0xFF;.
Hi John,
Do you have any suggestion about patch 4 and patch 5?
在 2021/12/24 14:07, chenxiang 写道:
From: Xiang Chen chenxiang66@hisilicon.com
There are some small fixes related to scsi and libsas:
- Use void for sas_discover_event() return code;
- Remove duplicatted setting fro task->task_state_flags;
- Remove unused parameter for function sas_ata_eh();
- Use spin_lock/unlock instead of spin_lock/unlock_irq or spin_lock_irqsave
/spin_unlock_irqrestore for asd_sas_port->dev_list_lock and sas_ha->phy_port_lock;
- Remove unused member cmd_pool for structure scsi_host_template;
- Remove unnecessary memset for scsilun;
Xiang Chen (7): {topost} scsi: libsas: Use void for sas_discover_event() return code {topost} scsi: libsas: Remove duplicated setting for task->task_state_flags {topost} scsi: libsas: Remove unused parameter for function sas_ata_eh() {topost} scsi: libsas: Use spin_lock/unlock() for asd_sas_port->dev_list_lock {topost} scsi: libsas: Use spin_lock/unlock() for sas_ha->phy_port_lock {topost} scsi: Remove unused member cmd_pool for structure scsi_host_template {topost} scsi: Remove unnecssary memset for scsilun
drivers/scsi/libsas/sas_ata.c | 8 +++----- drivers/scsi/libsas/sas_discover.c | 26 ++++++++++++-------------- drivers/scsi/libsas/sas_expander.c | 12 ++++++------ drivers/scsi/libsas/sas_port.c | 12 +++++------- drivers/scsi/libsas/sas_scsi_host.c | 12 +++++------- drivers/scsi/scsi_common.c | 2 -- include/scsi/libsas.h | 2 +- include/scsi/sas_ata.h | 6 ++---- include/scsi/scsi_host.h | 3 --- 9 files changed, 34 insertions(+), 49 deletions(-)
-
chenxiang -
chenxiang (M)
-
John Garry