| Message ID | 160763259402.26927.11602719287229380898.stgit@brunhilda (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | smartpqi updates | expand |
On Thu, 2020-12-10 at 14:36 -0600, Don Brace wrote: > From: Kevin Barnett <kevin.barnett@microchip.com> > > * Correct system hangs when resuming from hibernation after > first successful hibernation/resume cycle. > * Rare condition involving OFA. > > Reviewed-by: Scott Benesh <scott.benesh@microchip.com> > Signed-off-by: Kevin Barnett <kevin.barnett@microchip.com> > Signed-off-by: Don Brace <don.brace@microchip.com> > --- > drivers/scsi/smartpqi/smartpqi_init.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/scsi/smartpqi/smartpqi_init.c > b/drivers/scsi/smartpqi/smartpqi_init.c > index 40ae82470d8c..5ca265babaa2 100644 > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > @@ -8688,6 +8688,11 @@ static __maybe_unused int pqi_resume(struct > pci_dev *pci_dev) > pci_set_power_state(pci_dev, PCI_D0); > pci_restore_state(pci_dev); > > + pqi_ctrl_unblock_device_reset(ctrl_info); > + pqi_ctrl_unblock_requests(ctrl_info); > + pqi_scsi_unblock_requests(ctrl_info); > + pqi_ctrl_unblock_scan(ctrl_info); > + > return pqi_ctrl_init_resume(ctrl_info); > } Like I said in my comments on 14/25: pqi_ctrl_unblock_scan() and pqi_ctrl_unblock_device_reset() expand to mutex_unlock(). Unlocking an already-unlocked mutex is wrong, and a mutex has to be unlocked by the task that owns the lock. How can you be sure that these conditions are met here? Regards Martin
-----Original Message----- From: Martin Wilck [mailto:mwilck@suse.com] Subject: Re: [PATCH V3 23/25] smartpqi: correct system hangs when resuming from hibernation > @@ -8688,6 +8688,11 @@ static __maybe_unused int pqi_resume(struct > pci_dev *pci_dev) > pci_set_power_state(pci_dev, PCI_D0); > pci_restore_state(pci_dev); > > + pqi_ctrl_unblock_device_reset(ctrl_info); > + pqi_ctrl_unblock_requests(ctrl_info); > + pqi_scsi_unblock_requests(ctrl_info); > + pqi_ctrl_unblock_scan(ctrl_info); > + > return pqi_ctrl_init_resume(ctrl_info); } Like I said in my comments on 14/25: pqi_ctrl_unblock_scan() and pqi_ctrl_unblock_device_reset() expand to mutex_unlock(). Unlocking an already-unlocked mutex is wrong, and a mutex has to be unlocked by the task that owns the lock. How can you be sure that these conditions are met here? Don: I updated this patch to: @@ -8661,9 +8661,17 @@ static __maybe_unused int pqi_resume(struct pci_dev *pci_dev) return 0; } + pqi_ctrl_block_device_reset(ctrl_info); + pqi_ctrl_block_scan(ctrl_info); + pci_set_power_state(pci_dev, PCI_D0); pci_restore_state(pci_dev); + pqi_ctrl_unblock_device_reset(ctrl_info); + pqi_ctrl_unblock_requests(ctrl_info); + pqi_scsi_unblock_requests(ctrl_info); + pqi_ctrl_unblock_scan(ctrl_info); + return pqi_ctrl_init_resume(ctrl_info); } Don: So the mutexes are set and unset in the same task. I updated the other patch 14 accordingly, but I'll reply in that patch also. Is there a specific driver that initiates suspend/resume? Like acpi? Or some other pci driver? Thanks for your hard work on these patches Don
On Wed, 2021-01-27 at 17:39 +0000, Don.Brace@microchip.com wrote: > -----Original Message----- > From: Martin Wilck [mailto:mwilck@suse.com] > Subject: Re: [PATCH V3 23/25] smartpqi: correct system hangs when > resuming from hibernation > > > @@ -8688,6 +8688,11 @@ static __maybe_unused int pqi_resume(struct > > pci_dev *pci_dev) > > pci_set_power_state(pci_dev, PCI_D0); > > pci_restore_state(pci_dev); > > > > + pqi_ctrl_unblock_device_reset(ctrl_info); > > + pqi_ctrl_unblock_requests(ctrl_info); > > + pqi_scsi_unblock_requests(ctrl_info); > > + pqi_ctrl_unblock_scan(ctrl_info); > > + > > return pqi_ctrl_init_resume(ctrl_info); } > > Like I said in my comments on 14/25: > > pqi_ctrl_unblock_scan() and pqi_ctrl_unblock_device_reset() expand to > mutex_unlock(). Unlocking an already-unlocked mutex is wrong, and a > mutex has to be unlocked by the task that owns the lock. How can you > be sure that these conditions are met here? > > Don: I updated this patch to: > @@ -8661,9 +8661,17 @@ static __maybe_unused int pqi_resume(struct > pci_dev *pci_dev) > return 0; > } > > + pqi_ctrl_block_device_reset(ctrl_info); > + pqi_ctrl_block_scan(ctrl_info); > + > pci_set_power_state(pci_dev, PCI_D0); > pci_restore_state(pci_dev); > > + pqi_ctrl_unblock_device_reset(ctrl_info); > + pqi_ctrl_unblock_requests(ctrl_info); > + pqi_scsi_unblock_requests(ctrl_info); > + pqi_ctrl_unblock_scan(ctrl_info); > + > return pqi_ctrl_init_resume(ctrl_info); > } > Don: So the mutexes are set and unset in the same task. Yes, that looks much better to me. > I updated the other patch 14 accordingly, but I'll reply in that > patch also. Is there a specific driver that initiates suspend/resume? > Like acpi? Or some other pci driver? I'm no expert on suspend/resume. I think it's platform-dependent, you shouldn't make any specific assumptions what the platform actually does. Regards Martin
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c index 40ae82470d8c..5ca265babaa2 100644 --- a/drivers/scsi/smartpqi/smartpqi_init.c +++ b/drivers/scsi/smartpqi/smartpqi_init.c @@ -8688,6 +8688,11 @@ static __maybe_unused int pqi_resume(struct pci_dev *pci_dev) pci_set_power_state(pci_dev, PCI_D0); pci_restore_state(pci_dev); + pqi_ctrl_unblock_device_reset(ctrl_info); + pqi_ctrl_unblock_requests(ctrl_info); + pqi_scsi_unblock_requests(ctrl_info); + pqi_ctrl_unblock_scan(ctrl_info); + return pqi_ctrl_init_resume(ctrl_info); }