Heiko Carstens <hca@xxxxxxxxxxxxx> writes: > On Mon, Aug 03, 2020 at 06:05:24PM +0200, Thomas Gleixner wrote: >> +/** >> + * vdso_update_begin - Start of a VDSO update section >> + * >> + * Allows architecture code to safely update the architecture specific VDSO >> + * data. >> + */ >> +void vdso_update_begin(void) >> +{ >> + struct vdso_data *vdata = __arch_get_k_vdso_data(); >> + >> + raw_spin_lock(&timekeeper_lock); >> + vdso_write_begin(vdata); >> +} > > I would assume that this only works if vdso_update_begin() is called > with irqs disabled, otherwise it could deadlock, no? Yes. > Maybe something like: > > void vdso_update_begin(unsigned long *flags) > { > struct vdso_data *vdata = __arch_get_k_vdso_data(); > > raw_spin_lock_irqsave(&timekeeper_lock, *flags); > vdso_write_begin(vdata); Shudder. Why not returning flags? > } > > void vdso_update_end(unsigned long *flags) Ditto, why pointer and not value? > { > struct vdso_data *vdata = __arch_get_k_vdso_data(); > > vdso_write_end(vdata); > __arch_sync_vdso_data(vdata); > raw_spin_unlock_irqrestore(&timekeeper_lock, *flags); > } > > ? Just wondering. Thought about that briefly, but then hated the flags thing and delegated it to the caller. Lockdep will yell if that lock is taken with interrupts enabled :) But aside of the pointer vs. value thing, I'm fine with doing it in the functions. Thanks, tglx