[PATCH] scsi: pm80xx: Fix race condition caused by static variables

[PATCH] scsi: pm80xx: Fix race condition caused by static variables

[Francisco Gutierrez]

Eliminate the use of static variables within the log pull implementation
to resolve a race condition and prevent data gaps when pulling logs
from multiple controllers in parallel, ensuring each operation
is properly isolated.

Signed-off-by: Francisco Gutierrez <frankramirez@google.com>
---
 drivers/scsi/pm8001/pm8001_ctl.c  | 22 ++++++++++++----------
 drivers/scsi/pm8001/pm8001_init.c |  1 +
 drivers/scsi/pm8001/pm8001_sas.h  |  4 ++++
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
index 7618f9cc9986d..0c96875cf8fd1 100644
--- a/drivers/scsi/pm8001/pm8001_ctl.c
+++ b/drivers/scsi/pm8001/pm8001_ctl.c
@@ -534,23 +534,25 @@ static ssize_t pm8001_ctl_iop_log_show(struct device *cdev,
 	char *str = buf;
 	u32 read_size =
 		pm8001_ha->main_cfg_tbl.pm80xx_tbl.event_log_size / 1024;
-	static u32 start, end, count;
 	u32 max_read_times = 32;
 	u32 max_count = (read_size * 1024) / (max_read_times * 4);
 	u32 *temp = (u32 *)pm8001_ha->memoryMap.region[IOP].virt_ptr;
 
-	if ((count % max_count) == 0) {
-		start = 0;
-		end = max_read_times;
-		count = 0;
+	mutex_lock(&pm8001_ha->iop_log_lock);
+
+	if ((pm8001_ha->iop_log_count % max_count) == 0) {
+		pm8001_ha->iop_log_start = 0;
+		pm8001_ha->iop_log_end = max_read_times;
+		pm8001_ha->iop_log_count = 0;
 	} else {
-		start = end;
-		end = end + max_read_times;
+		pm8001_ha->iop_log_start = pm8001_ha->iop_log_end;
+		pm8001_ha->iop_log_end = pm8001_ha->iop_log_end + max_read_times;
 	}
 
-	for (; start < end; start++)
-		str += sprintf(str, "%08x ", *(temp+start));
-	count++;
+	for (; pm8001_ha->iop_log_start < pm8001_ha->iop_log_end; pm8001_ha->iop_log_start++)
+		str += sprintf(str, "%08x ", *(temp+pm8001_ha->iop_log_start));
+	pm8001_ha->iop_log_count++;
+	mutex_unlock(&pm8001_ha->iop_log_lock);
 	return str - buf;
 }
 static DEVICE_ATTR(iop_log, S_IRUGO, pm8001_ctl_iop_log_show, NULL);
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 599410bcdfea5..8ff4b89ff81e2 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -552,6 +552,7 @@ static struct pm8001_hba_info *pm8001_pci_alloc(struct pci_dev *pdev,
 	pm8001_ha->id = pm8001_id++;
 	pm8001_ha->logging_level = logging_level;
 	pm8001_ha->non_fatal_count = 0;
+	mutex_init(&pm8001_ha->iop_log_lock);
 	if (link_rate >= 1 && link_rate <= 15)
 		pm8001_ha->link_rate = (link_rate << 8);
 	else {
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 315f6a7523f09..cfb3bbe04b78c 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -539,6 +539,10 @@ struct pm8001_hba_info {
 	u32 ci_offset;
 	u32 pi_offset;
 	u32 max_memcnt;
+	u32 iop_log_start;
+	u32 iop_log_end;
+	u32 iop_log_count;
+	struct mutex iop_log_lock;
 };
 
 struct pm8001_work {
-- 
2.50.1.470.g6ba607880d-goog

Re: [PATCH] scsi: pm80xx: Fix race condition caused by static variables

[Martin K. Petersen]

Francisco,

> -	for (; start < end; start++)
> -		str += sprintf(str, "%08x ", *(temp+start));
> -	count++;

> +	for (; pm8001_ha->iop_log_start < pm8001_ha->iop_log_end; pm8001_ha->iop_log_start++)
> +		str += sprintf(str, "%08x ", *(temp+pm8001_ha->iop_log_start));
> +	pm8001_ha->iop_log_count++;

Since the start, end, and count variables are only used in this
function, what is the point of putting them in 'struct pm8001_hba_info'?
It makes the lines longer and harder to read...

-- 
Martin K. Petersen

Re: [PATCH] scsi: pm80xx: Fix race condition caused by static variables

[Francisco Gutierrez Ramirez]

Hi Martin,

Thanks very much for taking the time to review my patch and for your feedback.

I'm writing to provide a bit more context for the design. The function
in question needs to be called repeatedly, and the struct helps to
track state across those calls without using static variables, instead
tying the state to the specific controller instance. This ensures the
function is thread-safe.

Please let me know if that context clarifies my approach, or if you
have an alternative suggestion for managing state safely in this
scenario.

Thanks again for your help.

Best regards,
Francisco Gutierrez


On Tue, Aug 5, 2025 at 7:30 PM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Francisco,
>
> > -     for (; start < end; start++)
> > -             str += sprintf(str, "%08x ", *(temp+start));
> > -     count++;
>
> > +     for (; pm8001_ha->iop_log_start < pm8001_ha->iop_log_end; pm8001_ha->iop_log_start++)
> > +             str += sprintf(str, "%08x ", *(temp+pm8001_ha->iop_log_start));
> > +     pm8001_ha->iop_log_count++;
>
> Since the start, end, and count variables are only used in this
> function, what is the point of putting them in 'struct pm8001_hba_info'?
> It makes the lines longer and harder to read...
>
> --
> Martin K. Petersen

Re: [PATCH] scsi: pm80xx: Fix race condition caused by static variables

[Martin K. Petersen]

On Wed, 23 Jul 2025 18:35:43 +0000, Francisco Gutierrez wrote:

> Eliminate the use of static variables within the log pull implementation
> to resolve a race condition and prevent data gaps when pulling logs
> from multiple controllers in parallel, ensuring each operation
> is properly isolated.
> 
> 

Applied to 6.18/scsi-queue, thanks!

[1/1] scsi: pm80xx: Fix race condition caused by static variables
      https://git.kernel.org/mkp/scsi/c/d6477ee38ccf

-- 
Martin K. Petersen