Re: [PATCH] 1/2 Maple: Update bus driver to allow support of VMU

Adrian McMenamin
2008-03-20T22:23:17Z

On Thu, 2008-03-20 at 13:56 -0700, Andrew Morton wrote:

> urgh, down_trylock(). And a secret, undocumented one too.
>
> A trylock is always an exceptional thing. How is *any* reader of this code
> supposed to work out what the heck it's doing there? Convert it into
> down(), run the code and decrypt the lockdep warnings, I suspect.
>
> <looks>
>
> Nope, I can't see any other lock being held when we call this function.
>
> The trylocks are an utter mystery to me. Please don't write mysterious
> code.
>

OK, I am sure this is my problem but I have no idea why you are
describing down_trylock as undocumented - I simply took it out of page
111 of the "Linux Device Drivers" book - hardly the canonical source but
why are you telling me that it is undocumented?

The down_trylock is used here so that a passed in packet and the regular
every second check that the device has not been hot-unplugged don't
deadlock. If I used down then a deadlock is a near certainty. It simply
bounces the 1 second test here if the lock is already down. Is that a
big problem?

> > @@ -398,32 +403,38 @@ static int setup_maple_commands(struct device *device, void *ignored)
> > /* VBLANK bottom half - implemented via workqueue */
> > static void maple_vblank_handler(struct work_struct *work)
> > {
> > - if (!maple_dma_done())
> > - return;
> > if (!list_empty(&maple_sentq))
> > return;
> > + if (!maple_dma_done())
> > + return;
> > ctrl_outl(0, MAPLE_ENABLE);
> > - liststatus = 0;
> > bus_for_each_dev(&maple_bus_type, NULL, NULL,
> > setup_maple_commands);
> > if (time_after(jiffies, maple_pnp_time))
> > maple_pnp_time = jiffies + MAPLE_PNP_INTERVAL;
> > - if (liststatus && list_empty(&maple_sentq)) {
> > + mutex_lock(&maple_wlist_lock);
> > + if (!list_empty(&maple_waitq) && list_empty(&maple_sentq)) {
> > + mutex_unlock(&maple_wlist_lock);
> > + mutex_lock(&maple_list_lock);
> > INIT_LIST_HEAD(&maple_sentq);
> > + mutex_unlock(&maple_list_lock);
> > maple_send();
> > + } else {
> > + mutex_unlock(&maple_wlist_lock);
> > }
> > +
> > maplebus_dma_reset();
> > }
>
> This locking looks like a mess. I'd suggest that maple_list_lock and
> maple_wlist_lock now need documentation. Describe what data they protect
> and what their ranking rules are.

Well, they don't seem a mess to me, but I can document them, though I
thought thety were pretty self-documenting :( One locks the waiting
queue of packets, one the queue of output packets.

>
> > /* handle devices added via hotplugs - placing them on queue for DEVINFO*/
> > static void maple_map_subunits(struct maple_device *mdev, int submask)
> > {
> > - int retval, k, devcheck;
> > + int retval, k, devcheck, locking;
> > struct maple_device *mdev_add;
> > struct maple_device_specify ds;
> >
> > + ds.port = mdev->port;
> > for (k = 0; k < 5; k++) {
> > - ds.port = mdev->port;
> > ds.unit = k + 1;
> > retval > > bus_for_each_dev(&maple_bus_type, NULL, &ds,
> > @@ -437,9 +448,15 @@ static void maple_map_subunits(struct maple_device *mdev, int submask)
> > mdev_add = maple_alloc_dev(mdev->port, k + 1);
> > if (!mdev_add)
> > return;
> > + locking = down_trylock(&mdev_add->mq->sem);
> > + if (locking) {
> > + up(&mdev_add->mq->sem);
> > + down_interruptible(&mdev_add->mq->sem);
> > + }
>
> What the heck is that trying to do?!?!?!

Again, it is trying to avoid a deadlock between injected packets and
scans for hotplug events.

>
> And it's buggy. The return value of down_interruptible() is ignored, so
> the driver has lost track of whether or not the semphore was taken.

You are right about that and it should be fixed
>