You need to mention the fact that ctx->lock needs to be locked before calling this function.

No need to send a pointer to flags, just send the unsigned long value itself, if necessary. All callers currently send the address of an automatic anyway. Unlocking and then relocking ctx->lock by returning the new flags is inappropriate.

> + struct task_struct *task;> + unsigned long local_flags, new_flags;> + int state, ret;> +> +recheck:> + /*> + * task is NULL for system-wide context> + */> + task = ctx->task;> + state = ctx->state;> + local_flags = *flags;> +> + PFM_DBG("state=%d check_mask=0x%x", state, check_mask);> + /*> + * if the context is detached, then we do not touch> + * hardware, therefore there is not restriction on when we can> + * access it.> + */> + if (state == PFM_CTX_UNLOADED)> + return 0;> + /*> + * no command can operate on a zombie context.> + * A context becomes zombie when the file that identifies> + * it is closed while the context is still attached to the> + * thread it monitors.> + */> + if (state == PFM_CTX_ZOMBIE)> + return -EINVAL;> +> + /*> + * at this point, state is PFM_CTX_LOADED or PFM_CTX_MASKED> + */> +> + /*> + * some commands require the context to be unloaded to operate> + */> + if (check_mask & PFM_CMD_UNLOADED) {> + PFM_DBG("state=%d, cmd needs context unloaded", state);> + return -EBUSY;> + }> +> + /*> + * self-monitoring always ok.> + */> + if (task == current)> + return 0;> +> + /*> + * for syswide, the calling thread must be running on the cpu> + * the context is bound to. There cannot be preemption as we> + * check with interrupts disabled.> + */> + if (ctx->flags.system) {> + if (ctx->cpu != smp_processor_id())> + return -EBUSY;> + return 0;> + }> +> + /*> + * at this point, monitoring another thread> + */> +> + /*> + * the pfm_unload_context() command is allowed on masked context> + */> + if (state == PFM_CTX_MASKED && !(check_mask & PFM_CMD_UNLOAD))> + return 0;> +> + /*> + * When we operate on another thread, we must wait for it to be> + * stopped and completely off any CPU as we need to access the> + * PMU state (or machine state).> + *> + * A thread can be put in the STOPPED state in various ways> + * including PTRACE_ATTACH, or when it receives a SIGSTOP signal.> + * We enforce that the thread must be ptraced, so it is stopped> + * AND it CANNOT Wake up while we operate on it because this> + * would require an action for the ptracing parent which is the> + * thread that is calling this function.> + *> + * The dependency on ptrace, imposes that only the ptracing> + * parent can issue command on a thread. This is unfortunate> + * but we do not know of a better way of doing this.> + */> + if (check_mask & PFM_CMD_STOPPED) {> +> + spin_unlock_irqrestore(&ctx->lock, local_flags);> +> + /*> + * check that the thread is ptraced AND STOPPED> + */> + ret = ptrace_check_attach(task, 0);> +> + spin_lock_irqsave(&ctx->lock, new_flags);> +> + /*> + * flags may be different than when we released the lock> + */> + *flags = new_flags;

You can't do this, you'll need to either separate these functions out by having pfm_check_task_state() indicate by a return value that ptrace_check_attach() should be checked or that we've already failed, or come up with a non-locking solution.

Not really necessary for perfmon_disabled to be an atomic_t since it's only set in pfm_init() and we would have returned 0 in that function when we've initialized correctly. A simple unsigned char will work fine.

This comment doesn't make a lot of sense, you have to kfree(fptr) because it could have been set in pfm_get_args() and kfree() can take a NULL value, so why do you need a branch here at all? In fact, this branch looks like it will always be true since this is our ptr_free from a successful return of pfm_get_args() and this is on the success path.