RE: [PATCH V3 14/25] smartpqi: fix driver synchronization issues

[<Don.Brace@microchip.com>]

-----Original Message-----
From: Martin Wilck [mailto:mwilck@suse.com] 
Subject: Re: [PATCH V3 14/25] smartpqi: fix driver synchronization issues

On Thu, 2020-12-10 at 14:35 -0600, Don Brace wrote:
> From: Kevin Barnett <kevin.barnett@microchip.com>
>
> * synchronize: LUN resets, shutdowns, suspend, hibernate,
>   OFA, and controller offline events.
> * prevent I/O during the the above conditions.

This description is too terse for a complex patch like this.


The patch does not only address synchronization issues; it also changes
various other things that (given the size of the patch) should better
be handled elsewhere. I believe this patch could easily be split into
4 or more separate independent patches, which would ease review
considerably. I've added remarks below where I thought one or more
hunks could be separated out.

Don: I'll start answering questions. The overall answer is that I split this patch into 10 patches:
+ smartpqi-remove-timeouts-from-internal-cmds
+ smartpqi-add-support-for-wwid
+ smartpqi-update-event-handler
+ smartpqi-update-soft-reset-management-for-OFA
+ smartpqi-synchronize-device-resets-with-mutex
+ smartpqi-update-suspend-resume-and-shutdown
+ smartpqi-update-raid-bypass-handling
+ smartpqi-update-ofa-management
+ smartpqi-update-device-scan-operations
+ smartpqi-fix-driver-synchronization-issues

Don: and I'll answer the questions and give the new patch name that your question belongs to.

Thanks for your hard work. It really helps a lot.

PQI_FIRMWARE_FEATURE_RAID_BYPASS_ON_ENCRYPTED_NVME     15
> -#define PQI_FIRMWARE_FEATURE_MAXIMUM                           15
> +#define PQI_FIRMWARE_FEATURE_UNIQUE_WWID_IN_REPORT_PHYS_LUN    16
> +#define PQI_FIRMWARE_FEATURE_MAXIMUM                           16

What does the "unique WWID" feature have to do with synchronization
issues? This part should have gone into a separate patch.

DON: Correct, I moved all of the corresponding WWID HUNKs into patch:
smartpqi-add-support-for-wwid

> +static inline void pqi_ctrl_block_scan(struct pqi_ctrl_info
> *ctrl_info)
> +{
> +       ctrl_info->scan_blocked = true;
> +       mutex_lock(&ctrl_info->scan_mutex);
> +}

What do you need scan_blocked for? Can't you simply use
mutex_is_locked(&ctrl_info->scan_mutex)?
OTOH, using a mutex for this kind of condition feels dangerous
to me, see remark about ota_mutex() below.
Have you considered using a completion for this?

Don: This mutex is used for application initiated REGNEWD, driver initialization, during a controller restart after Online Firmware Initialization, and resume operation after suspend.

I believe that all of these operations are in their own thread and can be paused briefly to avoid updating any driver state. A wait_for_completion can cause a stack trace if the wait period is over 120 seconds. The authors intent was to provide the simplest mechanism to pause these operations.

I moved this functionality into patch 
smartpqi-update-suspend-resume-and-shutdown. 

> +
> +static inline void pqi_ctrl_unblock_scan(struct pqi_ctrl_info
> *ctrl_info)
> +{
> +       ctrl_info->scan_blocked = false;
> +       mutex_unlock(&ctrl_info->scan_mutex);
> +}
> +
> +static inline bool pqi_ctrl_scan_blocked(struct pqi_ctrl_info
> *ctrl_info)
> +{
> +       return ctrl_info->scan_blocked;
> +}
> +
>  static inline void pqi_ctrl_block_device_reset(struct pqi_ctrl_info
> *ctrl_info)
>  {
> -       ctrl_info->block_device_reset = true;
> +       mutex_lock(&ctrl_info->lun_reset_mutex);
> +}
> +
> +static inline void pqi_ctrl_unblock_device_reset(struct
> pqi_ctrl_info *ctrl_info)
> +{
> +       mutex_unlock(&ctrl_info->lun_reset_mutex);
> +}
> +
> +static inline void pqi_scsi_block_requests(struct pqi_ctrl_info
> *ctrl_info)
> +{
> +       struct Scsi_Host *shost;
> +       unsigned int num_loops;
> +       int msecs_sleep;
> +
> +       shost = ctrl_info->scsi_host;
> +
> +       scsi_block_requests(shost);
> +
> +       num_loops = 0;
> +       msecs_sleep = 20;
> +       while (scsi_host_busy(shost)) {
> +               num_loops++;
> +               if (num_loops == 10)
> +                       msecs_sleep = 500;
> +               msleep(msecs_sleep);
> +       }
> +}

Waiting for !scsi_host_busy() here looks like a layering violation to
me. Can't you use wait_event{_timeout}() here and wait for the sum of
of device->scsi_cmds_outstanding over to become zero (waking up the
queue in pqi_prep_for_scsi_done())? You could use the
total_scmnds_outstanding count that you introduce in patch 15/25.

Also, how does this interact/interfere with scsi EH?

Don: I moved this HUNK into 
smartpqi-update-suspend-resume-and-shutdown
The function pqi_scsi_block_requests is called during Online Firmware Activation (OFA), shutdown, and suspend. I believe that this HUNK was in reaction to test team issuing continuous device resets during these operations and each needed to block any new I/O requests and EH operations during this time-period.

Since I broke up the larger patch, I may be able to move the smartpqi_fix_host_qdepth_limit patch before the 10 patches created from the refactoring. I think I would like to get V4 up for re-evaluation and eliminate as many outstanding reviews before I attack this review.


>
> -static inline void pqi_ctrl_ofa_start(struct pqi_ctrl_info
> *ctrl_info)
> +static inline void pqi_ctrl_ofa_done(struct pqi_ctrl_info
> *ctrl_info)
>  {
> -       ctrl_info->in_ofa = true;
> +       mutex_unlock(&ctrl_info->ofa_mutex);
>  }

pqi_ctrl_ofa_done() is called in several places. For me, it's non-
obvious whether ofa_mutex is guaranteed to be locked when this happens.
It would be an error to call mutex_unlock() if that's not the case.
Also, is it always guaranteed that "The context (task) that acquired
the lock also releases it"
(https://www.kernel.org/doc/html/latest/locking/locktypes.html)?
If feel that's rather not the case, as pqi_ctrl_ofa_start() is run from
a work queue, whereas pqi_ctrl_ofa_done() is not, afaics.

Have you considered using a completion?
Or can you add some explanatory comments?

Don: 
I moved this into smartpqi-update-ofa-management and
Hope this will look cleaner and clearer. The corresponding mutex set is in pqi_ofa_memory_alloc_worker.


>  static inline u8 pqi_read_soft_reset_status(struct pqi_ctrl_info
> *ctrl_info)
>  {
> -       if (!ctrl_info->soft_reset_status)
> -               return 0;
> -
>         return readb(ctrl_info->soft_reset_status);
>  }

The new treatment of soft_reset_status is unrelated to the
synchronization issues mentioned in the patch description.

Don: I moved the soft_reset operations into patch
 smartpqi-update-soft-reset-management-for-OFA
Hopefully this cleans and clears things up.

> @@ -1464,6 +1471,9 @@ static int pqi_get_physical_device_info(struct
> pqi_ctrl_info *ctrl_info,
>                 sizeof(device->phys_connector));
>         device->bay = id_phys->phys_bay_in_box;
>
> +       memcpy(&device->page_83_identifier, &id_phys-
> >page_83_identifier,
> +               sizeof(device->page_83_identifier));
> +
>         return 0;
>  }
>

This hunk belongs to the "unique wwid" part, see above.
Don: moved HUNK into smartpqi-add-support-for-wwid


> @@ -1970,8 +1980,13 @@ static void pqi_update_device_list(struct
> pqi_ctrl_info *ctrl_info,
>
>         spin_unlock_irqrestore(&ctrl_info->scsi_device_list_lock,
> flags);
>
> -       if (pqi_ctrl_in_ofa(ctrl_info))
> -               pqi_ctrl_ofa_done(ctrl_info);
> +       if (pqi_ofa_in_progress(ctrl_info)) {
> +               list_for_each_entry_safe(device, next, &delete_list,
> delete_list_entry)
> +                       if (pqi_is_device_added(device))
> +                               pqi_device_remove_start(device);
> +               pqi_ctrl_unblock_device_reset(ctrl_info);
> +               pqi_scsi_unblock_requests(ctrl_info);
> +       }

I don't understand the purpose of is code. pqi_device_remove_start()
will be called again a few lines below. Why do it twice? I suppose
it's related to the unblocking, but that deserves an explanation.
Also, why do you unblock requests while OFA is "in progress"?

Don: I moved this code into patch smartpqi-update-ofa-management
The author's intent was to block any new incoming requests and to block retries while ofa is in progress to existing devices that will be deleted. Allow any pending resets to complete.
I'll add a comment. 
Thanks for all of your really hard work on this patch review.
It means a lot.

>
>         /* Remove all devices that have gone away. */
>         list_for_each_entry_safe(device, next, &delete_list,
> delete_list_entry) {
> @@ -1993,19 +2008,14 @@ static void pqi_update_device_list(struct
> pqi_ctrl_info *ctrl_info,

The following hunk is unrelated to synchronization.

Don: Moved this formatting HUNK into smartpqi-align-code-with-oob-driver

>          * Notify the SCSI ML if the queue depth of any existing
> device has
>          * changed.
>          */
> -       list_for_each_entry(device, &ctrl_info->scsi_device_list,
> -               scsi_device_list_entry) {
> -               if (device->sdev) {
> -                       if (device->queue_depth !=
> -                               device->advertised_queue_depth) {
> -                               device->advertised_queue_depth =
> device->queue_depth;
> -                               scsi_change_queue_depth(device->sdev,
> -                                       device-
> >advertised_queue_depth);
> -                       }
> -                       if (device->rescan) {
> -                               scsi_rescan_device(&device->sdev-
> >sdev_gendev);
> -                               device->rescan = false;
> -                       }
> +       list_for_each_entry(device, &ctrl_info->scsi_device_list,
> scsi_device_list_entry) {
> +               if (device->sdev && device->queue_depth != device-
> >advertised_queue_depth) {
> +                       device->advertised_queue_depth = device-
> >queue_depth;
> +                       scsi_change_queue_depth(device->sdev, device-
> >advertised_queue_depth);
> +               }
> +               if (device->rescan) {
> +                       scsi_rescan_device(&device->sdev-
> >sdev_gendev);
> +                       device->rescan = false;
>                 }

You've taken the reference to device->sdev->sdev_gendev out of the if
(device->sdev) clause. Can you be certain that device->sdev is non-
NULL?

Don: No. Corrected this HUNK in smartpqi-align-code-with-oob-driver.
Thank-you
>         }
>
> @@ -2073,6 +2083,16 @@ static inline bool pqi_expose_device(struct
> pqi_scsi_dev *device)
>         return !device->is_physical_device ||
> !pqi_skip_device(device->scsi3addr);
>  }
>

The following belongs to the "unique wwid" part.

Don: Moved WWID HUNKs into smartpqi-add-support-for-wwid

>
>  static int pqi_scan_scsi_devices(struct pqi_ctrl_info *ctrl_info)
>  {
> -       int rc = 0;
> +       int rc;
> +       int mutex_acquired;
>
>         if (pqi_ctrl_offline(ctrl_info))
>                 return -ENXIO;
>
> -       if (!mutex_trylock(&ctrl_info->scan_mutex)) {
> +       mutex_acquired = mutex_trylock(&ctrl_info->scan_mutex);
> +
> +       if (!mutex_acquired) {
> +               if (pqi_ctrl_scan_blocked(ctrl_info))
> +                       return -EBUSY;
>                 pqi_schedule_rescan_worker_delayed(ctrl_info);
> -               rc = -EINPROGRESS;
> -       } else {
> -               rc = pqi_update_scsi_devices(ctrl_info);
> -               if (rc)
> -
>                        pqi_schedule_rescan_worker_delayed(ctrl_info);
> -               mutex_unlock(&ctrl_info->scan_mutex);
> +               return -EINPROGRESS;
>         }
>
> +       rc = pqi_update_scsi_devices(ctrl_info);
> +       if (rc && !pqi_ctrl_scan_blocked(ctrl_info))
> +               pqi_schedule_rescan_worker_delayed(ctrl_info);
> +
> +       mutex_unlock(&ctrl_info->scan_mutex);
> +
>         return rc;
>  }
>
> @@ -2301,8 +2327,6 @@ static void pqi_scan_start(struct Scsi_Host
> *shost)
>         struct pqi_ctrl_info *ctrl_info;
>
>         ctrl_info = shost_to_hba(shost);
> -       if (pqi_ctrl_in_ofa(ctrl_info))
> -               return;
>
>         pqi_scan_scsi_devices(ctrl_info);
>  }
> @@ -2319,27 +2343,8 @@ static int pqi_scan_finished(struct Scsi_Host
> *shost,
>         return !mutex_is_locked(&ctrl_info->scan_mutex);
>  }
>
> -static void pqi_wait_until_scan_finished(struct pqi_ctrl_info
> *ctrl_info)
> -{
> -       mutex_lock(&ctrl_info->scan_mutex);
> -       mutex_unlock(&ctrl_info->scan_mutex);
> -}
> -
> -static void pqi_wait_until_lun_reset_finished(struct pqi_ctrl_info
> *ctrl_info)
> -{
> -       mutex_lock(&ctrl_info->lun_reset_mutex);
> -       mutex_unlock(&ctrl_info->lun_reset_mutex);
> -}
> -
> -static void pqi_wait_until_ofa_finished(struct pqi_ctrl_info
> *ctrl_info)
> -{
> -       mutex_lock(&ctrl_info->ofa_mutex);
> -       mutex_unlock(&ctrl_info->ofa_mutex);
> -}

Here, again, I wonder if this mutex_lock()/mutex_unlock() approach is
optimal. Have you considered using completions?

See above for rationale.

Don: The authors intent was to block threads that are initiated from applications. I moved these HUNKs into patch 
 smartpqi-update-device-scan-operations. 

> -
> -static inline void pqi_set_encryption_info(
> -       struct pqi_encryption_info *encryption_info, struct raid_map
> *raid_map,
> -       u64 first_block)
> +static inline void pqi_set_encryption_info(struct
> pqi_encryption_info *encryption_info,
> +       struct raid_map *raid_map, u64 first_block)
>  {
>         u32 volume_blk_size;

This whitespace change doesn't belong here.

Don: moved to smartpqi-align-code-with-oob-driver

>
> @@ -3251,8 +3256,8 @@ static void pqi_acknowledge_event(struct
> pqi_ctrl_info *ctrl_info,
>         put_unaligned_le16(sizeof(request) -
> PQI_REQUEST_HEADER_LENGTH,
>                 &request.header.iu_length);
>         request.event_type = event->event_type;
> -       request.event_id = event->event_id;
> -       request.additional_event_id = event->additional_event_id;
> +       put_unaligned_le16(event->event_id, &request.event_id);
> +       put_unaligned_le16(event->additional_event_id,
> &request.additional_event_id);

The different treatment of the event_id fields is unrelated to
synchronization, or am I missing something?

Don: Moved event HUNKS to patch smartpqi-update-event-handler

>
>         pqi_send_event_ack(ctrl_info, &request, sizeof(request));
>  }
> @@ -3263,8 +3268,8 @@ static void pqi_acknowledge_event(struct
> pqi_ctrl_info *ctrl_info,
>  static enum pqi_soft_reset_status pqi_poll_for_soft_reset_status(
>         struct pqi_ctrl_info *ctrl_info)
>  {
> -       unsigned long timeout;
>         u8 status;
> +       unsigned long timeout;
>
>         timeout = (PQI_SOFT_RESET_STATUS_TIMEOUT_SECS * PQI_HZ) +
> jiffies;
>
>
> -static void pqi_ofa_process_event(struct pqi_ctrl_info *ctrl_info,
> -       struct pqi_event *event)
> +static void pqi_ofa_memory_alloc_worker(struct work_struct *work)

Moving the ofa handling into work queues seems to be a key aspect of
this patch. The patch description should mention how this is will
improve synchronization. Naïve thinking suggests that making these
calls asynchronous could aggravate synchronization issues.

Repeating myself, I feel that completions would be the best way so
synchronize with these work items.

Don: I moved these HUNKs into smartpqi-update-ofa-management. 
 The author likes to use mutexes to synchronize threads. I'll investigate your synchronizations further. Perhaps V4 will help clarify. 

> @@ -3537,8 +3572,7 @@ static int pqi_process_event_intr(struct
> pqi_ctrl_info *ctrl_info)
>
>  #define PQI_LEGACY_INTX_MASK   0x1
>
> -static inline void pqi_configure_legacy_intx(struct pqi_ctrl_info
> *ctrl_info,
> -       bool enable_intx)
> +static inline void pqi_configure_legacy_intx(struct pqi_ctrl_info
> *ctrl_info, bool enable_intx)

another whitespace hunk

Don: Moved into patch smartpqi-align-code-with-oob-driver
>  {
>         u32 intx_mask;
>         struct pqi_device_registers __iomem *pqi_registers;
> @@ -4216,59 +4250,36 @@ static int
> pqi_process_raid_io_error_synchronous(
>         return rc;
>  }
>
> +static inline bool pqi_is_blockable_request(struct pqi_iu_header
> *request)
> +{
> +       return (request->driver_flags &
> PQI_DRIVER_NONBLOCKABLE_REQUEST) == 0;
> +}
> +
>  static int pqi_submit_raid_request_synchronous(struct pqi_ctrl_info
> *ctrl_info,
>         struct pqi_iu_header *request, unsigned int flags,
> -       struct pqi_raid_error_info *error_info, unsigned long
> timeout_msecs)
> +       struct pqi_raid_error_info *error_info)

The removal of the timeout_msecs argument to this function could be
a separate patch in its own right.

Don: Moved into new patch smartpqi-remove-timeouts-from-internal-cmds

>         if (flags & PQI_SYNC_FLAGS_INTERRUPTABLE) {
>                 if (down_interruptible(&ctrl_info->sync_request_sem))
>                         return -ERESTARTSYS;
>         } else {
> -               if (timeout_msecs == NO_TIMEOUT) {
> -                       down(&ctrl_info->sync_request_sem);
> -               } else {
> -                       start_jiffies = jiffies;
> -                       if (down_timeout(&ctrl_info-
> >sync_request_sem,
> -                               msecs_to_jiffies(timeout_msecs)))
> -                               return -ETIMEDOUT;
> -                       msecs_blocked =
> -                               jiffies_to_msecs(jiffies -
> start_jiffies);
> -                       if (msecs_blocked >= timeout_msecs) {
> -                               rc = -ETIMEDOUT;
> -                               goto out;
> -                       }
> -                       timeout_msecs -= msecs_blocked;
> -               }
> +               down(&ctrl_info->sync_request_sem);
>         }
>
>         pqi_ctrl_busy(ctrl_info);
> -       timeout_msecs = pqi_wait_if_ctrl_blocked(ctrl_info,
> timeout_msecs);
> -       if (timeout_msecs == 0) {
> -               pqi_ctrl_unbusy(ctrl_info);
> -               rc = -ETIMEDOUT;
> -               goto out;
> -       }
> +       if (pqi_is_blockable_request(request))
> +               pqi_wait_if_ctrl_blocked(ctrl_info);

You wait here after taking the semaphore - is that intended? Why?
Don: The author's intent was to prevent any new driver initiated requests that deal with administrative operations (such as updating reply queues, config table changes, OFA memory changes, ...

I'll add in a comment to new patch smartpqi-remove-timeouts-from-internal-cmds.

>
>         if (pqi_ctrl_offline(ctrl_info)) {

Should you test this before waiting, perhaps?
Don: There is a separate thread that will notice that the controller has gone offline. The intent was to see if there was an issue created during the administrative update.

> @@ -5277,12 +5276,6 @@ static inline int
> pqi_raid_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info,
>                 device, scmd, queue_group);
>  }
>

Below here, a new section starts that refacors the treatment of bypass
retries. I don't see how this is related to the synchronization issues
mentioned in the patch description.

Don: Moved bypass retry HUNKs into patch
 smartpqi-update-raid-bypass-handling


> @@ -5698,6 +5562,14 @@ static inline u16 pqi_get_hw_queue(struct
> pqi_ctrl_info *ctrl_info,
>         return hw_queue;
>  }
>
> +static inline bool pqi_is_bypass_eligible_request(struct scsi_cmnd
> *scmd)
> +{
> +       if (blk_rq_is_passthrough(scmd->request))
> +               return false;
> +
> +       return scmd->retries == 0;
> +}
> +

Nice, but this fits better into (or next to) 10/25 IMO.
Don: For now moved into smartpqi-update-raid-bypass-handling
Thanks for your hard work.


> +       while (atomic_read(&device->scsi_cmds_outstanding)) {
>                 pqi_check_ctrl_health(ctrl_info);
>                 if (pqi_ctrl_offline(ctrl_info))
>                         return -ENXIO;
> -
>                 if (timeout_secs != NO_TIMEOUT) {
>                         if (time_after(jiffies, timeout)) {
>                                 dev_err(&ctrl_info->pci_dev->dev,
> -                                       "timed out waiting for
> pending IO\n");
> +                                       "timed out waiting for
> pending I/O\n");
>                                 return -ETIMEDOUT;
>                         }
>                 }

Like I said above (wrt pqi_scsi_block_requests), have you considered
using wait_event_timeout() here?
Don: I will start an investigation. However, this may take a subsequent patch.
Thanks 

> @@ -7483,7 +7243,8 @@ static void
> pqi_ctrl_update_feature_flags(struct pqi_ctrl_info *ctrl_info,
>                 break;
>         case PQI_FIRMWARE_FEATURE_SOFT_RESET_HANDSHAKE:
>                 ctrl_info->soft_reset_handshake_supported =
> -                       firmware_feature->enabled;
> +                       firmware_feature->enabled &&
> +                       ctrl_info->soft_reset_status;

Should you use readb(ctrl_info->soft_reset_status) here, like above?

Don: Yes. Changed in new patch 
 smartpqi-update-soft-reset-management-for-OFA
I guess since it's a u8 that sparse did not care? It's still iomem...

>
>  static void pqi_process_firmware_features(
> @@ -7665,14 +7435,34 @@ static void
> pqi_process_firmware_features_section(
>         mutex_unlock(&pqi_firmware_features_mutex);
>  }
>

The hunks below look like yet another independent change.
(Handling of firmware_feature_section_present).

Don: squashed into patch 
 smartpqi-add-support-for-BMIC-sense-feature-cmd-and-feature-bits