Messages in this thread | | | Date | Fri, 25 Sep 2020 13:32:25 +0800 | From | Kent Gibson <> | Subject | Re: [PATCH v9 08/20] gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL |
| |
On Wed, Sep 23, 2020 at 06:41:45PM +0300, Andy Shevchenko wrote: > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > Add support for GPIO_V2_GET_LINEINFO_IOCTL and > > GPIO_V2_GET_LINEINFO_WATCH_IOCTL. > > > > +#ifdef CONFIG_GPIO_CDEV_V1 > > +static int lineinfo_ensure_abi_version(struct gpio_chardev_data *cdata, > > + unsigned int version) > > +{ > > > + int abiv = atomic_read(&cdata->watch_abi_version); > > + > > + if (abiv == 0) { > > > + atomic_cmpxchg(&cdata->watch_abi_version, 0, version); > > + abiv = atomic_read(&cdata->watch_abi_version); > > atomic_cmpxchng() returns a value. > Also there are no barriers here... > > > + } > > + if (abiv != version) > > + return -EPERM; > > I'm not sure I understand why this is atomic. > > Also this seems to be racy if cdata changed in background. > > Shouldn't be rather > > if (atomic_cmpxchg() == 0) { > if (atomic_read() != version) > return ...; > } >
Perhaps this is what you had in mind:
static bool lineinfo_is_abi_version(struct gpio_chardev_data *cdata, unsigned int version) { int abiv = atomic_cmpxchg(&cdata->watch_abi_version, 0, version);
if (abiv == version) return true; if (abiv == 0) /* first (and only) setter sees initial 0 value */ return true;
return false; }
That is functionally equivalent, but slightly shorter.
I was thinking the atomic_cmpxchg() is more expensive than an atomic_read(), and so initially checked the value with that, but for the cmp failure case it is probably the same, or at least not worth the bother - this isn't even vaguely near a hot path.
I've also switched the return value to bool in response to your other comments.
Cheers, Kent.
| |