scsi: pm80xx: Fix race condition caused by static variables
Subsystems (2)
PATCH 1/1 scsi: pm80xx: Fix race condition caused by static variables
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.
--- 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(-)
drivers/scsi/pm8001/pm8001_ctl.c +12 -10
@@ -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);
drivers/scsi/pm8001/pm8001_init.c +1 -0
@@ -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 {
drivers/scsi/pm8001/pm8001_sas.h +4 -0
@@ -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 {
Signature · 1 line
2.50.1.470.g6ba607880d-goog
Francisco,
drivers/scsi/pm8001/pm8001_ctl.c static ssize_t pm8001_ctl_iop_log_show(struct device *cdev, · line 554
Quoted 7 lines
drivers/scsi/pm8001/pm8001_ctl.c +3
-3-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...
Signature · 1 line
Martin K. Petersen
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
Quoted 18 lines · Martin K. Petersen
Francisco,
drivers/scsi/pm8001/pm8001_ctl.c static ssize_t pm8001_ctl_iop_log_show(struct device *cdev, · line 554
Quoted 7 lines
drivers/scsi/pm8001/pm8001_ctl.c +3
-3-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
Martin K. Petersen
AppliedQuoted 6 lines · 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.
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
Signature · 1 line
Martin K. Petersen