Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 19 Oct 2021 08:56:21 +0900
From:   William Breathitt Gray <vilhelm.gray@gmail.com>
To:     David Lechner <david@lechnology.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] counter: drop chrdev_lock
Message-ID: <YW4JpTaj20j9Flus@shinobu>
References: <20211017185521.3468640-1-david@lechnology.com>
 <YW0PVYT/GCKAnjN9@kroah.com>
 <YW03PSmpMkMVnHdp@shinobu>
 <YW06rLixA2Uush+n@kroah.com>
 <YW1Dl7ylRqyPxH2c@shinobu>
 <6aa0e966-478c-4233-fe9b-d16c3c9d4989@lechnology.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="b58fb5KMbhxV/18m"
Content-Disposition: inline
In-Reply-To: <6aa0e966-478c-4233-fe9b-d16c3c9d4989@lechnology.com>
Precedence: bulk


--b58fb5KMbhxV/18m
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Oct 18, 2021 at 11:14:15AM -0500, David Lechner wrote:
> On 10/18/21 4:51 AM, William Breathitt Gray wrote:
> > On Mon, Oct 18, 2021 at 11:13:16AM +0200, Greg KH wrote:
> >> On Mon, Oct 18, 2021 at 05:58:37PM +0900, William Breathitt Gray wrote:
> >>> On Mon, Oct 18, 2021 at 08:08:21AM +0200, Greg KH wrote:
> >>>> On Sun, Oct 17, 2021 at 01:55:21PM -0500, David Lechner wrote:
> >>>>> This removes the chrdev_lock from the counter subsystem. This was
> >>>>> intended to prevent opening the chrdev more than once. However, this
> >>>>> doesn't work in practice since userspace can duplicate file descrip=
tors
> >>>>> and pass file descriptors to other processes. Since this protection
> >>>>> can't be relied on, it is best to just remove it.
> >>>>
> >>>> Much better, thanks!
> >>>>
> >>>> One remaining question:
> >>>>
> >>>>> --- a/include/linux/counter.h
> >>>>> +++ b/include/linux/counter.h
> >>>>> @@ -297,7 +297,6 @@ struct counter_ops {
> >>>>>    * @events:		queue of detected Counter events
> >>>>>    * @events_wait:	wait queue to allow blocking reads of Counter ev=
ents
> >>>>>    * @events_lock:	lock to protect Counter events queue read operat=
ions
> >>>>> - * @chrdev_lock:	lock to limit chrdev to a single open at a time
> >>>>>    * @ops_exist_lock:	lock to prevent use during removal
> >>>>
> >>>> Why do you still need 2 locks for the same structure?
> >>>>
> >>>> thanks,
> >>>>
> >>>> greg k-h
> >>>
> >>> Originally there was only the events_lock mutex. Initially I tried us=
ing
> >>> it to also limit the chrdev to a single open, but then came across a
> >>> "lock held when returning to user space" warning:
> >>> https://lore.kernel.org/linux-arm-kernel/YOq19zTsOzKA8v7c@shinobu/T/#=
m6072133d418d598a5f368bb942c945e46cfab9a5
> >>>
> >>> Instead of losing the benefits of a mutex lock for protecting the
> >>> events, I ultimately implemented the chrdev_lock separately as an
> >>> atomic_t. If the chrdev_lock is removed, then we'll use events_lock
> >>> solely from now on for this structure.
> >>
> >> chrdev_lock should be removed, it doesn't really do what you think it
> >> does, as per the thread yesterday :)
> >>
> >> So does this mean you can also drop the ops_exist_lock?
> >>
> >> thanks,
> >>
> >> greg k-h
> >=20
> > When counter_unregister is called, the ops member is set to NULL to
> > indicate that the driver will be removed and that no new device
> > operations should occur (because the ops callbacks will no longer be
> > valid). The ops_exist_lock is used to allow existing ops callback
> > dereferences to complete before the driver is removed so that we do not
> > suffer a page fault.
> >=20
> > I don't believe we can remove this protection (or can we?) but perhaps
> > we can merge the three mutex locks (n_events_list_lock, events_lock, and
> > ops_exist_lock) into a single "counter_lock" that handles all mutex
> > locking for this structure.
> >=20
>=20
> The different mutexes protect individual parts of the counter struct
> rather than the struct as a whole (a linked list, kfifo reads, and
> callback ops), so I think it makes the code clearer having individual
> mutexes for each rather than having a global mutex for unrelated
> actions.

That's a fair point. Because these operations are independent of each
other, keeping the mutexes separate would allow users to configure the
next events watch list without necessarily stalling the current read()
of events. I doubt the difference is significant, but if the purpose of
these locks are well enough defined then independent mutex locks may not
be a problem.

William Breathitt Gray

--b58fb5KMbhxV/18m
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAmFuCZoACgkQhvpINdm7
VJJKMQ//fTXJ04FMXKrHomD7DqIgMlq+w8uC1Mcr97R3oNBg7r6HLN321eWPz/rn
4bC1V3ww9dj3WzMOMtE7dKILJyCCmnbue/+1v694Phj3DppsZ/bN8RLo2c0K5ObF
YS6evT9LZ/frqDOMAYm0b0g6HRxg9GF1A31Td1bwK2YmN4h1mfA351n+0sqxScF0
r7qs8T9fPdB9YngI7VwIe0WAMT1C1rjl5rCVTus3Y+ojQa8Xd9Ua3OgdNOlPiAcN
l62tl4R58BFztmsqtY4DtOabeYV85n8bBnUBpF7bcptpUcB9elU2imhNYXUcUxnT
6h1rbhn+bNdq0ClVZmyAD22JVg9RyAl2iJ8XvlAg3t5CGM9N6rFMnXpH+W6YiI7q
KXYA+JBksyAfuifhsv89AogTj3maf0vzrifz6UkkYLcBmkzed9h+qDHyBGW7qirI
TApiRqQMMMV/wd6h0U5Ai5C5O4dteOnOeNhYxjul+Xm2G87bFWXtPUT4xPWkkVb1
mxV9+K6YuX/ny4MH+55B1a0oa6o2hpfga09YSeSkuX2Ci6hq/K1ZEgHd+XMNptlz
PCl+ezqsxz7FQUoi5+ne3/8+HC9qI3zk049m3GrIDuMh27E0M6Ccj5zpT+JociXs
XnPPCg8RPThi7EUEdVB5zEWTdAg3IPsV7XVLNAALHN1CyQWsSac=
=zW9x
-----END PGP SIGNATURE-----

--b58fb5KMbhxV/18m--