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

[Adrian McMenamin]

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
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/