This small series includes the following:
- Rework how some shost values are set
- Fix a longstanding bug that the driver attempts to use tags before they
are configured
- Stop using {set, clear}_bit()
- Expose HW queues
Any testing would be appreciated as this driver is still broken for my
arm64 system, i.e. just broken.
John Garry (4):
scsi: pm8001: Rework shost initial values
scsi: pm8001: Setup tags before using them
scsi: pm8001: Use non-atomic bitmap ops for tag alloc + free
scsi: pm8001: Expose HW queues for pm80xx hw
drivers/scsi/pm8001/pm8001_hwi.c | 5 +++
drivers/scsi/pm8001/pm8001_init.c | 73 +++++++++++++++++++++----------
drivers/scsi/pm8001/pm8001_sas.c | 10 +++--
drivers/scsi/pm8001/pm8001_sas.h | 3 ++
drivers/scsi/pm8001/pm80xx_hwi.c | 46 +++++++++++++++----
5 files changed, 101 insertions(+), 36 deletions(-)
--
2.26.2
The current code is buggy in that the tags are set up after they are
needed in pm80xx_chip_init() -> pm80xx_set_sas_protocol_timer_config().
The tag depth is earlier read in pm80xx_chip_init() ->
read_main_config_table().
Add a post init callback to do the pm80xx work which needs to be done
after reading the tags. I don't see a better way to do this.
Signed-off-by: John Garry <[email protected]>
---
drivers/scsi/pm8001/pm8001_hwi.c | 5 +++++
drivers/scsi/pm8001/pm8001_init.c | 18 +++++++++---------
drivers/scsi/pm8001/pm8001_sas.h | 1 +
drivers/scsi/pm8001/pm80xx_hwi.c | 11 +++++++++--
4 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index f7466a895d3b..a37595621d51 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -699,6 +699,10 @@ static int pm8001_chip_init(struct pm8001_hba_info *pm8001_ha)
return 0;
}
+static void pm8001_chip_post_init(struct pm8001_hba_info *pm8001_ha)
+{
+}
+
static int mpi_uninit_check(struct pm8001_hba_info *pm8001_ha)
{
u32 max_wait_count;
@@ -4947,6 +4951,7 @@ pm8001_chip_sas_re_initialization(struct pm8001_hba_info *pm8001_ha)
const struct pm8001_dispatch pm8001_8001_dispatch = {
.name = "pmc8001",
.chip_init = pm8001_chip_init,
+ .chip_post_init = pm8001_chip_post_init,
.chip_soft_rst = pm8001_chip_soft_rst,
.chip_rst = pm8001_hw_chip_rst,
.chip_iounmap = pm8001_chip_iounmap,
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 4288c6b8f041..171f0a2cf2d6 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(link_rate, "Enable link rate.\n"
" 8: Link rate 12.0G\n");
static struct scsi_transport_template *pm8001_stt;
-static int pm8001_init_ccb_tag(struct pm8001_hba_info *, struct Scsi_Host *, struct pci_dev *);
+static int pm8001_init_ccb_tag(struct pm8001_hba_info *);
/*
* chip info structure to identify chip key functionality as
@@ -1119,10 +1119,12 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
goto err_out_ha_free;
}
- rc = pm8001_init_ccb_tag(pm8001_ha, shost, pdev);
+ rc = pm8001_init_ccb_tag(pm8001_ha);
if (rc)
goto err_out_enable;
+ PM8001_CHIP_DISP->chip_post_init(pm8001_ha);
+
rc = scsi_add_host(shost, &pdev->dev);
if (rc)
goto err_out_ha_free;
@@ -1172,16 +1174,14 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
/**
* pm8001_init_ccb_tag - allocate memory to CCB and tag.
* @pm8001_ha: our hba card information.
- * @shost: scsi host which has been allocated outside.
- * @pdev: pci device.
*/
-static int
-pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha, struct Scsi_Host *shost,
- struct pci_dev *pdev)
+static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
{
- int i = 0;
+ struct Scsi_Host *shost = pm8001_ha->shost;
+ struct device *dev = pm8001_ha->dev;
u32 max_out_io, ccb_count;
u32 can_queue;
+ int i;
max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);
@@ -1204,7 +1204,7 @@ pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha, struct Scsi_Host *shost,
goto err_out_noccb;
}
for (i = 0; i < ccb_count; i++) {
- pm8001_ha->ccb_info[i].buf_prd = dma_alloc_coherent(&pdev->dev,
+ pm8001_ha->ccb_info[i].buf_prd = dma_alloc_coherent(dev,
sizeof(struct pm8001_prd) * PM8001_MAX_DMA_SG,
&pm8001_ha->ccb_info[i].ccb_dma_handle,
GFP_KERNEL);
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 060ab680a7ed..c761a2fd25e5 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -172,6 +172,7 @@ struct forensic_data {
struct pm8001_dispatch {
char *name;
int (*chip_init)(struct pm8001_hba_info *pm8001_ha);
+ void (*chip_post_init)(struct pm8001_hba_info *pm8001_ha);
int (*chip_soft_rst)(struct pm8001_hba_info *pm8001_ha);
void (*chip_rst)(struct pm8001_hba_info *pm8001_ha);
int (*chip_ioremap)(struct pm8001_hba_info *pm8001_ha);
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 01c5e8ff4cc5..d5cc726c034e 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -1469,11 +1469,18 @@ static int pm80xx_chip_init(struct pm8001_hba_info *pm8001_ha)
} else
return -EBUSY;
+ return 0;
+}
+
+static void pm80xx_chip_post_init(struct pm8001_hba_info *pm8001_ha)
+{
/* send SAS protocol timer configuration page to FW */
- ret = pm80xx_set_sas_protocol_timer_config(pm8001_ha);
+ pm80xx_set_sas_protocol_timer_config(pm8001_ha);
/* Check for encryption */
if (pm8001_ha->chip->encrypt) {
+ int ret;
+
pm8001_dbg(pm8001_ha, INIT, "Checking for encryption\n");
ret = pm80xx_get_encrypt_info(pm8001_ha);
if (ret == -1) {
@@ -1485,7 +1492,6 @@ static int pm80xx_chip_init(struct pm8001_hba_info *pm8001_ha)
}
}
}
- return 0;
}
static int mpi_uninit_check(struct pm8001_hba_info *pm8001_ha)
@@ -5007,6 +5013,7 @@ void pm8001_set_phy_profile_single(struct pm8001_hba_info *pm8001_ha,
const struct pm8001_dispatch pm8001_80xx_dispatch = {
.name = "pmc80xx",
.chip_init = pm80xx_chip_init,
+ .chip_post_init = pm80xx_chip_post_init,
.chip_soft_rst = pm80xx_chip_soft_rst,
.chip_rst = pm80xx_hw_chip_rst,
.chip_iounmap = pm8001_chip_iounmap,
--
2.26.2
Some values in pm8001_prep_sas_ha_init() are set the same as they would be
set in scsi_host_alloc(), or could be in the sht (which would be better),
or later just overwritten, so rework the following:
- cmd_per_lun can be set in the sht
- max_lun and max_channel are as scsi_host_alloc() (so no need to set)
- can_queue is later overwritten (so don't set in
pm8001_prep_sas_ha_init())
Signed-off-by: John Garry <[email protected]>
---
drivers/scsi/pm8001/pm8001_init.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 9b04f1a6a67d..4288c6b8f041 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -109,6 +109,7 @@ static struct scsi_host_template pm8001_sht = {
#endif
.shost_groups = pm8001_host_groups,
.track_queue_depth = 1,
+ .cmd_per_lun = 32,
};
/*
@@ -605,12 +606,8 @@ static int pm8001_prep_sas_ha_init(struct Scsi_Host *shost,
shost->transportt = pm8001_stt;
shost->max_id = PM8001_MAX_DEVICES;
- shost->max_lun = 8;
- shost->max_channel = 0;
shost->unique_id = pm8001_id;
shost->max_cmd_len = 16;
- shost->can_queue = PM8001_CAN_QUEUE;
- shost->cmd_per_lun = 32;
return 0;
exit_free1:
kfree(arr_port);
--
2.26.2
In pm8001_tag_alloc() we don't require atomic set_bit() as we are already
in atomic context. In pm8001_tag_free() we should use the same host
spinlock to protect clearing the tag (and then don't require the atomic
clear_bit()).
Signed-off-by: John Garry <[email protected]>
---
drivers/scsi/pm8001/pm8001_sas.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 3a863d776724..8e3f2f9ddaac 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -66,7 +66,11 @@ static int pm8001_find_tag(struct sas_task *task, u32 *tag)
void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
{
void *bitmap = pm8001_ha->tags;
- clear_bit(tag, bitmap);
+ unsigned long flags;
+
+ spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
+ __clear_bit(tag, bitmap);
+ spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
}
/**
@@ -76,9 +80,9 @@ void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
*/
int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
{
- unsigned int tag;
void *bitmap = pm8001_ha->tags;
unsigned long flags;
+ unsigned int tag;
spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num);
@@ -86,7 +90,7 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
return -SAS_QUEUE_FULL;
}
- set_bit(tag, bitmap);
+ __set_bit(tag, bitmap);
spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
*tag_out = tag;
return 0;
--
2.26.2
In commit 05c6c029a44d ("scsi: pm80xx: Increase number of supported
queues"), support for 80xx chip was improved by enabling multiple HW
queues.
In this, like other SCSI MQ HBA drivers at the time, the HW queues were
not exposed to upper layer, and instead the driver managed the queues
internally.
However, this management duplicates blk-mq code. In addition, the HW queue
management is sub-optimal for a system where the number of CPUs exceeds
the HW queues - this is because queues are selected in a round-robin
fashion, when it would be better to make adjacent CPUs submit on the same
queue. And finally, the affinity of the completion queue interrupts is not
set to mirror the cpu<->HQ queue mapping, which is suboptimal.
As such, for when MSIX is supported, expose HW queues to upper layer. We
always use queue index #0 for "internal" commands, i.e. anything which
does not come from the block layer, so omit this from the affinity
spreading.
Signed-off-by: John Garry <[email protected]>
---
drivers/scsi/pm8001/pm8001_init.c | 50 ++++++++++++++++++++++++-------
drivers/scsi/pm8001/pm8001_sas.h | 2 ++
drivers/scsi/pm8001/pm80xx_hwi.c | 35 +++++++++++++++++-----
3 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 171f0a2cf2d6..a3a972c2e532 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -81,6 +81,18 @@ LIST_HEAD(hba_list);
struct workqueue_struct *pm8001_wq;
+static int pm8001_map_queues(struct Scsi_Host *shost)
+{
+ struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
+ struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
+ struct blk_mq_queue_map *qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT];
+
+ if (pm8001_ha->number_of_intr > 1)
+ blk_mq_pci_map_queues(qmap, pm8001_ha->pdev, 1);
+
+ return blk_mq_map_queues(qmap);
+}
+
/*
* The main structure which LLDD must register for scsi core.
*/
@@ -110,6 +122,7 @@ static struct scsi_host_template pm8001_sht = {
.shost_groups = pm8001_host_groups,
.track_queue_depth = 1,
.cmd_per_lun = 32,
+ .map_queues = pm8001_map_queues,
};
/*
@@ -928,31 +941,35 @@ static int pm8001_configure_phy_settings(struct pm8001_hba_info *pm8001_ha)
*/
static u32 pm8001_setup_msix(struct pm8001_hba_info *pm8001_ha)
{
- u32 number_of_intr;
- int rc, cpu_online_count;
unsigned int allocated_irq_vectors;
+ int rc;
/* SPCv controllers supports 64 msi-x */
if (pm8001_ha->chip_id == chip_8001) {
- number_of_intr = 1;
+ rc = pci_alloc_irq_vectors(pm8001_ha->pdev, 1, 1,
+ PCI_IRQ_MSIX);
} else {
- number_of_intr = PM8001_MAX_MSIX_VEC;
+ /*
+ * Queue index #0 is used always for housekeeping, so don't
+ * include in the affinity spreading.
+ */
+ struct irq_affinity desc = {
+ .pre_vectors = 1,
+ };
+ rc = pci_alloc_irq_vectors_affinity(
+ pm8001_ha->pdev, 2, PM8001_MAX_MSIX_VEC,
+ PCI_IRQ_MSIX | PCI_IRQ_AFFINITY, &desc);
}
- cpu_online_count = num_online_cpus();
- number_of_intr = min_t(int, cpu_online_count, number_of_intr);
- rc = pci_alloc_irq_vectors(pm8001_ha->pdev, number_of_intr,
- number_of_intr, PCI_IRQ_MSIX);
allocated_irq_vectors = rc;
if (rc < 0)
return rc;
/* Assigns the number of interrupts */
- number_of_intr = min_t(int, allocated_irq_vectors, number_of_intr);
- pm8001_ha->number_of_intr = number_of_intr;
+ pm8001_ha->number_of_intr = allocated_irq_vectors;
/* Maximum queue number updating in HBA structure */
- pm8001_ha->max_q_num = number_of_intr;
+ pm8001_ha->max_q_num = allocated_irq_vectors;
pm8001_dbg(pm8001_ha, INIT,
"pci_alloc_irq_vectors request ret:%d no of intr %d\n",
@@ -1123,8 +1140,19 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
if (rc)
goto err_out_enable;
+
PM8001_CHIP_DISP->chip_post_init(pm8001_ha);
+ if (pm8001_ha->number_of_intr > 1) {
+ shost->nr_hw_queues = pm8001_ha->number_of_intr - 1;
+ /*
+ * For now, ensure we're not sent too many commands by setting
+ * host_tagset. This is also required if we start using request
+ * tag.
+ */
+ shost->host_tagset = 1;
+ }
+
rc = scsi_add_host(shost, &pdev->dev);
if (rc)
goto err_out_ha_free;
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index c761a2fd25e5..c5e3f380a01c 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -55,6 +55,8 @@
#include <scsi/scsi_tcq.h>
#include <scsi/sas_ata.h>
#include <linux/atomic.h>
+#include <linux/blk-mq.h>
+#include <linux/blk-mq-pci.h>
#include "pm8001_defs.h"
#define DRV_NAME "pm80xx"
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index d5cc726c034e..3893f704602e 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -4351,6 +4351,29 @@ static int check_enc_sat_cmd(struct sas_task *task)
return ret;
}
+static u32 pm80xx_chip_get_q_index(struct sas_task *task)
+{
+ struct scsi_cmnd *scmd = NULL;
+ u32 blk_tag;
+
+ if (task->uldd_task) {
+ struct ata_queued_cmd *qc;
+
+ if (dev_is_sata(task->dev)) {
+ qc = task->uldd_task;
+ scmd = qc->scsicmd;
+ } else {
+ scmd = task->uldd_task;
+ }
+ }
+
+ if (!scmd)
+ return 0;
+
+ blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd));
+ return blk_mq_unique_tag_to_hwq(blk_tag);
+}
+
/**
* pm80xx_chip_ssp_io_req - send an SSP task to FW
* @pm8001_ha: our hba card information.
@@ -4366,7 +4389,7 @@ static int pm80xx_chip_ssp_io_req(struct pm8001_hba_info *pm8001_ha,
u32 tag = ccb->ccb_tag;
u64 phys_addr, end_addr;
u32 end_addr_high, end_addr_low;
- u32 q_index, cpu_id;
+ u32 q_index;
u32 opc = OPC_INB_SSPINIIOSTART;
memset(&ssp_cmd, 0, sizeof(ssp_cmd));
@@ -4387,8 +4410,7 @@ static int pm80xx_chip_ssp_io_req(struct pm8001_hba_info *pm8001_ha,
ssp_cmd.ssp_iu.efb_prio_attr |= (task->ssp_task.task_attr & 7);
memcpy(ssp_cmd.ssp_iu.cdb, task->ssp_task.cmd->cmnd,
task->ssp_task.cmd->cmd_len);
- cpu_id = smp_processor_id();
- q_index = (u32) (cpu_id) % (pm8001_ha->max_q_num);
+ q_index = pm80xx_chip_get_q_index(task);
/* Check if encryption is set */
if (pm8001_ha->chip->encrypt &&
@@ -4517,8 +4539,7 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
struct domain_device *dev = task->dev;
struct pm8001_device *pm8001_ha_dev = dev->lldd_dev;
struct ata_queued_cmd *qc = task->uldd_task;
- u32 tag = ccb->ccb_tag;
- u32 q_index, cpu_id;
+ u32 tag = ccb->ccb_tag, q_index;
struct sata_start_req sata_cmd;
u32 hdr_tag, ncg_tag = 0;
u64 phys_addr, end_addr;
@@ -4528,8 +4549,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
unsigned long flags;
u32 opc = OPC_INB_SATA_HOST_OPSTART;
memset(&sata_cmd, 0, sizeof(sata_cmd));
- cpu_id = smp_processor_id();
- q_index = (u32) (cpu_id) % (pm8001_ha->max_q_num);
+
+ q_index = pm80xx_chip_get_q_index(task);
if (task->data_dir == DMA_NONE && !task->ata_task.use_ncq) {
ATAP = 0x04; /* no data*/
--
2.26.2
On Fri, Jun 10, 2022 at 6:52 PM John Garry <[email protected]> wrote:
>
> Some values in pm8001_prep_sas_ha_init() are set the same as they would be
> set in scsi_host_alloc(), or could be in the sht (which would be better),
> or later just overwritten, so rework the following:
> - cmd_per_lun can be set in the sht
> - max_lun and max_channel are as scsi_host_alloc() (so no need to set)
> - can_queue is later overwritten (so don't set in
> pm8001_prep_sas_ha_init())
>
> Signed-off-by: John Garry <[email protected]>
lgtm!
Acked-by: Jack Wang <[email protected]>
> ---
> drivers/scsi/pm8001/pm8001_init.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 9b04f1a6a67d..4288c6b8f041 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -109,6 +109,7 @@ static struct scsi_host_template pm8001_sht = {
> #endif
> .shost_groups = pm8001_host_groups,
> .track_queue_depth = 1,
> + .cmd_per_lun = 32,
> };
>
> /*
> @@ -605,12 +606,8 @@ static int pm8001_prep_sas_ha_init(struct Scsi_Host *shost,
>
> shost->transportt = pm8001_stt;
> shost->max_id = PM8001_MAX_DEVICES;
> - shost->max_lun = 8;
> - shost->max_channel = 0;
> shost->unique_id = pm8001_id;
> shost->max_cmd_len = 16;
> - shost->can_queue = PM8001_CAN_QUEUE;
> - shost->cmd_per_lun = 32;
> return 0;
> exit_free1:
> kfree(arr_port);
> --
> 2.26.2
>
On 6/11/22 01:46, John Garry wrote:
> This small series includes the following:
> - Rework how some shost values are set
> - Fix a longstanding bug that the driver attempts to use tags before they
> are configured
> - Stop using {set, clear}_bit()
> - Expose HW queues
>
> Any testing would be appreciated as this driver is still broken for my
> arm64 system, i.e. just broken.
>
> John Garry (4):
> scsi: pm8001: Rework shost initial values
> scsi: pm8001: Setup tags before using them
> scsi: pm8001: Use non-atomic bitmap ops for tag alloc + free
> scsi: pm8001: Expose HW queues for pm80xx hw
>
> drivers/scsi/pm8001/pm8001_hwi.c | 5 +++
> drivers/scsi/pm8001/pm8001_init.c | 73 +++++++++++++++++++++----------
> drivers/scsi/pm8001/pm8001_sas.c | 10 +++--
> drivers/scsi/pm8001/pm8001_sas.h | 3 ++
> drivers/scsi/pm8001/pm80xx_hwi.c | 46 +++++++++++++++----
> 5 files changed, 101 insertions(+), 36 deletions(-)
>
Tested-by: Damien Le Moal <[email protected]>
--
Damien Le Moal
Western Digital Research
John,
> This small series includes the following:
> - Rework how some shost values are set
> - Fix a longstanding bug that the driver attempts to use tags before they
> are configured
> - Stop using {set, clear}_bit()
> - Expose HW queues
Applied to 5.20/scsi-staging, thanks!
--
Martin K. Petersen Oracle Linux Engineering
On 6/20/22 15:00, Hannes Reinecke wrote:
> On 6/10/22 18:46, John Garry wrote:
>> In pm8001_tag_alloc() we don't require atomic set_bit() as we are already
>> in atomic context. In pm8001_tag_free() we should use the same host
>> spinlock to protect clearing the tag (and then don't require the atomic
>> clear_bit()).
>>
>> Signed-off-by: John Garry <[email protected]>
>> ---
>> drivers/scsi/pm8001/pm8001_sas.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
>> index 3a863d776724..8e3f2f9ddaac 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.c
>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>> @@ -66,7 +66,11 @@ static int pm8001_find_tag(struct sas_task *task, u32 *tag)
>> void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
>> {
>> void *bitmap = pm8001_ha->tags;
>> - clear_bit(tag, bitmap);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
>> + __clear_bit(tag, bitmap);
>> + spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>> }
>>
> This spin lock is pretty much pointless; clear_bit() is always atomic.
But __clear_bit() is not atomic. I think it was the point of this patch,
to not use atomics and use the spinlock instead to protect bitmap.
Before the patch, pm8001_tag_alloc() takes the spinlock *and* use the
atomic set_bit(), which is an overkill. pm8001_tag_free() only clears the
bit using the the atomic clear_bit().
After the patch, spinlock guarantees atomicity for both alloc and free.
Not sure there is any gain from this.
>
> Cheers,
>
> Hannes
--
Damien Le Moal
Western Digital Research
On 6/10/22 18:46, John Garry wrote:
> In pm8001_tag_alloc() we don't require atomic set_bit() as we are already
> in atomic context. In pm8001_tag_free() we should use the same host
> spinlock to protect clearing the tag (and then don't require the atomic
> clear_bit()).
>
> Signed-off-by: John Garry <[email protected]>
> ---
> drivers/scsi/pm8001/pm8001_sas.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 3a863d776724..8e3f2f9ddaac 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -66,7 +66,11 @@ static int pm8001_find_tag(struct sas_task *task, u32 *tag)
> void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
> {
> void *bitmap = pm8001_ha->tags;
> - clear_bit(tag, bitmap);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
> + __clear_bit(tag, bitmap);
> + spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> }
>
This spin lock is pretty much pointless; clear_bit() is always atomic.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman
>
> After the patch, spinlock guarantees atomicity for both alloc and free.
>
> Not sure there is any gain from this.
+1
On 20/06/2022 07:07, Damien Le Moal wrote:
> On 6/20/22 15:00, Hannes Reinecke wrote:
>> On 6/10/22 18:46, John Garry wrote:
>>> In pm8001_tag_alloc() we don't require atomic set_bit() as we are already
>>> in atomic context. In pm8001_tag_free() we should use the same host
>>> spinlock to protect clearing the tag (and then don't require the atomic
>>> clear_bit()).
>>>
>>> Signed-off-by: John Garry <[email protected]>
>>> ---
>>> drivers/scsi/pm8001/pm8001_sas.c | 10 +++++++---
>>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
>>> index 3a863d776724..8e3f2f9ddaac 100644
>>> --- a/drivers/scsi/pm8001/pm8001_sas.c
>>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>>> @@ -66,7 +66,11 @@ static int pm8001_find_tag(struct sas_task *task, u32 *tag)
>>> void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
>>> {
>>> void *bitmap = pm8001_ha->tags;
>>> - clear_bit(tag, bitmap);
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
>>> + __clear_bit(tag, bitmap);
>>> + spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>>> }
>>>
>> This spin lock is pretty much pointless; clear_bit() is always atomic.
>
> But __clear_bit() is not atomic. I think it was the point of this patch,
> to not use atomics and use the spinlock instead to protect bitmap.
>
> Before the patch, pm8001_tag_alloc() takes the spinlock *and* use the
> atomic set_bit(), which is an overkill. pm8001_tag_free() only clears the
> bit using the the atomic clear_bit().
Right, so I could change to use __set_bit() in pm8001_find_tag(), but
rather use spinlock always.
>
> After the patch, spinlock guarantees atomicity for both alloc and free.
>
> Not sure there is any gain from this.
A few more points to note:
- On architectures which do not support atomic operations natively, they
have to use global spinlocks to create atomic context before doing
non-atomic bit clearing - see atomic64.c . As such, it's better to use
the already available pm8001_ha->bitmap_lock.
- spinlock does more than create atomic context, but also has barrier
semantics, so proper to use consistently for protecting the same region.
Thanks,
John
On Sat, 11 Jun 2022 00:46:38 +0800, John Garry wrote:
> This small series includes the following:
> - Rework how some shost values are set
> - Fix a longstanding bug that the driver attempts to use tags before they
> are configured
> - Stop using {set, clear}_bit()
> - Expose HW queues
>
> [...]
Applied to 5.20/scsi-queue, thanks!
[1/4] scsi: pm8001: Rework shost initial values
https://git.kernel.org/mkp/scsi/c/35a7e9dbff9a
[2/4] scsi: pm8001: Setup tags before using them
https://git.kernel.org/mkp/scsi/c/98132d842d4d
[3/4] scsi: pm8001: Use non-atomic bitmap ops for tag alloc + free
https://git.kernel.org/mkp/scsi/c/940f5efa6316
[4/4] scsi: pm8001: Expose HW queues for pm80xx hw
https://git.kernel.org/mkp/scsi/c/42f22fe36d51
--
Martin K. Petersen Oracle Linux Engineering