[PATCH v2 3/7] powerpc/rtas: serialize ibm,get-vpd service with papr-vpd sequences

[Nathan Lynch]

> Take the papr-vpd driver's internal mutex when sys_rtas performs
> ibm,get-vpd calls. This prevents sys_rtas(ibm,get-vpd) calls from
> interleaving with sequences performed by the driver, ensuring that
> such sequences are not disrupted.

....

> @@ -1861,6 +1862,28 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
>  		goto copy_return;
>  	}
>  
> +	if (token == rtas_function_token(RTAS_FN_IBM_GET_VPD)) {
> +		/*
> +		 * ibm,get-vpd potentially needs to be invoked
> +		 * multiple times to obtain complete results.
> +		 * Interleaved ibm,get-vpd sequences disrupt each
> +		 * other.
> +		 *
> +		 * /dev/papr-vpd doesn't have this problem and users
> +		 * do not need to be aware of each other to use it
> +		 * safely.
> +		 *
> +		 * We can prevent this call from disrupting a
> +		 * /dev/papr-vpd-initiated sequence in progress by
> +		 * reaching into the driver to take its internal
> +		 * lock. Unfortunately there is no way to prevent
> +		 * interference in the other direction without
> +		 * resorting to even worse hacks.
> +		 */
> +		pr_notice_once("Calling ibm,get-vpd via sys_rtas is allowed but deprecated. Use /dev/papr-vpd instead.\n");
> +		papr_vpd_mutex_lock();
> +	}
> +
>  	buff_copy = get_errorlog_buffer();
>  
>  	raw_spin_lock_irqsave(&rtas_lock, flags);
> @@ -1870,6 +1893,9 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
>  	do_enter_rtas(&rtas_args);
>  	args = rtas_args;
>  
> +	if (token == rtas_function_token(RTAS_FN_IBM_GET_VPD))
> +		papr_vpd_mutex_unlock();
> +

The mutex ought to nest entirely outside rtas_lock, this releases it
too early.

Anyway, I'm considering a different way to get the synchronization right
between drivers like papr-vpd and sys_rtas. Instead of having sys_rtas
acquire the driver's internal lock, rtas.c should provide a way for code
like papr-vpd to temporarily lock the syscall path.

Something like this:

// rtas.c

+ static DEFINE_MUTEX(rtas_syscall_lock);
+ void rtas_syscall_lock(void) { mutex_lock(&rtas_syscall_lock); }
+ void rtas_syscall_unlock(void) { mutex_unlock(&rtas_syscall_lock); }

  SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
  {
+        rtas_syscall_lock();

         ...

         do_enter_rtas(&rtas_args);

         ...

+        rtas_syscall_unlock();

         return 0;
  }

// papr-vpd.c

  static void vpd_sequence_begin(struct vpd_sequence *seq,
                                 const struct papr_location_code *loc_code)
  {
          static struct papr_location_code static_loc_code;
          papr_vpd_mutex_lock();
+         rtas_syscall_lock();
          static_loc_code = *loc_code;
          *seq = (struct vpd_sequence) {
                  .params = {
                          .work_area = rtas_work_area_alloc(SZ_4K),
                          .loc_code = &static_loc_code,
                          .sequence = 1,
                  },
          };
  }

  /**
   * vpd_sequence_end() - Finalize a VPD retrieval sequence.
   * @seq: Sequence state.
   *
   * Releases resources obtained by vpd_sequence_begin().
   */
  static void vpd_sequence_end(struct vpd_sequence *seq)
  {
          rtas_work_area_free(seq->params.work_area);
+         rtas_syscall_unlock();
          papr_vpd_mutex_unlock();
  }


This is a sketch to communicate the idea. The locking in the real code
could be finer, perhaps a mutex per RTAS function.