Hi,
Changelog v2
------------
- Rebase on top of v5.11-rc3
- Rebase on top of John's patch "scsi: libsas and users: Remove notifier
indirection", as it affects every other patch. Include it in this
series (patch #2).
- Introduce patches #13 => #19, which modify call sites back to use the
original libsas notifier function names without _gfp() suffix.
- Collect r-b tags
v1 Submission
-------------
https://lkml.kernel.org/r/[email protected]
Cover letter
------------
In the discussion about preempt count consistency across kernel
configurations:
https://lkml.kernel.org/r/[email protected]
it was concluded that the usage of in_interrupt() and related context
checks should be removed from non-core code.
This includes memory allocation mode decisions (GFP_*). In the long run,
usage of in_interrupt() and its siblings should be banned from driver
code completely.
This series addresses SCSI libsas. Basically, the function:
=> drivers/scsi/libsas/sas_init.c:
struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy)
{
...
gfp_t flags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
event = kmem_cache_zalloc(sas_event_cache, flags);
...
}
is transformed so that callers explicitly pass the gfp_t memory
allocation flags. Affected libsas clients are modified accordingly.
Patches #1, #2 => #7 have "Fixes: " tags and address bugs the were
noticed during the context analysis.
Thanks!
8<--------------
Ahmed S. Darwish (18):
Documentation: scsi: libsas: Remove notify_ha_event()
scsi: libsas: Introduce a _gfp() variant of event notifiers
scsi: mvsas: Pass gfp_t flags to libsas event notifiers
scsi: isci: port: link down: Pass gfp_t flags
scsi: isci: port: link up: Pass gfp_t flags
scsi: isci: port: broadcast change: Pass gfp_t flags
scsi: libsas: Pass gfp_t flags to event notifiers
scsi: pm80xx: Pass gfp_t flags to libsas event notifiers
scsi: aic94xx: Pass gfp_t flags to libsas event notifiers
scsi: hisi_sas: Pass gfp_t flags to libsas event notifiers
scsi: libsas: event notifiers API: Add gfp_t flags parameter
scsi: hisi_sas: Switch back to original libsas event notifiers
scsi: aic94xx: Switch back to original libsas event notifiers
scsi: pm80xx: Switch back to original libsas event notifiers
scsi: libsas: Switch back to original event notifiers API
scsi: isci: Switch back to original libsas event notifiers
scsi: mvsas: Switch back to original libsas event notifiers
scsi: libsas: Remove temporarily-added _gfp() API variants
John Garry (1):
scsi: libsas and users: Remove notifier indirection
Documentation/scsi/libsas.rst | 5 ++--
drivers/scsi/aic94xx/aic94xx_scb.c | 20 ++++++-------
drivers/scsi/hisi_sas/hisi_sas.h | 3 +-
drivers/scsi/hisi_sas/hisi_sas_main.c | 29 +++++++++----------
drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 6 ++--
drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 6 ++--
drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 ++--
drivers/scsi/isci/port.c | 11 +++----
drivers/scsi/libsas/sas_event.c | 27 ++++++++---------
drivers/scsi/libsas/sas_init.c | 17 ++++-------
drivers/scsi/libsas/sas_internal.h | 5 ++--
drivers/scsi/mvsas/mv_sas.c | 24 +++++++---------
drivers/scsi/pm8001/pm8001_hwi.c | 40 ++++++++++++--------------
drivers/scsi/pm8001/pm8001_sas.c | 12 +++-----
drivers/scsi/pm8001/pm80xx_hwi.c | 37 +++++++++++-------------
include/scsi/libsas.h | 9 +++---
16 files changed, 115 insertions(+), 142 deletions(-)
base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837
--
2.30.0
Use the new libsas event notifiers API, which requires callers to
explicitly pass the gfp_t memory allocation flags.
Context analysis:
- sas_enable_revalidation(): process, acquires mutex
- sas_resume_ha(): process, calls wait_event_timeout()
Signed-off-by: Ahmed S. Darwish <[email protected]>
Cc: John Garry <[email protected]>
Cc: Jason Yan <[email protected]>
---
drivers/scsi/libsas/sas_event.c | 3 ++-
drivers/scsi/libsas/sas_init.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
index 31fc32b9bb4e..922056644da5 100644
--- a/drivers/scsi/libsas/sas_event.c
+++ b/drivers/scsi/libsas/sas_event.c
@@ -109,7 +109,8 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
sas_phy = container_of(port->phy_list.next, struct asd_sas_phy,
port_phy_el);
- sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD);
+ sas_notify_port_event_gfp(sas_phy,
+ PORTE_BROADCAST_RCVD, GFP_KERNEL);
}
mutex_unlock(&ha->disco_mutex);
}
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 1c78347fbcc6..b8567902f0ce 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -404,7 +404,7 @@ void sas_resume_ha(struct sas_ha_struct *ha)
if (phy->suspended) {
dev_warn(&phy->phy->dev, "resume timeout\n");
- sas_notify_phy_event(phy, PHYE_RESUME_TIMEOUT);
+ sas_notify_phy_event_gfp(phy, PHYE_RESUME_TIMEOUT, GFP_KERNEL);
}
}
--
2.30.0
libsas event notifiers required an extension where gfp_t flags must be
explicitly passed. For bisectability, a temporary _gfp() variant of such
functions were added. All call sites then got converted use the _gfp()
variants and explicitly pass GFP context. Having no callers left, the
original libsas notifiers were then modified to accept gfp_t flags by
default.
Switch back to the original libas API, while still passing GFP context.
The libsas _gfp() variants will be removed afterwards.
Signed-off-by: Ahmed S. Darwish <[email protected]>
---
drivers/scsi/mvsas/mv_sas.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index e80f760f8abd..9336e2987879 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -229,7 +229,7 @@ static void mvs_bytes_dmaed(struct mvs_info *mvi, int i, gfp_t gfp_flags)
return;
}
- sas_notify_phy_event_gfp(sas_phy, PHYE_OOB_DONE, gfp_flags);
+ sas_notify_phy_event(sas_phy, PHYE_OOB_DONE, gfp_flags);
if (sas_phy->phy) {
struct sas_phy *sphy = sas_phy->phy;
@@ -261,7 +261,7 @@ static void mvs_bytes_dmaed(struct mvs_info *mvi, int i, gfp_t gfp_flags)
sas_phy->frame_rcvd_size = phy->frame_rcvd_size;
- sas_notify_port_event_gfp(sas_phy, PORTE_BYTES_DMAED, gfp_flags);
+ sas_notify_port_event(sas_phy, PORTE_BYTES_DMAED, gfp_flags);
}
void mvs_scan_start(struct Scsi_Host *shost)
@@ -1892,7 +1892,7 @@ static void mvs_work_queue(struct work_struct *work)
if (!(tmp & PHY_READY_MASK)) {
sas_phy_disconnected(sas_phy);
mvs_phy_disconnected(phy);
- sas_notify_phy_event_gfp(sas_phy,
+ sas_notify_phy_event(sas_phy,
PHYE_LOSS_OF_SIGNAL, GFP_ATOMIC);
mv_dprintk("phy%d Removed Device\n", phy_no);
} else {
@@ -1905,7 +1905,7 @@ static void mvs_work_queue(struct work_struct *work)
}
} else if (mwq->handler & EXP_BRCT_CHG) {
phy->phy_event &= ~EXP_BRCT_CHG;
- sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
+ sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
mv_dprintk("phy%d Got Broadcast Change\n", phy_no);
}
list_del(&mwq->entry);
--
2.30.0
Use the new libsas event notifiers API, which requires callers to
explicitly pass the gfp_t memory allocation flags.
Call chain analysis, pm8001_hwi.c:
pm8001_interrupt_handler_msix() || pm8001_interrupt_handler_intx() || pm8001_tasklet()
-> PM8001_CHIP_DISP->isr() = pm80xx_chip_isr()
-> process_oq [spin_lock_irqsave(&pm8001_ha->lock, ...)]
-> process_one_iomb()
-> mpi_hw_event()
-> hw_event_sas_phy_up()
-> pm8001_bytes_dmaed()
-> hw_event_sata_phy_up
-> pm8001_bytes_dmaed()
All functions are invoked by process_one_iomb(), which is invoked by the
interrupt service routine and the tasklet handler. A similar call chain
is also found at pm80xx_hwi.c. Pass GFP_ATOMIC.
For pm8001_sas.c, pm8001_phy_control() runs in task context as it calls
wait_for_completion() and msleep(). Pass GFP_KERNEL.
Signed-off-by: Ahmed S. Darwish <[email protected]>
Cc: Jack Wang <[email protected]>
---
drivers/scsi/pm8001/pm8001_hwi.c | 38 ++++++++++++++++----------------
drivers/scsi/pm8001/pm8001_sas.c | 9 ++++----
drivers/scsi/pm8001/pm80xx_hwi.c | 30 ++++++++++++-------------
3 files changed, 38 insertions(+), 39 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index dd15246d5b03..85b9a168794e 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -3179,7 +3179,7 @@ void pm8001_bytes_dmaed(struct pm8001_hba_info *pm8001_ha, int i)
pm8001_dbg(pm8001_ha, MSG, "phy %d byte dmaded.\n", i);
sas_phy->frame_rcvd_size = phy->frame_rcvd_size;
- sas_notify_port_event(sas_phy, PORTE_BYTES_DMAED);
+ sas_notify_port_event_gfp(sas_phy, PORTE_BYTES_DMAED, GFP_ATOMIC);
}
/* Get the link rate speed */
@@ -3336,7 +3336,7 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
else if (phy->identify.device_type != SAS_PHY_UNUSED)
phy->identify.target_port_protocols = SAS_PROTOCOL_SMP;
phy->sas_phy.oob_mode = SAS_OOB_MODE;
- sas_notify_phy_event(&phy->sas_phy, PHYE_OOB_DONE);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_OOB_DONE, GFP_ATOMIC);
spin_lock_irqsave(&phy->sas_phy.frame_rcvd_lock, flags);
memcpy(phy->frame_rcvd, &pPayload->sas_identify,
sizeof(struct sas_identify_frame)-4);
@@ -3379,7 +3379,7 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
phy->phy_type |= PORT_TYPE_SATA;
phy->phy_attached = 1;
phy->sas_phy.oob_mode = SATA_OOB_MODE;
- sas_notify_phy_event(&phy->sas_phy, PHYE_OOB_DONE);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_OOB_DONE, GFP_ATOMIC);
spin_lock_irqsave(&phy->sas_phy.frame_rcvd_lock, flags);
memcpy(phy->frame_rcvd, ((u8 *)&pPayload->sata_fis - 4),
sizeof(struct dev_to_host_fis));
@@ -3726,11 +3726,11 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
break;
case HW_EVENT_SATA_SPINUP_HOLD:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_SATA_SPINUP_HOLD\n");
- sas_notify_phy_event(&phy->sas_phy, PHYE_SPINUP_HOLD);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_SPINUP_HOLD, GFP_ATOMIC);
break;
case HW_EVENT_PHY_DOWN:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_PHY_DOWN\n");
- sas_notify_phy_event(&phy->sas_phy, PHYE_LOSS_OF_SIGNAL);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_LOSS_OF_SIGNAL, GFP_ATOMIC);
phy->phy_attached = 0;
phy->phy_state = 0;
hw_event_phy_down(pm8001_ha, piomb);
@@ -3739,7 +3739,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_PORT_INVALID\n");
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
/* the broadcast change primitive received, tell the LIBSAS this event
to revalidate the sas domain*/
@@ -3750,20 +3750,20 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
spin_lock_irqsave(&sas_phy->sas_prim_lock, flags);
sas_phy->sas_prim = HW_EVENT_BROADCAST_CHANGE;
spin_unlock_irqrestore(&sas_phy->sas_prim_lock, flags);
- sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD);
+ sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
break;
case HW_EVENT_PHY_ERROR:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_PHY_ERROR\n");
sas_phy_disconnected(&phy->sas_phy);
phy->phy_attached = 0;
- sas_notify_phy_event(&phy->sas_phy, PHYE_OOB_ERROR);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_OOB_ERROR, GFP_ATOMIC);
break;
case HW_EVENT_BROADCAST_EXP:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_BROADCAST_EXP\n");
spin_lock_irqsave(&sas_phy->sas_prim_lock, flags);
sas_phy->sas_prim = HW_EVENT_BROADCAST_EXP;
spin_unlock_irqrestore(&sas_phy->sas_prim_lock, flags);
- sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD);
+ sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
break;
case HW_EVENT_LINK_ERR_INVALID_DWORD:
pm8001_dbg(pm8001_ha, MSG,
@@ -3772,7 +3772,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
HW_EVENT_LINK_ERR_INVALID_DWORD, port_id, phy_id, 0, 0);
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_LINK_ERR_DISPARITY_ERROR:
pm8001_dbg(pm8001_ha, MSG,
@@ -3782,7 +3782,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
port_id, phy_id, 0, 0);
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_LINK_ERR_CODE_VIOLATION:
pm8001_dbg(pm8001_ha, MSG,
@@ -3792,7 +3792,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
port_id, phy_id, 0, 0);
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_LINK_ERR_LOSS_OF_DWORD_SYNCH:
pm8001_dbg(pm8001_ha, MSG,
@@ -3802,7 +3802,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
port_id, phy_id, 0, 0);
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_MALFUNCTION:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_MALFUNCTION\n");
@@ -3812,7 +3812,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
spin_lock_irqsave(&sas_phy->sas_prim_lock, flags);
sas_phy->sas_prim = HW_EVENT_BROADCAST_SES;
spin_unlock_irqrestore(&sas_phy->sas_prim_lock, flags);
- sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD);
+ sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
break;
case HW_EVENT_INBOUND_CRC_ERROR:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_INBOUND_CRC_ERROR\n");
@@ -3822,13 +3822,13 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
break;
case HW_EVENT_HARD_RESET_RECEIVED:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_HARD_RESET_RECEIVED\n");
- sas_notify_port_event(sas_phy, PORTE_HARD_RESET);
+ sas_notify_port_event_gfp(sas_phy, PORTE_HARD_RESET, GFP_ATOMIC);
break;
case HW_EVENT_ID_FRAME_TIMEOUT:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_ID_FRAME_TIMEOUT\n");
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_LINK_ERR_PHY_RESET_FAILED:
pm8001_dbg(pm8001_ha, MSG,
@@ -3838,20 +3838,20 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
port_id, phy_id, 0, 0);
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_PORT_RESET_TIMER_TMO:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_PORT_RESET_TIMER_TMO\n");
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_PORT_RECOVERY_TIMER_TMO:
pm8001_dbg(pm8001_ha, MSG,
"HW_EVENT_PORT_RECOVERY_TIMER_TMO\n");
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_PORT_RECOVER:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_PORT_RECOVER\n");
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index e21c6cfff4cb..a183293064b0 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -207,16 +207,16 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, enum phy_func func,
if (pm8001_ha->phy[phy_id].phy_state ==
PHY_STATE_LINK_UP_SPCV) {
sas_phy_disconnected(&phy->sas_phy);
- sas_notify_phy_event(&phy->sas_phy,
- PHYE_LOSS_OF_SIGNAL);
+ sas_notify_phy_event_gfp(&phy->sas_phy,
+ PHYE_LOSS_OF_SIGNAL, GFP_KERNEL);
phy->phy_attached = 0;
}
} else {
if (pm8001_ha->phy[phy_id].phy_state ==
PHY_STATE_LINK_UP_SPC) {
sas_phy_disconnected(&phy->sas_phy);
- sas_notify_phy_event(&phy->sas_phy,
- PHYE_LOSS_OF_SIGNAL);
+ sas_notify_phy_event_gfp(&phy->sas_phy,
+ PHYE_LOSS_OF_SIGNAL, GFP_KERNEL);
phy->phy_attached = 0;
}
}
@@ -1341,4 +1341,3 @@ int pm8001_clear_task_set(struct domain_device *dev, u8 *lun)
tmf_task.tmf = TMF_CLEAR_TASK_SET;
return pm8001_issue_ssp_tmf(dev, lun, &tmf_task);
}
-
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index f617177b7bb3..f849756e6ceb 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -3287,7 +3287,7 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
else if (phy->identify.device_type != SAS_PHY_UNUSED)
phy->identify.target_port_protocols = SAS_PROTOCOL_SMP;
phy->sas_phy.oob_mode = SAS_OOB_MODE;
- sas_notify_phy_event(&phy->sas_phy, PHYE_OOB_DONE);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_OOB_DONE, GFP_ATOMIC);
spin_lock_irqsave(&phy->sas_phy.frame_rcvd_lock, flags);
memcpy(phy->frame_rcvd, &pPayload->sas_identify,
sizeof(struct sas_identify_frame)-4);
@@ -3334,7 +3334,7 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
phy->phy_type |= PORT_TYPE_SATA;
phy->phy_attached = 1;
phy->sas_phy.oob_mode = SATA_OOB_MODE;
- sas_notify_phy_event(&phy->sas_phy, PHYE_OOB_DONE);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_OOB_DONE, GFP_ATOMIC);
spin_lock_irqsave(&phy->sas_phy.frame_rcvd_lock, flags);
memcpy(phy->frame_rcvd, ((u8 *)&pPayload->sata_fis - 4),
sizeof(struct dev_to_host_fis));
@@ -3417,7 +3417,7 @@ hw_event_phy_down(struct pm8001_hba_info *pm8001_ha, void *piomb)
}
if (port_sata && (portstate != PORT_IN_RESET))
- sas_notify_phy_event(&phy->sas_phy, PHYE_LOSS_OF_SIGNAL);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_LOSS_OF_SIGNAL, GFP_ATOMIC);
}
static int mpi_phy_start_resp(struct pm8001_hba_info *pm8001_ha, void *piomb)
@@ -3515,7 +3515,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
break;
case HW_EVENT_SATA_SPINUP_HOLD:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_SATA_SPINUP_HOLD\n");
- sas_notify_phy_event(&phy->sas_phy, PHYE_SPINUP_HOLD);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_SPINUP_HOLD, GFP_ATOMIC);
break;
case HW_EVENT_PHY_DOWN:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_PHY_DOWN\n");
@@ -3531,7 +3531,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_PORT_INVALID\n");
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
/* the broadcast change primitive received, tell the LIBSAS this event
to revalidate the sas domain*/
@@ -3542,20 +3542,20 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
spin_lock_irqsave(&sas_phy->sas_prim_lock, flags);
sas_phy->sas_prim = HW_EVENT_BROADCAST_CHANGE;
spin_unlock_irqrestore(&sas_phy->sas_prim_lock, flags);
- sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD);
+ sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
break;
case HW_EVENT_PHY_ERROR:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_PHY_ERROR\n");
sas_phy_disconnected(&phy->sas_phy);
phy->phy_attached = 0;
- sas_notify_phy_event(&phy->sas_phy, PHYE_OOB_ERROR);
+ sas_notify_phy_event_gfp(&phy->sas_phy, PHYE_OOB_ERROR, GFP_ATOMIC);
break;
case HW_EVENT_BROADCAST_EXP:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_BROADCAST_EXP\n");
spin_lock_irqsave(&sas_phy->sas_prim_lock, flags);
sas_phy->sas_prim = HW_EVENT_BROADCAST_EXP;
spin_unlock_irqrestore(&sas_phy->sas_prim_lock, flags);
- sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD);
+ sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
break;
case HW_EVENT_LINK_ERR_INVALID_DWORD:
pm8001_dbg(pm8001_ha, MSG,
@@ -3592,7 +3592,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
spin_lock_irqsave(&sas_phy->sas_prim_lock, flags);
sas_phy->sas_prim = HW_EVENT_BROADCAST_SES;
spin_unlock_irqrestore(&sas_phy->sas_prim_lock, flags);
- sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD);
+ sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
break;
case HW_EVENT_INBOUND_CRC_ERROR:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_INBOUND_CRC_ERROR\n");
@@ -3602,13 +3602,13 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
break;
case HW_EVENT_HARD_RESET_RECEIVED:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_HARD_RESET_RECEIVED\n");
- sas_notify_port_event(sas_phy, PORTE_HARD_RESET);
+ sas_notify_port_event_gfp(sas_phy, PORTE_HARD_RESET, GFP_ATOMIC);
break;
case HW_EVENT_ID_FRAME_TIMEOUT:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_ID_FRAME_TIMEOUT\n");
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_LINK_ERR_PHY_RESET_FAILED:
pm8001_dbg(pm8001_ha, MSG,
@@ -3618,7 +3618,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
port_id, phy_id, 0, 0);
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
break;
case HW_EVENT_PORT_RESET_TIMER_TMO:
pm8001_dbg(pm8001_ha, MSG, "HW_EVENT_PORT_RESET_TIMER_TMO\n");
@@ -3626,7 +3626,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
port_id, phy_id, 0, 0);
sas_phy_disconnected(sas_phy);
phy->phy_attached = 0;
- sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR);
+ sas_notify_port_event_gfp(sas_phy, PORTE_LINK_RESET_ERR, GFP_ATOMIC);
if (pm8001_ha->phy[phy_id].reset_completion) {
pm8001_ha->phy[phy_id].port_reset_status =
PORT_RESET_TMO;
@@ -3643,8 +3643,8 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
for (i = 0; i < pm8001_ha->chip->n_phy; i++) {
if (port->wide_port_phymap & (1 << i)) {
phy = &pm8001_ha->phy[i];
- sas_notify_phy_event(&phy->sas_phy,
- PHYE_LOSS_OF_SIGNAL);
+ sas_notify_phy_event_gfp(&phy->sas_phy,
+ PHYE_LOSS_OF_SIGNAL, GFP_ATOMIC);
port->wide_port_phymap &= ~(1 << i);
}
}
--
2.30.0
Use the new libsas event notifiers API, which requires callers to
explicitly pass the gfp_t memory allocation flags.
libsas sas_notify_port_event() is called from isci_port_link_up().
Below is the context analysis for all of its call chains:
host.c: isci_host_init() (@)
spin_lock_irq(isci_host::scic_lock)
-> sci_controller_initialize(), atomic (*)
-> port_config.c: sci_port_configuration_agent_initialize()
-> sci_mpc_agent_validate_phy_configuration()
-> port.c: sci_port_add_phy()
-> sci_port_general_link_up_handler()
-> sci_port_activate_phy()
-> isci_port_link_up()
port_config.c: apc_agent_timeout(), atomic, timer callback (*)
-> sci_apc_agent_configure_ports()
-> port.c: sci_port_add_phy()
-> sci_port_general_link_up_handler()
-> sci_port_activate_phy()
-> isci_port_link_up()
phy.c: enter SCI state: *SCI_PHY_SUB_FINAL* # Cont. from [1]
-> phy.c: sci_phy_starting_final_substate_enter()
-> phy.c: sci_change_state(SCI_PHY_READY)
-> enter SCI state: *SCI_PHY_READY*
-> phy.c: sci_phy_ready_state_enter()
-> host.c: sci_controller_link_up()
-> .link_up_handler()
== port_config.c: sci_apc_agent_link_up()
-> port.c: sci_port_link_up()
-> (continue at [A])
== port_config.c: sci_mpc_agent_link_up()
-> port.c: sci_port_link_up()
-> (continue at [A])
port_config.c: mpc_agent_timeout(), atomic, timer callback (*)
spin_lock_irqsave(isci_host::scic_lock, )
-> ->link_up_handler()
== port_config.c: sci_apc_agent_link_up()
-> port.c: sci_port_link_up()
-> (continue at [A])
== port_config.c: sci_mpc_agent_link_up()
-> port.c: sci_port_link_up()
-> (continue at [A])
[A] port.c: sci_port_link_up()
-> sci_port_activate_phy()
-> isci_port_link_up()
-> sci_port_general_link_up_handler()
-> sci_port_activate_phy()
-> isci_port_link_up()
[1] Call chains for entering SCI state: *SCI_PHY_SUB_FINAL*
-----------------------------------------------------------
host.c: power_control_timeout(), atomic, timer callback (*)
spin_lock_irqsave(isci_host::scic_lock, )
-> phy.c: sci_phy_consume_power_handler()
-> phy.c: sci_change_state(SCI_PHY_SUB_FINAL)
host.c: sci_controller_error_handler(): atomic, irq handler (*)
OR host.c: sci_controller_completion_handler(), atomic, tasklet (*)
-> sci_controller_process_completions()
-> sci_controller_unsolicited_frame()
-> phy.c: sci_phy_frame_handler()
-> sci_change_state(SCI_PHY_SUB_AWAIT_SAS_POWER)
-> sci_phy_starting_await_sas_power_substate_enter()
-> host.c: sci_controller_power_control_queue_insert()
-> phy.c: sci_phy_consume_power_handler()
-> sci_change_state(SCI_PHY_SUB_FINAL)
-> sci_change_state(SCI_PHY_SUB_FINAL)
-> sci_controller_event_completion()
-> phy.c: sci_phy_event_handler()
-> sci_phy_start_sata_link_training()
-> sci_change_state(SCI_PHY_SUB_AWAIT_SATA_POWER)
-> sci_phy_starting_await_sata_power_substate_enter
-> host.c: sci_controller_power_control_queue_insert()
-> phy.c: sci_phy_consume_power_handler()
-> sci_change_state(SCI_PHY_SUB_FINAL)
As can be seen from the "(*)" markers above, all the call-chains are
atomic. Pass GFP_ATOMIC to libsas port event notifier.
Note, the now-replaced libsas APIs used in_interrupt() to implicitly
decide which memory allocation type to use. This was only partially
correct, as it fails to choose the correct GFP flags when just
preemption or interrupts are disabled. Such buggy code paths are marked
with "(@)" in the call chains above.
Fixes: 1c393b970e0f ("scsi: libsas: Use dynamic alloced work to avoid sas event lost")
Signed-off-by: Ahmed S. Darwish <[email protected]>
Cc: Artur Paszkiewicz <[email protected]>
---
drivers/scsi/isci/port.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/isci/port.c b/drivers/scsi/isci/port.c
index a3c58718c260..10136ae466e2 100644
--- a/drivers/scsi/isci/port.c
+++ b/drivers/scsi/isci/port.c
@@ -223,7 +223,8 @@ static void isci_port_link_up(struct isci_host *isci_host,
/* Notify libsas that we have an address frame, if indeed
* we've found an SSP, SMP, or STP target */
if (success)
- sas_notify_port_event(&iphy->sas_phy, PORTE_BYTES_DMAED);
+ sas_notify_port_event_gfp(&iphy->sas_phy,
+ PORTE_BYTES_DMAED, GFP_ATOMIC);
}
--
2.30.0
Use the new libsas event notifiers API, which requires callers to
explicitly pass the gfp_t memory allocation flags.
sas_notify_phy_event() is exclusively called by isci_port_link_down().
Below is the context analysis for all of its call chains:
port.c: port_timeout(), atomic, timer callback (*)
spin_lock_irqsave(isci_host::scic_lock, )
-> port_state_machine_change(..., SCI_PORT_FAILED)
-> enter SCI port state: *SCI_PORT_FAILED*
-> sci_port_failed_state_enter()
-> isci_port_hard_reset_complete()
-> isci_port_link_down()
port.c: isci_port_perform_hard_reset()
spin_lock_irqsave(isci_host::scic_lock, )
-> port.c: sci_port_hard_reset(), atomic (*)
-> phy.c: sci_phy_reset()
-> sci_change_state(SCI_PHY_RESETTING)
-> enter SCI PHY state: *SCI_PHY_RESETTING*
-> sci_phy_resetting_state_enter()
-> port.c: sci_port_deactivate_phy()
-> isci_port_link_down()
port.c: enter SCI port state: *SCI_PORT_READY* # Cont. from [1]
-> sci_port_ready_state_enter()
-> isci_port_hard_reset_complete()
-> isci_port_link_down()
phy.c: enter SCI state: *SCI_PHY_STOPPED* # Cont. from [2]
-> sci_phy_stopped_state_enter()
-> host.c: sci_controller_link_down()
-> ->link_down_handler()
== port_config.c: sci_apc_agent_link_down()
-> port.c: sci_port_remove_phy()
-> sci_port_deactivate_phy()
-> isci_port_link_down()
== port_config.c: sci_mpc_agent_link_down()
-> port.c: sci_port_link_down()
-> sci_port_deactivate_phy()
-> isci_port_link_down()
phy.c: enter SCI state: *SCI_PHY_STARTING* # Cont. from [3]
-> sci_phy_starting_state_enter()
-> host.c: sci_controller_link_down()
-> ->link_down_handler()
== port_config.c: sci_apc_agent_link_down()
-> port.c: sci_port_remove_phy()
-> isci_port_link_down()
== port_config.c: sci_mpc_agent_link_down()
-> port.c: sci_port_link_down()
-> sci_port_deactivate_phy()
-> isci_port_link_down()
[1] Call chains for 'enter SCI port state: *SCI_PORT_READY*'
------------------------------------------------------------
host.c: isci_host_init() (@)
spin_lock_irq(isci_host::scic_lock)
-> sci_controller_initialize(), atomic (*)
-> port_config.c: sci_port_configuration_agent_initialize()
-> sci_mpc_agent_validate_phy_configuration()
-> port.c: sci_port_add_phy()
-> sci_port_general_link_up_handler()
-> port_state_machine_change(, SCI_PORT_READY)
-> enter port state *SCI_PORT_READY*
host.c: isci_host_start() (@)
spin_lock_irq(isci_host::scic_lock)
-> host.c: sci_controller_start(), atomic (*)
-> host.c: sci_port_start()
-> port.c: port_state_machine_change(, SCI_PORT_READY)
-> enter port state *SCI_PORT_READY*
port_config.c: apc_agent_timeout(), atomic, timer callback (*)
-> sci_apc_agent_configure_ports()
-> port.c: sci_port_add_phy()
-> sci_port_general_link_up_handler()
-> port_state_machine_change(, SCI_PORT_READY)
-> enter port state *SCI_PORT_READY*
port_config.c: mpc_agent_timeout(), atomic, timer callback (*)
spin_lock_irqsave(isci_host::scic_lock, )
-> ->link_up_handler()
== port.c: sci_apc_agent_link_up()
-> sci_port_general_link_up_handler()
-> port_state_machine_change(, SCI_PORT_READY)
-> enter port state *SCI_PORT_READY*
== port.c: sci_mpc_agent_link_up()
-> port.c: sci_port_link_up()
-> sci_port_general_link_up_handler()
-> port_state_machine_change(, SCI_PORT_READY)
-> enter port state *SCI_PORT_READY*
phy.c: enter SCI state: SCI_PHY_SUB_FINAL # Cont. from [1A]
-> sci_phy_starting_final_substate_enter()
-> sci_change_state(SCI_PHY_READY)
-> enter SCI state: *SCI_PHY_READY*
-> sci_phy_ready_state_enter()
-> host.c: sci_controller_link_up()
-> port_agent.link_up_handler()
== port_config.c: sci_apc_agent_link_up()
-> port.c: sci_port_link_up()
-> sci_port_general_link_up_handler()
-> port_state_machine_change(, SCI_PORT_READY)
-> enter port state *SCI_PORT_READY*
== port_config.c: sci_mpc_agent_link_up()
-> port.c: sci_port_link_up()
-> sci_port_general_link_up_handler()
-> port_state_machine_change(, SCI_PORT_READY)
-> enter port state *SCI_PORT_READY*
[1A] Call chains for entering SCI state: *SCI_PHY_SUB_FINAL*
------------------------------------------------------------
host.c: power_control_timeout(), atomic, timer callback (*)
spin_lock_irqsave(isci_host::scic_lock, )
-> phy.c: sci_phy_consume_power_handler()
-> phy.c: sci_change_state(SCI_PHY_SUB_FINAL)
host.c: sci_controller_error_handler(): atomic, irq handler (*)
OR host.c: sci_controller_completion_handler(), atomic, tasklet (*)
-> sci_controller_process_completions()
-> sci_controller_unsolicited_frame()
-> phy.c: sci_phy_frame_handler()
-> sci_change_state(SCI_PHY_SUB_AWAIT_SAS_POWER)
-> sci_phy_starting_await_sas_power_substate_enter()
-> host.c: sci_controller_power_control_queue_insert()
-> phy.c: sci_phy_consume_power_handler()
-> sci_change_state(SCI_PHY_SUB_FINAL)
-> sci_change_state(SCI_PHY_SUB_FINAL)
-> sci_controller_event_completion()
-> phy.c: sci_phy_event_handler()
-> sci_phy_start_sata_link_training()
-> sci_change_state(SCI_PHY_SUB_AWAIT_SATA_POWER)
-> sci_phy_starting_await_sata_power_substate_enter
-> host.c: sci_controller_power_control_queue_insert()
-> phy.c: sci_phy_consume_power_handler()
-> sci_change_state(SCI_PHY_SUB_FINAL)
[2] Call chains for entering state: *SCI_PHY_STOPPED*
-----------------------------------------------------
host.c: isci_host_init() (@)
spin_lock_irq(isci_host::scic_lock)
-> sci_controller_initialize(), atomic (*)
-> phy.c: sci_phy_initialize()
-> phy.c: sci_phy_link_layer_initialization()
-> phy.c: sci_change_state(SCI_PHY_STOPPED)
init.c: PCI ->remove() || PM_OPS ->suspend, process context (+)
-> host.c: isci_host_deinit()
-> sci_controller_stop_phys()
-> phy.c: sci_phy_stop()
-> sci_change_state(SCI_PHY_STOPPED)
phy.c: isci_phy_control()
spin_lock_irqsave(isci_host::scic_lock, )
-> sci_phy_stop(), atomic (*)
-> sci_change_state(SCI_PHY_STOPPED)
[3] Call chains for entering state: *SCI_PHY_STARTING*
------------------------------------------------------
phy.c: phy_sata_timeout(), atimer, timer callback (*)
spin_lock_irqsave(isci_host::scic_lock, )
-> sci_change_state(SCI_PHY_STARTING)
host.c: phy_startup_timeout(), atomic, timer callback (*)
spin_lock_irqsave(isci_host::scic_lock, )
-> sci_controller_start_next_phy()
-> sci_phy_start()
-> sci_change_state(SCI_PHY_STARTING)
host.c: isci_host_start() (@)
spin_lock_irq(isci_host::scic_lock)
-> sci_controller_start(), atomic (*)
-> sci_controller_start_next_phy()
-> sci_phy_start()
-> sci_change_state(SCI_PHY_STARTING)
phy.c: Enter SCI state *SCI_PHY_SUB_FINAL*, atomic, check above (*)
-> sci_change_state(SCI_PHY_SUB_FINAL)
-> sci_phy_starting_final_substate_enter()
-> sci_change_state(SCI_PHY_READY)
-> Enter SCI state: *SCI_PHY_READY*
-> sci_phy_ready_state_enter()
-> host.c: sci_controller_link_up()
-> sci_controller_start_next_phy()
-> sci_phy_start()
-> sci_change_state(SCI_PHY_STARTING)
phy.c: sci_phy_event_handler(), atomic, discussed earlier (*)
-> sci_change_state(SCI_PHY_STARTING), 11 instances
phy.c: enter SCI state: *SCI_PHY_RESETTING*, atomic, discussed (*)
-> sci_phy_resetting_state_enter()
-> sci_change_state(SCI_PHY_STARTING)
As can be seen from the "(*)" markers above, almost all the call-chains
are atomic. The only exception, marked with "(+)", is a PCI ->remove()
and PM_OPS ->suspend() cold path. Thus, pass GFP_ATOMIC to the libsas
phy event notifier.
Note, The now-replaced libsas APIs used in_interrupt() to implicitly
decide which memory allocation type to use. This was only partially
correct, as it fails to choose the correct GFP flags when just
preemption or interrupts are disabled. Such buggy code paths are marked
with "(@)" in the call chains above.
Fixes: 1c393b970e0f ("scsi: libsas: Use dynamic alloced work to avoid sas event lost")
Signed-off-by: Ahmed S. Darwish <[email protected]>
Cc: Artur Paszkiewicz <[email protected]>
---
drivers/scsi/isci/port.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/isci/port.c b/drivers/scsi/isci/port.c
index 8d9349738067..a3c58718c260 100644
--- a/drivers/scsi/isci/port.c
+++ b/drivers/scsi/isci/port.c
@@ -269,8 +269,8 @@ static void isci_port_link_down(struct isci_host *isci_host,
* isci_port_deformed and isci_dev_gone functions.
*/
sas_phy_disconnected(&isci_phy->sas_phy);
- sas_notify_phy_event(&isci_phy->sas_phy,
- PHYE_LOSS_OF_SIGNAL);
+ sas_notify_phy_event_gfp(&isci_phy->sas_phy,
+ PHYE_LOSS_OF_SIGNAL, GFP_ATOMIC);
dev_dbg(&isci_host->pdev->dev,
"%s: isci_port = %p - Done\n", __func__, isci_port);
--
2.30.0
libsas event notifiers required an extension where gfp_t flags must be
explicitly passed. For bisectability, a temporary _gfp() variant of such
functions were added. All call sites then got converted use the _gfp()
variants and explicitly pass GFP context. Having no callers left, the
original libsas notifiers were then modified to accept gfp_t flags by
default.
Switch back to the original libas API, while still passing GFP context.
The libsas _gfp() variants will be removed afterwards.
Signed-off-by: Ahmed S. Darwish <[email protected]>
Cc: John Garry <[email protected]>
---
drivers/scsi/hisi_sas/hisi_sas_main.c | 8 ++++----
drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 2 +-
drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 2 +-
drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 2 +-
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index f781b52c6441..625327e99b06 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -627,7 +627,7 @@ static void hisi_sas_bytes_dmaed(struct hisi_hba *hisi_hba, int phy_no,
return;
}
- sas_notify_phy_event_gfp(sas_phy, PHYE_OOB_DONE, gfp_flags);
+ sas_notify_phy_event(sas_phy, PHYE_OOB_DONE, gfp_flags);
if (sas_phy->phy) {
struct sas_phy *sphy = sas_phy->phy;
@@ -655,7 +655,7 @@ static void hisi_sas_bytes_dmaed(struct hisi_hba *hisi_hba, int phy_no,
}
sas_phy->frame_rcvd_size = phy->frame_rcvd_size;
- sas_notify_port_event_gfp(sas_phy, PORTE_BYTES_DMAED, gfp_flags);
+ sas_notify_port_event(sas_phy, PORTE_BYTES_DMAED, gfp_flags);
}
static struct hisi_sas_device *hisi_sas_alloc_dev(struct domain_device *device)
@@ -1430,7 +1430,7 @@ static void hisi_sas_rescan_topology(struct hisi_hba *hisi_hba, u32 state)
_sas_port = sas_port;
if (dev_is_expander(dev->dev_type))
- sas_notify_port_event_gfp(sas_phy,
+ sas_notify_port_event(sas_phy,
PORTE_BROADCAST_RCVD,
GFP_KERNEL);
}
@@ -2209,7 +2209,7 @@ void hisi_sas_phy_down(struct hisi_hba *hisi_hba, int phy_no, int rdy,
return;
}
/* Phy down and not ready */
- sas_notify_phy_event_gfp(sas_phy, PHYE_LOSS_OF_SIGNAL, gfp_flags);
+ sas_notify_phy_event(sas_phy, PHYE_LOSS_OF_SIGNAL, gfp_flags);
sas_phy_disconnected(sas_phy);
if (port) {
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index b0a72ffce4f0..c33f2881d3c4 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1423,7 +1423,7 @@ static irqreturn_t int_bcast_v1_hw(int irq, void *p)
}
if (!test_bit(HISI_SAS_RESET_BIT, &hisi_hba->flags))
- sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
+ sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
end:
hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index d8f8fb2ed63b..a9de1939c426 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -2825,7 +2825,7 @@ static void phy_bcast_v2_hw(int phy_no, struct hisi_hba *hisi_hba)
bcast_status = hisi_sas_phy_read32(hisi_hba, phy_no, RX_PRIMS_STATUS);
if ((bcast_status & RX_BCAST_CHG_MSK) &&
!test_bit(HISI_SAS_RESET_BIT, &hisi_hba->flags))
- sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
+ sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT0,
CHL_INT0_SL_RX_BCST_ACK_MSK);
hisi_sas_phy_write32(hisi_hba, phy_no, SL_RX_BCAST_CHK_MSK, 0);
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 87392de60e9d..9ffc429c8d42 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -1607,7 +1607,7 @@ static irqreturn_t phy_bcast_v3_hw(int phy_no, struct hisi_hba *hisi_hba)
bcast_status = hisi_sas_phy_read32(hisi_hba, phy_no, RX_PRIMS_STATUS);
if ((bcast_status & RX_BCAST_CHG_MSK) &&
!test_bit(HISI_SAS_RESET_BIT, &hisi_hba->flags))
- sas_notify_port_event_gfp(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
+ sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD, GFP_ATOMIC);
hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT0,
CHL_INT0_SL_RX_BCST_ACK_MSK);
hisi_sas_phy_write32(hisi_hba, phy_no, SL_RX_BCAST_CHK_MSK, 0);
--
2.30.0
On 12/01/2021 11:06, Ahmed S. Darwish wrote:
> Hi,
>
> Changelog v2
> ------------
>
> - Rebase on top of v5.11-rc3
>
> - Rebase on top of John's patch "scsi: libsas and users: Remove notifier
> indirection", as it affects every other patch. Include it in this
> series (patch #2).
>
> - Introduce patches #13 => #19, which modify call sites back to use the
> original libsas notifier function names without _gfp() suffix.
>
> - Collect r-b tags
>
> v1 Submission
> -------------
>
> https://lkml.kernel.org/r/[email protected]
>
> Cover letter
> ------------
>
> In the discussion about preempt count consistency across kernel
> configurations:
>
> https://lkml.kernel.org/r/[email protected]
>
> it was concluded that the usage of in_interrupt() and related context
> checks should be removed from non-core code.
>
> This includes memory allocation mode decisions (GFP_*). In the long run,
> usage of in_interrupt() and its siblings should be banned from driver
> code completely.
>
> This series addresses SCSI libsas. Basically, the function:
>
> => drivers/scsi/libsas/sas_init.c:
> struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy)
> {
> ...
> gfp_t flags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
> event = kmem_cache_zalloc(sas_event_cache, flags);
> ...
> }
>
> is transformed so that callers explicitly pass the gfp_t memory
> allocation flags. Affected libsas clients are modified accordingly.
>
> Patches #1, #2 => #7 have "Fixes: " tags and address bugs the were
> noticed during the context analysis.
>
> Thanks!
>
> 8<--------------
>
> Ahmed S. Darwish (18):
> Documentation: scsi: libsas: Remove notify_ha_event()
> scsi: libsas: Introduce a _gfp() variant of event notifiers
> scsi: mvsas: Pass gfp_t flags to libsas event notifiers
> scsi: isci: port: link down: Pass gfp_t flags
> scsi: isci: port: link up: Pass gfp_t flags
> scsi: isci: port: broadcast change: Pass gfp_t flags
> scsi: libsas: Pass gfp_t flags to event notifiers
> scsi: pm80xx: Pass gfp_t flags to libsas event notifiers
> scsi: aic94xx: Pass gfp_t flags to libsas event notifiers
> scsi: hisi_sas: Pass gfp_t flags to libsas event notifiers
> scsi: libsas: event notifiers API: Add gfp_t flags parameter
> scsi: hisi_sas: Switch back to original libsas event notifiers
> scsi: aic94xx: Switch back to original libsas event notifiers
> scsi: pm80xx: Switch back to original libsas event notifiers
> scsi: libsas: Switch back to original event notifiers API
> scsi: isci: Switch back to original libsas event notifiers
> scsi: mvsas: Switch back to original libsas event notifiers
> scsi: libsas: Remove temporarily-added _gfp() API variants
I'll give this a spin today and help review also then.
There's 18 patches here - it would be very convenient if they were on a
public branch :)
Cheers,
John
>
> John Garry (1):
> scsi: libsas and users: Remove notifier indirection
>
> Documentation/scsi/libsas.rst | 5 ++--
> drivers/scsi/aic94xx/aic94xx_scb.c | 20 ++++++-------
> drivers/scsi/hisi_sas/hisi_sas.h | 3 +-
> drivers/scsi/hisi_sas/hisi_sas_main.c | 29 +++++++++----------
> drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 6 ++--
> drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 6 ++--
> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 ++--
> drivers/scsi/isci/port.c | 11 +++----
> drivers/scsi/libsas/sas_event.c | 27 ++++++++---------
> drivers/scsi/libsas/sas_init.c | 17 ++++-------
> drivers/scsi/libsas/sas_internal.h | 5 ++--
> drivers/scsi/mvsas/mv_sas.c | 24 +++++++---------
> drivers/scsi/pm8001/pm8001_hwi.c | 40 ++++++++++++--------------
> drivers/scsi/pm8001/pm8001_sas.c | 12 +++-----
> drivers/scsi/pm8001/pm80xx_hwi.c | 37 +++++++++++-------------
> include/scsi/libsas.h | 9 +++---
> 16 files changed, 115 insertions(+), 142 deletions(-)
>
> base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837
> --
> 2.30.0
> .
>
On Tue, Jan 12, 2021 at 11:53:50AM +0000, John Garry wrote:
> On 12/01/2021 11:06, Ahmed S. Darwish wrote:
> > Hi,
> >
> > Changelog v2
> > ------------
...
>
> I'll give this a spin today and help review also then.
>
> There's 18 patches here - it would be very convenient if they were on a
> public branch :)
>
Konstantin's "b4" is your friend:
https://people.kernel.org/monsieuricon/introducing-b4-and-patch-attestation
It boils down to:
$ pip install b4
$ b4 am -v2 [email protected]
Kind regards,
--
Ahmed S. Darwish
Linutronix GmbH
- [email protected]
On 12/01/2021 13:19, Ahmed S. Darwish wrote:
> On Tue, Jan 12, 2021 at 11:53:50AM +0000, John Garry wrote:
>> On 12/01/2021 11:06, Ahmed S. Darwish wrote:
>>> Hi,
>>>
>>> Changelog v2
>>> ------------
> ...
>>
>> I'll give this a spin today and help review also then.
>>
I boot-tested on my machines which have hisi_sas v2 and v3 hw, and it's
ok. I will ask some guys to test a bit more.
And generally the changes look ok. But I just have a slight concern that
we don't pass the gfp_flags all the way from the origin caller.
So we have some really long callchains, for example:
host.c: sci_controller_error_handler(): atomic, irq handler (*)
OR host.c: sci_controller_completion_handler(), atomic, tasklet (*)
-> sci_controller_process_completions()
-> sci_controller_unsolicited_frame()
-> phy.c: sci_phy_frame_handler()
-> sci_change_state(SCI_PHY_SUB_AWAIT_SAS_POWER)
-> sci_phy_starting_await_sas_power_substate_enter()
-> host.c: sci_controller_power_control_queue_insert()
-> phy.c: sci_phy_consume_power_handler()
-> sci_change_state(SCI_PHY_SUB_FINAL)
-> sci_change_state(SCI_PHY_SUB_FINAL)
-> sci_controller_event_completion()
-> phy.c: sci_phy_event_handler()
-> sci_phy_start_sata_link_training()
-> sci_change_state(SCI_PHY_SUB_AWAIT_SATA_POWER)
-> sci_phy_starting_await_sata_power_substate_enter
-> host.c: sci_controller_power_control_queue_insert()
-> phy.c: sci_phy_consume_power_handler()
-> sci_change_state(SCI_PHY_SUB_FINAL)
So if someone rearranges the code later, adds new callchains, etc., it
could be missed that the context may have changed than what we assume at
the bottom. But then passing the flags everywhere is cumbersome, and all
the libsas users see little or no significant changes anyway, apart from
a couple.
Thanks,
John
On Tue, Jan 12, 2021 at 04:00:57PM +0000, John Garry wrote:
...
>
> I boot-tested on my machines which have hisi_sas v2 and v3 hw, and it's ok.
> I will ask some guys to test a bit more.
>
Thanks a lot!
> And generally the changes look ok. But I just have a slight concern that we
> don't pass the gfp_flags all the way from the origin caller.
>
> So we have some really long callchains, for example:
>
> host.c: sci_controller_error_handler(): atomic, irq handler (*)
> OR host.c: sci_controller_completion_handler(), atomic, tasklet (*)
> -> sci_controller_process_completions()
> -> sci_controller_unsolicited_frame()
> -> phy.c: sci_phy_frame_handler()
> -> sci_change_state(SCI_PHY_SUB_AWAIT_SAS_POWER)
> -> sci_phy_starting_await_sas_power_substate_enter()
> -> host.c: sci_controller_power_control_queue_insert()
> -> phy.c: sci_phy_consume_power_handler()
> -> sci_change_state(SCI_PHY_SUB_FINAL)
> -> sci_change_state(SCI_PHY_SUB_FINAL)
> -> sci_controller_event_completion()
> -> phy.c: sci_phy_event_handler()
> -> sci_phy_start_sata_link_training()
> -> sci_change_state(SCI_PHY_SUB_AWAIT_SATA_POWER)
> -> sci_phy_starting_await_sata_power_substate_enter
> -> host.c: sci_controller_power_control_queue_insert()
> -> phy.c: sci_phy_consume_power_handler()
> -> sci_change_state(SCI_PHY_SUB_FINAL)
>
> So if someone rearranges the code later, adds new callchains, etc., it could
> be missed that the context may have changed than what we assume at the
> bottom. But then passing the flags everywhere is cumbersome, and all the
> libsas users see little or no significant changes anyway, apart from a
> couple.
>
The deep call chains like the one you've quoted are all within the isci
Intel driver (patches #5 => #7), due to the *massive* state transitions
that driver has. But as the commit logs of these three patches show,
almost all of such transitions happened under atomic context anyway and
GFP_ATOMIC was thus used.
The GFP_KERNEL call-chains were all very simple: a workqueue, functions
already calling msleep() or wait_event_timeout() two or three lines
nearby, and so on.
All the other libsas clients (that is, except isci) also had normal call
chains that were IMHO easy to follow.
Thanks,
--
Ahmed S. Darwish
Linutronix GmbH
On 12/01/2021 17:33, Ahmed S. Darwish wrote:
> On Tue, Jan 12, 2021 at 04:00:57PM +0000, John Garry wrote:
> ...
>> I boot-tested on my machines which have hisi_sas v2 and v3 hw, and it's ok.
>> I will ask some guys to test a bit more.
>>
> Thanks a lot!
>
>> And generally the changes look ok. But I just have a slight concern that we
>> don't pass the gfp_flags all the way from the origin caller.
>>
>> So we have some really long callchains, for example:
>>
>> host.c: sci_controller_error_handler(): atomic, irq handler (*)
>> OR host.c: sci_controller_completion_handler(), atomic, tasklet (*)
>> -> sci_controller_process_completions()
>> -> sci_controller_unsolicited_frame()
>> -> phy.c: sci_phy_frame_handler()
>> -> sci_change_state(SCI_PHY_SUB_AWAIT_SAS_POWER)
>> -> sci_phy_starting_await_sas_power_substate_enter()
>> -> host.c: sci_controller_power_control_queue_insert()
>> -> phy.c: sci_phy_consume_power_handler()
>> -> sci_change_state(SCI_PHY_SUB_FINAL)
>> -> sci_change_state(SCI_PHY_SUB_FINAL)
>> -> sci_controller_event_completion()
>> -> phy.c: sci_phy_event_handler()
>> -> sci_phy_start_sata_link_training()
>> -> sci_change_state(SCI_PHY_SUB_AWAIT_SATA_POWER)
>> -> sci_phy_starting_await_sata_power_substate_enter
>> -> host.c: sci_controller_power_control_queue_insert()
>> -> phy.c: sci_phy_consume_power_handler()
>> -> sci_change_state(SCI_PHY_SUB_FINAL)
>>
>> So if someone rearranges the code later, adds new callchains, etc., it could
>> be missed that the context may have changed than what we assume at the
>> bottom. But then passing the flags everywhere is cumbersome, and all the
>> libsas users see little or no significant changes anyway, apart from a
>> couple.
>>
> The deep call chains like the one you've quoted are all within the isci
> Intel driver (patches #5 => #7), due to the*massive* state transitions
> that driver has. But as the commit logs of these three patches show,
> almost all of such transitions happened under atomic context anyway and
> GFP_ATOMIC was thus used.
>
> The GFP_KERNEL call-chains were all very simple: a workqueue, functions
> already calling msleep() or wait_event_timeout() two or three lines
> nearby, and so on.
>
> All the other libsas clients (that is, except isci) also had normal call
> chains that were IMHO easy to follow.
To me, the series looks fine. Well, the end result - I didn't go through
patch by patch. So:
Reviewed-by: John Garry <[email protected]>
I'm still hoping some guys are testing a bit for me, but I'll let you
know if any problem.
As an aside, your analysis showed some quite poor usage of spinlocks in
some drivers, specifically grabbing a lock and then calling into a depth
of 3 or 4 functions.
Thanks,
John
On Thu, Jan 14, 2021 at 09:51:35AM +0000, John Garry wrote:
...
>
> To me, the series looks fine. Well, the end result - I didn't go through
> patch by patch. So:
>
> Reviewed-by: John Garry <[email protected]>
>
Thanks!
Shall I add you r-b tag to the whole series then, or only to the ones
which directly touch libsas (#3, #12, #16, and #19)?
>
> As an aside, your analysis showed some quite poor usage of spinlocks in some
> drivers, specifically grabbing a lock and then calling into a depth of 3 or
> 4 functions.
>
Correct.
Kind regards,
--
Ahmed S. Darwish
On 15/01/2021 16:27, Ahmed S. Darwish wrote:
> Thanks!
>
> Shall I add you r-b tag to the whole series then, or only to the ones
> which directly touch libsas (#3, #12, #16, and #19)?
The whole series, if you like. But there was a nit about fitting some
code on a single line still, and I think Christoph also had some issue
on that related topic.
>
>> As an aside, your analysis showed some quite poor usage of spinlocks in some
>> drivers, specifically grabbing a lock and then calling into a depth of 3 or
>> 4 functions.
>>
> Correct.
BTW, testing report looked all good.
Thanks,
john
On Fri, Jan 15, 2021 at 04:29:51PM +0000, John Garry wrote:
> On 15/01/2021 16:27, Ahmed S. Darwish wrote:
> > Thanks!
> >
> > Shall I add you r-b tag to the whole series then, or only to the ones
> > which directly touch libsas (#3, #12, #16, and #19)?
>
> The whole series, if you like. But there was a nit about fitting some code
> on a single line still, and I think Christoph also had some issue on that
> related topic.
>
Nice. Then I'll send a v3 to fixing these 80 col issues -- including in
the intermediate patches.
> >
> > > As an aside, your analysis showed some quite poor usage of spinlocks in some
> > > drivers, specifically grabbing a lock and then calling into a depth of 3 or
> > > 4 functions.
> > >
> > Correct.
>
> BTW, testing report looked all good.
>
Oh, that's good to hear :)
Have a nice weekend,
--
Ahmed S. Darwish
Linutronix GmbH